1321496 - Convert PDF to EMF in Windows

Status: RESOLVED DUPLICATE of bug 1370488

(Core :: Printing: Output, defect, P2)

Description

[Farmer Tseng[:fatseng](inactive)]

I would like converting PDF to emf by PDFium library.
The purpose is to deal with PDF printing in Windows.

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: part1 Implement a class to create EMF DC

Comment 2

[Farmer Tseng[:fatseng](inactive)]

Attachment: part2 load pdfium dll

Comment 3

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #2)
> Created attachment 8816366 [details] [diff] [review]
> part2 load pdfium dll

This is temporary. waiting Bruce to build dll/lib in gecko.

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: pdfium.dll

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Attachment: test-emf-printing.patch

This patch is just for testing.

Comment 6

[Bruce Sun [:brsun]]

Attachment: pdfium_dll.patch

This patch is needed for |PDFium| repo in order to build |pdfium.dll| on Windows.

Comment 7

[Farmer Tseng[:fatseng](inactive)]

Attachment: part1 Implement a class to create EMF DC

Comment 8

[Farmer Tseng[:fatseng](inactive)]

Attachment: part2 load pdfium dll

Comment 9

[Farmer Tseng[:fatseng](inactive)]

Attachment: Part3-Enable SkiaPDF, convert to EMF, then print

Comment 10

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #9)
> Created attachment 8819815 [details] [diff] [review]
> Part3-Enable SkiaPDF, convert to EMF, then print

I enabled SkiaPDF in Windows, but it generated a big size PDF for one page, around 57 MB.
Is something wrong for enabling SkiaPDF?

Comment 11

[Farmer Tseng[:fatseng](inactive)]

Attachment: PDF only include a picture.

PDF only include a picture.

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Attachment: PDF includes a picture and font.

Comment 13

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #10)
> (In reply to Farmer Tseng[:fatseng] from comment #9)
> > Created attachment 8819815 [details] [diff] [review]
> > Part3-Enable SkiaPDF, convert to EMF, then print
> 
> I enabled SkiaPDF in Windows, but it generated a big size PDF for one page,
> around 57 MB.
> Is something wrong for enabling SkiaPDF?

I did a experiment, found the problem cames from embeded font.
Please see attachments.("PDF only include a picture" and "PDF includes a picture and font")

Comment 14

[Farmer Tseng[:fatseng](inactive)]

I create another bug to track it.
Bug 1324739

Comment 15

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8820214 [details]
PDF includes a picture and font.

>https://drive.google.com/file/d/0ByDCMf4yvcW_QjZTUVJ0RDJYUEk/view?usp=sharing
File size is too large, please download file from link.

Comment 16

[Yury Delendik (:yury)]

Security question, will pdfium.dll or any PDFium code be directly loaded into content or parent process without sandboxing?

Comment 17

[Farmer Tseng[:fatseng](inactive)]

(In reply to Yury Delendik (:yury) from comment #16)
> Security question, will pdfium.dll or any PDFium code be directly loaded
> into content or parent process without sandboxing?

We created a bug to discuss it. Bug 1325032

Comment 18

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #13)
> (In reply to Farmer Tseng[:fatseng] from comment #10)
> > (In reply to Farmer Tseng[:fatseng] from comment #9)
> > > Created attachment 8819815 [details] [diff] [review]
> > > Part3-Enable SkiaPDF, convert to EMF, then print
> > 
> > I enabled SkiaPDF in Windows, but it generated a big size PDF for one page,
> > around 57 MB.
> > Is something wrong for enabling SkiaPDF?
> 
> I did a experiment, found the problem cames from embeded font.
> Please see attachments.("PDF only include a picture" and "PDF includes a
> picture and font")

The PDF file which only includes a picture is 121 KB.
The PDF file which includes a picture and font is 17 MB.

Comment 19

[Farmer Tseng[:fatseng](inactive)]

Attachment: part2 load pdfium dll

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Attachment: part3-Enable SkiaPDF, convert to EMF, then print

Add error handling.

Comment 21

[Bruce Sun [:brsun]]

Comment on attachment 8821998 [details] [diff] [review]
part2 load pdfium dll

Review of attachment 8821998 [details] [diff] [review]:
-----------------------------------------------------------------

Suggest not to expose function pointers directly in order to avoid redundant null-pointer checking at the caller side.

::: gfx/thebes/PDFEngineExports.cpp
@@ +32,5 @@
> +
> +bool PDFEngineExports::Init()
> +{
> +  if (!mLoadPDFdll) {
> +    mLoadPDFdll = LoadLibrary(L"C:\\pdfium.dll");

nit: Suppose this |if| block can end after this line, right?

@@ +33,5 @@
> +bool PDFEngineExports::Init()
> +{
> +  if (!mLoadPDFdll) {
> +    mLoadPDFdll = LoadLibrary(L"C:\\pdfium.dll");
> +    if (mLoadPDFdll) {

nit: Use early return instead of indenting the whole block.

::: gfx/thebes/PDFEngineExports.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef PDFIUMLIB_H
> +#define PDFIUMLIB_H

nit: The include guard doesn't match with the file name.

@@ +5,5 @@
> +
> +#ifndef PDFIUMLIB_H
> +#define PDFIUMLIB_H
> +
> +#include "nsISupports.h"                // for NS_INLINE_DECL_REFCOUNTING

Do we need to implement |PDFEngineExports| as a reference countable class? If any single instance of |PDFEngineExports| won't be shared among multiple owners, probably using |UniquePtr| to hold the pointer of |PDFEngineExports| would suffice.

@@ +105,5 @@
> +                                       int rotate,
> +                                       double page_x,
> +                                       double page_y,
> +                                       int* device_x,
> +                                       int* device_y);

Suggest to move all |FPDF_*| stuffs out from the header file.

@@ +132,5 @@
> +  FPDF_ClosePage_Pfn FPDF_ClosePage;
> +  FPDF_RenderPage_Pfn FPDF_RenderPage;
> +
> +  FPDF_RenderPage_Close_Pfn FPDF_RenderPage_Close;
> +  FPDF_PageToDevice_Pfn FPDF_PageToDevice;

Suggest to use public member functions to call these function pointers instead of exposing function pointers as public member variables.

Comment 22

[Bruce Sun [:brsun]]

Comment on attachment 8819811 [details] [diff] [review]
part1 Implement a class to create EMF DC

Review of attachment 8819811 [details] [diff] [review]:
-----------------------------------------------------------------

Suppose there is only one possible sequence to call individual methods of |Emf| to have a correct behavior, and each method of |Emf| is relative simple by wrapping Windows functions, and |Emf| might be used in only one place (|nsDeviceContextSpecWin| I guess), how about moving some logic out from |nsDeviceContextSpecWin| into it? For example, using |PDFPrintHelper| or something as the class name, and expose similar functions as |bool LoadFromFile(AString& aFileName)|, |bool LoadFromMemory(char* aBuffer, size_t aSize)|, |size_t GetPageCount()|, and |bool PrintPage(HDC aPrinterDC, size_t aPageIndex)| as public member methods only. So that we can have clearer lgic in |nsDeviceContextSpecWin|.

::: gfx/thebes/PrintEmfWindows.cpp
@@ +16,5 @@
> +}
> +
> +Emf::~Emf()
> +{
> +  Close();

Suggest to close |mDC_EMF| as well. Otherwise there would be resource leakage if |FinishDocument| hasn't been called.

::: gfx/thebes/PrintEmfWindows.h
@@ +30,5 @@
> +  bool Init();
> +
> +  // Generates a new metafile that will record every GDI command, and will
> +  // be saved to |metafile_path|.
> +  bool InitToFile(const wchar_t* metafile_path);

nit: Remove this function if we don't need it.

@@ +33,5 @@
> +  // be saved to |metafile_path|.
> +  bool InitToFile(const wchar_t* metafile_path);
> +
> +  // Initializes the Emf with the data in |metafile_path|.
> +  bool InitFromFile(const wchar_t* metafile_path);

nit: Remove this function if we don't need it.

@@ +40,5 @@
> +  bool FinishDocument();
> +
> +  HDC GetDC();
> +
> +  bool Playback(HDC hdc, const RECT* rect);

nit: The behavior of these functions (said |FinishDocument|, |GetDC|, |Playback|) is not so obvious by simply glancing on their declaration. For example, what's the relationship between the |HDC| returned from |GetDC| and the |HDC| used in |Playback|, why the returned value of |GetDC| becomes |nullptr| after |FinishDocument| has been called, etc.

Comment 23

[Bruce Sun [:brsun]]

Comment on attachment 8821999 [details] [diff] [review]
part3-Enable SkiaPDF, convert to EMF, then print

Review of attachment 8821999 [details] [diff] [review]:
-----------------------------------------------------------------

nit: Suggest to move the conversion logic into another helper class.

::: widget/windows/nsDeviceContextSpecWin.cpp
@@ +331,5 @@
>  {
>    // To match the previous printing code we need to return 72 when printing to
>    // PDF and 144 when printing to a Windows surface.
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  return 72.0f;

nit: Do we need to check |mPrintViaSkPDF| at runtime?

@@ +343,5 @@
>  {
>    MOZ_ASSERT(mPrintSettings);
>  
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  return 1.0f;

nit: Do we need to check |mPrintViaSkPDF| at runtime?

@@ +480,5 @@
>  
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +void
> +nsDeviceContextSpecWin::Printjob()
> +{

nit: Do we need to check |mPrintViaSkPDF| at runtime?

@@ +484,5 @@
> +{
> +  int page_width = 0;
> +  int page_height = 0;
> +  int dc_width = 0;
> +  int dc_height = 0;

nit: Defer the declaration of variables until they are really needed to be used.

@@ +501,5 @@
> +
> +    dc_width = GetDeviceCaps(metafile.GetDC(), HORZRES);
> +    dc_height = GetDeviceCaps(metafile.GetDC(), VERTRES);
> +
> +    float scale_factor = 0.0;

nit: how about saving one possible assignment by using |1.0| as the initial value?

::: widget/windows/nsDeviceContextSpecWin.h
@@ +83,5 @@
> +  RefPtr<mozilla::gfx::PDFEngineExports> mPDFEngine;
> +  mozilla::gfx::FPDF_DOCUMENT     mpDoc;
> +  int               mTotal_page_num;
> +  int               mPage_count;
> +  mozilla::UniquePtr<char[]> mPDFbuffer;

Suppose we can have fewer member variables if we move the conversion logic into another helper class.

Comment 24

[Louis Chang [:louis]]

Comment on attachment 8819811 [details] [diff] [review]
part1 Implement a class to create EMF DC

Review of attachment 8819811 [details] [diff] [review]:
-----------------------------------------------------------------

Please see the comments.

::: gfx/thebes/PrintEmfWindows.cpp
@@ +19,5 @@
> +{
> +  Close();
> +}
> +
> +bool Emf::Init()

nit: Please add a newline after the return type. For example:
bool
Emf::Init()
{
  ...
}
And also rest of the member functions with return type.

@@ +40,5 @@
> +
> +bool Emf::FinishDocument()
> +{
> +  if (mDC_EMF)
> +  {

nit: Please keep the brace at the end of first line. For example:
if (...) {
 ...
}

@@ +49,5 @@
> +}
> +
> +void Emf::Close()
> +{
> +  if (mEmf)

nit: Please add the brace even there is only a single statement.

::: gfx/thebes/PrintEmfWindows.h
@@ +51,5 @@
> +};
> +
> +} // namespace gfx
> +} // namespace mozilla
> +

nit: Please remove the trailing space

Comment 25

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8819811 [details] [diff] [review]
part1 Implement a class to create EMF DC

Review of attachment 8819811 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/PrintEmfWindows.h
@@ +30,5 @@
> +  bool Init();
> +
> +  // Generates a new metafile that will record every GDI command, and will
> +  // be saved to |metafile_path|.
> +  bool InitToFile(const wchar_t* metafile_path);

I would like to keep this for debug purpose.

@@ +33,5 @@
> +  // be saved to |metafile_path|.
> +  bool InitToFile(const wchar_t* metafile_path);
> +
> +  // Initializes the Emf with the data in |metafile_path|.
> +  bool InitFromFile(const wchar_t* metafile_path);

I would like to keep this for debug purpose.

Comment 26

[Farmer Tseng[:fatseng](inactive)]

Attachment: part1 Implement a class to create EMF DC

Comment 27

[Farmer Tseng[:fatseng](inactive)]

Attachment: part2 load pdfium dll

Comment 28

[Farmer Tseng[:fatseng](inactive)]

Attachment: part3-Enable SkiaPDF, convert to EMF, then print

I would base on this patch to move the conversion logic into another helper class

Comment 29

[Farmer Tseng[:fatseng](inactive)]

Attachment: part3-create a PDFPrintHelper

create a PDFPrintHelper to convert PDF to EMF

Comment 30

[Farmer Tseng[:fatseng](inactive)]

Attachment: part4-Enable SkiaPDF, convert to EMF, then print

Comment 31

[Jonathan Watt [:jwatt]]

(In reply to Farmer Tseng[:fatseng] from comment #10)
> I enabled SkiaPDF in Windows, but it generated a big size PDF for one page,
> around 57 MB.
> Is something wrong for enabling SkiaPDF?

Clearing old needinfo since we've been discussing this in bug 1325032.

Comment 32

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #31)
> (In reply to Farmer Tseng[:fatseng] from comment #10)
> > I enabled SkiaPDF in Windows, but it generated a big size PDF for one page,
> > around 57 MB.
> > Is something wrong for enabling SkiaPDF?
> 
> Clearing old needinfo since we've been discussing this in bug 1325032.

I think it should be typo, we've been discussing is bug 1324739.

Comment 33

[Jonathan Watt [:jwatt]]

(In reply to Farmer Tseng[:fatseng] from comment #32)
> I think it should be typo, we've been discussing is bug 1324739.

Indeed, thanks for the correction.

Comment 34

[Bruce Sun [:brsun]]

Comment on attachment 8824359 [details] [diff] [review]
part1 Implement a class to create EMF DC

clear flags due to solution changes