Bug 48094 - Setting globalAlpha on canvas and drawing a video does not honor alpha value
Summary: Setting globalAlpha on canvas and drawing a video does not honor alpha value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-21 15:04 PDT by Aaron Colwell
Modified: 2010-10-29 15:33 PDT (History)
9 users (show)

See Also:


Attachments
Proposed Patch (1.74 KB, patch)
2010-10-21 15:04 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff
Proposed Patch (3.70 KB, patch)
2010-10-25 15:43 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff
Proposed Patch (3.77 KB, patch)
2010-10-26 12:46 PDT, Aaron Colwell
jamesr: review-
Details | Formatted Diff | Diff
Proposed Patch (210.97 KB, patch)
2010-10-27 16:24 PDT, Aaron Colwell
jamesr: review-
Details | Formatted Diff | Diff
Proposed Patch (144.77 KB, patch)
2010-10-28 10:24 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Colwell 2010-10-21 15:04:10 PDT
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
Comment 1 Andrew Scherkus 2010-10-22 11:52:27 PDT
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
Comment 2 Aaron Colwell 2010-10-25 15:43:17 PDT
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.
Comment 3 Aaron Colwell 2010-10-25 15:43:26 PDT
Created attachment 71804 [details]
Proposed Patch
Comment 4 Hin-Chung Lam 2010-10-26 11:39:28 PDT
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.
Comment 5 Aaron Colwell 2010-10-26 12:46:29 PDT
Created attachment 71930 [details]
Proposed Patch
Comment 6 Aaron Colwell 2010-10-26 12:50:58 PDT
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
Comment 7 Darin Adler 2010-10-26 12:52:09 PDT
Comment on attachment 71930 [details]
Proposed Patch

This is a Chromium-only fix. Is it a Chromium-only bug?
Comment 8 Aaron Colwell 2010-10-26 13:01:48 PDT
(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.
Comment 9 James Robinson 2010-10-27 11:19:26 PDT
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?
Comment 10 Aaron Colwell 2010-10-27 16:24:50 PDT
Created attachment 72108 [details]
Proposed Patch

New patch that includes expected files for new layout test
Comment 11 Aaron Colwell 2010-10-27 16:28:42 PDT
(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
Comment 12 James Robinson 2010-10-27 16:57:24 PDT
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.
Comment 13 Aaron Colwell 2010-10-28 10:24:05 PDT
Created attachment 72203 [details]
Proposed Patch

- Removed duplicate expectations
- Created getNormalizedAlpha() method as suggested & updated code appropriately
Comment 14 James Robinson 2010-10-29 11:18:46 PDT
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?
Comment 15 Aaron Colwell 2010-10-29 11:41:13 PDT
(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.
Comment 16 Aaron Colwell 2010-10-29 13:18:48 PDT
(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.
Comment 17 James Robinson 2010-10-29 13:29:14 PDT
Comment on attachment 72203 [details]
Proposed Patch

Mm, OK
Comment 18 WebKit Commit Bot 2010-10-29 13:49:55 PDT
Comment on attachment 72203 [details]
Proposed Patch

Clearing flags on attachment: 72203

Committed r70919: <http://trac.webkit.org/changeset/70919>
Comment 19 WebKit Commit Bot 2010-10-29 13:50:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Darin Adler 2010-10-29 14:35:30 PDT
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.
Comment 21 Aaron Colwell 2010-10-29 14:48:11 PDT
(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.
Comment 22 James Robinson 2010-10-29 14:50:55 PDT
(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.
Comment 23 Darin Adler 2010-10-29 15:22:08 PDT
(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.
Comment 24 Aaron Colwell 2010-10-29 15:24:46 PDT
(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
Comment 25 WebKit Review Bot 2010-10-29 15:33:46 PDT
http://trac.webkit.org/changeset/70919 might have broken Qt Linux Release
The following tests are not passing:
media/video-canvas-alpha.html