Bug 52868 - [chromium] Fix redundant video frame paint on CSS LayerChromium for <video>
Summary: [chromium] Fix redundant video frame paint on CSS LayerChromium for <video>
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-20 21:26 PST by Victoria Kirst
Modified: 2011-02-03 21:20 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.09 KB, patch)
2011-01-20 21:30 PST, Victoria Kirst
no flags Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2011-01-26 18:19 PST, Victoria Kirst
no flags Details | Formatted Diff | Diff
Patch (3.50 KB, patch)
2011-02-03 11:02 PST, Victoria Kirst
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victoria Kirst 2011-01-20 21:26:33 PST
[chromium] Fix redundant video frame paint on CSS LayerChromium for <video>
Comment 1 Victoria Kirst 2011-01-20 21:30:34 PST
Created attachment 79691 [details]
Patch
Comment 2 James Robinson 2011-01-24 17:51:40 PST
Comment on attachment 79691 [details]
Patch

Do paints from a hardware-accelerated video into a 2d canvas still work with this patch (and do we have a test for this)?  I'm not super familiar with that codepath for video.
Comment 3 James Robinson 2011-01-24 18:33:19 PST
Comment on attachment 79691 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:402
> +    // If we are using GPU to render video, ignore requests to paint frames into
> +    // canvas because it will be taken care of by VideoLayerChromium.
> +    if (acceleratedRenderingInUse())
> +        return;

Yeah, I'm pretty sure this will break painting from a h-w accelerated video into a 2d canvas.  Please check if we have a layout test to cover this case and if not please add one (create a <video> and a <canvas>, on the canvas' 2d context do .drawImage(video, ...), assert that the pixels show up).  I think the philip suite may have such a test.

2d and webgl canvases have the same issue.  The basic problem is that you want the paint() call to actually render when painting into a 2d canvas but you don't want it to happen when doing 'normal' rendering, so there's an explicit call in CanvasRenderingContext2D::drawImage() to differentiate the two: http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp#L1260.  makeRenderingResultsAvailable() ensures that the source canvas' ImageBuffer contains the actual bits of the source canvas, which is then drawn into the destination canvas.  I think you need something a bit different here, but whatever the code is it would go somewhere around here: http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp#L1337.
Comment 4 Vangelis Kokkevis 2011-01-25 10:20:33 PST
Comment on attachment 79691 [details]
Patch

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

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:402
>> +        return;
> 
> Yeah, I'm pretty sure this will break painting from a h-w accelerated video into a 2d canvas.  Please check if we have a layout test to cover this case and if not please add one (create a <video> and a <canvas>, on the canvas' 2d context do .drawImage(video, ...), assert that the pixels show up).  I think the philip suite may have such a test.
> 
> 2d and webgl canvases have the same issue.  The basic problem is that you want the paint() call to actually render when painting into a 2d canvas but you don't want it to happen when doing 'normal' rendering, so there's an explicit call in CanvasRenderingContext2D::drawImage() to differentiate the two: http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp#L1260.  makeRenderingResultsAvailable() ensures that the source canvas' ImageBuffer contains the actual bits of the source canvas, which is then drawn into the destination canvas.  I think you need something a bit different here, but whatever the code is it would go somewhere around here: http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp#L1337.

Good catch, James. 

Interestingly enough, MediaPlayerPrivateQuickTimeWin.cpp (used by Safari on windows), has a similar early out to what Victoria implemented on our side which likely means that video frames cannot be copied on canvas(?).  However, the mac version in MediaPlayerPrivateQTKit.mm does no such thing but it might be happening at some lower level within QT.

One possible solution (although not super clean) would be to add a flag to LayerRendererChromium that indicates whether it's updating layer contents.  WebMediaPlayerClientImpl can then check that via a call like: m_videoLayer->layerRenderer()->inLayerContentUpdate() to be able to differentiate between the two.
Comment 5 Victoria Kirst 2011-01-26 18:19:46 PST
Created attachment 80286 [details]
Patch
Comment 6 Victoria Kirst 2011-01-26 18:48:41 PST
Huh, there seems to be an easy fix for this!

So, normal video paint calls (i.e. originating from <video> software rendering) use the "paint()" method to render frames in a canvas. It seems like Canvas/WebGL use paintCurrentFrameIntoContext(), which by default just calls paint(). But of course we could just override it to behave slightly differently.

This patch has paintCurrentFrameIntoContext() do a software-paint into the given context blindly, while paint() does a check first to make sure we're not re-doing the work of VideoLayerChromium. Seems to work but I need to run layout tests to be confident, will update with results.
Comment 7 James Robinson 2011-01-27 13:56:17 PST
Comment on attachment 80286 [details]
Patch

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

Seems good.  Could you please confirm that this code change is hit by existing tests (at least some of the webgl ones) and that they still work with this patch applied?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:-486
> -
> -#if USE(ACCELERATED_COMPOSITING)
> -    Frame* frame = static_cast<HTMLMediaElement*>(
> -        client->m_mediaPlayer->mediaPlayerClient())->document()->frame();
> -
> -    // This does not actually check whether the hardware can support accelerated
> -    // compositing, but only if the flag is set. However, this is checked lazily
> -    // in WebViewImpl::setIsAcceleratedCompositingActive() and will fail there
> -    // if necessary.
> -    client->m_supportsAcceleratedCompositing =
> -        frame->contentRenderer()->compositor()->hasAcceleratedCompositing();
> -#endif

I don't understand this code move - is this part of the same patch?
Comment 8 Vangelis Kokkevis 2011-01-27 23:16:23 PST
(In reply to comment #6)
> Huh, there seems to be an easy fix for this!
> 
> So, normal video paint calls (i.e. originating from <video> software rendering) use the "paint()" method to render frames in a canvas. It seems like Canvas/WebGL use paintCurrentFrameIntoContext(), which by default just calls paint(). But of course we could just override it to behave slightly differently.
> 
> This patch has paintCurrentFrameIntoContext() do a software-paint into the given context blindly, while paint() does a check first to make sure we're not re-doing the work of VideoLayerChromium. Seems to work but I need to run layout tests to be confident, will update with results.


Great!  I think this will work.  Well done :)
Comment 9 Victoria Kirst 2011-02-03 11:02:27 PST
Created attachment 81088 [details]
Patch
Comment 10 Victoria Kirst 2011-02-03 11:07:57 PST
> I don't understand this code move - is this part of the same patch?

Yeah, that change should go in a different patch. Thanks! I uploaded the patch with that part omitted.

This patch is now ready to go, and the GPU layout tests and the canvas/webgl tests all pass, but from what I can tell the GPU tests don't seem to test the paths correctly (see email thread to chrome-gpu). Not quite sure what to do...
Comment 11 James Robinson 2011-02-03 11:18:26 PST
Comment on attachment 81088 [details]
Patch

R=me.  Let's investigate that test issue in parallel.
Comment 12 WebKit Review Bot 2011-02-03 12:03:20 PST
Comment on attachment 81088 [details]
Patch

Rejecting attachment 81088 [details] from commit-queue.

vrk@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 13 Victoria Kirst 2011-02-03 12:36:11 PST
Aww haha I don't have commit queue privileges. Can someone cq+ for me? Thanks! :D
Comment 14 WebKit Commit Bot 2011-02-03 21:20:16 PST
Comment on attachment 81088 [details]
Patch

Clearing flags on attachment: 81088

Committed r77599: <http://trac.webkit.org/changeset/77599>
Comment 15 WebKit Commit Bot 2011-02-03 21:20:22 PST
All reviewed patches have been landed.  Closing bug.