Bug 48297

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.

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 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 on attachment 95887 [details] Patch after fixing coding style issue reported by EWS The comment is out of date. Should it be removed?

We have implemented destination-atop,so should we keep the comment for now & remove if we need to add source-atop also?

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.

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

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

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

> 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".

The chromium EWS bot doesn't run pixel tests currently.

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

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

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 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).

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

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 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.

This has been resolved in https://bugs.webkit.org/show_bug.cgi?id=66036. Can someone please mark it as RESOLVED DUPLICATE

OK. *** This bug has been marked as a duplicate of bug 66036 ***

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.