Bug 50422 – [Qt] Canvas shadow offset should not be affected by any transformation

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Helder Correia]

Currently, shadows are affected by all transformations except scaling.  The correct behavior is not being affected by any kind of transformation, and keep the same offset in relation to the shape/image.  NOTE: this is the correct behavior when operating on a canvas, but not with box shadows (where offsets must follow the transformation matrix).

[Helder Correia]

Created attachment 75432 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 75432 [details]
Patch

Why does this qt-only change affect shared layout test results?

[Helder Correia]

(In reply to comment #2)
> (From update of attachment 75432 [details])
> Why does this qt-only change affect shared layout test results?

I'm assuming you're referring to canvas-strokePath. I wrote that test very recently. I'm changing it slightly now it by using a lineWidth > 1 to make it simultaneously easier to test and more fair among all ports, since we can be dealing with different transformation smoothness/aliasing settings in the different ports.

[Helder Correia]

Created attachment 75453 [details]
Patch

[Luiz Agostini]

Comment on attachment 75453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75453&action=review

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:31
> +#include <QDebug>

Is it needed?

[Helder Correia]

Comment on attachment 75453 [details]
Patch

QDebug include is just a leftover. Will clean that up. Also need to resolve rebase conflicts.

[Helder Correia]

Created attachment 75472 [details]
Resolve conflicts and remove QDebug leftovers

[Helder Correia]

Comment on attachment 75472 [details]
Resolve conflicts and remove QDebug leftovers

Need to fix the build issues for Cairo.

[Helder Correia]

Created attachment 75481 [details]
Don't break Cairo port build

[Helder Correia]

Comment on attachment 75481 [details]
Don't break Cairo port build

Removing R? since I found a case where the patch needs to be fixed.

[Helder Correia]

Created attachment 76006 [details]
Refactored, more comments, added new test

[Ariya Hidayat]

Comment on attachment 76006 [details]
Refactored, more comments, added new test

View in context: https://bugs.webkit.org/attachment.cgi?id=76006&action=review

> WebCore/platform/graphics/ContextShadow.cpp:165
> +    QTransform transform = p->transform();

Use const ref.

> WebCore/platform/graphics/ContextShadow.cpp:167
> +        QPolygonF transDestPolygon = transform.map(QPolygonF(m_layerArea));

transformedPolygon? I don't think Dest is necessary here (there is no Source and Dest per se, for transformation)

> WebCore/platform/graphics/ContextShadow.h:110
> +    bool shadowsIgnoreTransforms() { return m_shadowsIgnoreTransforms; }

Make it a const.

> WebCore/platform/graphics/ContextShadow.h:127
> +    // Enclosing int rect where shadow needs to be drawn to using the layer context.
>      IntRect m_layerRect;
> +    // Same as m_layerRec but has full precision (useful for lossless transformations).
> +    FloatRect m_layerFloatRect;
> +    // Rect occupied by the original shape or image.
> +    FloatRect m_layerArea;
> +    // Buffer to where the temporary shadow will be drawn to.
>      PlatformImage m_layerImage;

Instead of m_layerArea, m_layerFloatRect and m_layerInflation, I propose using m_layerOrigin (top left corner for endShadowLayer) and m_sourceRect (for the scratch image). These variables are potentially easier to understand.

[Helder Correia]

Created attachment 76155 [details]
Consts, refactoring, separate ports calc function for now

[Ariya Hidayat]

Comment on attachment 76155 [details]
Consts, refactoring, separate ports calc function for now

View in context: https://bugs.webkit.org/attachment.cgi?id=76155&action=review

> LayoutTests/ChangeLog:5
> +        [Qt] No shadow offset should be affected by any transformation

I suggest simplifying this to "Canvas shadow offset should not be affected..." (and the bug title as well)

> LayoutTests/fast/canvas/canvas-transforms-fillRect-shadow-expected.txt:1
> +Ensure correct behavior of canvas with fillRect+shadow after translation+rotation+scaling. A blue and red checkered pattern should be displayed.

Isn't the box solid and not patterned?

> WebCore/platform/graphics/ContextShadow.cpp:159
> +    int inflation = 0;

Is it possible to make this 'float' instead? Beside removing confusion (enlarging float rect with int), we are prepared for possible further enhancement when the blur function supports non-integer blur radius.

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:140
> +    // Set the origin as the top left corner of the scratch image.
> +    if (p->transform().isIdentity()) {
> +        m_layerContext->translate(offset());
> +        m_layerContext->translate(-layerRect.x(), -layerRect.y());
> +    } else {
> +        const int frameSize = (m_sourceRect.width() - layerArea.width()) / 2;
> +        m_layerContext->translate(-layerArea.x() + frameSize, -layerArea.y() + frameSize);
> +    }

Can we get away with the specialization of the identity transform? I can't see why it's different.

[Helder Correia]

(In reply to comment #14)
> Isn't the box solid and not patterned?

"Pattern" does not refer to a canvas pattern brush, but rather to a pattern in the literal meaning of the word (i.e. the test displays a pattern). I've used the very same sentence in all my canvas tests, as all of them result in the same pattern. 

> Is it possible to make this 'float' instead?

Absolutely, this is something I forgot to change.

> Can we get away with the specialization of the identity transform? I can't see why it's different.

Again, absolutely, I also forgot to change that before uploading the last patch.

I'll upload a new version shortly.

[Helder Correia]

Created attachment 76425 [details]
Add suggestions in comment #14

[Ariya Hidayat]

Comment on attachment 76425 [details]
Add suggestions in comment #14

LGTM.

[WebKit Review Bot]

Comment on attachment 76425 [details]
Add suggestions in comment #14

Rejecting attachment 76425 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--non-interactive', 76425]" exit_code: 2
Last 500 characters of output:
 (offset -3 lines).
Hunk #6 succeeded at 700 (offset -3 lines).
Hunk #7 succeeded at 708 (offset -3 lines).
Hunk #8 succeeded at 732 (offset -3 lines).
Hunk #9 succeeded at 756 (offset -3 lines).
Hunk #10 succeeded at 954 with fuzz 2 (offset -3 lines).
2 out of 10 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsContextQt.cpp.rej

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Ariya Hidayat', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7056001

[Helder Correia]

Created attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict

[Ariya Hidayat]

Comment on attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict

LGTM.

[WebKit Commit Bot]

Comment on attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict

Clearing flags on attachment: 76509

Committed r74040: <http://trac.webkit.org/changeset/74040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74040 might have broken Qt Linux Release
The following tests are not passing:
canvas/philip/tests/2d.shadow.canvas.transparent.2.html
canvas/philip/tests/2d.shadow.image.transparent.2.html

[Alejandro G. Castro]

Apparently this commit broke these tests in the gtk bots:

fast/canvas/canvas-scale-fillPath-shadow.html -> failed
fast/canvas/canvas-scale-fillRect-shadow.html -> failed
fast/canvas/canvas-scale-strokePath-shadow.html -> failed
fast/canvas/canvas-transforms-fillRect-shadow.html -> failed

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r74042%20(17079)/results.html

I've checked the results and not sure if it is a problem with the patch or it could be the same issue we have in bug 49964.

[Helder Correia]

(In reply to comment #24)
> Apparently this commit broke these tests in the gtk bots:

I've reverted this patch as it breaks 2 Qt tests, but I ran the GTK tests and they work fine. It's very unlikely that they're failing because of the patch. I can double check later.

[Antonio Gomes]

Rolled out.

[Helder Correia]

(In reply to comment #23)
> http://trac.webkit.org/changeset/74040 might have broken Qt Linux Release
> The following tests are not passing:
> canvas/philip/tests/2d.shadow.canvas.transparent.2.html
> canvas/philip/tests/2d.shadow.image.transparent.2.html

I found out this is a clipping issue introduced by the patch when we take transformations into account. Essentially, the origin of m_layerContext must be adjusted again in case the shadow is partially clipped out. I have a patch that fixes it, will upload shortly.

[Helder Correia]

Created attachment 76620 [details]
Add changes in comment #27 fixing failing tests

[Ariya Hidayat]

Comment on attachment 76620 [details]
Add changes in comment #27 fixing failing tests

View in context: https://bugs.webkit.org/attachment.cgi?id=76620&action=review

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:139
> +    const FloatSize clippedOffset = m_unclippedLayerOrigin - m_layerOrigin;
> +    const float translationX = -layerArea.x() + frameSize - abs(clippedOffset.width());
> +    const float translationY = -layerArea.y() + frameSize - abs(clippedOffset.height());
> +    m_layerContext->translate(translationX, translationY);

Having both m_unclippedLayerOrigin and m_layerOrigin will be confusing. In particular since we never need the former.

Here is an alternative: precompute the translation (as a point) in calculateLayerBoundingRect and save it and the use it later here.

[Helder Correia]

Created attachment 76635 [details]
Add suggestions from comment #29

[Ariya Hidayat]

Comment on attachment 76635 [details]
Add suggestions from comment #29

View in context: https://bugs.webkit.org/attachment.cgi?id=76635&action=review

> WebCore/platform/graphics/qt/ContextShadowQt.cpp:135
> +    // Set the origin as the top left corner of the scratch image, or, in case there's a clipped
> +    // out region, set the origin accordingly to the full bounding rect's top-left corner.
> +    m_layerContext->translate(m_layerContextTranslation);

The comment is confusing if it is paced here. Maybe in calculateLayerBoundingRect?

[Helder Correia]

Created attachment 76687 [details]
Add suggestion from comment #31

[Ariya Hidayat]

Comment on attachment 76687 [details]
Add suggestion from comment #31

LGTM.

[WebKit Commit Bot]

Comment on attachment 76687 [details]
Add suggestion from comment #31

Clearing flags on attachment: 76687

Committed r74154: <http://trac.webkit.org/changeset/74154>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

(In reply to comment #25)
> (In reply to comment #24)
> > Apparently this commit broke these tests in the gtk bots:
> 
> I've reverted this patch as it breaks 2 Qt tests, but I ran the GTK tests and they work fine. It's very unlikely that they're failing because of the patch. I can double check later.

This patch has broken GTK+ bots, as was warned in comment #24.

[Xan Lopez]

cq wouldn't allow me to revert the patch, so I rolled it out manually in r74187.

[Helder Correia]

Created attachment 76804 [details]
Doesn't break GTK when opening tests in launcher

[Helder Correia]

(In reply to comment #38)
> Doesn't break GTK when opening tests in launcher

So when I test using the launcher, all is fine. Using DRT, OTOH, fails miserably. Could it be that the launcher is using X11, and DRT is using Image (or whatever Cairo's raster backend is called)?

[Helder Correia]

Created attachment 76825 [details]
Remove GTK specific expected results files, tests pass now

[Helder Correia]

Created attachment 76826 [details]
Remove GTK specific expected results files, tests pass now

[Helder Correia]

Created attachment 76831 [details]
Upload manually, git would rename files no matter what

[Helder Correia]

Patch was rolled out.

[Helder Correia]

Created attachment 76835 [details]
Once more, to force the bots to work

[Martin Robinson]

Created attachment 76913 [details]
Cross-platform version of the previous patch

[Helder Correia]

(In reply to comment #45)
> Created an attachment (id=76913) [details]
> Cross-platform version of the previous patch

Martin, you need to upload the patch manually, you have the same issue I ran into yesterday: Git is considering that the new transforms test is a rename from one of your the platform scale expected files. There should be 3 new files and 3 deleted files. Just do a git show of the commit to a file and upload manually.

[Early Warning System Bot]

Attachment 76913 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7216027

[Martin Robinson]

Created attachment 76915 [details]
Cross platform version of the previous patch (#2)

Okay. Here's the output of 'git diff'

[Martin Robinson]

Created attachment 76917 [details]
Cross platform version of the previous patch (#3)

Uploading a new patch that builds on Qt.

[Martin Robinson]

Created attachment 76918 [details]
Entire patch

[Eric Seidel (no email)]

Comment on attachment 76915 [details]
Cross platform version of the previous patch (#2)

Cleared Martin Robinson's review+ from obsolete attachment 76915 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 76918 [details]
Entire patch

Rejecting attachment 76918 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76918]" exit_code: 2
Last 500 characters of output:
raphicsContextQt.cpp
Hunk #1 FAILED at 53.
Hunk #2 FAILED at 510.
Hunk #3 FAILED at 548.
Hunk #4 succeeded at 664 with fuzz 2 (offset 106 lines).
Hunk #5 FAILED at 790.
Hunk #6 FAILED at 798.
Hunk #7 FAILED at 822.
Hunk #8 FAILED at 846.
Hunk #9 FAILED at 1026.
8 out of 9 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsContextQt.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Ariya Hidayat', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7220034

[Martin Robinson]

Committed r74317: <http://trac.webkit.org/changeset/74317>

[Helder Correia]

(In reply to comment #53)
> Committed r74317: <http://trac.webkit.org/changeset/74317>

The patch that was landed is not the same. The gtk platform expected results files were not deleted, and the new canvas-transforms-shadow test is not there either.

[Helder Correia]

Seems to be fine.