Bug 119392

Summary: [Qt] REGRESSION(r) Two pixel result fail after r153522
Product: WebKit Reporter: Zoltan Arvai <zarvai>
Component: Tools / TestsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: abrhm, allan.jensen, commit-queue, hausmann, jturcotte, kadam, noam, vsavchuk
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79666, 119263    
Attachments:
Description Flags
svg results
none
image-rescale-actual
none
image-small-width-height-actual
none
Patch
none
Patch
none
Patch none

Zoltan Arvai
Reported 2013-08-01 05:42:50 PDT
Created attachment 207915 [details] svg results There are many results that look much better :-) Those are updated in r153579. Also there are two test that seem to be fail after changing scale values: svg/custom/image-rescale.svg svg/custom/image-small-width-height.svg Results are attached to the bug.
Attachments
svg results (441.22 KB, application/x-gzip)
2013-08-01 05:42 PDT, Zoltan Arvai 		no flags
Details
image-rescale-actual (151.88 KB, image/png)
2013-08-01 05:43 PDT, Zoltan Arvai 		no flags
Details
image-small-width-height-actual (42.31 KB, image/png)
2013-08-01 05:44 PDT, Zoltan Arvai 		no flags
Details
Patch (5.92 KB, patch)
2013-08-06 07:41 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (5.88 KB, patch)
2013-08-06 09:15 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (5.91 KB, patch)
2013-08-07 09:32 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Zoltan Arvai
Comment 1 2013-08-01 05:43:50 PDT
Created attachment 207916 [details] image-rescale-actual
Zoltan Arvai
Comment 2 2013-08-01 05:44:23 PDT
Created attachment 207917 [details] image-small-width-height-actual
Allan Sandfeld Jensen
Comment 3 2013-08-06 06:30:07 PDT
Yes, I am assuming this is caused by a QPainter with an upscale transform applied. In this case we end up downscaling and then upscaling again.
Allan Sandfeld Jensen
Comment 4 2013-08-06 07:41:31 PDT
Created attachment 208190 [details] Patch
Allan Sandfeld Jensen
Comment 5 2013-08-06 09:15:40 PDT
Created attachment 208196 [details] Patch Fix logic in checking for 2x downscale, which gave up too easy.
Jocelyn Turcotte
Comment 6 2013-08-07 07:31:03 PDT
Comment on attachment 208196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208196&action=review lgtm, some nitpicks but r=me anyway if you want to land directly. > Source/WebCore/platform/graphics/qt/ImageQt.cpp:247 > + if (transform.type() > QTransform::TxScale) Nit: This breaks QTransform's abstraction by assuming details from the implementation and requires opening qtransform.h to understand. Straight & and | on the unsupported flags would be clearer. > Source/WebCore/platform/graphics/qt/ImageQt.cpp:303 > + image = prescaleImageIfRequired(ctxt->platformContext(), image, &prescaledBuffer, normalizedDst, normalizedSrc); Nit: Passing normalizedSrc as a QRectF* would make it clear from the call site that there might be side effects on it.
Allan Sandfeld Jensen
Comment 7 2013-08-07 07:55:27 PDT
(In reply to comment #6) > (From update of attachment 208196 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208196&action=review > > lgtm, some nitpicks but r=me anyway if you want to land directly. > > > Source/WebCore/platform/graphics/qt/ImageQt.cpp:247 > > + if (transform.type() > QTransform::TxScale) > > Nit: This breaks QTransform's abstraction by assuming details from the implementation and requires opening qtransform.h to understand. Straight & and | on the unsupported flags would be clearer. > That is how QTransform::type() is documented. It is not a set of flags, it just returns the highest included transform. > > Source/WebCore/platform/graphics/qt/ImageQt.cpp:303 > > + image = prescaleImageIfRequired(ctxt->platformContext(), image, &prescaledBuffer, normalizedDst, normalizedSrc); > > Nit: Passing normalizedSrc as a QRectF* would make it clear from the call site that there might be side effects on it. Okay. I can change that.
Allan Sandfeld Jensen
Comment 8 2013-08-07 09:32:30 PDT
Created attachment 208274 [details] Patch
Allan Sandfeld Jensen
Comment 9 2013-08-07 09:33:11 PDT
Committed r153790: <http://trac.webkit.org/changeset/153790>
Jocelyn Turcotte
Comment 10 2013-08-08 01:53:28 PDT
Comment on attachment 208274 [details] Patch Removing the flag (I guess you didn't want r? here)
Note You need to log in before you can comment on or make changes to this bug.