Bug 25417 - Skia CompositeCopy should be mapped to kSrc_Mode not kSrcOver_Mode
Summary: Skia CompositeCopy should be mapped to kSrc_Mode not kSrcOver_Mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25956
  Show dependency treegraph
 
Reported: 2009-04-27 01:33 PDT by Dean McNamee
Modified: 2009-05-22 05:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.24 KB, patch)
2009-04-27 01:37 PDT, Dean McNamee
eric: review-
Details | Formatted Diff | Diff
Adding layout test (29.00 KB, text/plain)
2009-05-08 14:12 PDT, Shinichiro Hamaji
no flags Details
Adding layout test (30.96 KB, patch)
2009-05-08 14:22 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Adding layout test (52.34 KB, patch)
2009-05-11 16:03 PDT, Shinichiro Hamaji
eric: review-
Details | Formatted Diff | Diff
Adding layout test (32.91 KB, patch)
2009-05-18 19:56 PDT, Shinichiro Hamaji
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean McNamee 2009-04-27 01:33:57 PDT
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
Comment 1 Dean McNamee 2009-04-27 01:37:13 PDT
Created attachment 29813 [details]
Patch
Comment 2 Eric Seidel (no email) 2009-04-27 10:08:33 PDT
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.
Comment 3 Dean McNamee 2009-04-27 10:17:24 PDT
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.
Comment 4 Dean McNamee 2009-04-27 10:29:52 PDT
canvas-composite.html doesn't hit it because it doesn't use alpha.  I will have to create a new layout test.
Comment 5 Eric Seidel (no email) 2009-04-27 10:37:00 PDT
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).
Comment 6 Shinichiro Hamaji 2009-05-08 14:12:29 PDT
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(-)
Comment 7 Shinichiro Hamaji 2009-05-08 14:22:40 PDT
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(-)
Comment 8 Shinichiro Hamaji 2009-05-08 14:29:47 PDT
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(-)
> 

Comment 9 Eric Seidel (no email) 2009-05-08 15:42:50 PDT
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.
Comment 10 Shinichiro Hamaji 2009-05-08 15:51:04 PDT
(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.
Comment 11 Shinichiro Hamaji 2009-05-11 16:03:39 PDT
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(-)
Comment 12 Shinichiro Hamaji 2009-05-11 16:11:17 PDT
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.
Comment 13 Shinichiro Hamaji 2009-05-13 10:40:06 PDT
Could someone review this? Thanks,
Comment 14 Tony Chang 2009-05-14 16:04:06 PDT
Comment on attachment 30142 [details]
Adding layout test

I believe the newest patch obsoletes this one.
Comment 15 Eric Seidel (no email) 2009-05-14 16:27:07 PDT
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.
Comment 16 Shinichiro Hamaji 2009-05-18 09:34:17 PDT
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.
> 

Comment 17 Eric Seidel (no email) 2009-05-18 17:20:24 PDT
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
Comment 18 Shinichiro Hamaji 2009-05-18 19:56:32 PDT
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(-)
Comment 19 Shinichiro Hamaji 2009-05-18 20:01:11 PDT
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.
Comment 20 Eric Seidel (no email) 2009-05-18 20:32:01 PDT
Comment on attachment 30457 [details]
Adding layout test

That's a very nice test case.  Thank you for the updates!  r=me
Comment 21 Shinichiro Hamaji 2009-05-19 09:02:39 PDT
(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.
Comment 22 Eric Seidel (no email) 2009-05-20 05:26:30 PDT
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
Comment 23 Mark Rowe (bdash) 2009-05-20 19:29:19 PDT
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?
Comment 24 Mark Rowe (bdash) 2009-05-20 19:32:29 PDT
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.
Comment 25 Shinichiro Hamaji 2009-05-20 22:09:29 PDT
(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.
Comment 26 Mark Rowe (bdash) 2009-05-20 22:12:41 PDT
Thanks for the update.
Comment 27 Oliver Hunt 2009-05-20 22:49:14 PDT
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
Comment 28 Shinichiro Hamaji 2009-05-22 03:04:11 PDT
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
Comment 29 Shinichiro Hamaji 2009-05-22 03:08:26 PDT
> https://bugs.webkit.org/show_bug.cgi?id=25512

Oops! The URL was incorrect.

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