Bug 114742

Summary: [texmap] draw content if graphics layer displays a video
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cmarcelo, commit-queue, darin, esprehn+autocc, koivisto, luiz, noam, pnormand, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 86410    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
dont' update drawsContents if contents is media
none
Patch
none
Patch
none
Patch none

Description Víctor M. Jáquez L. 2013-04-17 07:30:04 PDT
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.
Comment 1 Víctor M. Jáquez L. 2013-04-17 08:00:35 PDT
Created attachment 198511 [details]
Patch
Comment 2 Víctor M. Jáquez L. 2013-04-17 08:26:54 PDT
Created attachment 198528 [details]
Patch
Comment 3 Simon Fraser (smfr) 2013-04-17 09:08:00 PDT
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.
Comment 4 Víctor M. Jáquez L. 2013-04-18 10:31:30 PDT
Created attachment 198745 [details]
Patch
Comment 5 Víctor M. Jáquez L. 2013-04-22 07:41:23 PDT
Ping?
Comment 6 Simon Fraser (smfr) 2013-04-22 09:31:50 PDT
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.
Comment 7 Víctor M. Jáquez L. 2013-04-25 08:34:41 PDT
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.
Comment 8 Víctor M. Jáquez L. 2013-04-25 10:02:38 PDT
Created attachment 199679 [details]
Patch
Comment 9 Build Bot 2013-04-25 10:29:11 PDT
Comment on attachment 199679 [details]
Patch

Attachment 199679 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/38564
Comment 10 Víctor M. Jáquez L. 2013-04-25 11:06:32 PDT
(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.
Comment 11 Víctor M. Jáquez L. 2013-04-26 10:09:13 PDT
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
Comment 12 Simon Fraser (smfr) 2013-04-26 10:38:15 PDT
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?
Comment 13 Víctor M. Jáquez L. 2013-04-26 11:12:16 PDT
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.
Comment 14 Víctor M. Jáquez L. 2013-05-02 07:41:33 PDT
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.
Comment 15 Víctor M. Jáquez L. 2013-05-02 07:45:33 PDT
(In reply to comment #14)

missing text:

> Every time a frame is updated, 

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

> Hence..
Comment 16 Noam Rosenthal 2013-05-02 07:59:20 PDT
(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.
Comment 17 Víctor M. Jáquez L. 2013-05-03 02:53:47 PDT
Created attachment 200398 [details]
Patch
Comment 18 Víctor M. Jáquez L. 2013-05-03 03:01:55 PDT
Created attachment 200400 [details]
Patch
Comment 19 Noam Rosenthal 2013-05-03 06:32:23 PDT
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.
Comment 20 Víctor M. Jáquez L. 2013-05-03 09:50:27 PDT
Created attachment 200427 [details]
Patch
Comment 21 Víctor M. Jáquez L. 2013-05-03 09:56:36 PDT
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
Comment 22 Víctor M. Jáquez L. 2013-05-06 07:09:54 PDT
Simon, may I ask for your review? and cq+, if it is possible. :)

Thanks!
Comment 23 Noam Rosenthal 2013-05-06 07:49:46 PDT
(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.
Comment 24 Víctor M. Jáquez L. 2013-05-06 08:01:47 PDT
(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.
Comment 25 WebKit Commit Bot 2013-05-06 08:41:53 PDT
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: ap@webkit.org, jochen@chromium.org, and rniwa@webkit.org)
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html bug 114208 (authors: abarth@webkit.org and rniwa@webkit.org)
media/video-played-collapse.html bug 58630 (authors: annacc@chromium.org, jamesr@chromium.org, pnormand@igalia.com, and vrk@chromium.org)
fast/loader/javascript-url-in-object.html bug 114210 (authors: rniwa@webkit.org and sam@webkit.org)
platform/mac/editing/deleting/deletionUI-single-instance.html bug 114181 (author: rniwa@webkit.org)
svg/animations/smil-leak-elements.svg bug 114280 (authors: fmalita@chromium.org and timothy_horton@apple.com)
svg/animations/smil-leak-dynamically-added-element-instances.svg bug 114281 (authors: fmalita@chromium.org and timothy_horton@apple.com)
transitions/color-transition-rounding.html bug 114182 (author: simon.fraser@apple.com)
transitions/cubic-bezier-overflow-svg-length.html bug 114183 (author: peter@chromium.org)
transitions/interrupt-zero-duration.html bug 114184 (authors: cmarrin@apple.com, rniwa@webkit.org, and simon.fraser@apple.com)
transitions/multiple-background-transitions.html bug 114185 (author: simon.fraser@apple.com)
transitions/cubic-bezier-overflow-color.html bug 114186 (author: peter@chromium.org)
transitions/mismatched-shadow-transitions.html bug 114188 (author: simon.fraser@apple.com)
transitions/color-transition-all.html bug 114189 (authors: ossy@webkit.org and simon.fraser@apple.com)
transitions/cubic-bezier-overflow-shadow.html bug 114191 (author: peter@chromium.org)
transitions/min-max-width-height-transitions.html bug 114192 (author: simon.fraser@apple.com)
transitions/cancel-transition.html bug 114193 (authors: ojan@chromium.org, rniwa@webkit.org, and simon.fraser@apple.com)
transitions/border-radius-transition.html bug 114194 (author: simon.fraser@apple.com)
transitions/flex-transitions.html bug 114195 (author: tony@chromium.org)
transitions/mixed-type.html bug 114196 (author: mikelawther@chromium.org)
transitions/multiple-mask-transitions.html bug 114197 (author: simon.fraser@apple.com)
transitions/color-transition-premultiplied.html bug 114198 (author: simon.fraser@apple.com)
transitions/mismatched-shadow-styles.html bug 114199 (author: simon.fraser@apple.com)
transitions/mask-transitions.html bug 114200 (authors: ojan@chromium.org, oliver@apple.com, and simon.fraser@apple.com)
transitions/cubic-bezier-overflow-length.html bug 114201 (author: peter@chromium.org)
transitions/multiple-background-size-transitions.html bug 114202 (authors: mitz@webkit.org and simon.fraser@apple.com)
transitions/clip-transition.html bug 114203 (authors: dglazkov@chromium.org, krit@webkit.org, and simon.fraser@apple.com)
transitions/cubic-bezier-overflow-transform.html bug 114204 (author: peter@chromium.org)
transitions/interrupted-accelerated-transition.html bug 56242 (authors: rniwa@webkit.org, simon.fraser@apple.com, and tonyg@chromium.org)
transitions/background-transitions.html bug 114206 (author: simon.fraser@apple.com)
The commit-queue is continuing to process your patch.
Comment 26 WebKit Commit Bot 2013-05-06 08:42:47 PDT
Comment on attachment 200427 [details]
Patch

Clearing flags on attachment: 200427

Committed r149599: <http://trac.webkit.org/changeset/149599>
Comment 27 WebKit Commit Bot 2013-05-06 08:42:52 PDT
All reviewed patches have been landed.  Closing bug.