Bug 48291 – Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.copy.html

• Component: Canvas
• Status: RESOLVED DUPLICATE

[Mike Lawther]

This layout tests fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506

[Swapna P]

Created attachment 95568 [details]
Patch to check for CompositeCopy to paint transparency for remaining canvas area

The existing code was only looking for CompositeSourceIn/CompositeSourceOut while painting transparency for the remaining canvas area.We have also added to check for CompositeCopy

[Julien Chaffraix]

Comment on attachment 95568 [details]
Patch to check for CompositeCopy to paint transparency for remaining canvas area

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

> Source/WebCore/ChangeLog:9
> +        transparency for remaining canvas area

The spec does not mention that we need to display transparency everywhere for copy. Could you explain why this is needed? Should the spec be changed or the test itself?

> Source/WebCore/ChangeLog:11
> +        Test: LayoutTests\canvas\philip\tests\2d.composite.uncovered.fill.copy.html

This test is skipped on most platform. You need to re-enable it by removing it from the Skipped & test_expectations.txt files for all platforms.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #2)
> (From update of attachment 95568 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95568&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        transparency for remaining canvas area
> 
> The spec does not mention that we need to display transparency everywhere for copy. Could you explain why this is needed? Should the spec be changed or the test itself?

Yes, even I did not find anything explicitly mentioned in the Spec. I went by what is mentioned in test case.

The spec mentions how Copy/fillRect should behave, but it does not mention what is the expected behavior in failure cases i.e. destination rect area out side the canvas area.

Can anyone having more depth knowledge of the spec clarify this please?

[Julien Chaffraix]

> The spec mentions how Copy/fillRect should behave, but it does not mention what is the expected behavior in failure cases i.e. destination rect area out side the canvas area.
> 
> Can anyone having more depth knowledge of the spec clarify this please?

It looks like the spec is silent on this point. However Philip's test suite seems to be testing exactly that. Ian, what is your reading on this? Should we report that on the WHATWG or somewhere else?

[Ian 'Hixie' Hickson]

What's the question, exactly?

[Mustafizur Rahaman (rahaman)]

(In reply to comment #5)
> What's the question, exactly?

Hi Ian, The HTML Canvas spec (http://www.w3.org/TR/2dcontext/#compositing) does not explicitly mentions what would be the exact behavior in case of an invalid copy operation. E.g. if i am copying an image to an area which is out side the valid canvas area, what would happen to my canvas area? Should it contain whatever was already painted or the canvas area would be transparent or something else?

[Ian 'Hixie' Hickson]

The drawing model is described here:

   http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model

For _all_ the composition operators ('copy' isn't special here) the drawing model composites an infinite image over the canvas. The whole canvas is always affected.

[Mustafizur Rahaman (rahaman)]

As per discussion with Ian in IRC, this is a valid test case. So would any one be able to review this patch?

[Julien Chaffraix]

(In reply to comment #8)
> As per discussion with Ian in IRC, this is a valid test case. So would any one be able to review this patch?

I mentioned previously that this patch is missing an update of the platforms' Skipped files as you are fixing a test-case. The fix makes sense once taken into account the drawing model (a comment about that would be nice).

[Swapna P]

Created attachment 96720 [details]
Patch after incorporating julien's review comments.

Hi Julien,

I have removed the relevant test case from skipped list [I forgot to do this before as I was completely involved in the discussion regarding the expected spec behavior with you & Ian :-(]. 

Also I have put a comment for my changes regarding the reference to drawing model as per your suggestion.

[Swapna P]

Comment on attachment 96720 [details]
Patch after incorporating julien's review comments.

cancelling the review by myself, as EWS reported problem applying the patch.

Will investigate & upload the correct patch.

[Mustafizur Rahaman (rahaman)]

Created attachment 96749 [details]
Updated Patch

Updated patch after fixing the coding style issue & incorporating Julien's comment

[Darin Adler]

Comment on attachment 96749 [details]
Updated Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1512
> +    // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model
> +    // We should display transparency for everywhere else for CompositeCopy, too

Where does it say that? Are you sure that's right?

Section 4.8.11.1.3 mentions this for source-atop, source-in, and source-out. But not for copy.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #13)
> (From update of attachment 96749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96749&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1512
> > +    // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model
> > +    // We should display transparency for everywhere else for CompositeCopy, too
> 
> Where does it say that? Are you sure that's right?
> 
> Section 4.8.11.1.3 mentions this for source-atop, source-in, and source-out. But not for copy.

Darin, Please refer to Ian's comment above [https://bugs.webkit.org/show_bug.cgi?id=48291#c7 ]. Also I had discussed with Ian in IRC to confirm this.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 96749 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96749&action=review
> > 
> > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1512
> > > +    // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model
> > > +    // We should display transparency for everywhere else for CompositeCopy, too
> > 
> > Where does it say that? Are you sure that's right?
> > 
> > Section 4.8.11.1.3 mentions this for source-atop, source-in, and source-out. But not for copy.
> 
> Darin, Please refer to Ian's comment above [https://bugs.webkit.org/show_bug.cgi?id=48291#c7 ]. Also I had discussed with Ian in IRC to confirm this.

Hi Darin, Do you want me to explain this further with more details or is it Ok to review this change now?

[Darin Adler]

(In reply to comment #15)
> Hi Darin, Do you want me to explain this further with more details or is it Ok to review this change now?

You’ve answered my question.

Someone can review your patch at any time. Since your patch did not apply in the EWS you probably want to make a new version that successfully applies.

When I do get around to reviewing I will probably want to think a little further about whether there are any other modes that are handled wrong. Not reviewing right now.

Someone else is welcome to review; please don’t wait for me just because I made a comment.

[Mustafizur Rahaman (rahaman)]

Created attachment 97068 [details]
Recreated patch on the latest code base

As the previous patch was not applied by the EWS, re-created the patch again.

[WebKit Review Bot]

Attachment 97068 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mustafizur Rahaman (rahaman)]

Created attachment 97070 [details]
Fixed the "tabs" in Changelog reported by EWS in previous patch

Fixed the style ("tab") issue reported by EWS

[WebKit Review Bot]

Comment on attachment 97070 [details]
Fixed the "tabs" in Changelog reported by EWS in previous patch

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

New failing tests:
fast/canvas/canvas-composite-alpha.html

[WebKit Review Bot]

Created attachment 97077 [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

[Mustafizur Rahaman (rahaman)]

Created attachment 97097 [details]
Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html

Updated patch after rebaselining the failed test case

[Julien Chaffraix]

Comment on attachment 97097 [details]
Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html

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

> Source/WebCore/ChangeLog:11
> +        Test: LayoutTests\canvas\philip\tests\2d.composite.uncovered.fill.copy.html 

Looks like you forgot to regenerate your changelog as it should mention canvas-composite-alpha.html here.

> LayoutTests/ChangeLog:8
> +        from failed/skipped test list

For information specific to a file, we usually prefer to put them next or along to the file name(s) below in the ChangeLog. I usually put a global description here if it makes sense (like for big patches to explain the gist of it). See the other ChangeLog entries to see how other people do.

[Darin Adler]

Comment on attachment 97097 [details]
Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html

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

>> Source/WebCore/ChangeLog:11
>> +        Test: LayoutTests\canvas\philip\tests\2d.composite.uncovered.fill.copy.html 
> 
> Looks like you forgot to regenerate your changelog as it should mention canvas-composite-alpha.html here.

We don’t use backslashes for paths. We use forward slashes.

[Mustafizur Rahaman (rahaman)]

Created attachment 97332 [details]
Incorporated review comments of Julien(comment#23) & Darin(comment#24)

Updated patch after incorporating Julien & Darin's review comment

[Oliver Hunt]

Comment on attachment 97332 [details]
Incorporated review comments of Julien(comment#23) & Darin(comment#24)

this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes?  This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc).  Have we also verified that this behaviour matches other browsers?

[Mustafizur Rahaman (rahaman)]

(In reply to comment #26)
> (From update of attachment 97332 [details])
> this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes?  This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc).  Have we also verified that this behaviour matches other browsers?

I haven't verified the performance related aspect because to be compliant with the spec behavior, we need this change (as per my understanding).

Secondly, I have verified that this behavior after my changes match with FF4 & Opera 11.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #26)
> (From update of attachment 97332 [details])
> this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes?  This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc).  Have we also verified that this behaviour matches other browsers?

Regarding your comment ("I suspect that at the moment we're taking a huge hit if you do a copy into the canvas that fills the entire canvas") during our discussion on IRC, I did some further debugging & found that if my copy is filling entire canvas, i am not clearing out unnecessarily. So, as per my observation, we are only clearing that area which we are not going to fill by our copy operation.

Can mdelaney/kling please comment here?

[Mustafizur Rahaman (rahaman)]

(In reply to comment #26)
> (From update of attachment 97332 [details])
> this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes?  This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc).  Have we also verified that this behaviour matches other browsers?

Hi Oliver,

I have tried a CompositeCopy on a 1000*500 canvas [copying to the entire canvas region] on WebKit windows build [Winlauncher]. In each iterations, I am doing fillRect for 100 times & this whole exercise I have done for 10 iterations to get a stable result[loading the page 10 times and noting the result].

With Changes: Total time taken(sum of 10 iterations) = 1139 ms ==> avg=1.13 ms per fillRect

WithOUT Changes: Total time taken(sum of 10 iterations) = 1020 ms ==> avg=1.02 ms per fillRect.

Please let me know your opinion & whether you want any modification in the patch/in the existing logic of this filling transparency elsewhere.

[Mustafizur Rahaman (rahaman)]

Created attachment 98157 [details]
Test case to take performance figure for CompositeCopy

Attaching the test case I have used to take the performance figure for CompositeCopy

[Mustafizur Rahaman (rahaman)]

(In reply to comment #30)
> Created an attachment (id=98157) [details]
> Test case to take performance figure for CompositeCopy
> 
> Attaching the test case I have used to take the performance figure for CompositeCopy

The above test case is modified version of 2d.composite.uncovered.fill.copy.html which should be run from LayoutTests/canvas/philip/tests/

[James Robinson]

Comment on attachment 97332 [details]
Incorporated review comments of Julien(comment#23) & Darin(comment#24)

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

I'd be interesting in seeing perf comparisons that do not cover the entire canvas.  If the entire canvas is being copied then the clipOut() will exclude the whole canvas so that fill will be a no-op, so it's not surprising that performance is unaffected.  I'm also not sure about using clipOut for copy - it seems that for a path that isn't exactly on pixel boundaries this will cause issues with antialiasing.

If the clip region is small relative to the canvas, would it be more efficient to just clear the whole canvas without setting a clipOut at all before the actual draw?  On GPU implementations I could imagine this doing quite well.

This will change the pixel output for fast/canvas/canvas-composite.html, at least.  Could you update the pixel expectations for at least one platform so we can see that it's correct?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1514
> +    // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model
> +    // We should display transparency for everywhere else for CompositeCopy, too

This comment does not seem useful, the comment on 1510 explains why we are displaying transparency elsewhere

[Ben Wells]

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