Print Preview: Add option to rasterize PDFs and add JPEG compression.

pdf/pdfium/pdfium_engine.cc

Index: pdf/pdfium/pdfium_engine.cc
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index 35c28964955714d9612abb991075277b904480e0..e907e3b85a7168a45ea96701ca70e18e7f597d9c 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -57,6 +57,7 @@
 #include "third_party/pdfium/public/fpdf_transformpage.h"
 #include "third_party/pdfium/public/fpdfview.h"
 #include "ui/events/keycodes/keyboard_codes.h"
+#include "ui/gfx/codec/jpeg_codec.h"
 #include "ui/gfx/geometry/rect.h"
 #include "v8/include/v8.h"
 
@@ -1355,7 +1356,9 @@ bool PDFiumEngine::HandleEvent(const pp::InputEvent& event) {
 uint32_t PDFiumEngine::QuerySupportedPrintOutputFormats() {
   if (!HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))

[Tom Sepez, 2016/12/14 20:32:54]

nit: 
  Can a document have high_quality but not low_quality?  If so,
  can we assemble the permissions along the lines of

  uint32_t result = 0;
  if (HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
    result |= PP_PRINTOUTPUTFORMAT_RASTER;
  if (HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY))
    result |= PP_PRINTOUTPUTFORMAT_PDF;
  return result;

which reads clearer to me -- and double check that I've got the right
permissions paired up above as I'm confused reading it.

[rbpotter, 2016/12/15 00:50:59]

Based on the logic in HasPermission, if a document has high quality it will
always also have low quality. I changed this anyway to make it more readable -
let me know if it is any better now.

[Tom Sepez, 2016/12/15 21:09:23]

Lots better, thanks.

     return 0;
-  return PP_PRINTOUTPUTFORMAT_PDF;
+  if (!HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY))
+    return PP_PRINTOUTPUTFORMAT_RASTER;
+  return PP_PRINTOUTPUTFORMAT_PDF | PP_PRINTOUTPUTFORMAT_RASTER;
 }
 
 void PDFiumEngine::PrintBegin() {
@@ -1366,13 +1369,32 @@ pp::Resource PDFiumEngine::PrintPages(
     const PP_PrintPageNumberRange_Dev* page_ranges, uint32_t page_range_count,
     const PP_PrintSettings_Dev& print_settings) {
   ScopedSubstFont scoped_subst_font(this);
-  if (HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY))
+  if (HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY) &&
+      (print_settings.format & PP_PRINTOUTPUTFORMAT_PDF)) {
     return PrintPagesAsPDF(page_ranges, page_range_count, print_settings);
-  else if (HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
+  } else if (HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY)) {
     return PrintPagesAsRasterPDF(page_ranges, page_range_count, print_settings);
+  }
+
   return pp::Resource();
 }
 
+struct simple_buf {
+  uint32_t size;
+  uint8_t *data;

[Tom Sepez, 2016/12/14 20:32:54]

nit: uint8_t* data; per style guide (several places, e.g. 1391, 1437).

[rbpotter, 2016/12/15 00:50:58]

Done.

+};
+
+int GetBlockForJpeg(void *param,

[Tom Sepez, 2016/12/14 20:32:54]

the return type should probably be unsigned long to match |size|.

[rbpotter, 2016/12/15 00:50:59]

Unfortunately the signature is determined by the FPDF_FILEACCESS structure, so
it has to return an int. However it just needs to return something non-zero for
success, so I changed this to 1 to prevent the unsigned long to int cast.

+                    unsigned long pos,
+                    unsigned char* pBuf,

[Tom Sepez, 2016/12/14 20:32:55]

nit: uint8_t consistently throughout, hungarian notation only in pdfium itself
(eg. buf, not pBuf here), also this is probably const uint8_t* buf;

[rbpotter, 2016/12/15 00:50:59]

The signature for this function is defined by the FPDF_FILEACCESS structure as
using unsigned char* (and unsigned long). It will compile at least on Linux with
uint8_t* and size_t, but not sure if this is guaranteed to compile on all
platforms this way. Should I stick to the function signature from PDFium or
change it to be consistent with other variables?

+                    unsigned long size) {
+  simple_buf * buf_p = static_cast<simple_buf *>(param);
+  if (pos + size < pos || pos + size > buf_p->size)

[dsinclair, 2016/12/14 19:54:38]

The pos + size < pos seems like it would always be false? Should the second pos
be related to the buffer?

[Tom Sepez, 2016/12/14 20:32:54]

This is checking for overflow, and is allowed in this manner so long as the
types are unsigned, which they are.

+    return 0;
+  memcpy(pBuf, buf_p->data, size);

[dsinclair, 2016/12/14 19:54:38]

Should this be copying from buf_p->data + pos? Won't this always copy from the
beginning of the buffer?

[dsinclair, 2016/12/14 19:54:38]

Is it safe to assume pBuf is always big enough here?

[rbpotter, 2016/12/15 00:50:58]

Since the buffer is provided by the caller I don't think there is any way to
check from here; the assumption for the FPDF_FILEACCESS seems to be that the
caller will ensure the size of the buffer is at least equal to size.

[rbpotter, 2016/12/15 00:50:58]

Done.

[Tom Sepez, 2016/12/15 21:09:23]

I think this is still an issue.

[rbpotter, 2016/12/16 00:06:31]

This is now copying size bytes from buf_p->data.size + pos, so an offset of pos
bytes into the "file", to the user provided buffer. I believe this is correct
based on the description of this function in the definition of FPDF_FILEACCESS,
but let me know if I am missing something.

+  return size;
+}

[dsinclair, 2016/12/14 19:54:38]

Move up into the anonymous namespace.

[rbpotter, 2016/12/15 00:50:59]

Done.

+
 FPDF_DOCUMENT PDFiumEngine::CreateSinglePageRasterPdf(
     double source_page_width,
     double source_page_height,
@@ -1412,6 +1434,8 @@ FPDF_DOCUMENT PDFiumEngine::CreateSinglePageRasterPdf(
                         print_settings.orientation,
                         FPDF_ANNOT | FPDF_PRINTING | FPDF_NO_CATCH);
 
+  unsigned char * bitmap_data =
+      static_cast<unsigned char *>(FPDFBitmap_GetBuffer(bitmap));
   double ratio_x = ConvertUnitDouble(bitmap_size.width(),
                                      print_settings.dpi,
                                      kPointsPerInch);
@@ -1422,7 +1446,36 @@ FPDF_DOCUMENT PDFiumEngine::CreateSinglePageRasterPdf(
   // Add the bitmap to an image object and add the image object to the output
   // page.
   FPDF_PAGEOBJECT temp_img = FPDFPageObj_NewImgeObj(temp_doc);
-  FPDFImageObj_SetBitmap(&temp_page, 1, temp_img, bitmap);
+
+  std::vector <unsigned char> compressed_bitmap_data;

[Tom Sepez, 2016/12/14 20:32:54]

nit: std::vector<uint8_t>

Did you "git cl format" this?  I thought format would take care of these style
issues.

[rbpotter, 2016/12/15 00:50:58]

Done.

+  int quality = 40;
+  bool encode_success = gfx::JPEGCodec::Encode(bitmap_data,

[dsinclair, 2016/12/14 19:54:38]

Do we always want to do the JPEG encode, or only when the user selected the UI
option?

[Tom Sepez, 2016/12/14 20:32:54]

nit: local not needed

[rbpotter, 2016/12/15 00:50:58]

Not sure. The JPEG encode saves space for big/image heavy documents. However, it
is probably safer to keep the existing behavior unless the UI option is
selected, as a JPEG image printout will probably be expected in that case, while
it might not be if the document permissions are what is preventing a standard
PDF print. So I have changed it to only encode when the UI option is selected,
thanks for the suggestion.

+                                               gfx::JPEGCodec::FORMAT_BGRA,
+                                               FPDFBitmap_GetWidth(bitmap),
+                                               FPDFBitmap_GetHeight(bitmap),
+                                               FPDFBitmap_GetStride(bitmap),
+                                               quality,
+                                               &compressed_bitmap_data);
+
+  if (encode_success) {
+    simple_buf buffer;
+    buffer.size = compressed_bitmap_data.size();

[Tom Sepez, 2016/12/14 20:32:54]

beware, this truncates a size_t to an uint32_t.  You might want to make the
lenghts size_t throughout.

[rbpotter, 2016/12/15 00:50:58]

Done.

+    buffer.data = new uint8_t[buffer.size];

[Tom Sepez, 2016/12/14 20:36:28]

actually, we can set buffer.data to compressed_bitmap_data.data(), since vector
is guaranteed to be contiguous and avoid the alloc/copy/free.

[rbpotter, 2016/12/15 00:50:58]

Done.

+    memcpy(buffer.data, &(compressed_bitmap_data[0]), buffer.size);
+
+    FPDF_FILEACCESS file_access;
+    memset(&file_access, '\0', sizeof(file_access));
+    file_access.m_FileLen =
+        static_cast<unsigned long>(compressed_bitmap_data.size());
+    file_access.m_GetBlock = &GetBlockForJpeg;
+    file_access.m_Param = &buffer;
+
+    FPDFImageObj_LoadJpegFileInline(&temp_page, 1, temp_img, &file_access);
+    delete buffer.data;

[Tom Sepez, 2016/12/14 20:32:54]

delete[] buffer.data?

[rbpotter, 2016/12/15 00:50:59]

N/A

+  } else {
+    FPDFImageObj_SetBitmap(&temp_page, 1, temp_img, bitmap);
+  }
+
   FPDFImageObj_SetMatrix(temp_img, ratio_x, 0, 0, ratio_y, 0, 0);
   FPDFPage_InsertObject(temp_page, temp_img);
   FPDFPage_GenerateContent(temp_page);