Bug 119392 - [Qt] REGRESSION(r) Two pixel result fail after r153522
Summary: [Qt] REGRESSION(r) Two pixel result fail after r153522
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords: Qt
Depends on:
Blocks: 79666 119263
  Show dependency treegraph
 
Reported: 2013-08-01 05:42 PDT by Zoltan Arvai
Modified: 2013-08-08 01:53 PDT (History)
8 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (5.88 KB, patch)
2013-08-06 09:15 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2013-08-07 09:32 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Arvai 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.
Comment 1 Zoltan Arvai 2013-08-01 05:43:50 PDT
Created attachment 207916 [details]
image-rescale-actual
Comment 2 Zoltan Arvai 2013-08-01 05:44:23 PDT
Created attachment 207917 [details]
image-small-width-height-actual
Comment 3 Allan Sandfeld Jensen 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.
Comment 4 Allan Sandfeld Jensen 2013-08-06 07:41:31 PDT
Created attachment 208190 [details]
Patch
Comment 5 Allan Sandfeld Jensen 2013-08-06 09:15:40 PDT
Created attachment 208196 [details]
Patch

Fix logic in checking for 2x downscale, which gave up too easy.
Comment 6 Jocelyn Turcotte 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.
Comment 7 Allan Sandfeld Jensen 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.
Comment 8 Allan Sandfeld Jensen 2013-08-07 09:32:30 PDT
Created attachment 208274 [details]
Patch
Comment 9 Allan Sandfeld Jensen 2013-08-07 09:33:11 PDT
Committed r153790: <http://trac.webkit.org/changeset/153790>
Comment 10 Jocelyn Turcotte 2013-08-08 01:53:28 PDT
Comment on attachment 208274 [details]
Patch

Removing the flag (I guess you didn't want r? here)