Bug 48094 – Setting globalAlpha on canvas and drawing a video does not honor alpha value

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

[Aaron Colwell]

Created attachment 71500 [details]
Proposed Patch

Drawing a video frame on a canvas always uses an alpha of 1 independent of what globalAlpha is set to on the canvas' 2D context.

For more info see this Chromium bug http://code.google.com/p/chromium/issues/detail?id=51941

[Andrew Scherkus]

Comment on attachment 71500 [details]
Proposed Patch

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

few style nits but we should consider a layout test

> WebKit/chromium/ChangeLog:7
> +

no mention of any test

perhaps we should write a new pixel layout test?

> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:403
> +        if (alpha > 255) {

webkit style:
  - no braces on single line if statements
  - indents are 4 spaces, not 2

> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:409
> +        canvas->saveLayerAlpha(0, alpha);

so WebCanvas alpha is in 0-255, but the platform GraphicsContext is in float?!

> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:415
>          m_webMediaPlayer->paint(context->platformContext(), rect);

this code is for mac (CG = CoreGraphics) and I'm guessing we'll need to do something similar here

[Aaron Colwell]

Comment on attachment 71500 [details]
Proposed Patch

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

>> WebKit/chromium/ChangeLog:7
>> +
> 
> no mention of any test
> 
> perhaps we should write a new pixel layout test?

new patch has a layout test now.

>> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:403
>> +        if (alpha > 255) {
> 
> webkit style:
>   - no braces on single line if statements
>   - indents are 4 spaces, not 2

fixed

>> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:409
>> +        canvas->saveLayerAlpha(0, alpha);
> 
> so WebCanvas alpha is in 0-255, but the platform GraphicsContext is in float?!

yes. Essentially copied this code from another part of the codebase.

>> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:415
>>          m_webMediaPlayer->paint(context->platformContext(), rect);
> 
> this code is for mac (CG = CoreGraphics) and I'm guessing we'll need to do something similar here

Verified that the issue doesn't exist on Mac. The context is passed down on Mac instead of the raw canvas.

[Aaron Colwell]

Created attachment 71804 [details]
Proposed Patch

[Hin-Chung Lam]

Comment on attachment 71804 [details]
Proposed Patch

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

> WebKit/chromium/ChangeLog:5
> +        Fix globalAlpha support when using drawImage() to copy a video frame to a 2D canvas context.A

Remove stray A at the end.

> WebKit/chromium/ChangeLog:7
> +

After this empty line add some description about how this fix works.

> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:400
> +        int alpha = round(256 * context->platformContext()->getAlpha());

shouldn't this be 255? If alpha is 1.0 then the value is 256.

> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:-514
> -#endif  // ENABLE(VIDEO)

don't need to fix style in WebKit code.

> LayoutTests/media/video-canvas-alpha.html:30
> +   <video controls="true"></video>

Prefer pixel test without controls, since each time there's changes to controls you need to generate new images.

Also just <video controls> is enough.

[Aaron Colwell]

Created attachment 71930 [details]
Proposed Patch

[Aaron Colwell]

Comment on attachment 71804 [details]
Proposed Patch

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

Made suggested changes and posted a new patch.

>> WebKit/chromium/ChangeLog:5
>> +        Fix globalAlpha support when using drawImage() to copy a video frame to a 2D canvas context.A
> 
> Remove stray A at the end.

done

>> WebKit/chromium/ChangeLog:7
>> +
> 
> After this empty line add some description about how this fix works.

done

>> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:400
>> +        int alpha = round(256 * context->platformContext()->getAlpha());
> 
> shouldn't this be 255? If alpha is 1.0 then the value is 256.

It seemed a little odd to me as well, but I copied this from the code that is used for images.
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/skia/ImageSkia.cpp#L239

>> WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:-514
>> -#endif  // ENABLE(VIDEO)
> 
> don't need to fix style in WebKit code.

done

>> LayoutTests/media/video-canvas-alpha.html:30
>> +   <video controls="true"></video>
> 
> Prefer pixel test without controls, since each time there's changes to controls you need to generate new images.
> 
> Also just <video controls> is enough.

done

[Darin Adler]

Comment on attachment 71930 [details]
Proposed Patch

This is a Chromium-only fix. Is it a Chromium-only bug?

[Aaron Colwell]

(In reply to comment #7)
> (From update of attachment 71930 [details])
> This is a Chromium-only fix. Is it a Chromium-only bug?

I believe so. Chromium is the only thing I've looked at. The fix also only applies to the non-mac versions of Chromium because the Mac code actually uses the platform context instead of the raw skia canvas.

[James Robinson]

Comment on attachment 71930 [details]
Proposed Patch

This looks like a pixel test, so it needs expectations (video-canvas-alpha-expected.txt, .checksum, and .png).

Are you sure about that rounding/clipping logic?  Is it needed?  Do we do this anywhere else?

[Aaron Colwell]

Created attachment 72108 [details]
Proposed Patch

New patch that includes expected files for new layout test

[Aaron Colwell]

(In reply to comment #9)
> (From update of attachment 71930 [details])
> This looks like a pixel test, so it needs expectations (video-canvas-alpha-expected.txt, .checksum, and .png).

I've submitted a new patch that contains the expectation files.

> 
> Are you sure about that rounding/clipping logic?  Is it needed?  Do we do this anywhere else?

The clipping logic is needed especially since no clipping is enforced in the setAlpha() on the platform context. This code is modeled after the following pieces of code that I ran into when trying to see how <img>s are drawn to the canvas under similar circumstances.

http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/skia/ImageSkia.cpp#L239
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/skia/PlatformContextSkia.cpp#L196

[James Robinson]

Comment on attachment 72108 [details]
Proposed Patch

Based on the checksums the windows and linux expectations are dups of each other - please remove the chromium-linux version (it'll fall back to chromium-win if there is nothing in chromium-linux).

If we already have this rounding logic twice, it really should be shared somewhere.  Could you put this somewhere shared (maybe another getter on PlatformContextSkia - getNormalizedAlpha() - and change ImageSkia.cpp to use that as well)?

R- for the dupe expectations.

[Aaron Colwell]

Created attachment 72203 [details]
Proposed Patch

- Removed duplicate expectations
- Created getNormalizedAlpha() method as suggested & updated code appropriately

[James Robinson]

Comment on attachment 72203 [details]
Proposed Patch

Looks good!  Can you please patch PlatformContextSkia::State::applyAlpha() to use getNormalizeAlpha() as well so there's only one copy of that logic?

[Aaron Colwell]

(In reply to comment #14)
> (From update of attachment 72203 [details])
> Looks good!  Can you please patch PlatformContextSkia::State::applyAlpha() to use getNormalizeAlpha() as well so there's only one copy of that logic?

Thanks. I'm a little wary of updating applyAlpha() because this case is slightly different. It isn't doing a simple clip. This code appears to have special behavior if m_alpha is outside of [0-1.0]. I don't really understand why someone did this but the code looks deliberate.

[Aaron Colwell]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 72203 [details] [details])
> > Looks good!  Can you please patch PlatformContextSkia::State::applyAlpha() to use getNormalizeAlpha() as well so there's only one copy of that logic?
> 
> Thanks. I'm a little wary of updating applyAlpha() because this case is slightly different. It isn't doing a simple clip. This code appears to have special behavior if m_alpha is outside of [0-1.0]. I don't really understand why someone did this but the code looks deliberate.

Ok. I have a better answer for why applyAlpha() shouldn't be updated to use getNormalizedAlpha(). 

The rounding in the 2 functions are used for different purposes. In getNormalizedAlpha() the code simply translates the [0-1] range to [0-255]. In applyAlpha() the rounding is used to determine how many 256ths to multiply the color's alpha by. In applyAlpha() you need to be able to differentiate between 255 & 256. 256 indicates that you don't need to multiply and 255 means that you need to multiply the color's alpha by 255/256. I don't think it would make the code clearer if I changed it to use getNormalizedAlpha() and then added an extra check to differentiate the 255 vs 256 cases.

[James Robinson]

Comment on attachment 72203 [details]
Proposed Patch

Mm, OK

[WebKit Commit Bot]

Comment on attachment 72203 [details]
Proposed Patch

Clearing flags on attachment: 72203

Committed r70919: <http://trac.webkit.org/changeset/70919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This check-in caused the Qt and GTK bots to start failing, because the test requires video. This needs to be added to the Skipped list, presumably. Aaron, you should ideally fix this yourself, putting a patch for the issue into a new bug report.

[Aaron Colwell]

(In reply to comment #20)
> This check-in caused the Qt and GTK bots to start failing, because the test requires video. This needs to be added to the Skipped list, presumably. Aaron, you should ideally fix this yourself, putting a patch for the issue into a new bug report.

I don't understand what you are asking me to do. The little "qt" & "gtk" boxes next to the patch are green. Isn't that supposed to indicate whether I broke something? Can you point me to instructions on how to do what you want. Also what should I do differently to avoid this in the future.

[James Robinson]

(In reply to comment #21)
> (In reply to comment #20)
> > This check-in caused the Qt and GTK bots to start failing, because the test requires video. This needs to be added to the Skipped list, presumably. Aaron, you should ideally fix this yourself, putting a patch for the issue into a new bug report.
> 
> I don't understand what you are asking me to do. The little "qt" & "gtk" boxes next to the patch are green. Isn't that supposed to indicate whether I broke something? Can you point me to instructions on how to do what you want. Also what should I do differently to avoid this in the future.

Those boxes indicate that this patch compiled successfully on qt and gtk, not that the tests pass.  On http://build.webkit.org/console?category=core you can see that the qt and gtk tests went red at this revision.  I believe at some point those boxes will also run tests but I'm not sure what the status of that is.

To avoid this in the future, you need to watch the waterfall after your patch lands and see if there are any regressions.

[Darin Adler]

(In reply to comment #21)
> (In reply to comment #20)
> > This check-in caused the Qt and GTK bots to start failing, because the test requires video. This needs to be added to the Skipped list, presumably. Aaron, you should ideally fix this yourself, putting a patch for the issue into a new bug report.
> 
> I don't understand what you are asking me to do. The little "qt" & "gtk" boxes next to the patch are green. Isn't that supposed to indicate whether I broke something? Can you point me to instructions on how to do what you want. Also what should I do differently to avoid this in the future.

As James Robinson explained, the EWS checks to see if you break compiling on other platforms. Your patch compiles on those platforms, but introduced tests that fail on those platforms.

The instructions for this are on <http://webkit.org/coding/contributing.html> in the section “Keeping the tree green”. The instructions are part of the project too, so if you find they are missing something we can take patches to add what you learn to them.

You may need to get on IRC and talk to some Qt and GTK experts to learn what to do and exactly how to do it. Or you may need 1-on-1 help from someone with experience landing WebKit patches.

[Aaron Colwell]

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > This check-in caused the Qt and GTK bots to start failing, because the test requires video. This needs to be added to the Skipped list, presumably. Aaron, you should ideally fix this yourself, putting a patch for the issue into a new bug report.
> > 
> > I don't understand what you are asking me to do. The little "qt" & "gtk" boxes next to the patch are green. Isn't that supposed to indicate whether I broke something? Can you point me to instructions on how to do what you want. Also what should I do differently to avoid this in the future.
> 
> As James Robinson explained, the EWS checks to see if you break compiling on other platforms. Your patch compiles on those platforms, but introduced tests that fail on those platforms.
> 
> The instructions for this are on <http://webkit.org/coding/contributing.html> in the section “Keeping the tree green”. The instructions are part of the project too, so if you find they are missing something we can take patches to add what you learn to them.
> 
> You may need to get on IRC and talk to some Qt and GTK experts to learn what to do and exactly how to do it. Or you may need 1-on-1 help from someone with experience landing WebKit patches.

ok. Sorry about this. Newbie mistake. The patch is available here
https://bugs.webkit.org/show_bug.cgi?id=48688

[WebKit Review Bot]

http://trac.webkit.org/changeset/70919 might have broken Qt Linux Release
The following tests are not passing:
media/video-canvas-alpha.html