Bug 53857 – [Chromium] Layout Test canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

[Chromium] Layout Test canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas.

[Naoki Takano]

Created attachment 81369 [details]
Patch

[Naoki Takano]

Please review my patch.

[Kenneth Russell]

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

[Naoki Takano]

Created attachment 81581 [details]
Patch

[Naoki Takano]

Comment on attachment 81581 [details]
Patch

Please review it.

[James Robinson]

We probably need this for fillPath() as well.

[Naoki Takano]

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.

[Eric Seidel (no email)]

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?

[Naoki Takano]

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?

[Eric Seidel (no email)]

I'm not familiar with PlatformContextSkia::State::applyAlpha.  What's the purpose?

[Naoki Takano]

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?

[Eric Seidel (no email)]

Ah yes, I misread things.  This has nothing to do with the Color class as you correctly stated.

[Naoki Takano]

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.

[James Robinson]

Just add another patch to this bug that fixes both issues and updates the ChangeLog. Thanks!

[Naoki Takano]

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!

[Naoki Takano]

Created attachment 81729 [details]
Patch

[Naoki Takano]

Comment on attachment 81729 [details]
Patch

Please review.

[James Robinson]

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.

[Naoki Takano]

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.

[Naoki Takano]

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,

[Naoki Takano]

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.

[James Robinson]

In the meantime, this test could go in fast/canvas/

[Naoki Takano]

Ok, I'll do that.

(In reply to comment #22)
> In the meantime, this test could go in fast/canvas/

[Naoki Takano]

Created attachment 82095 [details]
Patch

[Naoki Takano]

Comment on attachment 82095 [details]
Patch

According to Jame's suggestion, I move my test, and change some checking routine.

[Naoki Takano]

Comment on attachment 82095 [details]
Patch

Could you reivew?

[James Robinson]

Comment on attachment 82095 [details]
Patch

R=me.  Thanks!

[WebKit Commit Bot]

Comment on attachment 82095 [details]
Patch

Clearing flags on attachment: 82095

Committed r78315: <http://trac.webkit.org/changeset/78315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.