Bug 52868 – [chromium] Fix redundant video frame paint on CSS LayerChromium for <video>

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

[chromium] Fix redundant video frame paint on CSS LayerChromium for <video>

[Victoria Kirst]

Created attachment 79691 [details]
Patch

[James Robinson]

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.

[James Robinson]

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.

[Vangelis Kokkevis]

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.

[Victoria Kirst]

Created attachment 80286 [details]
Patch

[Victoria Kirst]

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.

[James Robinson]

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?

[Vangelis Kokkevis]

(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 :)

[Victoria Kirst]

Created attachment 81088 [details]
Patch

[Victoria Kirst]

> 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...

[James Robinson]

Comment on attachment 81088 [details]
Patch

R=me.  Let's investigate that test issue in parallel.

[WebKit Review Bot]

Comment on attachment 81088 [details]
Patch

Rejecting attachment 81088 [details] from commit-queue.

[e-mail] 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.

[Victoria Kirst]

Aww haha I don't have commit queue privileges. Can someone cq+ for me? Thanks! :D

[WebKit Commit Bot]

Comment on attachment 81088 [details]
Patch

Clearing flags on attachment: 81088

Committed r77599: <http://trac.webkit.org/changeset/77599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.