Bug 75551 – [chromium][aura] WebExternalTextureLayerImpl::drawsContent() returns incorrect value, causing accelerated content to not display in Aura desktop

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[W. James MacLean]

As of http://trac.webkit.org/changeset/103990, rendering accelerated content in a browser window on the Aura desktop is broken. Instead of displaying the content, a uniform grey background is displayed instead.

To reproduce, compile chromium with "use_aura=1", and attempt to view any accelerated content (e.g. poster circle).

[W. James MacLean]

As a starting point, reverting the change to WebExternalTextureLayerImpl.cpp:54, so that it is just

return !!textureId();

instead of

return !!textureId() && LayerChromium::drawsContent();

causes the problem to go away *in Aura* (I haven't tested this elsewhere). It seems to suggest that this class is not correctly informing the base class about the drawing of content.

[W. James MacLean]

Created attachment 121117 [details]
Patch

[W. James MacLean]

It seems that PluginLayerChromium never calls setIsDrawable(), leaving a default of false when you call LayerChromium::drawsContent(). Shawn, does this look reasonable?

[Shawn Singh]

(In reply to comment #3)
> It seems that PluginLayerChromium never calls setIsDrawable(), leaving a default of false when you call LayerChromium::drawsContent(). Shawn, does this look reasonable?

Yeah, setIsDrawable needs to be explicitly called for any layers that intend to display stuff.  good catch =)

However, I don't think layers should set their own isDrawable by calling it.  that should happen somewhere by whoever owns the reference to this layer.  For example, in webcore this occurs in GraphicsLayer (i.e. GraphicsLayerChromium).  So, I would guess that wherever aura creates its layers that end up being PluginLayerChromium types, that's where this initialization should occur.

[James Robinson]

Comment on attachment 121117 [details]
Patch

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

Tests please.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Nope, this will fail an SVN presubmit check. You need to remove this line and ideally write some tests covering this code change.

[W. James MacLean]

(In reply to comment #5)
> (From update of attachment 121117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121117&action=review
> 
> Tests please.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> Nope, this will fail an SVN presubmit check. You need to remove this line and ideally write some tests covering this code change.

Tests for sure, but I wanted some feedback on the approach first :-)

[W. James MacLean]

Created attachment 121127 [details]
Patch

[W. James MacLean]

Comment on attachment 121127 [details]
Patch

Does this look OK?

If so, will a simple unit test suffice that creates a WebExternalTextureLayer and sets the drawability then checks it was set ok suffice?

[James Robinson]

Comment on attachment 121127 [details]
Patch

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

Is this what you meant, Shawn?

> Source/WebKit/chromium/src/WebExternalTextureLayer.cpp:49
> +void WebExternalTextureLayer::setIsDrawable(bool drawsContent)

why isn't this bool called "drawable"?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Shawn Singh]

What I was expecting the following:  WebExternalTextureLayer owns a WebExternalTextureLayerImpl that is essentially a PluginLayerChromium.  So, if we want the WebExternalTextureLayer to assume that its Impl always drawable, it would unwrap the PluginLayerChromium and call setIsDrawable in the constructor of WebExternalTextureLayer.

I wouldn't expect that we need to modify the public API.  I'm also not clear how setIsDrawable ever gets called, anyway, with this patch?   But maybe I'm completely missing something here, since I'm not entirely familiar with this part of code =)

wjmaclean, will it be reasonable to call setIsDrawable in the WebExternalTextureLayer constructor?

[W. James MacLean]

(In reply to comment #11)
> What I was expecting the following:  WebExternalTextureLayer owns a WebExternalTextureLayerImpl that is essentially a PluginLayerChromium.  So, if we want the WebExternalTextureLayer to assume that its Impl always drawable, it would unwrap the PluginLayerChromium and call setIsDrawable in the constructor of WebExternalTextureLayer.
> 
> I wouldn't expect that we need to modify the public API.  I'm also not clear how setIsDrawable ever gets called, anyway, with this patch?   But maybe I'm completely missing something here, since I'm not entirely familiar with this part of code =)

In ui/gfx/compositor/layer.cc .

> wjmaclean, will it be reasonable to call setIsDrawable in the WebExternalTextureLayer constructor?

Sure, I just though you wanted the layer to be set isDrawable in Aura (as per your comment #4), which is on the "chromium side of the fence", hence the modification to the public API.

So it's OK just to have the layer to default to isDrawable() in the contructor?

[Shawn Singh]

(In reply to comment #12)
> (In reply to comment #11)
> > What I was expecting the following:  WebExternalTextureLayer owns a WebExternalTextureLayerImpl that is essentially a PluginLayerChromium.  So, if we want the WebExternalTextureLayer to assume that its Impl always drawable, it would unwrap the PluginLayerChromium and call setIsDrawable in the constructor of WebExternalTextureLayer.
> > 
> > I wouldn't expect that we need to modify the public API.  I'm also not clear how setIsDrawable ever gets called, anyway, with this patch?   But maybe I'm completely missing something here, since I'm not entirely familiar with this part of code =)
> 
> In ui/gfx/compositor/layer.cc .
> 
> > wjmaclean, will it be reasonable to call setIsDrawable in the WebExternalTextureLayer constructor?
> 
> Sure, I just though you wanted the layer to be set isDrawable in Aura (as per your comment #4), which is on the "chromium side of the fence", hence the modification to the public API.
> 
> So it's OK just to have the layer to default to isDrawable() in the contructor?

Yeah, sorry if I was unclear, as far as I see I think that's the right place.  I'm making the distinction between WebExternalTextureLayer and the WebExternalTextureLayerImpl class, since one owns the other, very much like how GraphicsLayerChromium owns various LayerChromiums.

[W. James MacLean]

(In reply to comment #13)
> 
> > Sure, I just though you wanted the layer to be set isDrawable in Aura (as per your comment #4), which is on the "chromium side of the fence", hence the modification to the public API.
> > 
> > So it's OK just to have the layer to default to isDrawable() in the contructor?
> 
> Yeah, sorry if I was unclear, as far as I see I think that's the right place.  I'm making the distinction between WebExternalTextureLayer and the WebExternalTextureLayerImpl class, since one owns the other, very much like how GraphicsLayerChromium owns various LayerChromiums.

OK, new patch in a few minutes ...

[W. James MacLean]

Created attachment 121136 [details]
Patch

[W. James MacLean]

How does this look?

[Shawn Singh]

Comment on attachment 121136 [details]
Patch

Looks good to me; one very trivial nit.

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

> Source/WebKit/chromium/tests/WebLayerTest.cpp:81
> +    : WebExternalTextureLayer(node)

style nit: this should probably be indented by 4 more spaces.

[W. James MacLean]

(In reply to comment #17)
> (From update of attachment 121136 [details])
> Looks good to me; one very trivial nit.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=121136&action=review
> 
> > Source/WebKit/chromium/tests/WebLayerTest.cpp:81
> > +    : WebExternalTextureLayer(node)
> 
> style nit: this should probably be indented by 4 more spaces.

Sure, np!

Before I resubmit the patch, did you have anything James?

[James Robinson]

Comment on attachment 121136 [details]
Patch

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

Nope, I think this looks good except for style nits

> Source/WebKit/chromium/tests/WebLayerTest.cpp:30
>  #include "CompositorFakeWebGraphicsContext3D.h"
> +#include "WebExternalTextureLayerImpl.h"
>  #include "WebCompositor.h"

this doesn't look sorted correctly

> Source/WebKit/chromium/tests/WebLayerTest.cpp:67
> +class WebExternalTextureLayerImplTest: public WebExternalTextureLayerImpl {

space before the :

> Source/WebKit/chromium/tests/WebLayerTest.cpp:72
> +class WebExternalTextureLayerTest: public WebExternalTextureLayer {

space before the :

> Source/WebKit/chromium/tests/WebLayerTest.cpp:79
> +private:

newline before private: please

> Source/WebKit/chromium/tests/WebLayerTest.cpp:80
> +    WebExternalTextureLayerTest(const PassRefPtr<WebExternalTextureLayerImpl>& node)

explicit, please

[W. James MacLean]

Created attachment 121146 [details]
Patch

[W. James MacLean]

(In reply to comment #19)
> (From update of attachment 121136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121136&action=review
> 
> Nope, I think this looks good except for style nits
> 
> > Source/WebKit/chromium/tests/WebLayerTest.cpp:30
> >  #include "CompositorFakeWebGraphicsContext3D.h"
> > +#include "WebExternalTextureLayerImpl.h"
> >  #include "WebCompositor.h"
> 
> this doesn't look sorted correctly

Done.

> > Source/WebKit/chromium/tests/WebLayerTest.cpp:67
> > +class WebExternalTextureLayerImplTest: public WebExternalTextureLayerImpl {
> 
> space before the :

Done.

> > Source/WebKit/chromium/tests/WebLayerTest.cpp:72
> > +class WebExternalTextureLayerTest: public WebExternalTextureLayer {
> 
> space before the :

Done.

> > Source/WebKit/chromium/tests/WebLayerTest.cpp:79
> > +private:
> 
> newline before private: please

Done.

> > Source/WebKit/chromium/tests/WebLayerTest.cpp:80
> > +    WebExternalTextureLayerTest(const PassRefPtr<WebExternalTextureLayerImpl>& node)
> 
> explicit, please

Done.

[James Robinson]

Comment on attachment 121146 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 121146 [details]
Patch

Attachment 121146 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11110285

[W. James MacLean]

Created attachment 121275 [details]
Patch

[W. James MacLean]

(In reply to comment #23)
> (From update of attachment 121146 [details])
> Attachment 121146 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11110285

This compiles and runs fine when I use "make webkit_unit_tests" on my development machine, but it looks as if the EWS bot is perhaps expecting WEBKIT_IMPLEMENTATION to be defined? 

The error

Source/WebKit/chromium/tests/WebLayerTest.cpp:82: error: no matching function for call to 'WebKit::WebExternalTextureLayer::WebExternalTextureLayer(const WTF::PassRefPtr<WebKit::WebExternalTextureLayerImpl>&)'

suggests it's not seeing the definition inside the #if WEBKIT_IMPLEMENTATION in WebExternalTextureLayer.h, but the corresponding definition inside WebExternalTextureLayer.cpp isn't inside the same #if.

Also, I've updated the patch to be a bit more explicit about the distinction between WebExternalTextureLayerImpl* and WebExternalTextureLayerImplTest*.

[W. James MacLean]

Created attachment 121306 [details]
Patch

[W. James MacLean]

Revised approach, with impl-specific test.

[James Robinson]

Comment on attachment 121306 [details]
Patch

R=me - looks great

[WebKit Review Bot]

Comment on attachment 121306 [details]
Patch

Rejecting attachment 121306 [details] from commit-queue.

New failing tests:
http/tests/inspector/network/download.html
Full output: http://queues.webkit.org/results/11145068

[James Robinson]

Comment on attachment 121306 [details]
Patch

Looks like flake, trying again

[WebKit Review Bot]

Comment on attachment 121306 [details]
Patch

Clearing flags on attachment: 121306

Committed r104209: <http://trac.webkit.org/changeset/104209>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.