Bug 39027 - http://philip.html5.org/tests/canvas/suite/tests/2d.composite.uncovered.fill.source-in.html fails
Summary: http://philip.html5.org/tests/canvas/suite/tests/2d.composite.uncovered.fill....
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
: 48294 (view as bug list)
Depends on:
Blocks: 20553 46506
  Show dependency treegraph
 
Reported: 2010-05-12 14:39 PDT by Chang Shu
Modified: 2011-05-25 14:06 PDT (History)
12 users (show)

See Also:


Attachments
This patch fixes the problem (5.52 KB, patch)
2010-06-07 10:27 PDT, Ariya Hidayat
no flags Details | Formatted Diff | Diff
Patch (11.34 KB, patch)
2011-05-05 11:53 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (4.90 MB, application/zip)
2011-05-05 13:16 PDT, WebKit Review Bot
no flags Details
Updated patch with tests passing (19.38 KB, patch)
2011-05-06 18:11 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch (20.29 KB, patch)
2011-05-16 18:31 PDT, Julien Chaffraix
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.14 MB, application/zip)
2011-05-16 19:02 PDT, WebKit Review Bot
no flags Details
Updated patch - no more SourceAtop reference as it was not needed (19.44 KB, patch)
2011-05-18 19:06 PDT, Julien Chaffraix
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.42 MB, application/zip)
2011-05-18 19:24 PDT, WebKit Review Bot
no flags Details
Same as previously with a comment as to why SourceAtop is covered (18.70 KB, patch)
2011-05-19 15:33 PDT, Julien Chaffraix
jamesr: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.52 MB, application/zip)
2011-05-19 16:53 PDT, WebKit Review Bot
no flags Details
Same as previous attachment but with the expected results (19.52 KB, patch)
2011-05-19 19:22 PDT, Julien Chaffraix
jamesr: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.51 MB, application/zip)
2011-05-19 19:54 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2010-05-12 14:39:29 PDT
The above test case fails on Mac and Qt, at least.
Comment 1 Chang Shu 2010-05-12 14:45:58 PDT
The spec and Philip's test require that the pixels not covered by the source object should be cleared to (0,0,0,0). However, the behavior of the platform is to leave them unchanged. It seems to me it is arguable to go either way. We have to hack some code in WebCore layer to match the behavior in spec. Can anyone give me a signal if I should proceed? Thanks!
Btw, Firefox works.
Comment 2 Ariya Hidayat 2010-06-07 10:27:45 PDT
Created attachment 58044 [details]
This patch fixes the problem

I believe the bug should be fixed. Opera also passes the tests.

The question is rather: will it break other apps like Dashboard widgets etc. Perhaps Apple folks who were involved in the original Canvas design and implementation can shed some light on this particular problem?

Note that the patch above is still missing the unskipping of the passed tests. I just want to have preliminary review and/or whether we should go on or not.
Comment 3 WebKit Review Bot 2010-06-07 10:30:25 PDT
Attachment 58044 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/canvas/CanvasRenderingContext2D.cpp:761:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:762:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:763:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:764:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:1056:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:1057:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:1058:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/html/canvas/CanvasRenderingContext2D.cpp:1059:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Kenneth Rohde Christiansen 2010-06-28 07:18:17 PDT
Comment on attachment 58044 [details]
This patch fixes the problem

WebCore/html/canvas/CanvasRenderingContext2D.cpp:762
 +          state().m_globalComposite == CompositeSourceOut ||
This is wrong coding style, the || should go on the other side
Comment 5 Andreas Kling 2010-07-17 13:55:31 PDT
See also https://bugs.webkit.org/show_bug.cgi?id=39177
Comment 6 Ariya Hidayat 2010-08-11 10:14:54 PDT
Comment on attachment 58044 [details]
This patch fixes the problem

Clear the review flag, this patch is not completely ready.
Comment 7 Carol Szabo 2010-12-21 14:11:38 PST
*** Bug 48294 has been marked as a duplicate of this bug. ***
Comment 8 Julien Chaffraix 2011-05-05 11:53:36 PDT
Created attachment 92440 [details]
Patch
Comment 9 WebKit Review Bot 2011-05-05 13:16:10 PDT
Attachment 92440 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8587027
New failing tests:
fast/canvas/canvas-composite-alpha.html
Comment 10 WebKit Review Bot 2011-05-05 13:16:19 PDT
Created attachment 92457 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 11 Julien Chaffraix 2011-05-05 18:29:54 PDT
Comment on attachment 92440 [details]
Patch

Clearing the review flag while I investigate the failure.
Comment 12 Julien Chaffraix 2011-05-06 18:11:41 PDT
Created attachment 92671 [details]
Updated patch with tests passing
Comment 13 James Robinson 2011-05-09 19:50:17 PDT
This change has the chance of breaking content designed to work with the CoreGraphics model and might break some WebKit-specific content.  Chromium would be happy making this change to match other browsers so long as Apple was planning to make the change.  We definitely don't want to diverge.
Comment 14 Oliver Hunt 2011-05-13 17:02:06 PDT
(In reply to comment #13)
> This change has the chance of breaking content designed to work with the CoreGraphics model and might break some WebKit-specific content.  Chromium would be happy making this change to match other browsers so long as Apple was planning to make the change.  We definitely don't want to diverge.

My opinion is that we should make this change, if it turns out to be problematic we can always shove it behind the dashboard compatibility flags -- my assumption is mostly that it's dashboard widgets that are going to rely on such quirks.
Comment 15 James Robinson 2011-05-13 17:05:38 PDT
Comment on attachment 92671 [details]
Updated patch with tests passing

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:945
> +        // FIXME: CompositeSourceAtop should follow the same behavior however I am not sure
> +        // our test coverage is enough.

i'm not sure exactly what this FIXME means.  should this reference a webkit bug?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:947
> +        if (state().m_globalComposite == CompositeSourceIn
> +            || state().m_globalComposite == CompositeSourceOut) {

this should be on one line

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1068
> +    // FIXME: CompositeSourceAtop should follow the same behavior however I am not sure
> +    // our test coverage is enough.
> +    if (state().m_globalComposite == CompositeSourceIn
> +        || state().m_globalComposite == CompositeSourceOut) {

same comments as above

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1077
> +        FloatRect canvasRect(0, 0, canvas()->width(), canvas()->height());
> +        canvasRect = state().m_transform.inverse().mapRect(canvasRect);
> +
> +        c->save();
> +        c->clipOut(enclosingIntRect(rect));
> +        c->setCompositeOperation(CompositeClear);
> +        c->fillRect(canvasRect);
> +        c->restore();

this smells like bad copy-pasta.  can you refactor this into a helper?
Comment 16 Julien Chaffraix 2011-05-13 20:00:15 PDT
Comment on attachment 92671 [details]
Updated patch with tests passing

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:945
>> +        // our test coverage is enough.
> 
> i'm not sure exactly what this FIXME means.  should this reference a webkit bug?

It was more like a gut feeling as philip's test suite does not cover source-atop as it does with source-in or source-out (unless I missed something). I had a quick look and we cover some of source-atop in canvas-composition-alpha.html. I can either squash this change or use a FIXME with a new bug. Do you have a preference?

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:947
>> +            || state().m_globalComposite == CompositeSourceOut) {
> 
> this should be on one line

Done.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1068
>> +        || state().m_globalComposite == CompositeSourceOut) {
> 
> same comments as above

Done.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1077
>> +        c->restore();
> 
> this smells like bad copy-pasta.  can you refactor this into a helper?

OK, I will have to use some template magic as rect and m_path are different types but it makes sense.
Comment 17 Julien Chaffraix 2011-05-16 18:31:30 PDT
Created attachment 93725 [details]
Patch
Comment 18 WebKit Review Bot 2011-05-16 19:02:25 PDT
Comment on attachment 93725 [details]
Patch

Attachment 93725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8701926

New failing tests:
canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
Comment 19 WebKit Review Bot 2011-05-16 19:02:31 PDT
Created attachment 93728 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 20 Julien Chaffraix 2011-05-17 18:39:31 PDT
(In reply to comment #18)
> (From update of attachment 93725 [details])
> Attachment 93725 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8701926
> 
> New failing tests:
> canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
> canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html

I looked into those failures and I think this is just the EWS being confused. The diffs reported are the exact diffs that I included in the expected files as part of the patch. Also on a local Chromium build, I don't have the failures.
Comment 21 Julien Chaffraix 2011-05-18 18:54:55 PDT
Jamesr asked me to handle the SourceAtop case as part of this patch. I had a look at our implementation and we are already matching the specification. Thus we don't need to clear the surface contrary to my assumption. I will post a patch without the FIXME for review.
Comment 22 Julien Chaffraix 2011-05-18 19:06:27 PDT
Created attachment 94027 [details]
Updated patch - no more SourceAtop reference as it was not needed
Comment 23 WebKit Review Bot 2011-05-18 19:24:49 PDT
Comment on attachment 94027 [details]
Updated patch - no more SourceAtop reference as it was not needed

Attachment 94027 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8702985

New failing tests:
canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
Comment 24 WebKit Review Bot 2011-05-18 19:24:55 PDT
Created attachment 94031 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 25 Julien Chaffraix 2011-05-19 15:33:04 PDT
Created attachment 94133 [details]
Same as previously with a comment as to why SourceAtop is covered
Comment 26 WebKit Review Bot 2011-05-19 16:53:02 PDT
Comment on attachment 94133 [details]
Same as previously with a comment as to why SourceAtop is covered

Attachment 94133 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8722018

New failing tests:
canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
Comment 27 WebKit Review Bot 2011-05-19 16:53:09 PDT
Created attachment 94147 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 28 James Robinson 2011-05-19 16:59:02 PDT
The baselines look bad - it seems you have either an extra newline or are missing one.  Can you generate good text baselines for those tests and re-upload?
Comment 29 James Robinson 2011-05-19 16:59:17 PDT
Comment on attachment 94133 [details]
Same as previously with a comment as to why SourceAtop is covered

R- for failing test
Comment 30 Julien Chaffraix 2011-05-19 18:31:34 PDT
(In reply to comment #28)
> The baselines look bad - it seems you have either an extra newline or are missing one.  Can you generate good text baselines for those tests and re-upload?

Good catch, the latest patch is missing the good text baselines (the previous were not). This is due to a bug (bug 61162) in webkit-patch that I hit by mistake when doing some changes to the patch. Basically the newlines in the expected results are just discarded silently :-(

Also please ignore the bots' failures as they are hitting the same bug. I will check locally that the baselines work on Chromium linux before manually landing the patch.
Comment 31 Julien Chaffraix 2011-05-19 19:22:04 PDT
Created attachment 94161 [details]
Same as previous attachment but with the expected results
Comment 32 WebKit Review Bot 2011-05-19 19:54:01 PDT
Comment on attachment 94161 [details]
Same as previous attachment but with the expected results

Attachment 94161 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8722053

New failing tests:
canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
Comment 33 WebKit Review Bot 2011-05-19 19:54:07 PDT
Created attachment 94163 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 34 James Robinson 2011-05-23 17:11:57 PDT
Comment on attachment 94161 [details]
Same as previous attachment but with the expected results

R=me
Comment 35 Matthew Delaney 2011-05-23 19:12:26 PDT
Comment on attachment 94161 [details]
Same as previous attachment but with the expected results

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1509
> +

Is this assert really necessary?
Comment 36 Julien Chaffraix 2011-05-24 12:24:25 PDT
Comment on attachment 94161 [details]
Same as previous attachment but with the expected results

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1509
>> +
> 
> Is this assert really necessary?

It is not strictly necessary. This is just a safeguard against misuse and unneeded calls (I am sure we can find a platform where clipping out a path is expensive). I don't feel too strong about it though.
Comment 37 Julien Chaffraix 2011-05-25 11:46:13 PDT
Committed r87307: <http://trac.webkit.org/changeset/87307>
Comment 39 James Robinson 2011-05-25 14:06:29 PDT
(In reply to comment #38)
> http://trac.webkit.org/changeset/87307 seems to have broken fast/canvas/canvas-composite.html for Chromium/WebKit, see results here:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fcanvas%2Fcanvas-composite.html

That's a progression (except for the antaliasing artifacts on source-out).

Can you file a new bug about the antialiasing on source-out being broken and suppress?  The bug is that we're seeing a 'ring' around the top left part of the circle.