[PDF] Avoid rendering into a white bitmap for pages with transparencies

For certain PDFs, pdfium_test has no problem rendering them, while the
PDF Viewer does. Looking at the differences, when a page has
transparencies, pdfium_test fills the bitmap it renders into with
pixel value 0, while the PDF Viewer fills them with 0xFFFFFFFF.
Simply switching the PDF Viewer to match pdfium_test's fill color
results in weird smeared rendering, as this has been attempted several
times without success.

The key to fixing this is ask the PaintManager to draw the page's white
background first into the actual rendering surface, before drawing the
bitmap with the contents. Then everything renders more correctly, so
update PaintManagerTest to match the new behavior. The remaining issue
is premultiplied alpha handling.

It turns out the bitmap PDFium draws into had always been premultiplied
in the PDF Viewer, which is not right, considering PDFium assumes the
bitmap has straight alpha. However, this did not matter, because all the
pixels had 0xFF as the alpha value, so the bitmap was effectively
opaque. Now that the bitmap actually has alpha, the wrong bitmap type
causes color differences due to straight vs. premultiplied alpha
confusion. Fix this by marking the bitmap as un-premultiplied.

Considering some of this PDF Viewer code has been behaving the same way
for 10+ years, and the last change caused this bug, play it safe this
time with a kill switch. Preserve the old behavior in a fallback path,
while enabling the code path with the fix by default.

At the same time, use pdfium_test's drawing logic in
PDFiumPage::GenerateThumbnail() to fix the same issue with thumbnails.
Update tests with the new expectations.

Bug: 40216952
Change-Id: Ieadf2e76b9253351b74c5b7ccf21e74d3c4e3c05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5940452
Reviewed-by: Andy Phan <andyphan@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1370910}

pdf/paint_manager.cc

@@ -14,12 +14,14 @@
 #include "base/auto_reset.h"
 #include "base/check.h"
 #include "base/check_op.h"
+#include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/location.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
 #include "pdf/paint_ready_rect.h"
+#include "pdf/pdf_features.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkRect.h"
@@ -285,7 +287,18 @@ void PaintManager::DoPaint() {
   }
 
   for (const auto& ready_rect : ready_now) {
-    SkRect skia_rect = gfx::RectToSkRect(ready_rect.rect());
+    const SkRect skia_rect = gfx::RectToSkRect(ready_rect.rect());
+
+    if (base::FeatureList::IsEnabled(
+            features::kPdfPaintManagerDrawsBackground)) {
+      // Paint the page's white background, and then paint the page's contents.
+      // If `ready_rect.image()` has transparencies, this is necessary to paint
+      // over the stale data in `skia_rect` in `surface_`.
+      SkPaint paint;
+      paint.setColor(SK_ColorWHITE);
+      surface_->getCanvas()->drawRect(skia_rect, paint);
+    }
+
     surface_->getCanvas()->drawImageRect(
         ready_rect.image(), skia_rect, skia_rect, SkSamplingOptions(), nullptr,
         SkCanvas::kStrict_SrcRectConstraint);

pdf/paint_manager_unittest.cc

@@ -88,6 +88,28 @@ class PaintManagerTest : public testing::Test {
     return saved_snapshot;
   }
 
+  // Generates the expectations for use with TestPaintImage().
+  //
+  // - `plugin_size`: The expected image size.
+  // - `paint_rect`: Expected to be painted white.
+  // - `overlapped_rect`: Expected to be painted red.
+  //                      May paint over `paint_rect`.
+  SkBitmap GeneratePaintImageExpectation(const gfx::Size& plugin_size,
+                                         const gfx::Rect& paint_rect,
+                                         const gfx::Rect& overlapped_rect) {
+    sk_sp<SkSurface> surface =
+        CreateSkiaSurfaceForTesting(plugin_size, SK_ColorMAGENTA);
+    SkCanvas* canvas = surface->getCanvas();
+    canvas->clipIRect(gfx::RectToSkIRect(paint_rect));
+    canvas->clear(SK_ColorWHITE);
+    canvas->clipIRect(gfx::RectToSkIRect(overlapped_rect));
+    canvas->clear(SK_ColorRED);
+
+    SkBitmap bitmap;
+    EXPECT_TRUE(surface->makeImageSnapshot()->asLegacyBitmap(&bitmap));
+    return bitmap;
+  }
+
   void TestPaintImage(const gfx::Size& plugin_size,
                       const gfx::Size& source_size,
                       const gfx::Rect& paint_rect,
@@ -105,7 +127,7 @@ class PaintManagerTest : public testing::Test {
         /*fake_pending=*/{});
     ASSERT_TRUE(snapshot);
 
-    // Check if snapshot has `overlapped_rect` painted red.
+    // Check if `snapshot` matches `expected_bitmap`.
     snapshot = snapshot->makeSubset(
         static_cast<GrDirectContext*>(nullptr),
         SkIRect::MakeWH(plugin_size.width(), plugin_size.height()));
@@ -114,16 +136,8 @@ class PaintManagerTest : public testing::Test {
     SkBitmap snapshot_bitmap;
     ASSERT_TRUE(snapshot->asLegacyBitmap(&snapshot_bitmap));
 
-    sk_sp<SkSurface> expected_surface =
-        CreateSkiaSurfaceForTesting(plugin_size, SK_ColorMAGENTA);
-    expected_surface->getCanvas()->clipIRect(
-        gfx::RectToSkIRect(overlapped_rect));
-    expected_surface->getCanvas()->clear(SK_ColorRED);
-
-    SkBitmap expected_bitmap;
-    ASSERT_TRUE(expected_surface->makeImageSnapshot()->asLegacyBitmap(
-        &expected_bitmap));
-
+    SkBitmap expected_bitmap =
+        GeneratePaintImageExpectation(plugin_size, paint_rect, overlapped_rect);
     EXPECT_TRUE(cc::MatchesBitmap(snapshot_bitmap, expected_bitmap,
                                   cc::ExactPixelComparator()));
   }

pdf/pdf_features.cc

@@ -27,6 +27,12 @@ BASE_FEATURE(kPdfIncrementalLoading,
 
 BASE_FEATURE(kPdfOopif, "PdfOopif", base::FEATURE_DISABLED_BY_DEFAULT);
 
+// Kill switch in case this goes horribly wrong.
+// TODO(crbug.com/40216952): Remove after this lands safely in a Stable release.
+BASE_FEATURE(kPdfPaintManagerDrawsBackground,
+             "PdfPaintManagerDrawsBackground",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 // "Partial loading" refers to loading only specific parts of the PDF.
 // TODO(crbug.com/40123601): Remove this once partial loading is fixed.
 BASE_FEATURE(kPdfPartialLoading,

pdf/pdf_features.h

@@ -19,6 +19,7 @@ BASE_DECLARE_FEATURE(kAccessiblePDFForm);
 BASE_DECLARE_FEATURE(kPdfCr23);
 BASE_DECLARE_FEATURE(kPdfIncrementalLoading);
 BASE_DECLARE_FEATURE(kPdfOopif);
+BASE_DECLARE_FEATURE(kPdfPaintManagerDrawsBackground);
 BASE_DECLARE_FEATURE(kPdfPartialLoading);
 BASE_DECLARE_FEATURE(kPdfPortfolio);
 BASE_DECLARE_FEATURE(kPdfSearchify);

pdf/pdf_view_web_plugin.cc

@@ -2153,8 +2153,15 @@ void PdfViewWebPlugin::OnViewportChanged(
   const gfx::Size new_image_size =
       PaintManager::GetNewContextSize(old_image_size, plugin_rect_.size());
   if (new_image_size != old_image_size) {
-    image_data_.allocPixels(
-        SkImageInfo::MakeN32Premul(gfx::SizeToSkISize(new_image_size)));
+    SkAlphaType alpha_type;
+    if (base::FeatureList::IsEnabled(
+            features::kPdfPaintManagerDrawsBackground)) {
+      alpha_type = kUnpremul_SkAlphaType;
+    } else {
+      alpha_type = kPremul_SkAlphaType;
+    }
+    image_data_.allocPixels(SkImageInfo::MakeN32(
+        new_image_size.width(), new_image_size.height(), alpha_type));
     first_paint_ = true;
   }
 

pdf/pdfium/pdfium_engine.cc

@@ -3147,12 +3147,24 @@ bool PDFiumEngine::ContinuePaint(size_t progressive_index,
   const gfx::Rect& dirty = paint.rect();
   const gfx::Rect pdfium_rect = GetPDFiumRect(paint.page_index(), dirty);
 
-  bool has_alpha = !!FPDFPage_HasTransparency(page);
+  const bool has_alpha = !!FPDFPage_HasTransparency(page);
+  uint32_t fill_color;
+  if (base::FeatureList::IsEnabled(features::kPdfPaintManagerDrawsBackground)) {
+    // When `has_alpha` is true, use a transparent bitmap to render correctly.
+    // If the bitmap was painted white, then certain blend operations would
+    // blend into the white color and draw incorrectly. Instead, PaintManager
+    // will draw the white page under this bitmap.
+    fill_color = has_alpha ? 0x00000000 : 0xFFFFFFFF;
+  } else {
+    // In the old code, which is here as a fallback, `fill_color` is always
+    // white, as PaintManager does not draw the page's background.
+    fill_color = 0xFFFFFFFF;
+  }
   ScopedFPDFBitmap new_bitmap = CreateBitmap(dirty, has_alpha, image_data);
   FPDF_BITMAP new_bitmap_ptr = new_bitmap.get();
   paint.SetBitmapAndImageData(std::move(new_bitmap), image_data);
   FPDFBitmap_FillRect(new_bitmap_ptr, pdfium_rect.x(), pdfium_rect.y(),
-                      pdfium_rect.width(), pdfium_rect.height(), 0xFFFFFFFF);
+                      pdfium_rect.width(), pdfium_rect.height(), fill_color);
   return FPDF_RenderPageBitmap_Start(
              new_bitmap_ptr, page, pdfium_rect.x(), pdfium_rect.y(),
              pdfium_rect.width(), pdfium_rect.height(),

pdf/pdfium/pdfium_page.cc

@@ -6,6 +6,7 @@
 
 #include <math.h>
 #include <stddef.h>
+#include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ -13,6 +14,7 @@
 
 #include "base/check_op.h"
 #include "base/containers/to_vector.h"
+#include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/metrics/histogram_functions.h"
@@ -24,6 +26,7 @@
 #include "pdf/accessibility_helper.h"
 #include "pdf/accessibility_structs.h"
 #include "pdf/buildflags.h"
+#include "pdf/pdf_features.h"
 #include "pdf/pdfium/pdfium_api_string_buffer_adapter.h"
 #include "pdf/pdfium/pdfium_engine.h"
 #include "pdf/pdfium/pdfium_ocr.h"
@@ -1766,29 +1769,38 @@ void PDFiumPage::RequestThumbnail(float device_pixel_ratio,
 Thumbnail PDFiumPage::GenerateThumbnail(float device_pixel_ratio) {
   DCHECK(available());
 
+  FPDF_PAGE page = GetPage();
+  const bool has_alpha = !!FPDFPage_HasTransparency(page);
+  const int format = has_alpha ? FPDFBitmap_BGRA : FPDFBitmap_BGRx;
+  uint32_t fill_color;
+  if (base::FeatureList::IsEnabled(features::kPdfPaintManagerDrawsBackground)) {
+    fill_color = has_alpha ? 0x00000000 : 0xFFFFFFFF;
+  } else {
+    fill_color = 0xFFFFFFFF;
+  }
+
   Thumbnail thumbnail = GetThumbnail(device_pixel_ratio);
   const gfx::Size& image_size = thumbnail.image_size();
 
-  ScopedFPDFBitmap fpdf_bitmap(FPDFBitmap_CreateEx(
-      image_size.width(), image_size.height(), FPDFBitmap_BGRA,
-      thumbnail.GetImageData().data(), thumbnail.stride()));
+  // Create and initialize the bitmap.
+  ScopedFPDFBitmap fpdf_bitmap(
+      FPDFBitmap_CreateEx(image_size.width(), image_size.height(), format,
+                          thumbnail.GetImageData().data(), thumbnail.stride()));
 
-  // Clear the bitmap.
   FPDFBitmap_FillRect(fpdf_bitmap.get(), /*left=*/0, /*top=*/0,
-                      image_size.width(), image_size.height(),
-                      /*color=*/0xFFFFFFFF);
+                      image_size.width(), image_size.height(), fill_color);
 
   // The combination of the `FPDF_REVERSE_BYTE_ORDER` rendering flag and the
   // `FPDFBitmap_BGRA` format when initializing `fpdf_bitmap` results in an RGBA
   // rendering, which is the format required by HTML <canvas>.
   constexpr int kRenderingFlags = FPDF_ANNOT | FPDF_REVERSE_BYTE_ORDER;
-  FPDF_RenderPageBitmap(fpdf_bitmap.get(), GetPage(), /*start_x=*/0,
+  FPDF_RenderPageBitmap(fpdf_bitmap.get(), page, /*start_x=*/0,
                         /*start_y=*/0, image_size.width(), image_size.height(),
                         ToPDFiumRotation(PageOrientation::kOriginal),
                         kRenderingFlags);
 
   // Draw the forms.
-  FPDF_FFLDraw(engine_->form(), fpdf_bitmap.get(), GetPage(), /*start_x=*/0,
+  FPDF_FFLDraw(engine_->form(), fpdf_bitmap.get(), page, /*start_x=*/0,
                /*start_y=*/0, image_size.width(), image_size.height(),
                ToPDFiumRotation(PageOrientation::kOriginal), kRenderingFlags);
 

pdf/pdfium/pdfium_page_unittest.cc

@@ -1179,7 +1179,6 @@ TEST_P(PDFiumPageThumbnailTest, GenerateThumbnailForAnnotation) {
                         "signature_widget");
 }
 
-// TODO(crbug.com/40216952): The thumbnail should not be blank.
 TEST_P(PDFiumPageThumbnailTest, GenerateThumbnailWithTransparency) {
   TestClient client;
   std::unique_ptr<PDFiumEngine> engine =