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

RESOLVED DUPLICATE of bug 1388951

Status

()

Core
Printing: Output
P3
normal
RESOLVED DUPLICATE of bug 1388951
9 months ago
8 months ago

People

(Reporter: fatseng, Assigned: fatseng)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 months ago
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.
(Assignee)

Updated

9 months ago
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
(Assignee)

Updated

9 months ago
Depends on: 1358076
(Assignee)

Updated

9 months ago
Priority: -- → P3
(Assignee)

Comment 1

9 months ago
Created attachment 8891267 [details] [diff] [review]
comparebitmap.patch (WIP)

Update a WIP patch.
This patch draws EMF to a bitmap and compares with reference one.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

9 months ago
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.
Attachment #8893264 - Flags: feedback?(brsun)
(Assignee)

Updated

9 months ago
Attachment #8891267 - Attachment is obsolete: true

Comment 4

9 months ago
mozreview-review
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.

Updated

9 months ago
Attachment #8893264 - Flags: feedback?(brsun)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8893264 - Flags: feedback?(brsun)
(Assignee)

Updated

9 months ago
Attachment #8893264 - Flags: feedback?(brsun)
(Assignee)

Comment 7

9 months ago
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.
(Assignee)

Comment 8

8 months ago
Since we already implemented emf contents comparison on Bug 1388951, we don't need to compare the bitmap.
(Assignee)

Updated

8 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1388951
You need to log in before you can comment on or make changes to this bug.