Bug 48297 - Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.image.destination-atop.html
Summary: Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.image.destination-...
Status: RESOLVED DUPLICATE of bug 66036
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 46506
  Show dependency treegraph
 
Reported: 2010-10-25 22:26 PDT by Mike Lawther
Modified: 2011-10-06 22:31 PDT (History)
11 users (show)

See Also:


Attachments
Drawing transparency in remaining canvas area before drawImage() (5.77 KB, patch)
2011-06-03 02:20 PDT, Mahesh Kumar
no flags Details | Formatted Diff | Diff
Patch after fixing coding style issue reported by EWS (5.97 KB, patch)
2011-06-03 04:37 PDT, Mahesh Kumar
eric: review-
Details | Formatted Diff | Diff
Updated patch after incorporating review comments (13.15 KB, patch)
2011-06-16 23:01 PDT, Mahesh Kumar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.35 MB, application/zip)
2011-06-16 23:17 PDT, WebKit Review Bot
no flags Details
Patch to fix to failed layout test on chromium-ews (15.62 KB, patch)
2011-06-17 03:06 PDT, Mahesh Kumar
jamesr: review-
Details | Formatted Diff | Diff
Proposed patch with drawImage() changes (8.78 KB, patch)
2011-07-11 04:08 PDT, Mustafizur Rahaman (rahaman)
jamesr: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2010-10-25 22:26:55 PDT
This layout test fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506
Comment 1 Mahesh Kumar 2011-06-03 02:20:35 PDT
Created attachment 95876 [details]
Drawing transparency in remaining canvas area before drawImage()

https://bugs.webkit.org/show_bug.cgi?id=39027 added CanvasRenderingContext2D::shouldDisplayTransparencyElsewhere() & CanvasRenderingContext2D::displayTransparencyElsewhere() to draw transparency for the remaining canvas area for CompositeSourceIn & CompositeSourceOut.

I have also added to check for CompositeDestinationAtop to draw transparency in remaining canvas area before Image draw for destination-atop compositing.
Comment 2 Mahesh Kumar 2011-06-03 04:37:38 PDT
Created attachment 95887 [details]
Patch after fixing coding style issue reported by EWS

Created the same patch after fixing the coding style issue reported by EWS
Comment 3 Eric Seidel (no email) 2011-06-05 12:12:11 PDT
Comment on attachment 95887 [details]
Patch after fixing coding style issue reported by EWS

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1513
>      // CompositeSourceAtop is not listed here as the platforms already implement the specification's behavior.

This comment no longer makes sense.
Comment 4 Eric Seidel (no email) 2011-06-05 12:12:41 PDT
Comment on attachment 95887 [details]
Patch after fixing coding style issue reported by EWS

The comment is out of date.  Should it be removed?
Comment 5 Mustafizur Rahaman (rahaman) 2011-06-05 12:14:42 PDT
We have implemented destination-atop,so should we keep the comment for now & remove if we need to add source-atop also?
Comment 6 Mustafizur Rahaman (rahaman) 2011-06-05 12:24:43 PDT
Hi Jamesr, could you please review my code. I have seen you have reviewed https://bugs.webkit.org/show_bug.cgi?id=30297 & this fix is on the same line.
Comment 7 Mustafizur Rahaman (rahaman) 2011-06-05 12:26:50 PDT
(In reply to comment #6)
> Hi Jamesr, could you please review my code. I have seen you have reviewed https://bugs.webkit.org/show_bug.cgi?id=30297 & this fix is on the same line.

Oops, I meant https://bugs.webkit.org/show_bug.cgi?id=39027 and NOT 30297.
Comment 8 Julien Chaffraix 2011-06-06 20:15:28 PDT
(In reply to comment #3)
> (From update of attachment 95887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95887&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1513
> >      // CompositeSourceAtop is not listed here as the platforms already implement the specification's behavior.
> 
> This comment no longer makes sense.

I disagree, it still makes sense. CompositeSourceAtop is marked as needed transparency everywhere else in the spec. However the platforms already implement the right behavior, thus there is no need to re-do the work here.

I am quite concerned with having different behaviors for source-atop vs destination-atop. If your testing shows that this change is needed, I would rather remove the comment and make sure both work the same.

Also if you change the behavior for source-atop, shouldn't fast/canvas/canvas-composite-alpha.html need a rebaseline too?
Comment 9 Mustafizur Rahaman (rahaman) 2011-06-07 12:01:34 PDT
(In reply to comment #8)
> (In reply to comment #3)
> > (From update of attachment 95887 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95887&action=review
> > 
> > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1513
> > >      // CompositeSourceAtop is not listed here as the platforms already implement the specification's behavior.
> > 
> > This comment no longer makes sense.
> 
> I disagree, it still makes sense. CompositeSourceAtop is marked as needed transparency everywhere else in the spec. However the platforms already implement the right behavior, thus there is no need to re-do the work here.
> 
> I am quite concerned with having different behaviors for source-atop vs destination-atop. If your testing shows that this change is needed, I would rather remove the comment and make sure both work the same.

I have used the same test case LayoutTests/canvas/philip/tests/2d.composite.uncovered.image.destination-atop.html & modified it to source-atop(changed only this line ctx.globalCompositeOperation = 'source-atop';). 

When I verified this behavior in both FF & Safari, the source-atop & destination-atop behaviors were different & as per my understanding from the spec for these two test cases, the behaviors should be different.
> 
> Also if you change the behavior for source-atop, shouldn't fast/canvas/canvas-composite-alpha.html need a rebaseline too?

So, without my changes, source-atop works the same way in Safari, FF & Opera, i.e. no code changes required. In my opinion, we should only fix the destination-atop issue for WebKit code.
Comment 10 Julien Chaffraix 2011-06-09 17:22:21 PDT
> I have used the same test case LayoutTests/canvas/philip/tests/2d.composite.uncovered.image.destination-atop.html & modified it to source-atop(changed only this line ctx.globalCompositeOperation = 'source-atop';). 
> 
> When I verified this behavior in both FF & Safari, the source-atop & destination-atop behaviors were different & as per my understanding from the spec for these two test cases, the behaviors should be different.

I confirm that we have a difference between source-atop and destination-atop that is not present in any other browser.

> > Also if you change the behavior for source-atop, shouldn't fast/canvas/canvas-composite-alpha.html need a rebaseline too?
> 
> So, without my changes, source-atop works the same way in Safari, FF & Opera, i.e. no code changes required. In my opinion, we should only fix the destination-atop issue for WebKit code.

Sorry, I meant destination-atop in my question. I tried your patch and it does work properly, however I see the need for a rebaseline on Qt at least. I am not sure why it is not picked up by the chromium EWS as chromium does not have the right behavior.

Rethinking over the difference in behavior, I don't see a good way around that. Could you please amend the comment about source-atop explaining why destination-atop needs the "display transparency everywhere".
Comment 11 James Robinson 2011-06-09 17:23:42 PDT
The chromium EWS bot doesn't run pixel tests currently.
Comment 12 Mahesh Kumar 2011-06-16 23:01:39 PDT
Created attachment 97545 [details]
Updated patch after incorporating review comments

Updated the patch after incorporating review comments from Julien.

Rebaselined canvas-composite-alpha.html for destination-atop.

As part of this change, https://bugs.webkit.org/show_bug.cgi?id=48298, https://bugs.webkit.org/show_bug.cgi?id=48299, https://bugs.webkit.org/show_bug.cgi?id=48300, https://bugs.webkit.org/show_bug.cgi?id=48292 & https://bugs.webkit.org/show_bug.cgi?id=48302 are also resolved. So, updated the Changelogs accordingly.
Comment 13 WebKit Review Bot 2011-06-16 23:17:33 PDT
Comment on attachment 97545 [details]
Updated patch after incorporating review comments

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

New failing tests:
canvas/philip/tests/2d.composite.uncovered.image.source-out.html
canvas/philip/tests/2d.composite.uncovered.image.destination-in.html
canvas/philip/tests/2d.composite.uncovered.pattern.destination-atop.html
canvas/philip/tests/2d.composite.uncovered.image.source-in.html
canvas/philip/tests/2d.composite.uncovered.image.destination-atop.html
Comment 14 WebKit Review Bot 2011-06-16 23:17:39 PDT
Created attachment 97547 [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 15 Mahesh Kumar 2011-06-17 03:06:25 PDT
Created attachment 97568 [details]
Patch to fix to failed layout test on chromium-ews

I think the test cases failed because of missing trailing line in the expected.txt. Updating the patch after adding the necessary trailing lines in the respective files.
Comment 16 Julien Chaffraix 2011-06-17 09:59:24 PDT
Comment on attachment 97568 [details]
Patch to fix to failed layout test on chromium-ews

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1517
> +    // to display transparency elsewhere, therefore adding the check for CompositeDestinationAtop

I would rather see a 'why' comment than a 'what' comment. Everybody can check against HTML5 that we are doing the right stuff here. However knowing *why* we need to add CompositeDestinationAtop but not CompositeSourceAtop is much more valuable to people reading the code and wondering why we have such a discrepancy.

> LayoutTests/platform/gtk/Skipped:-1104
> -canvas/philip/tests/2d.composite.uncovered.image.source-out.html

Removing passing test from the Skipped list is great! However we prefer atomic patches. I would bet that source-{in|out} were passing without your change and thus are orthogonal to the current change. Could you split the patch in 2 and move the orthogonal Skipped changes to the other bugs (or correct me if I missed something).
Comment 17 Mustafizur Rahaman (rahaman) 2011-06-17 10:19:25 PDT
(In reply to comment #16)
> (From update of attachment 97568 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97568&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1517
> > +    // to display transparency elsewhere, therefore adding the check for CompositeDestinationAtop
> 
> I would rather see a 'why' comment than a 'what' comment. Everybody can check against HTML5 that we are doing the right stuff here. However knowing *why* we need to add CompositeDestinationAtop but not CompositeSourceAtop is much more valuable to people reading the code and wondering why we have such a discrepancy.

I agree with you. Actually, I found that the comment was becoming too long to explain this in details, so thought of making it shorter. Anyway, will try to brief & precisely explain it.
> 
> > LayoutTests/platform/gtk/Skipped:-1104
> > -canvas/philip/tests/2d.composite.uncovered.image.source-out.html
> 
> Removing passing test from the Skipped list is great! However we prefer atomic patches. I would bet that source-{in|out} were passing without your change and thus are orthogonal to the current change. Could you split the patch in 2 and move the orthogonal Skipped changes to the other bugs (or correct me if I missed something).

If you see, we have removed the following test cases

-BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.fill.destination-atop.html = TEXT
-BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.pattern.destination-atop.html = TEXT
[The above test cases are related to our current changes of adding CompositeDestinationAtop in shouldDisplayTransparencyElsewhere()]

-BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.destination-atop.html = TEXT
-BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.destination-in.html = TEXT
-BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.source-in.html = TEXT
-BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.source-out.html = TEXT
[The above test cases are related to calling shouldDisplayTransparencyElsewhere() & displayTransparencyElsewhere() from drawImage(). So these test cases are also passing because of these changes, they were not passing before.]

Therefore, I don't see any need of splitting the changes. Let me know if I answered you.

Will update the patch with proper review comment.
Comment 18 Julien Chaffraix 2011-06-17 16:36:45 PDT
Comment on attachment 97568 [details]
Patch to fix to failed layout test on chromium-ews

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

>>> LayoutTests/platform/gtk/Skipped:-1104
>>> -canvas/philip/tests/2d.composite.uncovered.image.source-out.html
>> 
>> Removing passing test from the Skipped list is great! However we prefer atomic patches. I would bet that source-{in|out} were passing without your change and thus are orthogonal to the current change. Could you split the patch in 2 and move the orthogonal Skipped changes to the other bugs (or correct me if I missed something).
> 
> If you see, we have removed the following test cases
> 
> -BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.fill.destination-atop.html = TEXT
> -BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.pattern.destination-atop.html = TEXT
> [The above test cases are related to our current changes of adding CompositeDestinationAtop in shouldDisplayTransparencyElsewhere()]
> 
> -BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.destination-atop.html = TEXT
> -BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.destination-in.html = TEXT
> -BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.source-in.html = TEXT
> -BUGWK39177 : canvas/philip/tests/2d.composite.uncovered.image.source-out.html = TEXT
> [The above test cases are related to calling shouldDisplayTransparencyElsewhere() & displayTransparencyElsewhere() from drawImage(). So these test cases are also passing because of these changes, they were not passing before.]
> 
> Therefore, I don't see any need of splitting the changes. Let me know if I answered you.
> 
> Will update the patch with proper review comment.

I think you just showed that there is actually 2 orthogonal changes in one here by splitting the test cases in 2 different groups.
Comment 19 James Robinson 2011-07-10 23:26:00 PDT
Comment on attachment 97568 [details]
Patch to fix to failed layout test on chromium-ews

Please fix one bug per patch.  The changes at line 1514 are redundant with https://bugs.webkit.org/show_bug.cgi?id=48292, feel free to upload a new patch with just the drawImage() change.
Comment 20 Mustafizur Rahaman (rahaman) 2011-07-11 04:08:02 PDT
Created attachment 100262 [details]
Proposed patch with drawImage() changes

Updated patch with only drawImage() changes for composite operation. The rest of the changes from the previous patch has already been committed in https://bugs.webkit.org/show_bug.cgi?id=48292
Comment 21 James Robinson 2011-07-22 15:42:26 PDT
Comment on attachment 100262 [details]
Proposed patch with drawImage() changes

This will produce weird results if the destination rectangle is not pixel-aligned.  I think in general the displayTransparencyElsewhere() approach is not going to work perfectly.

I think we should investigate using an intermediate surface for these composite operations and see what the performance looks like.  This is what firefox does and is closer to the rendering model in the spec.  If it's not too expensive, it'll be a lot simpler and correct in all of these tricky cases.
Comment 22 Mustafizur Rahaman (rahaman) 2011-09-18 23:11:41 PDT
This has been resolved in https://bugs.webkit.org/show_bug.cgi?id=66036. Can someone please mark it as RESOLVED DUPLICATE
Comment 23 Eric Seidel (no email) 2011-09-19 10:51:32 PDT
OK.

*** This bug has been marked as a duplicate of bug 66036 ***
Comment 24 Priyanka 2011-10-06 22:31:40 PDT
I tested this issue with Windows 7 machine with Webkit version below 
Last Changed Rev: 95852
Last Changed Date: 2011-09-23 15:56:21 -0400 (Fri, 23 Sep 2011)

This issue is still present, and it is not a duplicate of 66036 (https://bugs.webkit.org/show_bug.cgi?id=66036) because 66036 takes care of transparency else where, when the source and destination are both canvas and we use fillRect() for drawing shapes.

In this test source is an external image and destination is the canvas.
This test passes for Opera and Firefox.