From 0a17fafd723e8684d1deb4b5ceea58967a0154da Mon Sep 17 00:00:00 2001 From: tonikitoo Date: Thu, 15 Sep 2016 13:50:50 -0700 Subject: Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on a text markup definition, the coordinates used to draw the annotation come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. TEST=testing/resources/pixel/bug_585_*.in BUG=pdfium:585 Review-Url: https://codereview.chromium.org/2289293005 --- DEPS | 2 +- core/fpdfdoc/cpdf_annot.cpp | 69 ++++++-- core/fpdfdoc/cpvt_generateap.cpp | 42 +++-- core/fpdfdoc/include/cpdf_annot.h | 5 + testing/resources/pixel/bug_585.in | 175 +++++++++++++++++++++ testing/resources/pixel/bug_585_expected.pdf.0.png | Bin 0 -> 3892 bytes 6 files changed, 267 insertions(+), 26 deletions(-) create mode 100644 testing/resources/pixel/bug_585.in create mode 100644 testing/resources/pixel/bug_585_expected.pdf.0.png diff --git a/DEPS b/DEPS index e3ef8e8372..e57e5af206 100644 --- a/DEPS +++ b/DEPS @@ -14,7 +14,7 @@ vars = { 'gmock_revision': '29763965ab52f24565299976b936d1265cb6a271', 'gtest_revision': '8245545b6dc9c4703e6496d1efd19e975ad2b038', 'icu_revision': '2341038bf72869a5683a893a2b319a48ffec7f62', - 'pdfium_tests_revision': '8485b3093524ddb7319e0381ab10c576e59d5091', + 'pdfium_tests_revision': 'e86a1bac556194ee572b0cd37d04ea646c7b5320', 'skia_revision': '39f7a10a04a914384944d8bf62621144ac4eeaa3', 'tools_memory_revision': '427f10475e1a8d72424c29d00bf689122b738e5d', 'trace_event_revision': '54b8455be9505c2cb0cf5c26bb86739c236471aa', diff --git a/core/fpdfdoc/cpdf_annot.cpp b/core/fpdfdoc/cpdf_annot.cpp index bee96a62ff..2cc767a066 100644 --- a/core/fpdfdoc/cpdf_annot.cpp +++ b/core/fpdfdoc/cpdf_annot.cpp @@ -20,6 +20,15 @@ namespace { +char kPDFiumKey_HasGeneratedAP[] = "PDFIUM_HasGeneratedAP"; + +bool IsTextMarkupAnnotation(CPDF_Annot::Subtype type) { + return type == CPDF_Annot::Subtype::HIGHLIGHT || + type == CPDF_Annot::Subtype::SQUIGGLY || + type == CPDF_Annot::Subtype::STRIKEOUT || + type == CPDF_Annot::Subtype::UNDERLINE; +} + bool ShouldGenerateAPForAnnotation(CPDF_Dictionary* pAnnotDict) { // If AP dictionary exists, we use the appearance defined in the // existing AP dictionary. @@ -40,6 +49,8 @@ CPDF_Annot::CPDF_Annot(CPDF_Dictionary* pDict, m_bOpenState(false), m_pPopupAnnot(nullptr) { m_nSubtype = StringToAnnotSubtype(m_pAnnotDict->GetStringFor("Subtype")); + m_bIsTextMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype); + m_bHasGeneratedAP = m_pAnnotDict->GetBooleanFor(kPDFiumKey_HasGeneratedAP); GenerateAPIfNeeded(); } @@ -53,24 +64,30 @@ void CPDF_Annot::GenerateAPIfNeeded() { if (!ShouldGenerateAPForAnnotation(m_pAnnotDict)) return; + bool result = false; if (m_nSubtype == CPDF_Annot::Subtype::CIRCLE) - CPVT_GenerateAP::GenerateCircleAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateCircleAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::HIGHLIGHT) - CPVT_GenerateAP::GenerateHighlightAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateHighlightAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::INK) - CPVT_GenerateAP::GenerateInkAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateInkAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::POPUP) - CPVT_GenerateAP::GeneratePopupAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GeneratePopupAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::SQUARE) - CPVT_GenerateAP::GenerateSquareAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateSquareAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::SQUIGGLY) - CPVT_GenerateAP::GenerateSquigglyAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateSquigglyAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::STRIKEOUT) - CPVT_GenerateAP::GenerateStrikeOutAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateStrikeOutAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::TEXT) - CPVT_GenerateAP::GenerateTextAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateTextAP(m_pDocument, m_pAnnotDict); else if (m_nSubtype == CPDF_Annot::Subtype::UNDERLINE) - CPVT_GenerateAP::GenerateUnderlineAP(m_pDocument, m_pAnnotDict); + result = CPVT_GenerateAP::GenerateUnderlineAP(m_pDocument, m_pAnnotDict); + + if (result) { + m_pAnnotDict->SetBooleanFor(kPDFiumKey_HasGeneratedAP, result); + m_bHasGeneratedAP = result; + } } bool CPDF_Annot::ShouldDrawAnnotation() { @@ -91,11 +108,23 @@ CPDF_Annot::Subtype CPDF_Annot::GetSubtype() const { return m_nSubtype; } +CFX_FloatRect CPDF_Annot::RectForDrawing() const { + if (!m_pAnnotDict) + return CFX_FloatRect(); + + bool bShouldUseQuadPointsCoords = + m_bIsTextMarkupAnnotation && m_bHasGeneratedAP; + if (bShouldUseQuadPointsCoords) + return RectFromQuadPoints(m_pAnnotDict); + + return m_pAnnotDict->GetRectFor("Rect"); +} + CFX_FloatRect CPDF_Annot::GetRect() const { if (!m_pAnnotDict) return CFX_FloatRect(); - CFX_FloatRect rect = m_pAnnotDict->GetRectFor("Rect"); + CFX_FloatRect rect = RectForDrawing(); rect.Normalize(); return rect; } @@ -175,6 +204,26 @@ static CPDF_Form* FPDFDOC_Annot_GetMatrix(const CPDF_Page* pPage, return pForm; } +// Static. +CFX_FloatRect CPDF_Annot::RectFromQuadPoints(CPDF_Dictionary* pAnnotDict) { + CPDF_Array* pArray = pAnnotDict->GetArrayFor("QuadPoints"); + if (!pArray) + return CFX_FloatRect(); + + // QuadPoints are defined with 4 pairs of numbers + // ([ pair0, pair1, pair2, pair3 ]), where + // pair0 = top_left + // pair1 = top_right + // pair2 = bottom_left + // pair3 = bottom_right + // + // On the other hand, /Rect is define as 2 pairs [pair0, pair1] where: + // pair0 = bottom_left + // pair1 = top_right. + return CFX_FloatRect(pArray->GetNumberAt(4), pArray->GetNumberAt(5), + pArray->GetNumberAt(2), pArray->GetNumberAt(3)); +} + // Static. bool CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) { return !!(pAnnotDict->GetIntegerFor("F") & ANNOTFLAG_HIDDEN); diff --git a/core/fpdfdoc/cpvt_generateap.cpp b/core/fpdfdoc/cpvt_generateap.cpp index decc408c33..d37aaf3acb 100644 --- a/core/fpdfdoc/cpvt_generateap.cpp +++ b/core/fpdfdoc/cpvt_generateap.cpp @@ -586,7 +586,8 @@ CPDF_Dictionary* GenerateResourceDict(CPDF_Dictionary* pExtGStateDict, void GenerateAndSetAPDict(CPDF_Document* pDoc, CPDF_Dictionary* pAnnotDict, const CFX_ByteTextBuf& sAppStream, - CPDF_Dictionary* pResourceDict) { + CPDF_Dictionary* pResourceDict, + bool bIsTextMarkupAnnotation) { CPDF_Dictionary* pAPDict = new CPDF_Dictionary; pAnnotDict->SetFor("AP", pAPDict); @@ -602,7 +603,9 @@ void GenerateAndSetAPDict(CPDF_Document* pDoc, pStreamDict->SetStringFor("Subtype", "Form"); pStreamDict->SetMatrixFor("Matrix", CFX_Matrix()); - CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect"); + CFX_FloatRect rect = bIsTextMarkupAnnotation + ? CPDF_Annot::RectFromQuadPoints(pAnnotDict) + : pAnnotDict->GetRectFor("Rect"); pStreamDict->SetRectFor("BBox", rect); pStreamDict->SetFor("Resources", pResourceDict); @@ -783,7 +786,8 @@ bool CPVT_GenerateAP::GenerateCircleAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + false /*IsTextMarkupAnnotation*/); return true; } @@ -797,7 +801,7 @@ bool CPVT_GenerateAP::GenerateHighlightAP(CPDF_Document* pDoc, CPVT_Color(CPVT_Color::kRGB, 1, 1, 0), PaintOperation::FILL); - CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect"); + CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict); rect.Normalize(); sAppStream << rect.left << " " << rect.top << " m " << rect.right << " " @@ -809,7 +813,8 @@ bool CPVT_GenerateAP::GenerateHighlightAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Multiply"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + true /*IsTextMarkupAnnotation*/); return true; } @@ -863,7 +868,8 @@ bool CPVT_GenerateAP::GenerateInkAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + false /*IsTextMarkupAnnotation*/); return true; } @@ -885,7 +891,8 @@ bool CPVT_GenerateAP::GenerateTextAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + false /*IsTextMarkupAnnotation*/); return true; } @@ -899,7 +906,7 @@ bool CPVT_GenerateAP::GenerateUnderlineAP(CPDF_Document* pDoc, CPVT_Color(CPVT_Color::kRGB, 0, 0, 0), PaintOperation::STROKE); - CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect"); + CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict); rect.Normalize(); FX_FLOAT fLineWidth = 1.0; @@ -911,7 +918,8 @@ bool CPVT_GenerateAP::GenerateUnderlineAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + true /*IsTextMarkupAnnotation*/); return true; } @@ -949,7 +957,8 @@ bool CPVT_GenerateAP::GeneratePopupAP(CPDF_Document* pDoc, return false; sAppStream << GetPopupContentsString(pDoc, *pAnnotDict, pDefFont, sFontName); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + false /*IsTextMarkupAnnotation*/); return true; } @@ -996,7 +1005,8 @@ bool CPVT_GenerateAP::GenerateSquareAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + false /*IsTextMarkupAnnotation*/); return true; } @@ -1010,7 +1020,7 @@ bool CPVT_GenerateAP::GenerateSquigglyAP(CPDF_Document* pDoc, CPVT_Color(CPVT_Color::kRGB, 0, 0, 0), PaintOperation::STROKE); - CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect"); + CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict); rect.Normalize(); FX_FLOAT fLineWidth = 1.0; @@ -1044,7 +1054,8 @@ bool CPVT_GenerateAP::GenerateSquigglyAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + true /*IsTextMarkupAnnotation*/); return true; } @@ -1058,7 +1069,7 @@ bool CPVT_GenerateAP::GenerateStrikeOutAP(CPDF_Document* pDoc, CPVT_Color(CPVT_Color::kRGB, 0, 0, 0), PaintOperation::STROKE); - CFX_FloatRect rect = pAnnotDict->GetRectFor("Rect"); + CFX_FloatRect rect = CPDF_Annot::RectFromQuadPoints(pAnnotDict); rect.Normalize(); FX_FLOAT fLineWidth = 1.0; @@ -1070,7 +1081,8 @@ bool CPVT_GenerateAP::GenerateStrikeOutAP(CPDF_Document* pDoc, GenerateExtGStateDict(*pAnnotDict, sExtGSDictName, "Normal"); CPDF_Dictionary* pResourceDict = GenerateResourceDict(pExtGStateDict, nullptr); - GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict); + GenerateAndSetAPDict(pDoc, pAnnotDict, sAppStream, pResourceDict, + true /*IsTextMarkupAnnotation*/); return true; } diff --git a/core/fpdfdoc/include/cpdf_annot.h b/core/fpdfdoc/include/cpdf_annot.h index c16decc7d6..43f58931d3 100644 --- a/core/fpdfdoc/include/cpdf_annot.h +++ b/core/fpdfdoc/include/cpdf_annot.h @@ -71,6 +71,7 @@ class CPDF_Annot { static CPDF_Annot::Subtype StringToAnnotSubtype( const CFX_ByteString& sSubtype); static CFX_ByteString AnnotSubtypeToString(CPDF_Annot::Subtype nSubtype); + static CFX_FloatRect RectFromQuadPoints(CPDF_Dictionary* pAnnotDict); CPDF_Annot(CPDF_Dictionary* pDict, CPDF_Document* pDocument, bool bToOwnDict); ~CPDF_Annot(); @@ -102,6 +103,8 @@ class CPDF_Annot { void GenerateAPIfNeeded(); bool ShouldDrawAnnotation(); + CFX_FloatRect RectForDrawing() const; + // For regular annotations, |m_pAnnotDict| is not owned. For // our artificially created popup annotations, |m_pAnnotDict| // is owned by this class. @@ -112,6 +115,8 @@ class CPDF_Annot { std::map> m_APMap; // |m_bOpenState| is only set for popup annotations. bool m_bOpenState; + bool m_bHasGeneratedAP; + bool m_bIsTextMarkupAnnotation; // Not owned. If there is a valid pointer in |m_pPopupAnnot|, // then this annot is never a popup. CPDF_Annot* m_pPopupAnnot; diff --git a/testing/resources/pixel/bug_585.in b/testing/resources/pixel/bug_585.in new file mode 100644 index 0000000000..b8b56ad427 --- /dev/null +++ b/testing/resources/pixel/bug_585.in @@ -0,0 +1,175 @@ +{{header}} +{{object 1 0}} << + /Type /Catalog + /Pages 2 0 R +>> +endobj +{{object 2 0}} << + /Type /Pages + /Count 1 + /Kids [ + 10 0 R + ] +>> +endobj +% Page number 0. +{{object 10 0}} << + /Type /Page + /Parent 2 0 R + /Resources << + /Font <> + >> + /MediaBox [0 0 612 792] + /Annots [ + 22 0 R + 23 0 R + 24 0 R + 25 0 R + ] + /Tabs /R +>> +endobj + +{{object 22 0}} << + /Type /Annot + /Subtype /Highlight + /Rect [ -1 -1 -1 -1 ] + /NM (Annot-1) + /F 4 + /QuadPoints [ 475 688 512 688 475 679 512 679 ] + /C [ 0.0001108646 0.001760244 0.9982184 ] + /Contents () +>> +endobj + +{{object 23 0}} << + /Border [ + 0 + 0 + 1 + ] + /C [ + 0.294118 + 0.6 + 1 + ] + /CA 1 + /CreationDate (D:20150312175256+08'00') + /F 4 + /M (D:20150312175256+08'00') + /NM (7f264ba2-e270-42a1-a390-eb41278072ff) + /QuadPoints [ + 227.567 + 688.016 + 298.115 + 688.016 + 227.567 + 679.292 + 298.115 + 679.292 + ] + /RC (

\r\n

\r\n\r\n) + /Rect [ + -1 + -1 + -1 + -1 + ] + /Subj (Squiggly) + /Subtype /Squiggly + /T (Administrator) + /Type /Annot +>> +endobj + +{{object 24 0}} << + /Border [ + 0 + 0 + 1 + ] + /C [ + 0.278431 + 0.603922 + 0.6 + ] + /CA 1 + /CreationDate (D:20150312175350+08'00') + /F 4 + /IRT 6 0 R + /IT /StrikeOutTextEdit + /M (D:20150312175350+08'00') + /NM (2912a3cf-3b30-48a7-8531-431ce468e6a8) + /P 4 0 R + /QuadPoints [ + 186.875 + 714.836 + 293.483 + 714.836 + 186.875 + 706.064 + 293.483 + 706.064 + ] + /RT /Group + /Rect [ + -1 + -1 + -1 + -1 + ] + /Subj (Replace) + /Subtype /StrikeOut + /T (Administrator) + /Type /Annot +>> +endobj + +{{object 25 0}} << + /Border [ + 0 + 0 + 1 + ] + /C [ + 0.2 + 0.619608 + 0 + ] + /CA 1 + /CreationDate (D:20150312175318+08'00') + /F 4 + /M (D:20150312175318+08'00') + /NM (c99bdf16-3040-4bf6-9b8a-a80cc563a6a7) + /P 4 0 R + /Popup 6 0 R + /QuadPoints [ + 204.395 + 647.984 + 403.895 + 647.984 + 204.395 + 636.872 + 403.895 + 636.872 + ] + /RC (

\r\n

\r\n\r\n) + /Rect [ + -1 + -1 + -1 + -1 + ] + /Subj (Underline) + /Subtype /Underline + /T (Administrator) + /Type /Annot +>> +endobj + +{{xref}} +trailer << + /Root 1 0 R +>> +{{startxref}} +%%EOF diff --git a/testing/resources/pixel/bug_585_expected.pdf.0.png b/testing/resources/pixel/bug_585_expected.pdf.0.png new file mode 100644 index 0000000000..34bc892231 Binary files /dev/null and b/testing/resources/pixel/bug_585_expected.pdf.0.png differ -- cgit v1.2.3