Bug 53857 - [Chromium] Layout Test canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas.
Summary: [Chromium] Layout Test canvas/philip/tests/2d.composite.globalAlpha.fill.html...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Naoki Takano
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-05 12:08 PST by Naoki Takano
Modified: 2011-02-10 22:38 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.63 KB, patch)
2011-02-05 12:12 PST, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2011-02-07 21:28 PST, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2011-02-08 19:00 PST, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2011-02-10 20:37 PST, Naoki Takano
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Naoki Takano 2011-02-05 12:08:28 PST
[Chromium] Layout Test canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas.
Comment 1 Naoki Takano 2011-02-05 12:12:26 PST
Created attachment 81369 [details]
Patch
Comment 2 Naoki Takano 2011-02-05 12:14:31 PST
Please review my patch.
Comment 3 Kenneth Russell 2011-02-07 15:21:33 PST
Comment on attachment 81369 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:103
> +#if PLATFORM(SKIA)
> +        int a = SkAlphaMul(c.alpha(), s);
> +#else
> +        int a = (c.alpha() * s) >> 8;
> +#endif

The Skia-specific code path is not really necessary here. I'd just remove it.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:104
> +        return Color((c.rgb() & 0x00FFFFFF) | (a << 24));

I think you should avoid relying on Color's internal representation. You could write this portably as

return Color(c.red(), c.green(), c.blue(), a);
Comment 4 Naoki Takano 2011-02-07 21:28:09 PST
Created attachment 81581 [details]
Patch
Comment 5 Naoki Takano 2011-02-07 21:28:33 PST
Comment on attachment 81581 [details]
Patch

Please review it.
Comment 6 James Robinson 2011-02-07 21:39:57 PST
We probably need this for fillPath() as well.
Comment 7 Naoki Takano 2011-02-07 23:22:05 PST
So do you mean I have to fix at the same time?

Or do I have to make another bug entry?

(In reply to comment #6)
> We probably need this for fillPath() as well.
Comment 8 Eric Seidel (no email) 2011-02-07 23:25:22 PST
Comment on attachment 81581 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:90
> +    Color applyAlpha(const Color& c)

So this is just premultiplying?  Should this be a member on Color instead?
Comment 9 Naoki Takano 2011-02-07 23:35:33 PST
Can I add the function into the Color.h? Because Color class is very primitive, so I think we need to be careful.

As I wrote in the comment, applyAlpha() is almost the same as PlatformContextSkia::State::applyAlpha(). But the type is difference.
If PlatformContextSkia::State::applyAlpha() were in SkColor, I would include the function into Color.h.

What do you think?

(In reply to comment #8)
> (From update of attachment 81581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81581&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:90
> > +    Color applyAlpha(const Color& c)
> 
> So this is just premultiplying?  Should this be a member on Color instead?
Comment 10 Eric Seidel (no email) 2011-02-07 23:41:21 PST
I'm not familiar with PlatformContextSkia::State::applyAlpha.  What's the purpose?
Comment 11 Naoki Takano 2011-02-07 23:51:27 PST
The purpose is to multiply globalAlpha() to fillStyle alpha.

So if only software rendering is used, applyAlpha() is correctly applied.
But if hardware is used, applyAlpha() is not called, and globalAlpha() is not applied(). That is the problem here.

(In reply to comment #10)
> I'm not familiar with PlatformContextSkia::State::applyAlpha.  What's the purpose?
Comment 12 Eric Seidel (no email) 2011-02-07 23:57:43 PST
Ah yes, I misread things.  This has nothing to do with the Color class as you correctly stated.
Comment 13 Naoki Takano 2011-02-08 00:20:55 PST
James,

You are right, I confirmed the bug with my new test case.

So how should I treat the bug? Should I make another entry?

(In reply to comment #7)
> So do you mean I have to fix at the same time?
> 
> Or do I have to make another bug entry?
> 
> (In reply to comment #6)
> > We probably need this for fillPath() as well.
Comment 14 James Robinson 2011-02-08 00:53:30 PST
Just add another patch to this bug that fixes both issues and updates the ChangeLog. Thanks!
Comment 15 Naoki Takano 2011-02-08 12:26:03 PST
Also I will add another test related to fillPaht() problem.

(In reply to comment #14)
> Just add another patch to this bug that fixes both issues and updates the ChangeLog. Thanks!
Comment 16 Naoki Takano 2011-02-08 19:00:51 PST
Created attachment 81729 [details]
Patch
Comment 17 Naoki Takano 2011-02-08 19:01:19 PST
Comment on attachment 81729 [details]
Patch

Please review.
Comment 18 James Robinson 2011-02-08 19:04:00 PST
The canvas/philip/ directory contains an imported test suite from Philip Taylor.  Instead of modifying that directory, you should ask him to add your test to the suite or add the new test in some other directory. Otherwise it'll be difficult to update our copy of the suite in the future.
Comment 19 Naoki Takano 2011-02-08 19:33:53 PST
I see.

I didn't know the policy how to add a test.

Thank you for letting me know.

(In reply to comment #18)
> The canvas/philip/ directory contains an imported test suite from Philip Taylor.  Instead of modifying that directory, you should ask him to add your test to the suite or add the new test in some other directory. Otherwise it'll be difficult to update our copy of the suite in the future.
Comment 20 Naoki Takano 2011-02-08 19:37:02 PST
Taylor,

Can I add my test case as my upload patch?

The new test is
canvas/philip/tests/2d.composite.globalAlpha.fillPath.html
In the patch, I fixed fillRect() but also fillPath().
My new test case is for fillPath().

Thanks,
Comment 21 Naoki Takano 2011-02-09 20:18:41 PST
James,

It looks like hard to contact Taylor.

If he doesn't like to include my test case, which directory is the good place to go?
Do you have any idea?

Thanks,

(In reply to comment #18)
> The canvas/philip/ directory contains an imported test suite from Philip Taylor.  Instead of modifying that directory, you should ask him to add your test to the suite or add the new test in some other directory. Otherwise it'll be difficult to update our copy of the suite in the future.
Comment 22 James Robinson 2011-02-10 13:33:01 PST
In the meantime, this test could go in fast/canvas/
Comment 23 Naoki Takano 2011-02-10 13:54:37 PST
Ok, I'll do that.

(In reply to comment #22)
> In the meantime, this test could go in fast/canvas/
Comment 24 Naoki Takano 2011-02-10 20:37:35 PST
Created attachment 82095 [details]
Patch
Comment 25 Naoki Takano 2011-02-10 20:38:35 PST
Comment on attachment 82095 [details]
Patch

According to Jame's suggestion, I move my test, and change some checking routine.
Comment 26 Naoki Takano 2011-02-10 20:38:56 PST
Comment on attachment 82095 [details]
Patch

Could you reivew?
Comment 27 James Robinson 2011-02-10 20:46:56 PST
Comment on attachment 82095 [details]
Patch

R=me.  Thanks!
Comment 28 WebKit Commit Bot 2011-02-10 22:38:52 PST
Comment on attachment 82095 [details]
Patch

Clearing flags on attachment: 82095

Committed r78315: <http://trac.webkit.org/changeset/78315>
Comment 29 WebKit Commit Bot 2011-02-10 22:38:59 PST
All reviewed patches have been landed.  Closing bug.