Bug 114742 – [texmap] draw content if graphics layer displays a video

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Víctor M. Jáquez L.]

This patch comes from bug #86410, but I decided to split it, because this part needs more attention since touches the rendering.

When the video is rendered through the texture mapper, the graphics layer needs to be displayed as a whole (the complete frame), not just a section of it. We need to enable this behavior, since currently the frame are not displayed if the cursor is not above the video area.

[Víctor M. Jáquez L.]

Created attachment 198511 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 198528 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 198528 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:1883
> +    if (m_graphicsLayer) {
> +        if (m_graphicsLayer->hasContentsLayer())
> +            m_graphicsLayer->setContentsNeedsDisplay();
> +        else if (m_graphicsLayer->drawsContent())
> +            m_graphicsLayer->setNeedsDisplay();
> +    }

Calling setContentsNeedsDisplay is undesirable on platforms that don't need it. It will cauase the contents layer to get backing store in cases where it shouldn't have it (e.g. background color).

You also need to do a setNeedsDisplay() on the main graphics layer even if you have a contents layer, since the main graphics layer may be painting backgrounds and borders.

[Víctor M. Jáquez L.]

Created attachment 198745 [details]
Patch

[Víctor M. Jáquez L.]

Ping?

[Simon Fraser (smfr)]

Comment on attachment 198745 [details]
Patch

These changes don't really make sense for other platforms (certainly for Mac). I think you're going to have to abstract out the way your contents layers are different from Mac, maybe via  GraphicsLayer::contentsLayerDrawsContent() or something.

[Víctor M. Jáquez L.]

Thanks a lot Simon for your kind reviews.

I realize that I've failed to explain correctly the bug in question.

What we are trying to accomplish (bug #86410) is a composited/accelerated video, using GStreamer and the TextureMapper.

Right now we have a prototype rendering 1080p videos using gstreamer-vaapi and the TextureMapper. You can watch a demo here http://youtu.be/ekt3oj5oApo

But we found an issue that we traced up to RenderLayerBacking: the video stops of being painted in the texture-mapper layer when no media-control is drawn above it (or, in other words, when the mouse cursor is not above of the video area).

The initial patch was based on a patch from Arnaud Renevier for the bug #86410, but this approach, as Simon reviewed, is too invasive for other platforms.

So we dug a bit more in the issue, and we found that the m_graphicsLayer (the texturemapper instance) has set its drawContent() to false when the media control is not drawn. Then the problem is, if I understand it correctly, to keep drawContent() true while the video is being played.

[Víctor M. Jáquez L.]

Created attachment 199679 [details]
Patch

[Build Bot]

Comment on attachment 199679 [details]
Patch

Attachment 199679 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/38564

[Víctor M. Jáquez L.]

(In reply to comment #9)
> (From update of attachment 199679 [details])
> Attachment 199679 [details] did not pass win-ews (win):
> Output: http://webkit-queues.appspot.com/results/38564

This doesn't seem to be caused by this patch.

[Víctor M. Jáquez L.]

Created attachment 199840 [details]
dont' update drawsContents if contents is media

This is another approach to this bug, but limiting the changes to GraphicsLayerTextureMapper.

If ::setContentsToMedia() is called, then ::setDrawsContent() halts if it is set to false.

Sadly this approach somewhat goes in opposite direction of the bug #109658

[Simon Fraser (smfr)]

Comment on attachment 199679 [details]
Patch

This is also wrong. I'm not sure why you think that having accelerated video means that you have to call setDrawsContent() on the main graphics layer.

The graphics layer can have a contents layer for various reasons: video, WebGL, image, solid color. On Mac at least, none of those types of content layer need to be explicitly drawn (although WebGL does need a hint that the contents have changed sometimes).

In the TextureMapper case, does the video go into a contents layer? In this case, is it considered like painted content that needs backing store?

[Víctor M. Jáquez L.]

Hi Simon,

Thanks your patience. 

(In reply to comment #12)
> (From update of attachment 199679 [details])
> This is also wrong. I'm not sure why you think that having accelerated video means that you have to call setDrawsContent() on the main graphics layer.

That seems to me more "logical", but yeah, I spent all this day trying to figure out what mac does, and I've realized that what I'm proposing it is not right.

> 
> The graphics layer can have a contents layer for various reasons: video, WebGL, image, solid color. On Mac at least, none of those types of content layer need to be explicitly drawn (although WebGL does need a hint that the contents have changed sometimes).

Yes, I saw that today.

> 
> In the TextureMapper case, does the video go into a contents layer?

AFAIK, Yes it does.

In bug #86410, we defined the MediaPlayerPrivate as a TextureMapperPlatformLayer. After updating the texture with the video buffer, the MediaPlayerPrivate will request a ::repaint() to the RenderObject.

Here's a sequence diagram which help to understand the code flow:

http://people.igalia.com/vjaquez/images/repaint-seq.png

But the buffer won't be draw if setDrawContents() is set to false, and that happens when no media controls are drawn.


> In this case, is it considered like painted content that needs backing store?

Sorry, I don't follow your question.

[Víctor M. Jáquez L.]

After chatting a bit with Simon, he told me that

20:25 < smfr> so when m_texture->updateContents() is called, something should just tell AcceleratedCompositingContextGL to just render a new scene
20:25 < smfr> you shouldn't be running webcore painting code for every frame

I reviewed how WebKitGTK+ paints WebGL and here's a backtrace:

http://people.igalia.com/vjaquez/files/webgl-traceback.txt

Every time a frame is updated, 

Hence, if I understand correctly, we should call WebCore::GraphicsLayerTextureMapper:setContentsNeedsDisplay when a new frame is available, directly from WebCore::MediaPlayerPrivateGStreamerBase

The question is how WebCore::MediaPlayerPrivateGStreamerBase would access to WebCore::GraphicsLayerTextureMapper, or more abstractly: how MediaPlayerPrivate would access GraphicsLayer in an AC setup.

[Víctor M. Jáquez L.]

(In reply to comment #14)

missing text:

> Every time a frame is updated, 

... WebCore::RenderLayer::contentChanged is called.

> Hence..

[Noam Rosenthal]

(In reply to comment #14)
> After chatting a bit with Simon, he told me that
> 
> 20:25 < smfr> so when m_texture->updateContents() is called, something should just tell AcceleratedCompositingContextGL to just render a new scene
> 20:25 < smfr> you shouldn't be running webcore painting code for every frame
> 
> I reviewed how WebKitGTK+ paints WebGL and here's a backtrace:
> 
> http://people.igalia.com/vjaquez/files/webgl-traceback.txt
> 
> Every time a frame is updated, 
> 
> Hence, if I understand correctly, we should call WebCore::GraphicsLayerTextureMapper:setContentsNeedsDisplay when a new frame is available, directly from WebCore::MediaPlayerPrivateGStreamerBase
> 
> The question is how WebCore::MediaPlayerPrivateGStreamerBase would access to WebCore::GraphicsLayerTextureMapper, or more abstractly: how MediaPlayerPrivate would access GraphicsLayer in an AC setup.

I suggest adding a "Client" to TextureMapperPlatformLayer, that would have something like a setLayerNeedsDisplay. the TextureMapperLayer would inherit from that client and MediaPlayerPrivateGStreamerBase would invoke it when it's ready.

[Víctor M. Jáquez L.]

Created attachment 200398 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 200400 [details]
Patch

[Noam Rosenthal]

Comment on attachment 200400 [details]
Patch

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

Where is the code that uses the new function?

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:112
> +    virtual void setLayerNeedsDisplay() { setContentsNeedsDisplay(); }

Let's call this setPlatformLayerNeedsDisplay?
Also add OVERRIDE.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:47
> +    void setClient(TextureMapperPlatformLayer::Client* client) { m_client = client; }

Maybe new line before and after?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:56
> +    void setLayerNeedsDisplay()

I think this should be done in the actual layer implementation.

[Víctor M. Jáquez L.]

Created attachment 200427 [details]
Patch

[Víctor M. Jáquez L.]

Hi Noam!

Thanks a lot for your review.

(In reply to comment #19)
> (From update of attachment 200400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200400&action=review
> 
> Where is the code that uses the new function?

It is used in this patch for bug #86410

https://bugs.webkit.org/attachment.cgi?id=200421&action=review

> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:112
> > +    virtual void setLayerNeedsDisplay() { setContentsNeedsDisplay(); }
> 
> Let's call this setPlatformLayerNeedsDisplay?
> Also add OVERRIDE.

Done

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:47
> > +    void setClient(TextureMapperPlatformLayer::Client* client) { m_client = client; }
> 
> Maybe new line before and after?

Done

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:56
> > +    void setLayerNeedsDisplay()
> 
> I think this should be done in the actual layer implementation.

Done

[Víctor M. Jáquez L.]

Simon, may I ask for your review? and cq+, if it is possible. :)

Thanks!

[Noam Rosenthal]

(In reply to comment #22)
> Simon, may I ask for your review? and cq+, if it is possible. :)
> 
> Thanks!

I've already r+ed this, if you want cq? you should set the flag.

[Víctor M. Jáquez L.]

(In reply to comment #23)
> (In reply to comment #22)
> > Simon, may I ask for your review? and cq+, if it is possible. :)
> > 
> > Thanks!
> 
> I've already r+ed this, if you want cq? you should set the flag.

D'oh! Done! Thanks.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 200427 [details]:

http/tests/security/cookies/third-party-cookie-blocking-user-action.html bug 114511 (authors: [e-mail], [e-mail], and [e-mail])
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html bug 114208 (authors: [e-mail] and [e-mail])
media/video-played-collapse.html bug 58630 (authors: [e-mail], [e-mail], [e-mail], and [e-mail])
fast/loader/javascript-url-in-object.html bug 114210 (authors: [e-mail] and [e-mail])
platform/mac/editing/deleting/deletionUI-single-instance.html bug 114181 (author: [e-mail])
svg/animations/smil-leak-elements.svg bug 114280 (authors: [e-mail] and [e-mail])
svg/animations/smil-leak-dynamically-added-element-instances.svg bug 114281 (authors: [e-mail] and [e-mail])
transitions/color-transition-rounding.html bug 114182 (author: [e-mail])
transitions/cubic-bezier-overflow-svg-length.html bug 114183 (author: [e-mail])
transitions/interrupt-zero-duration.html bug 114184 (authors: [e-mail], [e-mail], and [e-mail])
transitions/multiple-background-transitions.html bug 114185 (author: [e-mail])
transitions/cubic-bezier-overflow-color.html bug 114186 (author: [e-mail])
transitions/mismatched-shadow-transitions.html bug 114188 (author: [e-mail])
transitions/color-transition-all.html bug 114189 (authors: [e-mail] and [e-mail])
transitions/cubic-bezier-overflow-shadow.html bug 114191 (author: [e-mail])
transitions/min-max-width-height-transitions.html bug 114192 (author: [e-mail])
transitions/cancel-transition.html bug 114193 (authors: [e-mail], [e-mail], and [e-mail])
transitions/border-radius-transition.html bug 114194 (author: [e-mail])
transitions/flex-transitions.html bug 114195 (author: [e-mail])
transitions/mixed-type.html bug 114196 (author: [e-mail])
transitions/multiple-mask-transitions.html bug 114197 (author: [e-mail])
transitions/color-transition-premultiplied.html bug 114198 (author: [e-mail])
transitions/mismatched-shadow-styles.html bug 114199 (author: [e-mail])
transitions/mask-transitions.html bug 114200 (authors: [e-mail], [e-mail], and [e-mail])
transitions/cubic-bezier-overflow-length.html bug 114201 (author: [e-mail])
transitions/multiple-background-size-transitions.html bug 114202 (authors: [e-mail] and [e-mail])
transitions/clip-transition.html bug 114203 (authors: [e-mail], [e-mail], and [e-mail])
transitions/cubic-bezier-overflow-transform.html bug 114204 (author: [e-mail])
transitions/interrupted-accelerated-transition.html bug 56242 (authors: [e-mail], [e-mail], and [e-mail])
transitions/background-transitions.html bug 114206 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 200427 [details]
Patch

Clearing flags on attachment: 200427

Committed r149599: <http://trac.webkit.org/changeset/149599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.