[PDF Ink Signatures] Update Ink render transform

Sometimes the Ink strokes applied to a PDF page can be slightly shifted
in position compared to when the stroke was in the process of being
drawn by the user.  This is particularly noticeable for PDF pages whose
size in points has a non-zero fractional component.

To ensure consistent positioning between in-progress and applied
strokes, update the GetInkRenderTransform() logic to match the approach
that PDFium's CPDF_Page::GetDisplayMatrix() uses when drawing PDF
pages.  This includes omitting some instances of doing minus one for
90, 180, and 270 degree rotations.

Bug: 380038343, 399133248
Change-Id: Ie9b8ca573420d558b5e17f0c67e732743f37455f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6311352
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Alan Screen <awscreen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1426629}

pdf/pdf_ink_module.cc

@@ -172,7 +172,6 @@ void PdfInkModule::Draw(SkCanvas& canvas) {
 
   const gfx::Vector2dF origin_offset = client_->GetViewportOriginOffset();
   const PageOrientation rotation = client_->GetOrientation();
-  const float zoom = client_->GetZoom();
 
   auto in_progress_stroke = CreateInProgressStrokeSegmentsFromInputs();
   CHECK(!in_progress_stroke.empty());
@@ -180,8 +179,10 @@ void PdfInkModule::Draw(SkCanvas& canvas) {
   DrawingStrokeState& state = drawing_stroke_state();
 
   const gfx::Rect content_rect = client_->GetPageContentsRect(state.page_index);
-  const ink::AffineTransform transform =
-      GetInkRenderTransform(origin_offset, rotation, content_rect, zoom);
+  const gfx::SizeF page_size_in_points =
+      client_->GetPageSizeInPoints(state.page_index);
+  const ink::AffineTransform transform = GetInkRenderTransform(
+      origin_offset, rotation, content_rect, page_size_in_points);
 
   SkAutoCanvasRestore save_restore(&canvas, /*doSave=*/true);
   canvas.clipRect(GetDrawPageClipRect(content_rect, origin_offset));

pdf/pdf_ink_transform.cc

@@ -19,6 +19,8 @@
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/geometry/vector2d_f.h"
 
+using printing::kUnitConversionFactorPixelsToPoints;
+
 namespace chrome_pdf {
 
 namespace {
@@ -62,6 +64,21 @@ gfx::PointF CanonicalPositionToScreenPosition(
   return screen_position;
 }
 
+gfx::Size GetOriginalUnrotatedSize(PageOrientation orientation,
+                                   const gfx::Size& size) {
+  switch (orientation) {
+    case PageOrientation::kOriginal:
+    case PageOrientation::kClockwise180:
+      return size;
+    case PageOrientation::kClockwise90:
+    case PageOrientation::kClockwise270:
+      gfx::Size transposed_size(size);
+      transposed_size.Transpose();
+      return transposed_size;
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 gfx::PointF EventPositionToCanonicalPosition(const gfx::PointF& event_position,
@@ -98,29 +115,40 @@ ink::AffineTransform GetInkRenderTransform(
     const gfx::Vector2dF& viewport_origin_offset,
     PageOrientation orientation,
     const gfx::Rect& page_content_rect,
-    float scale_factor) {
+    const gfx::SizeF& page_size_in_points) {
   CHECK_GE(viewport_origin_offset.x(), 0.0f);
   CHECK_GE(viewport_origin_offset.y(), 0.0f);
-  CHECK_GT(scale_factor, 0.0f);
   CHECK(!page_content_rect.IsEmpty());
+  CHECK(!page_size_in_points.IsEmpty());
 
+  // To avoid a noticeable shift in position of an in-progress vs. applied
+  // Ink stroke, the rendering transform generated here needs to match the
+  // matrix setup done in PDFium's `CPDF_Page::GetDisplayMatrix()`.
   const float dx = viewport_origin_offset.x() + page_content_rect.x();
   const float dy = viewport_origin_offset.y() + page_content_rect.y();
+  const gfx::Size original_unrotated_page_size =
+      GetOriginalUnrotatedSize(orientation, page_content_rect.size());
+  const float scale_factor_x = original_unrotated_page_size.width() *
+                               kUnitConversionFactorPixelsToPoints /
+                               page_size_in_points.width();
+  const float scale_factor_y = original_unrotated_page_size.height() *
+                               kUnitConversionFactorPixelsToPoints /
+                               page_size_in_points.height();
 
   switch (orientation) {
     case PageOrientation::kOriginal:
-      return ink::AffineTransform(scale_factor, 0, dx, 0, scale_factor, dy);
+      return ink::AffineTransform(scale_factor_x, 0, dx, 0, scale_factor_y, dy);
     case PageOrientation::kClockwise90:
-      return ink::AffineTransform(0, -scale_factor,
-                                  dx + page_content_rect.width() - 1,
-                                  scale_factor, 0, dy);
+      return ink::AffineTransform(0, -scale_factor_x,
+                                  dx + page_content_rect.width(),
+                                  scale_factor_y, 0, dy);
     case PageOrientation::kClockwise180:
       return ink::AffineTransform(
-          -scale_factor, 0, dx + page_content_rect.width() - 1, 0,
-          -scale_factor, dy + page_content_rect.height() - 1);
+          -scale_factor_x, 0, dx + page_content_rect.width(), 0,
+          -scale_factor_y, dy + page_content_rect.height());
     case PageOrientation::kClockwise270:
-      return ink::AffineTransform(0, scale_factor, dx, -scale_factor, 0,
-                                  dy + page_content_rect.height() - 1);
+      return ink::AffineTransform(0, scale_factor_x, dx, -scale_factor_y, 0,
+                                  dy + page_content_rect.height());
   }
   NOTREACHED();
 }
@@ -174,10 +202,10 @@ gfx::Rect CanonicalInkEnvelopeToInvalidationScreenRect(
 
 gfx::AxisTransform2d GetCanonicalToPdfTransform(float page_height) {
   CHECK_GE(page_height, 0);
-  constexpr float kScreenToPageScale =
-      static_cast<float>(printing::kPointsPerInch) / printing::kPixelsPerInch;
   return gfx::AxisTransform2d::FromScaleAndTranslation(
-      {kScreenToPageScale, -kScreenToPageScale}, {0, page_height});
+      {kUnitConversionFactorPixelsToPoints,
+       -kUnitConversionFactorPixelsToPoints},
+      {0, page_height});
 }
 
 }  // namespace chrome_pdf

pdf/pdf_ink_transform.h

@@ -48,9 +48,7 @@ gfx::PointF EventPositionToCanonicalPosition(const gfx::PointF& event_position,
                                              float scale_factor);
 
 // Generates the affine transformation for rendering a page's strokes to the
-// screen, based on the page and its position within the viewport.  Parameters
-// are the same as for `EventPositionToCanonicalPosition()`, with the addition
-// of:
+// screen, based on the page and its position within the viewport.
 // - `viewport_origin_offset`:
 //     The offset within the rendering viewport to where the page images will
 //     be drawn.  Since the offset is a location within the viewport, it must
@@ -95,11 +93,19 @@ gfx::PointF EventPositionToCanonicalPosition(const gfx::PointF& event_position,
 //                       |             | |            +
 //                       +-------------+ +------------+
 //
+// - `orientation`:
+//     Same as for `EventPositionToCanonicalPosition()`.
+// - `page_content_rect`:
+//     Same as for `EventPositionToCanonicalPosition()`.
+// - `page_size_in_points`:
+//     The size of the page in points for the PDF document.  I.e., no scaling
+//     or orientation changes are applied to this size.
+//
 ink::AffineTransform GetInkRenderTransform(
     const gfx::Vector2dF& viewport_origin_offset,
     PageOrientation orientation,
     const gfx::Rect& page_content_rect,
-    float scale_factor);
+    const gfx::SizeF& page_size_in_points);
 
 // Returns the transform used when rendering a thumbnail on a canvas of
 // `canvas_size`, given the other parameters. Compared to

pdf/pdf_ink_transform_unittest.cc

@@ -9,6 +9,7 @@
 #include "pdf/page_orientation.h"
 #include "pdf/pdf_ink_conversions.h"
 #include "pdf/test/pdf_ink_test_helpers.h"
+#include "printing/units.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/ink/src/ink/geometry/affine_transform.h"
@@ -19,11 +20,13 @@
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/geometry/vector2d_f.h"
 
+using printing::kUnitConversionFactorPixelsToPoints;
+
 namespace chrome_pdf {
 
 namespace {
 
-// Standard page size for tests.
+// Standard page size in pixels for tests.
 constexpr gfx::Size kPageSizePortrait(50, 60);
 constexpr gfx::Size kPageSizePortrait2x(kPageSizePortrait.width() * 2,
                                         kPageSizePortrait.height() * 2);
@@ -32,8 +35,15 @@ constexpr gfx::Size kPageSizeLandscape(kPageSizePortrait.height(),
 constexpr gfx::Size kPageSizeLandscape2x(kPageSizeLandscape.width() * 2,
                                          kPageSizeLandscape.height() * 2);
 
-// A page size in points (199.9f, 201.1f) has fractional component, which gets
-// truncated when converted to integer pixels.
+// Standard page size in points for tests, corresponding to page size in pixels
+// above.
+constexpr gfx::SizeF kPageSizePortraitInPoints(
+    kPageSizePortrait.width() * kUnitConversionFactorPixelsToPoints,
+    kPageSizePortrait.height() * kUnitConversionFactorPixelsToPoints);
+
+// A page size in points `kPageSizePortraitFractionalInPoints` has fractional
+// component, which gets truncated when converted to integer pixels.
+constexpr gfx::SizeF kPageSizePortraitFractionalInPoints(199.9f, 201.1f);
 constexpr gfx::Size kPageSizePortraitFractional(266, 268);
 constexpr gfx::Size kPageSizeLandscapeFractional(268, 266);
 
@@ -217,7 +227,7 @@ TEST(PdfInkTransformTest,
 TEST(PdfInkTransformTest, RenderTransformIdentity) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kOriginal,
-      kPageContentAreaPortraitNoOffset, kScaleFactor1x);
+      kPageContentAreaPortraitNoOffset, kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
               InkAffineTransformEq(1.0f, 0.0f, 0.0f, 0.0f, 1.0f, 0.0f));
 }
@@ -225,7 +235,7 @@ TEST(PdfInkTransformTest, RenderTransformIdentity) {
 TEST(PdfInkTransformTest, RenderTransformZoom) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kOriginal,
-      kPageContentAreaPortraitNoOffset2x, kScaleFactor2x);
+      kPageContentAreaPortraitNoOffset2x, kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
               InkAffineTransformEq(2.0f, 0.0f, 0.0f, 0.0f, 2.0f, 0.0f));
 }
@@ -233,32 +243,32 @@ TEST(PdfInkTransformTest, RenderTransformZoom) {
 TEST(PdfInkTransformTest, RenderTransformRotateClockwise90) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise90,
-      kPageContentAreaLandscapeNoOffset, kScaleFactor1x);
+      kPageContentAreaLandscapeNoOffset, kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
-              InkAffineTransformEq(0.0f, -1.0f, 59.0f, 1.0f, 0.0f, 0.0f));
+              InkAffineTransformEq(0.0f, -1.0f, 60.0f, 1.0f, 0.0f, 0.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformRotateClockwise180) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise180,
-      kPageContentAreaPortraitNoOffset, kScaleFactor1x);
+      kPageContentAreaPortraitNoOffset, kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
-              InkAffineTransformEq(-1.0f, 0.0f, 49.0f, 0.0f, -1.0f, 59.0f));
+              InkAffineTransformEq(-1.0f, 0.0f, 50.0f, 0.0f, -1.0f, 60.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformRotateClockwise270) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise270,
-      kPageContentAreaLandscapeNoOffset, kScaleFactor1x);
+      kPageContentAreaLandscapeNoOffset, kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
-              InkAffineTransformEq(0.0f, 1.0f, 0.0f, -1.0f, 0.0f, 49.0f));
+              InkAffineTransformEq(0.0f, 1.0, 0.0f, -1.0f, 0.0f, 50.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformScrolled) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kOriginal,
       /*page_content_rect=*/gfx::Rect(gfx::Point(-8, -14), kPageSizePortrait),
-      kScaleFactor1x);
+      kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
               InkAffineTransformEq(1.0f, 0.0f, -8.0f, 0.0f, 1.0f, -14.0f));
 }
@@ -268,7 +278,7 @@ TEST(PdfInkTransformTest, RenderTransformOffsetScrolled) {
       /*viewport_origin_offset=*/gfx::Vector2dF(18.0f, 24.0f),
       PageOrientation::kOriginal,
       /*page_content_rect=*/gfx::Rect(gfx::Point(0, -14), kPageSizePortrait),
-      kScaleFactor1x);
+      kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
               InkAffineTransformEq(1.0f, 0.0f, 18.0f, 0.0f, 1.0f, 10.0f));
 }
@@ -277,9 +287,10 @@ TEST(PdfInkTransformTest, RenderTransformZoomScrolledClockwise90) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise90,
       /*page_content_rect=*/
-      gfx::Rect(gfx::Point(-16, -28), kPageSizeLandscape2x), kScaleFactor2x);
+      gfx::Rect(gfx::Point(-16, -28), kPageSizeLandscape2x),
+      kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
-              InkAffineTransformEq(0.0f, -2.0f, 103.0f, 2.0f, 0.0f, -28.0f));
+              InkAffineTransformEq(0.0f, -2.0, 104.0f, 2.0f, 0.0f, -28.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformOffsetZoomScrolledClockwise90) {
@@ -287,53 +298,45 @@ TEST(PdfInkTransformTest, RenderTransformOffsetZoomScrolledClockwise90) {
       /*viewport_origin_offset=*/gfx::Vector2dF(18.0f, 24.0f),
       PageOrientation::kClockwise90,
       /*page_content_rect=*/gfx::Rect(gfx::Point(0, -28), kPageSizeLandscape2x),
-      kScaleFactor2x);
+      kPageSizePortraitInPoints);
   EXPECT_THAT(transform,
-              InkAffineTransformEq(0.0f, -2.0f, 137.0f, 2.0f, 0.0f, -4.0f));
+              InkAffineTransformEq(0.0f, -2.0, 138.0f, 2.0f, 0.0f, -4.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformFractionalPointsSizeIdentity) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kOriginal,
-      gfx::Rect(gfx::Point(), kPageSizePortraitFractional), kScaleFactor1x);
-  // TODO(crbug.com/380038343): Update the transform values once
-  // `GetInkRenderTransform()` matches what is generated by PDFium's
-  // `CPDF_Page::GetDisplayMatrix()`.
-  EXPECT_THAT(transform,
-              InkAffineTransformEq(1.0f, 0.0f, 0.0f, 0.0f, 1.0f, 0.0f));
+      gfx::Rect(gfx::Point(), kPageSizePortraitFractional),
+      kPageSizePortraitFractionalInPoints);
+  EXPECT_THAT(transform, InkAffineTransformEq(0.997999012f, 0.0f, 0.0f, 0.0f,
+                                              0.999502718f, 0.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformFractionalPointsSizeClockwise90) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise90,
-      gfx::Rect(gfx::Point(), kPageSizeLandscapeFractional), kScaleFactor1x);
-  // TODO(crbug.com/380038343): Update the transform values once
-  // `GetInkRenderTransform()` matches what is generated by PDFium's
-  // `CPDF_Page::GetDisplayMatrix()`.
-  EXPECT_THAT(transform,
-              InkAffineTransformEq(0.0f, -1.0f, 267.0f, 1.0f, 0.0f, 0.0f));
+      gfx::Rect(gfx::Point(), kPageSizeLandscapeFractional),
+      kPageSizePortraitFractionalInPoints);
+  EXPECT_THAT(transform, InkAffineTransformEq(0.0f, -0.997999012f, 268.0f,
+                                              0.999502718f, 0.0f, 0.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformFractionalPointsSizeClockwise180) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise180,
-      gfx::Rect(gfx::Point(), kPageSizePortraitFractional), kScaleFactor1x);
-  // TODO(crbug.com/380038343): Update the transform values once
-  // `GetInkRenderTransform()` matches what is generated by PDFium's
-  // `CPDF_Page::GetDisplayMatrix()`.
-  EXPECT_THAT(transform,
-              InkAffineTransformEq(-1.0f, 0.0f, 265.0f, 0.0f, -1.0f, 267.0f));
+      gfx::Rect(gfx::Point(), kPageSizePortraitFractional),
+      kPageSizePortraitFractionalInPoints);
+  EXPECT_THAT(transform, InkAffineTransformEq(-0.997999012f, 0.0f, 266.0f, 0.0f,
+                                              -0.999502718f, 268.0f));
 }
 
 TEST(PdfInkTransformTest, RenderTransformFractionalPointsSizeClockwise270) {
   ink::AffineTransform transform = GetInkRenderTransform(
       kViewportOriginOffsetNone, PageOrientation::kClockwise270,
-      gfx::Rect(gfx::Point(), kPageSizeLandscapeFractional), kScaleFactor1x);
-  // TODO(crbug.com/380038343): Update the transform values once
-  // `GetInkRenderTransform()` matches what is generated by PDFium's
-  // `CPDF_Page::GetDisplayMatrix()`.
-  EXPECT_THAT(transform,
-              InkAffineTransformEq(0.0f, 1.0f, 0.0f, -1.0f, 0.0f, 265.0f));
+      gfx::Rect(gfx::Point(), kPageSizeLandscapeFractional),
+      kPageSizePortraitFractionalInPoints);
+  EXPECT_THAT(transform, InkAffineTransformEq(0.0f, 0.997999012f, 0.0f,
+                                              -0.999502718f, 0.0f, 266.0f));
 }
 
 TEST(PdfInkTransformTest, ThumbnailTransformNoZoom) {

pdf/pdf_view_web_plugin_unittest.cc

@@ -45,6 +45,7 @@
 #include "pdf/test/test_helpers.h"
 #include "pdf/test/test_pdfium_engine.h"
 #include "printing/metafile_skia.h"
+#include "printing/units.h"
 #include "services/network/public/mojom/referrer_policy.mojom-shared.h"
 #include "services/screen_ai/buildflags/buildflags.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -97,6 +98,8 @@
 #include "third_party/ink/src/ink/strokes/stroke.h"
 #endif
 
+using printing::kUnitConversionFactorPixelsToPoints;
+
 namespace chrome_pdf {
 
 namespace {
@@ -2964,9 +2967,14 @@ TEST_F(PdfViewWebPluginInkTest, AnnotationModeSetsFormAndClearsText) {
 TEST_F(PdfViewWebPluginInkTest, DrawInProgressStroke) {
   plugin_->set_in_paint_for_testing(true);
   constexpr gfx::Rect kScreenRect(kCanvasSize);
+  constexpr gfx::SizeF kPageSizeInPoints(
+      kCanvasSize.width() * kUnitConversionFactorPixelsToPoints,
+      kCanvasSize.height() * kUnitConversionFactorPixelsToPoints);
   ON_CALL(*engine_ptr_, GetPageContentsRect)
       .WillByDefault(
           [kScreenRect](int page_index) -> gfx::Rect { return kScreenRect; });
+  ON_CALL(*engine_ptr_, GetPageSizeInPoints)
+      .WillByDefault(Return(kPageSizeInPoints));
   ON_CALL(*engine_ptr_, GetThumbnailSize)
       .WillByDefault(Return(gfx::Size(50, 50)));
   ON_CALL(*engine_ptr_, IsPageVisible)