1382512 - [PDF gtest] convert EMF to bitmap and compare the bitmap content with reference

Status: RESOLVED DUPLICATE of bug 1388951

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

Description

[Farmer Tseng[:fatseng](inactive)]

I would like to implement a gtest to convert EMF to bitmap and compare the bitmap content with reference one to make sure EMF accuracy.

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: comparebitmap.patch (WIP)

Update a WIP patch.
This patch draws EMF to a bitmap and compares with reference one.

Comment 2

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1382512 - Convert the EMF to the bitmap and compare the bitmap content with reference

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

Comment 3

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8893264 [details]
Bug 1382512 - Convert the EMF to the bitmap and compare the bitmap content with reference

I implemented a gtest to convert the EMF to the bitmap and compare the bitmap content with reference one.

Comment 4

[Bruce Sun [:brsun]]

Comment on attachment 8893264 [details]
Bug 1382512 - Convert the EMF to the bitmap and compare the bitmap content with reference

https://reviewboard.mozilla.org/r/164302/#review169680

::: widget/windows/gtest/TestEMFConversion.cpp:143
(Diff revision 1)
>  
>    PDFHelper->CloseDocument();
>  }
>  
> +bool
> +CompareBitmaps(HDC aDC, HBITMAP aHBitmapLeft, HBITMAP aHBitmapRight)

Some suggestions:
 - The meaning of the return value of CompareXXX() might be ambiguous if readers don't dig into your implementation. Comparability is different from equality. Suggest to change the function name into IsEqualXXX() or something like that.
 - Using 'Left' and 'Right' on the variable name would mislead readers with unnecessary association (expecially we are referring to pixel data here). I think using dummy suffixes (ex. 'A', 'B' or '1', '2') would suffice.

::: widget/windows/gtest/TestEMFConversion.cpp:262
(Diff revision 1)
> +	::SelectObject(aDC, oldBrush);
> +  return true;
> +}
> +
> +bool
> +DrawEMFtoMemDC(nsAutoString aEMF, HDC aScreenDC, HDC aMemDC, HBITMAP& aBitmap)

Some suggestions:
 - Don't use the word 'Screen' as the parameter name since it won't ease the mental efforts of readers anyway. Or even better, don't pass HDC of the screen between functions by creating your memory DC within this funciton.
 - This function have one output parameter, but there are no hints from the function name or any comments. Suggest not to design a function with multiple functionalities. Simples functions would be easier to be maintained. How about directly get the bitmap contents from an EMF file?

::: widget/windows/gtest/TestEMFConversion.cpp:264
(Diff revision 1)
> +}
> +
> +bool
> +DrawEMFtoMemDC(nsAutoString aEMF, HDC aScreenDC, HDC aMemDC, HBITMAP& aBitmap)
> +{
> +	// Get the Handle from the enhanced metafile

There are many weird indentations. Please check your patch again.

::: widget/windows/gtest/TestEMFConversion.cpp:291
(Diff revision 1)
> +  RECT	rect;
> +	rect.top = static_cast<LONG>(emh.rclFrame.top * pixelsY / (mmY * 100.0f));
> +	rect.left = static_cast<LONG>(emh.rclFrame.left * pixelsX / (mmX * 100.0f));
> +	rect.right = static_cast<LONG>(emh.rclFrame.right * pixelsX / (mmX * 100.0f));
> +	rect.bottom = static_cast<LONG>(emh.rclFrame.bottom * pixelsY /
> +                                  (mmY * 100.0f));

These calculations are a little mystery to me. Could you explain more about the intention?

::: widget/windows/gtest/TestEMFConversion.cpp:301
(Diff revision 1)
> +
> +	// Populate the rect structure
> +	rect.left = 0;
> +	rect.top = 0;
> +	rect.right = width;
> +	rect.bottom = height;

Don't give one variable (i.e. rect) multiple meanings .

::: widget/windows/gtest/TestEMFConversion.cpp:316
(Diff revision 1)
> +	HBITMAP oldBitmap = reinterpret_cast<HBITMAP>(::SelectObject(aMemDC, aBitmap));
> +
> +	// Paint the background of the DC to White
> +	bool result = SetBackColorToWhite(aMemDC);
> +  if (!result) {
> +    ::DeleteEnhMetaFile(hEMF);

Suggest to use RAII pattern to ensure your resource will be released at every return path.

::: widget/windows/gtest/TestEMFConversion.cpp:374
(Diff revision 1)
> +
> +  ::DeleteObject(bitmap);
> +  ::DeleteObject(memDC);
> +  ::DeleteObject(bitmapRef);
> +  ::DeleteObject(memDCRef);
> +  ::ReleaseDC(NULL, screenDC);

Suggest to use the RAII pattern for easing the burden of releasing resources.

::: widget/windows/gtest/TestEMFConversion.cpp:422
(Diff revision 1)
> +                                      "PrinterTestPage_ref.emf",
> +                                      emfPathRef);
> +  ASSERT_TRUE(NS_SUCCEEDED(rv));
> +
> +  // Compare gtest.emf and PrinterTestPage_ref.emf by bitmap.
> +  ASSERT_TRUE(CompareEMFByBitmap(emfPath, emfPathRef));

Since checking whether two bitmaps are exactly the same or not (ex. memcmp() in your codes) is relatively simpler than comparing all GDI commands, I would suggest to have a new function which can generate bitmap content (ex. Vector<char> in your codes) from a EMF file, and then directly compare the content within this test case.

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8893264 [details]
Bug 1382512 - Convert the EMF to the bitmap and compare the bitmap content with reference

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

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8893264 [details]
Bug 1382512 - Convert the EMF to the bitmap and compare the bitmap content with reference

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

Comment 7

[Farmer Tseng[:fatseng](inactive)]

I found that the Windows API depends on current screen resolution to generate the EMF. It doesn't allow to compare the bitmap with pre-generated EMF and will be failed on try server.

Comment 8

[Farmer Tseng[:fatseng](inactive)]

Since we already implemented emf contents comparison on Bug 1388951, we don't need to compare the bitmap.