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 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 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.
Created attachment 71804 [details] Proposed Patch
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.
Created attachment 71930 [details] Proposed Patch
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 on attachment 71930 [details] Proposed Patch This is a Chromium-only fix. Is it a Chromium-only bug?
(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 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?
Created attachment 72108 [details] Proposed Patch New patch that includes expected files for new layout test
(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 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.
Created attachment 72203 [details] Proposed Patch - Removed duplicate expectations - Created getNormalizedAlpha() method as suggested & updated code appropriately
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?
(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.
(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 on attachment 72203 [details] Proposed Patch Mm, OK
Comment on attachment 72203 [details] Proposed Patch Clearing flags on attachment: 72203 Committed r70919: <http://trac.webkit.org/changeset/70919>
All reviewed patches have been landed. Closing bug.
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.
(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.
(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.
(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.
(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
http://trac.webkit.org/changeset/70919 might have broken Qt Linux Release The following tests are not passing: media/video-canvas-alpha.html