1388951 - [PDF gtest] Compare EMF contents with reference

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement, P3)

Description

[Farmer Tseng[:fatseng](inactive)]

I would like to build a dynamic library of pdfium from Chromium, say pdfium_ref.dll. Use the pdfium_ref.dll to convert PrinterTestPage.pdf to an EMF as a reference.
In another hand, use the pdfium.dll which is built from gecko to convert PrinterTestPage.pdf to anothere EMF. Then, compare this EMF with reference one.

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: pdfium.def

Comment 2

[Farmer Tseng[:fatseng](inactive)]

Attachment: buildpdfium.patch

Comment 3

[Farmer Tseng[:fatseng](inactive)]

I write down the steps to show how I build pdfium.dll from google source.

1. mkdir src
2. cd src
3. gclient config --unmanaged https://pdfium.googlesource.com/pdfium.git
4. gclient sync
5. cd pdfium
6. git checkout 84213b529908d2b9095ad4c33ecc9fdf5d881df5
7. cd ..
8. gclient sync
9. gn gen out\Default
10. gn args  out\Default
  Add following to args.gn:
    pdf_is_standalone = true
    is_component_build = false
    is_official_build = true
    is_debug = false
    pdf_enable_xfa = false
    pdf_enable_v8 = false
    target_cpu = "x86"
  or
    target_cpu = "x64"

11. add pdfium.def(attachment 8896128 [details]) to pdfium folder
12. apply attached patch(buildpdfium.patch)
13. ninja -C out\Default pdfium

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

Review commit: https://reviewboard.mozilla.org/r/167428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167428/

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1388951 - Implement a gtest to compare the EMF contents with the reference.

Built pdfium_ref_x86.dll and pdfium_ref_x64.dll from google source tree. Use them to generate an EMF as the reference.

Review commit: https://reviewboard.mozilla.org/r/167430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167430/

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

Hi Bruce, Could you please help feedback my patches?

Comment 7

[Bruce Sun [:brsun]]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

https://reviewboard.mozilla.org/r/167428/#review173278

::: widget/windows/PDFViaEMFPrintHelper.h:33
(Diff revision 1)
>   */
>  class PDFViaEMFPrintHelper
>  {
>  public:
>    PDFViaEMFPrintHelper();
> +  explicit PDFViaEMFPrintHelper(const nsACString& aExternalDLL);

I would suggest not to let PDFViaEMFPrintHelper to choose DLL. It is not very clear what would happen if there are multiple existing PDFViaEMFPrintHelper instances specify different DLLs at the same time.

::: widget/windows/PDFiumEngineShim.cpp:16
(Diff revision 1)
>  
>  static PDFiumEngineShim* sPDFiumEngineShim;
>  
>  /* static */
>  already_AddRefed<PDFiumEngineShim>
> -PDFiumEngineShim::GetInstanceOrNull()
> +PDFiumEngineShim::GetInstanceOrNull(const char* aExternalDLL)

What would be the expected behavior if a different DLL is specified when the singleton PDFiumEngineShim has been created already?

Comment 8

[Bruce Sun [:brsun]]

Comment on attachment 8896142 [details]
Bug 1388951 - Implement a gtest to compare the EMF contents with the reference.

https://reviewboard.mozilla.org/r/167430/#review173292

::: widget/windows/gtest/TestEMFConversion.cpp:136
(Diff revision 1)
>  
> -  // Check the file size of EMF and make sure it is greater than zero
> -  RefPtr<nsLocalFile> savedFile = new nsLocalFile();
> -  savedFile->InitWithPath(emfPath);
> -  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream();
> -  rv = inputStream->Init(savedFile,
> +  Vector<char> emfContents;
> +  uint32_t size = GetEMFContents(emfPath, emfContents);
> +  ASSERT_GT(size, static_cast<uint32_t>(0));
> +
> +  PDFHelper = nullptr;

I think this line can reset the singleton PDFiumEngineShim, but I am afraid that it would be too mystery for future maintainers. Suggest to add more explainations for this line to reveal your intention.

Comment 9

[Bruce Sun [:brsun]]

Comment on attachment 8896142 [details]
Bug 1388951 - Implement a gtest to compare the EMF contents with the reference.

How about using functions from your reference dll directly? So that you don't need to hack your original design.

Comment 10

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167428/diff/1-2/

Comment 11

[Bruce Sun [:brsun]]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

https://reviewboard.mozilla.org/r/167428/#review173892

::: widget/windows/gtest/TestEMFConversion.cpp:92
(Diff revision 2)
>  
>    return PDFPrintHelper;
>  }
>  
> -// Convert PrinterTestPage.pdf to an EMF file
> -TEST(TestEMFConversion, SaveEMFtoFile)
> +uint32_t
> +GetEMFContents(const nsAutoString aEMFFile, Vector<char>& aBuf)

Suggest to remove 'EMF' related words from the naming of this function because this function doesn't do anything special to EMF stuffs.

::: widget/windows/gtest/TestEMFConversion.cpp:297
(Diff revision 2)
> +  int dcWidth = ::GetDeviceCaps(aDC, HORZRES);
> +  int dcHeight = ::GetDeviceCaps(aDC, VERTRES);
> +  float scaleFactor = ComputeScaleFactor(dcWidth, dcHeight,
> +                                         aPageWidth, aPageHeight);
> +  if (scaleFactor <= 0.0) {
> +    return false;

Is it ok to return here without calling cleanup functions like 'mFPDF_ClosePage' and 'RestoreDC'? Please change accordingly if they are needed.

::: widget/windows/gtest/TestEMFConversion.cpp:361
(Diff revision 2)
> -  // Check the file size of EMF and make sure it is greater than zero
> -  RefPtr<nsLocalFile> savedFile = new nsLocalFile();
> -  savedFile->InitWithPath(emfPath);
> -  RefPtr<nsFileInputStream> inputStream = new nsFileInputStream();
> -  rv = inputStream->Init(savedFile,
> -                         /* ioFlags */ PR_RDONLY,
> +  Vector<char> emfContents;
> +  uint32_t size = GetEMFContents(emfPath, emfContents);
> +  ASSERT_GT(size, static_cast<uint32_t>(0));
> +
> +
> +  // Convert a PDF file to an EMF file by external library. 

nit: trim trailing spaces

::: widget/windows/gtest/TestEMFConversion.cpp:383
(Diff revision 2)
> +  result = ExtHelper->DrawPageToFile(emfPathRef.get(), 0,
> +                                     pageWidth, pageHeight);
> +  ASSERT_TRUE(result);
> +
> +  ExtHelper->CloseDocument();
> +  

nit: trim empty spaces.

::: widget/windows/gtest/TestEMFConversion.cpp:386
(Diff revision 2)
> +
> +  ExtHelper->CloseDocument();
> +  
> +  Vector<char> emfContentsRef;
> +  uint32_t sizeRef = GetEMFContents(emfPathRef, emfContentsRef);
> +  ASSERT_GT(size, static_cast<uint32_t>(0));

I guess you want to check 'sizeRef' instead of 'size' here, right?

Comment 12

[Bruce Sun [:brsun]]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

f+ if comments are addressed.

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167428/diff/2-3/

Comment 14

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

Hi Jwatt, please help review my patch.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Farmer Tseng[:fatseng] from comment #0)
> I would like to build a dynamic library of pdfium from Chromium, say
> pdfium_ref.dll. Use the pdfium_ref.dll to convert PrinterTestPage.pdf to an
> EMF as a reference.
> In another hand, use the pdfium.dll which is built from gecko to convert
> PrinterTestPage.pdf to anothere EMF. Then, compare this EMF with reference
> one.

Why not generate the EMF reference files and commit those, instead of committing the DLLs that generate them? Would we need more than two variants of the EMF reference files? If so, why is that?

Comment 16

[Farmer Tseng[:fatseng](inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #15)
> (In reply to Farmer Tseng[:fatseng] from comment #0)
> > I would like to build a dynamic library of pdfium from Chromium, say
> > pdfium_ref.dll. Use the pdfium_ref.dll to convert PrinterTestPage.pdf to an
> > EMF as a reference.
> > In another hand, use the pdfium.dll which is built from gecko to convert
> > PrinterTestPage.pdf to anothere EMF. Then, compare this EMF with reference
> > one.
> 
> Why not generate the EMF reference files and commit those, instead of
> committing the DLLs that generate them? Would we need more than two variants
> of the EMF reference files? If so, why is that?

I found that the Windows APIs use the current screen resolution to generate the EMF.
The EMF file also includes the information of screen resolution.
It will be failed on try server if we compare the reference which is generated from my NB.

Comment 17

[Jonathan Watt [:jwatt]]

I see. :/ Do you know how many versions of the EMF file we would need?

I'm not entirely convinced this test is useful enough to warrant adding two DLLs of not insignificant size into the repository. Essentially the test is having two versions of PDFium convert a PDF file to EMF, and testing that the output from the PDFium built from our tree is the same as the output from the PDFium built from the Chromium tree. Since our PDFium comes from the Chromium tree, and since the API calls are so limited (we're barely doing more than just passing a PDF file), it seems like we're not really testing much of our code (we're more just testing that the build options we use to build our version of PDFium aren't resulting in broken output).

I assume that we're going to be adding tests to check that the EMF output when printing various webpages is "correct" to a greater or lesser extent. Those tests would test that our PDFium build works, that our API interactions are correct, *and* test more of our printing pipeline. At that point I think this test would be redundant. What would you think of just relying on those tests so we can avoid adding these DLLs?

Comment 18

[Jonathan Watt [:jwatt]]

Farmer made the very good point on IRC that we disable various PDFium modules and swap out some of the libraries that it uses in favor of our own. It seems we also aren't going to have printing reftests that test the EMF codepaths any time soon. So on balance I think we should add this test.

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

https://reviewboard.mozilla.org/r/167428/#review174934

::: commit-message-6f52f:1
(Diff revision 3)
> +Bug 1388951 - Implement a gtest to compare the EMF contents with the reference.

Please make the commit comment:

Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's. r=jwatt

The patches in modules/pdfium/patches disable various PDFium options and swap
out some PDFium components for our own.  This test primarily checks that our
modified version of PDFium still produces the same EMF output for a printing
test page as the Chromium version of PDFium.  The files pdfium_ref_x86.dll and
pdfium_ref_x64.dll were built from the Chromium source tree using the following
steps:

1. mkdir src
2. cd src
3. gclient config --unmanaged https://pdfium.googlesource.com/pdfium.git
4. gclient sync
5. cd pdfium
6. git checkout 84213b529908d2b9095ad4c33ecc9fdf5d881df5
7. cd ..
8. gclient sync
9. gn gen out\Default
10. gn args  out\Default
  Add following to args.gn:
    pdf_is_standalone = true
    is_component_build = false
    is_official_build = true
    is_debug = false
    pdf_enable_xfa = false
    pdf_enable_v8 = false
    target_cpu = "x86"
  or
    target_cpu = "x64"
11. add pdfium.def (bugzilla attachment 8896128 [details]) to the pdfium folder
12. apply the buildpdfium.patch in bug 1388951
13. ninja -C out\Default pdfium

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8896141 [details]
Bug 1388951 - Add a gtest to compare the EMF output from our version of PDFium, to Chromium's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167428/diff/3-4/

Comment 21

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c256bf68cc9c5b777aab74dbf71f81e2e8f0dbb4

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/265131deb98b
Add a gtest to compare the EMF output from our version of PDFium, to Chromium's. r=jwatt

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/265131deb98b