commit df60f7f4c1c22f903850822d4e58cc2eefdffd07
author Lei Zhang <thestig@chromium.org> Fri Feb 23 18:26:10 2024 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Feb 23 18:26:10 2024 +0000
tree 14d8d60b55c0fce05d672d7724277ddec4b783cf
parent 074387f08eebc377a8e79e717987bb03241caf41

Fix incorrect matrix outputs in CPDF_PageContentGenerator

When CPDF_PageContentGenerator generates content stream data for a page
object, it currently does not take the object's container's current
transformation matrix into account. As a result, the generated output
effectively has the CTM applied twice for page objects whose CTM is not
the identity matrix, as shown in recently added PDFEditImgTest cases.

Change CPDF_PageContentGenerator to take the CTM into account for images
and forms. Then update the related PDFEditImgTest cases so they pass
with the expected results. Add TODOs for CPDF_PageContentGenerator
methods that process text and paths, as there are no associated tests
for those.

Along the way, use more local matrix variables to avoid repeated calls
to getters.

Bug: pdfium:2132
Change-Id: I8d4c783ddee5c44dfeca32ec5575d228572e2417
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/116933
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Thomas Sepez <tsepez@google.com>
Commit-Queue: Lei Zhang <thestig@chromium.org>

core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp

diff --git a/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp b/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
index 617891c..c427478 100644
--- a/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
+++ b/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
@@ -174,8 +174,10 @@
 
     // Set the default graphic state values
     buf << "q\n";
-    if (!m_pObjHolder->GetLastCTM().IsIdentity())
-      WriteMatrix(buf, m_pObjHolder->GetLastCTM().GetInverse()) << " cm\n";
+    const CFX_Matrix& ctm = m_pObjHolder->GetLastCTM();
+    if (!ctm.IsIdentity()) {
+      WriteMatrix(buf, ctm.GetInverse()) << " cm\n";
+    }
 
     ProcessDefaultGraphics(&buf);
     streams[dirty_stream] = std::move(buf);
@@ -397,8 +399,8 @@
 
 void CPDF_PageContentGenerator::ProcessImage(fxcrt::ostringstream* buf,
                                              CPDF_ImageObject* pImageObj) {
-  if ((pImageObj->matrix().a == 0 && pImageObj->matrix().b == 0) ||
-      (pImageObj->matrix().c == 0 && pImageObj->matrix().d == 0)) {
+  CFX_Matrix matrix = pImageObj->matrix();
+  if ((matrix.a == 0 && matrix.b == 0) || (matrix.c == 0 && matrix.d == 0)) {
     return;
   }
 
@@ -411,7 +413,12 @@
     return;
 
   *buf << "q ";
-  WriteMatrix(*buf, pImageObj->matrix()) << " cm ";
+
+  const CFX_Matrix& ctm = m_pObjHolder->GetLastCTM();
+  if (!ctm.IsIdentity()) {
+    matrix.Concat(ctm.GetInverse());
+  }
+  WriteMatrix(*buf, matrix) << " cm ";
 
   bool bWasInline = pStream->IsInline();
   if (bWasInline)
@@ -430,8 +437,8 @@
 
 void CPDF_PageContentGenerator::ProcessForm(fxcrt::ostringstream* buf,
                                             CPDF_FormObject* pFormObj) {
-  if ((pFormObj->form_matrix().a == 0 && pFormObj->form_matrix().b == 0) ||
-      (pFormObj->form_matrix().c == 0 && pFormObj->form_matrix().d == 0)) {
+  CFX_Matrix matrix = pFormObj->form_matrix();
+  if ((matrix.a == 0 && matrix.b == 0) || (matrix.c == 0 && matrix.d == 0)) {
     return;
   }
 
@@ -443,7 +450,13 @@
   pFormObj->SetResourceName(name);
 
   *buf << "q\n";
-  WriteMatrix(*buf, pFormObj->form_matrix()) << " cm ";
+
+  const CFX_Matrix& ctm = m_pObjHolder->GetLastCTM();
+  if (!ctm.IsIdentity()) {
+    matrix.Concat(ctm.GetInverse());
+  }
+  WriteMatrix(*buf, matrix) << " cm ";
+
   *buf << "/" << PDF_NameEncode(name) << " Do Q\n";
 }
 
@@ -502,6 +515,8 @@
                                             CPDF_PathObject* pPathObj) {
   ProcessGraphics(buf, pPathObj);
 
+  // TODO(crbug.com/pdfium/2132): Does this need to take the current
+  // transformation matrix in `m_pObjHolder` into account?
   WriteMatrix(*buf, pPathObj->matrix()) << " cm ";
   ProcessPathPoints(buf, &pPathObj->path());
 
@@ -654,6 +669,8 @@
                                             CPDF_TextObject* pTextObj) {
   ProcessGraphics(buf, pTextObj);
   *buf << "BT ";
+  // TODO(crbug.com/pdfium/2132): Does this need to take the current
+  // transformation matrix in `m_pObjHolder` into account?
   WriteMatrix(*buf, pTextObj->GetTextMatrix()) << " Tm ";
   RetainPtr<CPDF_Font> pFont(pTextObj->GetFont());
   if (!pFont)

fpdfsdk/fpdf_editimg_embeddertest.cpp

diff --git a/fpdfsdk/fpdf_editimg_embeddertest.cpp b/fpdfsdk/fpdf_editimg_embeddertest.cpp
index c06d495..d16cef3 100644
--- a/fpdfsdk/fpdf_editimg_embeddertest.cpp
+++ b/fpdfsdk/fpdf_editimg_embeddertest.cpp
@@ -219,9 +219,7 @@
 
   UnloadPage(page);
 
-  // TODO(crbug.com/pdfium/2132): Should be `kExpectedChecksum`.
-  constexpr char kWrongChecksum[] = "d01c62c9ee094f5be545f006148808b5";
-  VerifySavedDocument(kExpectedWidth, kExpectedHeight, kWrongChecksum);
+  VerifySavedDocument(kExpectedWidth, kExpectedHeight, kExpectedChecksum);
 }
 
 TEST_F(PDFEditImgTest, GetAndSetMatrixForFormWithImage) {
@@ -289,7 +287,5 @@
 
   UnloadPage(page);
 
-  // TODO(crbug.com/pdfium/2132): Should be `kExpectedChecksum`.
-  constexpr char kWrongChecksum[] = "308e2263f01744b2749d68a9ca711fc6";
-  VerifySavedDocument(kExpectedWidth, kExpectedHeight, kWrongChecksum);
+  VerifySavedDocument(kExpectedWidth, kExpectedHeight, kExpectedChecksum);
 }