Bug 25417 – Skia CompositeCopy should be mapped to kSrc_Mode not kSrcOver_Mode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dean McNamee]

This is part of Chromium the bug:

http://code.google.com/p/chromium/issues/detail?id=6609

Currently CompositeCopy is mapped kSrcOver_Mode with a TODO:

WebCore/platform/graphics/skia/SkiaUtils.cpp:    { CompositeCopy,            SkPorterDuff::kSrcOver_Mode },  // TODO

[Dean McNamee]

Created attachment 29813 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 29813 [details]
Patch

Does this change results on a layout test?  If so, this change looks fine.  Otherwise we need a way to test it.

[Dean McNamee]

I think it should be covered by at least ./fast/canvas/canvas-composite.html, although I didn't test it specifically.  I tested it manually.

[Dean McNamee]

canvas-composite.html doesn't hit it because it doesn't use alpha.  I will have to create a new layout test.

[Eric Seidel (no email)]

Comment on attachment 29813 [details]
Patch

Thank you.  I will mark this r- for now, missing the layout test.  I'm happy to r+ and land your new patch with additional layout test (or modifications to the existing layout test).

[Shinichiro Hamaji]

Created attachment 30141 [details]
Adding layout test

 .../canvas/canvas-composite-alpha-expected.txt     |   82 ++++
 .../fast/canvas/canvas-composite-alpha.html        |  474 ++++++++++++++++++++
 WebCore/platform/graphics/skia/SkiaUtils.cpp       |    4 +-
 3 files changed, 558 insertions(+), 2 deletions(-)

[Shinichiro Hamaji]

Created attachment 30142 [details]
Adding layout test

 LayoutTests/ChangeLog                              |    9 +
 .../canvas/canvas-composite-alpha-expected.txt     |   82 ++++
 .../fast/canvas/canvas-composite-alpha.html        |  491 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   12 +
 WebCore/platform/graphics/skia/SkiaUtils.cpp       |    4 +-
 5 files changed, 596 insertions(+), 2 deletions(-)

[Shinichiro Hamaji]

I took the layout test work over from Dean. I added a layout test which exercises a bunch of canvas' composite operations with alpha blending. Could you take another look?

The test looks like

http://shinh.skr.jp/t/canvas-composite-alpha.html

I wanted to add more test cases, but I didn't actually add because there are several bugs (maybe) in several platforms. For example, WebKit on Mac seems to do nothing when alpha=0, but it may not be the right behavior. FYI, here is the version of the layout tests which I created at first:

http://shinh.skr.jp/t/canvas-composite-alpha-full.html

And, please ignore my previous patch. I uploaded a patch without staging ChangeLog.

Thanks!

(In reply to comment #7)
> Created an attachment (id=30142) [review]
> Adding layout test
> 
>  LayoutTests/ChangeLog                              |    9 +
>  .../canvas/canvas-composite-alpha-expected.txt     |   82 ++++
>  .../fast/canvas/canvas-composite-alpha.html        |  491 ++++++++++++++++++++
>  WebCore/ChangeLog                                  |   12 +
>  WebCore/platform/graphics/skia/SkiaUtils.cpp       |    4 +-
>  5 files changed, 596 insertions(+), 2 deletions(-)
> 

[Eric Seidel (no email)]

It's better to test too much than too little. :)  If we have other canvas related bugs it's best to get those tested, although those can be done via separate bugs/separate test cases if you prefer.

[Shinichiro Hamaji]

(In reply to comment #9)
> It's better to test too much than too little. :)  If we have other canvas
> related bugs it's best to get those tested, although those can be done via
> separate bugs/separate test cases if you prefer.
> 

Thanks for the quick response! Yes, I want to work on fixing other canvas issues, but I'd like to separate these bugs from this bug as it's difficult for me to fix all bugs at once.

[Shinichiro Hamaji]

Created attachment 30207 [details]
Adding layout test

 LayoutTests/ChangeLog                              |    9 +
 .../canvas/canvas-composite-alpha-expected.txt     |  148 ++++
 .../fast/canvas/canvas-composite-alpha.html        |  776 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   12 +
 WebCore/platform/graphics/skia/SkiaUtils.cpp       |    4 +-
 5 files changed, 947 insertions(+), 2 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 30207 [details]
Adding layout test

Added test cases except for alpha=0 case. When alpha is zero, some graphic engine does nothing for optimization but it's not correct behavior for some composite operations such as copy.
I also updated the function which compares the value of RGB in layout test so that this compares the actual colors users see (it compares [R*A, G*A, B*A] instead of [R, G, B]). I did this because Skia's outputs have some errors due to optimization for the cases alpha is small.

[Shinichiro Hamaji]

Could someone review this? Thanks,

[Tony Chang]

Comment on attachment 30142 [details]
Adding layout test

I believe the newest patch obsoletes this one.

[Eric Seidel (no email)]

Comment on attachment 30207 [details]
Adding layout test

I think this test would be much more readable if it used some JS to generate the HTML instead of all the copy/paste HTML.

 16       // FIXME: we don't exercise corner cases such as alpha=0 intentionally as many platforms are not good enough for now.

It's OK to check in failing tests.  I'd rather have the tests which fail on some platforms, with understood behavior, than to have no tests at all.

I find the arrays of colors confusing.  Giving i and j longer english names would be a start at helping.

If you didn't write this test, would you know what expectedColors[i][j]; was supposed to mean?

i an j shoudl be locally scoped.  No reason for them not to be.

This javascript doesn't match the WebKit coding style guidelines: http://webkit.org/coding/coding-style.html  However we're generally not as strict about code style for test cases.  Still... we should try. :)

So the test takes an array of color values, composites them with something, and then checks to make sure the array is correct.  Seems that a few clearer variable names and a little less HTML/copy paste would go a long way towards making this test more readable.

Otherwise the change looks fine.

[Shinichiro Hamaji]

Thanks for the review!

(In reply to comment #15)
> (From update of attachment 30207 [details] [review])
> I think this test would be much more readable if it used some JS to generate
> the HTML instead of all the copy/paste HTML.
> 
>  16       // FIXME: we don't exercise corner cases such as alpha=0
> intentionally as many platforms are not good enough for now.
> 
> It's OK to check in failing tests.  I'd rather have the tests which fail on
> some platforms, with understood behavior, than to have no tests at all.

I thought we cannot add a failing tests because the guideline is saying that all tests should pass.

http://webkit.org/coding/contributing.html

The test case for alpha=0 will fail even on Mac OSX Leopard. Is this acceptable? If so, I'll add test cases for these cases and upload another patch. Of course, I'll also fix other parts according to your comments.

Thanks,

> 
> I find the arrays of colors confusing.  Giving i and j longer english names
> would be a start at helping.
> 
> If you didn't write this test, would you know what expectedColors[i][j]; was
> supposed to mean?
> 
> i an j shoudl be locally scoped.  No reason for them not to be.
> 
> This javascript doesn't match the WebKit coding style guidelines:
> http://webkit.org/coding/coding-style.html  However we're generally not as
> strict about code style for test cases.  Still... we should try. :)
> 
> So the test takes an array of color values, composites them with something, and
> then checks to make sure the array is correct.  Seems that a few clearer
> variable names and a little less HTML/copy paste would go a long way towards
> making this test more readable.
> 
> Otherwise the change looks fine.
> 

[Eric Seidel (no email)]

By failing test, I mean that one of the sub-tests may fail, and your "expected results" will include that FAIL printed along next to a whole bunch of PASS messages.  That way if we ever do change how we handle alpha=0 on some platform then we'll know that we changed it, instead of now were we could "regress" alpha=0 handling w/o knowing it because we have no test for it.  Does that make sense?

Here is an example tests where we fail a few of the sub-tests, but we test them anyway (and give explanations as to why we fail) so that if anyone is making changes in that code later we'll already have tests for those changes:
http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Document/createElementNS-namespace-err-expected.txt

[Shinichiro Hamaji]

Created attachment 30457 [details]
Adding layout test

 LayoutTests/ChangeLog                              |    9 +
 .../canvas/canvas-composite-alpha-expected.txt     |  181 +++++++++
 .../fast/canvas/canvas-composite-alpha.html        |  390 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   12 +
 WebCore/platform/graphics/skia/SkiaUtils.cpp       |    4 +-
 5 files changed, 594 insertions(+), 2 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 30457 [details]
Adding layout test

Thanks for your description! Now I see what you meant.

I added tests for alpha=0 cases and add expected texts for these cases. I also fixed style issues you pointed. Now the layout test generate HTML table on the fly, some variables have better name, and JS style was fixed.

[Eric Seidel (no email)]

Comment on attachment 30457 [details]
Adding layout test

That's a very nice test case.  Thank you for the updates!  r=me

[Shinichiro Hamaji]

(In reply to comment #20)
> (From update of attachment 30457 [details] [review])
> That's a very nice test case.  Thank you for the updates!  r=me

Thanks for your review. Could you check this patch in for me? I'll send another change which fixes alpha=0 bugs.

[Eric Seidel (no email)]

Thanks for the patch!

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/canvas/canvas-composite-alpha-expected.txt
	A	LayoutTests/fast/canvas/canvas-composite-alpha.html
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/skia/SkiaUtils.cpp
Committed r43902

[Mark Rowe (bdash)]

This change landed a test with failing results.  Why does the expected results include failures?  On which platforms does the test completely pass?  Where is the bug that tracks fixing the failures on the platforms that it currently fails on?

[Mark Rowe (bdash)]

The test output in DRT is less than great as well — there's no indication which specific cases are failing, so I'm forced to examine the test in the browser to make sense of what is going on.  A good test makes it clear in the results what has gone wrong, and the manner in which it has done so.

[Shinichiro Hamaji]

(In reply to comment #23)
> This change landed a test with failing results.  Why does the expected results
> include failures?  On which platforms does the test completely pass?  Where is
> the bug that tracks fixing the failures on the platforms that it currently
> fails on?

Eric said that it's OK to add failing test in comment #17. Linux/Gtk/Cairo passes this test. I think I know how to fix this for Leopard and Skia. I'll file another bug and send a patch for failing test soon.

(In reply to comment #24)
> The test output in DRT is less than great as well  there's no indication which
> specific cases are failing, so I'm forced to examine the test in the browser to
> make sense of what is going on.  A good test makes it clear in the results what
> has gone wrong, and the manner in which it has done so.

Sorry for this. I'll fix the format of its output as well.

[Mark Rowe (bdash)]

Thanks for the update.

[Oliver Hunt]

One major concern i have is that this behavoiur (while correct) actually means that skia's transparency model is different from that expected by WebKit.

The blend modes should only effect the region being drawn to, yet currently skia has followed the cairo model and as rendering a number of modes as if it were drawing a clipregion sized piece of content, with 0 alpha outside the region that is being drawn to.

I'm hoping to fix the canvas implementation to do the right thing soon, but i am concerned that there may be cases (especially in svg) where skia and cairo end up doing the wrong thing.

--Oliver

[Shinichiro Hamaji]

FYI, I've filed another bug for fixing alpha=0 issue and error message format in the layout test.

https://bugs.webkit.org/show_bug.cgi?id=25512

[Shinichiro Hamaji]

> https://bugs.webkit.org/show_bug.cgi?id=25512

Oops! The URL was incorrect.

https://bugs.webkit.org/show_bug.cgi?id=25956