Bug 50422 - [Qt] Canvas shadow offset should not be affected by any transformation
Summary: [Qt] Canvas shadow offset should not be affected by any transformation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 51050 51183
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-02 16:47 PST by Helder Correia
Modified: 2010-12-18 14:18 PST (History)
12 users (show)

See Also:


Attachments
Patch (22.04 KB, patch)
2010-12-02 16:53 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Patch (25.55 KB, patch)
2010-12-02 19:22 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Resolve conflicts and remove QDebug leftovers (25.80 KB, patch)
2010-12-03 01:13 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Don't break Cairo port build (25.85 KB, patch)
2010-12-03 02:22 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Refactored, more comments, added new test (34.81 KB, patch)
2010-12-08 19:51 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Consts, refactoring, separate ports calc function for now (34.75 KB, patch)
2010-12-09 18:39 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Add suggestions in comment #14 (34.58 KB, patch)
2010-12-13 13:29 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Resolve "m_common->state" -> "m_state" conflict (34.50 KB, patch)
2010-12-14 00:31 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Add changes in comment #27 fixing failing tests (35.17 KB, patch)
2010-12-14 20:10 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Add suggestions from comment #29 (35.20 KB, patch)
2010-12-15 00:07 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Add suggestion from comment #31 (35.21 KB, patch)
2010-12-15 14:33 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Doesn't break GTK when opening tests in launcher (36.82 KB, patch)
2010-12-16 12:54 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Remove GTK specific expected results files, tests pass now (39.65 KB, patch)
2010-12-16 16:21 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Remove GTK specific expected results files, tests pass now (39.65 KB, patch)
2010-12-16 16:26 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Upload manually, git would rename files no matter what (41.66 KB, patch)
2010-12-16 17:04 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Once more, to force the bots to work (41.66 KB, patch)
2010-12-16 17:38 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Cross-platform version of the previous patch (47.59 KB, patch)
2010-12-17 14:41 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Cross platform version of the previous patch (#2) (39.67 KB, patch)
2010-12-17 15:20 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Cross platform version of the previous patch (#3) (39.67 KB, patch)
2010-12-17 15:24 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Entire patch (52.18 KB, patch)
2010-12-17 15:30 PST, Martin Robinson
ariya.hidayat: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Helder Correia 2010-12-02 16:47:10 PST
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).
Comment 1 Helder Correia 2010-12-02 16:53:03 PST
Created attachment 75432 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-12-02 17:27:33 PST
Comment on attachment 75432 [details]
Patch

Why does this qt-only change affect shared layout test results?
Comment 3 Helder Correia 2010-12-02 18:03:50 PST
(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.
Comment 4 Helder Correia 2010-12-02 19:22:35 PST
Created attachment 75453 [details]
Patch
Comment 5 Luiz Agostini 2010-12-02 20:59:52 PST
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?
Comment 6 Helder Correia 2010-12-03 00:00:51 PST
Comment on attachment 75453 [details]
Patch

QDebug include is just a leftover. Will clean that up. Also need to resolve rebase conflicts.
Comment 7 Helder Correia 2010-12-03 01:13:41 PST
Created attachment 75472 [details]
Resolve conflicts and remove QDebug leftovers
Comment 8 Helder Correia 2010-12-03 02:13:56 PST
Comment on attachment 75472 [details]
Resolve conflicts and remove QDebug leftovers

Need to fix the build issues for Cairo.
Comment 9 Helder Correia 2010-12-03 02:22:43 PST
Created attachment 75481 [details]
Don't break Cairo port build
Comment 10 Helder Correia 2010-12-03 18:46:09 PST
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.
Comment 11 Helder Correia 2010-12-08 19:51:07 PST
Created attachment 76006 [details]
Refactored, more comments, added new test
Comment 12 Ariya Hidayat 2010-12-08 20:10:09 PST
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.
Comment 13 Helder Correia 2010-12-09 18:39:21 PST
Created attachment 76155 [details]
Consts, refactoring, separate ports calc function for now
Comment 14 Ariya Hidayat 2010-12-11 23:28:48 PST
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.
Comment 15 Helder Correia 2010-12-13 13:21:04 PST
(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.
Comment 16 Helder Correia 2010-12-13 13:29:41 PST
Created attachment 76425 [details]
Add suggestions in comment #14
Comment 17 Ariya Hidayat 2010-12-13 20:32:24 PST
Comment on attachment 76425 [details]
Add suggestions in comment #14

LGTM.
Comment 18 WebKit Review Bot 2010-12-13 20:49:25 PST
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
Comment 19 Helder Correia 2010-12-14 00:31:58 PST
Created attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict
Comment 20 Ariya Hidayat 2010-12-14 09:39:08 PST
Comment on attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict

LGTM.
Comment 21 WebKit Commit Bot 2010-12-14 10:30:51 PST
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>
Comment 22 WebKit Commit Bot 2010-12-14 10:30:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 WebKit Review Bot 2010-12-14 11:05:29 PST
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
Comment 24 Alejandro G. Castro 2010-12-14 12:25:16 PST
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.
Comment 25 Helder Correia 2010-12-14 12:37:12 PST
(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.
Comment 26 Antonio Gomes 2010-12-14 13:28:26 PST
Rolled out.
Comment 27 Helder Correia 2010-12-14 19:36:51 PST
(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.
Comment 28 Helder Correia 2010-12-14 20:10:33 PST
Created attachment 76620 [details]
Add changes in comment #27 fixing failing tests
Comment 29 Ariya Hidayat 2010-12-14 21:47:09 PST
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.
Comment 30 Helder Correia 2010-12-15 00:07:58 PST
Created attachment 76635 [details]
Add suggestions from comment #29
Comment 31 Ariya Hidayat 2010-12-15 08:31:30 PST
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?
Comment 32 Helder Correia 2010-12-15 14:33:50 PST
Created attachment 76687 [details]
Add suggestion from comment #31
Comment 33 Ariya Hidayat 2010-12-15 14:43:57 PST
Comment on attachment 76687 [details]
Add suggestion from comment #31

LGTM.
Comment 34 WebKit Commit Bot 2010-12-15 15:48:42 PST
Comment on attachment 76687 [details]
Add suggestion from comment #31

Clearing flags on attachment: 76687

Committed r74154: <http://trac.webkit.org/changeset/74154>
Comment 35 WebKit Commit Bot 2010-12-15 15:48:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 36 Xan Lopez 2010-12-16 06:19:00 PST
(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.
Comment 37 Xan Lopez 2010-12-16 06:45:28 PST
cq wouldn't allow me to revert the patch, so I rolled it out manually in r74187.
Comment 38 Helder Correia 2010-12-16 12:54:45 PST
Created attachment 76804 [details]
Doesn't break GTK when opening tests in launcher
Comment 39 Helder Correia 2010-12-16 14:29:17 PST
(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)?
Comment 40 Helder Correia 2010-12-16 16:21:07 PST
Created attachment 76825 [details]
Remove GTK specific expected results files, tests pass now
Comment 41 Helder Correia 2010-12-16 16:26:23 PST
Created attachment 76826 [details]
Remove GTK specific expected results files, tests pass now
Comment 42 Helder Correia 2010-12-16 17:04:17 PST
Created attachment 76831 [details]
Upload manually, git would rename files no matter what
Comment 43 Helder Correia 2010-12-16 17:28:52 PST
Patch was rolled out.
Comment 44 Helder Correia 2010-12-16 17:38:05 PST
Created attachment 76835 [details]
Once more, to force the bots to work
Comment 45 Martin Robinson 2010-12-17 14:41:11 PST
Created attachment 76913 [details]
Cross-platform version of the previous patch
Comment 46 Helder Correia 2010-12-17 14:58:46 PST
(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.
Comment 47 Early Warning System Bot 2010-12-17 15:17:22 PST
Attachment 76913 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7216027
Comment 48 Martin Robinson 2010-12-17 15:20:16 PST
Created attachment 76915 [details]
Cross platform version of the previous patch (#2)

Okay. Here's the output of 'git diff'
Comment 49 Martin Robinson 2010-12-17 15:24:31 PST
Created attachment 76917 [details]
Cross platform version of the previous patch (#3)

Uploading a new patch that builds on Qt.
Comment 50 Martin Robinson 2010-12-17 15:30:44 PST
Created attachment 76918 [details]
Entire patch
Comment 51 Eric Seidel (no email) 2010-12-17 16:40:13 PST
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.
Comment 52 WebKit Commit Bot 2010-12-18 10:45:24 PST
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
Comment 53 Martin Robinson 2010-12-18 10:45:27 PST
Committed r74317: <http://trac.webkit.org/changeset/74317>
Comment 54 Helder Correia 2010-12-18 13:08:59 PST
(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.
Comment 55 Helder Correia 2010-12-18 14:18:47 PST
Seems to be fine.