Bug 119392 – [Qt] REGRESSION(r) Two pixel result fail after r153522

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zoltan Arvai]

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.

[Zoltan Arvai]

Created attachment 207916 [details]
image-rescale-actual

[Zoltan Arvai]

Created attachment 207917 [details]
image-small-width-height-actual

[Allan Sandfeld Jensen]

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]

Created attachment 208190 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 208196 [details]
Patch

Fix logic in checking for 2x downscale, which gave up too easy.

[Jocelyn Turcotte]

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]

(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]

Created attachment 208274 [details]
Patch

[Allan Sandfeld Jensen]

Committed r153790: <http://trac.webkit.org/changeset/153790>

[Jocelyn Turcotte]

Comment on attachment 208274 [details]
Patch

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