Bug 93010 – [chromium] Add WebExternalTextureProvider; an alternate means for providing textures to a TextureLayerChromium

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Sean Hunt]

This is inteded as part of the browser plugin implementation, which will use this design to allow its texture handling to occur on the compositor thread when the threaded compositor is active rather than on the main thread.

This is roughly a clone of WebVideoFrameProvider, except without various considerations such as multi-threaded code.

I have not moved existing clients over but allowed both approaches to be used on TextureLayerChromium. It would probably be good to move everything over to this more generic implementation, but I do not know if I have the time available.

[Sean Hunt]

Created attachment 156129 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Sean Hunt]

Created attachment 156503 [details]
Patch

[Sean Hunt]

The changelog isn't properly written since I haven't written tests yet.

[James Robinson]

Comment on attachment 156503 [details]
Patch

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

Here's some initial feedback.  The startup and shutdown paths are always very tricky with these sorts of things - could you describe exactly what the ordering is for different possible sequences?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:55
> +        // Retrieve a graphics context for the provider's use.

This comment does not match the function it is associated with

> Source/Platform/chromium/public/WebExternalTextureProvider.h:56
> +        virtual unsigned int insertSyncPoint() = 0; 

just "unsigned", it's cleaner

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:60
> +        if (m_textureId || m_textureProvider)
>              layerTreeHost()->acquireLayerTextures();

acquireLayerTextures() is a synchronization mechanism used when the texture is owned by a main-thread object and we need to make sure that the compositor thread is not using it. Since the texture provide is a compositor thread object, I think that any synchronization around the texture should be managed directly on the thread, not through this indirection.

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:101
> +    if ((m_textureId || m_textureProvider) && layerTreeHost())

why are you checking for a non-null m_textureProvider in setTextureId()? Do you expect someone to be calling TextureLayerChromium::setTextureId() when the provider is non-null? That would greatly surprise me

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:124
> +    if ((m_textureId || m_textureProvider) && layerTreeHost() && host != layerTreeHost())

I doubt you need this

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:133
> +    if ((m_textureProvider || m_textureId) && layerTreeHost())

Again checking m_textureProvider and m_textureId - when do you expect them both to be set?

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:134
> +        layerTreeHost()->acquireLayerTextures();

I don't think you need this

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:161
> +    if (!m_textureProvider) {

the fact that this bypasses so much of TextureLayerChromium's code makes me think that you really should have a different type for provider-driven layers and share any logic you need to share with a base class

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:110
> +    WebKit::WebExternalTextureProvider *m_textureProvider;

The * goes with the type, not the variable name

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:60
> -    m_externalTextureResource = resourceProvider->createResourceFromExternalTexture(m_textureId);
> +    m_externalTextureResource = resourceProvider->createResourceFromExternalTexture(textureId());

why did you change this?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:67
> -    quadList.append(CCTextureDrawQuad::create(sharedQuadState, quadRect, m_externalTextureResource, m_premultipliedAlpha, m_uvRect, m_flipped));
> +    quadList.append(CCTextureDrawQuad::create(sharedQuadState, quadRect, m_externalTextureResource, premultipliedAlpha(), uvRect(), flipped()));

why are these all changed?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:95
> +    // FIXME: Does more need to be done here?

Quite an intriguing FIXME here - this probably deserves some careful thought before landing

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:37
> +    public CCLayerImpl,

this is overwrapped, i'd put this back on the line it was on

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:62
> +    void setTextureId(unsigned id)
> +    {
> +      ASSERT(!hasTextureProvider());

here since you're replacing so much functionality it seems you really want a different type

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:90
> +      return hasTextureProvider() ? m_provider->premultipliedAlpha() : m_premultipliedAlpha;

why are these properties all pull instead of push? that seems unnecessary, it'd be a lot more natural if the provider gave you these values if they changed

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:98
> +      return hasTextureProvider() ? (FloatRect) m_provider->uvRect() : m_uvRect;

we don't use c-style casts in WebKit (or chromium for that matter) - use a C++ function style cast for something like this (or avoid it by using another type)

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:106
> +    WebKit::WebExternalTextureProvider *m_provider;

*s belong with the type, not the variable

[Sean Hunt]

> --- Comment #5 from James Robinson <[e-mail]>  2012-08-03 22:07:25 PST ---
> (From update of attachment 156503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156503&action=review
>
> Here's some initial feedback.  The startup and shutdown paths are always very tricky with these sorts of things - could you describe exactly what the ordering is for different possible sequences?

I'll refactor to split into two classes (how does
WebProvidedTextureLayer sound? is there a better name?) and then
address this.

>> Source/Platform/chromium/public/WebExternalTextureProvider.h:55
>> +        // Retrieve a graphics context for the provider's use.
>
> This comment does not match the function it is associated with

Thanks. Fixed.

>> Source/Platform/chromium/public/WebExternalTextureProvider.h:56
>> +        virtual unsigned int insertSyncPoint() = 0;
>
> just "unsigned", it's cleaner

Fixed.

>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:60
>> +        if (m_textureId || m_textureProvider)
>>              layerTreeHost()->acquireLayerTextures();
>
> acquireLayerTextures() is a synchronization mechanism used when the texture is owned by a main-thread object and we need to make sure that the compositor thread is not using it. Since the texture provide is a compositor thread object, I think that any synchronization around the texture should be managed directly on the thread, not through this indirection.

Ok. I'll look into that in the refactor.

>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:101
>> +    if ((m_textureId || m_textureProvider) && layerTreeHost())
>
> why are you checking for a non-null m_textureProvider in setTextureId()? Do you expect someone to be calling TextureLayerChromium::setTextureId() when the provider is non-null? That would greatly surprise me

Good point; this will go away with the refactor.

>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:124
>> +    if ((m_textureId || m_textureProvider) && layerTreeHost() && host != layerTreeHost())
>
> I doubt you need this

You're probably right.

>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:133
>> +    if ((m_textureProvider || m_textureId) && layerTreeHost())
>
> Again checking m_textureProvider and m_textureId - when do you expect them both to be set?
>
>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:134
>> +        layerTreeHost()->acquireLayerTextures();
>
> I don't think you need this
>
>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:161
>> +    if (!m_textureProvider) {
>
> the fact that this bypasses so much of TextureLayerChromium's code makes me think that you really should have a different type for provider-driven layers and share any logic you need to share with a base class

Yeah. Hindsight :)

>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:110
>> +    WebKit::WebExternalTextureProvider *m_textureProvider;
>
> The * goes with the type, not the variable name

Thanks. Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:60
>> -    m_externalTextureResource = resourceProvider->createResourceFromExternalTexture(m_textureId);
>> +    m_externalTextureResource = resourceProvider->createResourceFromExternalTexture(textureId());
>
> why did you change this?

The idea was it might either have a provider or not. Switching to a
member function that produces the texture ID would correctly delegate
to the provider if appropriate.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:67
>> -    quadList.append(CCTextureDrawQuad::create(sharedQuadState, quadRect, m_externalTextureResource, m_premultipliedAlpha, m_uvRect, m_flipped));
>> +    quadList.append(CCTextureDrawQuad::create(sharedQuadState, quadRect, m_externalTextureResource, premultipliedAlpha(), uvRect(), flipped()));
>
> why are these all changed?

Same thing here.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:95
>> +    // FIXME: Does more need to be done here?
>
> Quite an intriguing FIXME here - this probably deserves some careful thought before landing

Yeah. I'll think about that when I do the refactor.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:37
>> +    public CCLayerImpl,
>
> this is overwrapped, i'd put this back on the line it was on

Thanks. I'd momentarily forgotten that WebKit wraps differently from Chromium.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:62
>> +    void setTextureId(unsigned id)
>> +    {
>> +      ASSERT(!hasTextureProvider());
>
> here since you're replacing so much functionality it seems you really want a different type

Yup.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:90
>> +      return hasTextureProvider() ? m_provider->premultipliedAlpha() : m_premultipliedAlpha;
>
> why are these properties all pull instead of push? that seems unnecessary, it'd be a lot more natural if the provider gave you these values if they changed

Cargo culting VideoLayer; the properties are pull and the layer only
gets informed when they change so it needs to redraw. I'm personally
partial to the pull model, but the push model would be fine too. A
push model might allow this all to be one class, actually.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:98
>> +      return hasTextureProvider() ? (FloatRect) m_provider->uvRect() : m_uvRect;
>
> we don't use c-style casts in WebKit (or chromium for that matter) - use a C++ function style cast for something like this (or avoid it by using another type)

Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:106
>> +    WebKit::WebExternalTextureProvider *m_provider;
>
> *s belong with the type, not the variable

Fixed.

Sean

[Sean Hunt]

Created attachment 157813 [details]
Patch

[Sean Hunt]

I just uploaded the tweaks; I haven't done the refactor because I want to know whether to go with a push or pull model.

[James Robinson]

(In reply to comment #8)
> I just uploaded the tweaks; I haven't done the refactor because I want to know whether to go with a push or pull model.

Are you expecting somebody in particular to tell you the answer to this question?  I'm assuming that you will figure out how you want to do do this and then update the patch, if not please indicate who you are waiting for or this patch will likely sit in limbo.

[Sean Hunt]

I'd appreciate your opinion if you have one, but I likely won't get to it as this is my last week at Google and I am focusing on documentation.

[James Robinson]

OK, that makes sense. Do you know who's taking this work over?

[Sean Hunt]

(In reply to comment #11)
> OK, that makes sense. Do you know who's taking this work over?

That hasn't really been worked out yet. Probably one of {fsamuel, lazyboy}

Created attachment 168092 [details]
Patch

[James Robinson]

Comment on attachment 168092 [details]
Patch

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

We need some tests for this - take a look at Tools/DumpRenderTree/chromium/TestWebPlugin.cpp for one way to approach tests in layout tests.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:84
> +    virtual void setTextureProvider(WebExternalTextureProvider*) { };

no ;

> Source/Platform/chromium/public/WebExternalTextureProvider.h:8
> + *     * Redistributions of source code must retain the above copyright

2-clause is preferred for new files. See Source/WebCore/LICENSE-APPLE

> Source/Platform/chromium/public/WebExternalTextureProvider.h:52
> +        // Notifies the provider's client that a call to getCurrentFrame() will return new data.

I don't see any getCurrentFrame() - what is this comment referring to?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:56
> +        // Insert a synchronization point in the graphics command stream
> +        virtual unsigned insertSyncPoint() = 0; 

What does this mean and what is the return value? The inserted sync point?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:59
> +    virtual void setTextureProviderClient(Client*) = 0;

This could use some documentation - what are the threading requirements of this call? How often can it be called?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:64
> +    virtual bool premultipliedAlpha() const = 0;
> +    virtual bool flipped() const = 0;
> +    virtual WebFloatRect uvRect() const = 0;

Why are these all pull?  Is it expected that these could change from frame to frame and that the user of this provider has to check their value every time it presents?

> Source/WebKit/chromium/public/WebPluginContainer.h:75
> +    virtual void setBackingTextureProvider(WebExternalTextureProvider*) = 0;

Please separate this with blank lines from adjacent functions.  Also say something about how this interacts with setBackingTexture() / setBackingIOSurfaceId().  I believe they're all mutually exclusive but having that explicit here would be easier for callers.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:401
> +    if (m_textureProvider == provider)

Do we really want to support setting different providers over the same plugin's lifetime?  That seems a bit tricksy to manage on the impl side and not something that we need for video layers currently

Created attachment 168708 [details]
Patch

> We need some tests for this - take a look at Tools/DumpRenderTree/chromium/TestWebPlugin.cpp for one way to approach tests in layout tests.

Thanks for the pointer, once the rest of the texture layer changes related to this are in place, I'll look into DRT test cases.

[James Robinson]

Comment on attachment 168708 [details]
Patch

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

Needs a bit more polish.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:83
> +    // TODO: make pure virtual once the chromium impl is changed and deps roll

WebKit spells this FIXME, not TODO.

I would put this next to setTextureId() and document how this interacts with setTextureId() and WebExternalTextureLayerClient.  Needing 3 different ways to provide a texture is kind of unfortunate :/

Do we need a way to set this after the fact or could this be another ::create() override? The advantage of having this be ::create() is that you don't have to worry about the provider changing after creation

> Source/Platform/chromium/public/WebExternalTextureProvider.h:33
> +class WebGraphicsContext3D;

this appears to be unused

> Source/Platform/chromium/public/WebExternalTextureProvider.h:52
> +        virtual void didUpdatePremultipliedAlpha(bool) = 0;
> +        virtual void didUpdateFlipped(bool) = 0;
> +        virtual void didUpdateUVRect(const WebFloatRect&) = 0;

Do these mean that the various state bits on the WebExternalTextureLayer are not used?  Do you really need this level of granularity on properties for your use case?  Please document

> Source/Platform/chromium/public/WebExternalTextureProvider.h:58
> +    // This is called on impl thread and happens during pushProperties

The "happens during pushProperties" part is not really useful to people reading this header since it's not part of the public API.  Much more useful would be knowing how often this is called (once) and what the lifetime of the parameter is.

nit: end comments with a period

> Source/WebKit/chromium/src/WebPluginContainerImpl.h:106
> +    // This is mutually exclusive with setBackingTextureId and setBackingIOSurfaceId

This is not the most useful place to document this since it's the implementation, not the public API.

Comments are supposed to be sentences and thus end with a period.

Created attachment 170486 [details]
Patch

Sorry for the long turnaround, I spent some time iterating with a prototype.

I think this should address the issues of too many usage paths in the WebExternalTextureLayer and improve the docs.

Created attachment 170490 [details]
Patch

[James Robinson]

Comment on attachment 170490 [details]
Patch

Why is this now a totally new layer type?  Do you plan to unify one of the other layer types with this one?

Would you mind writing up a description somewhere of how this interface will interact with the rest of the system, or does such a design doc exist already?  Without seeing the rest of the iceberg it's hard to provide solid feedback here.

(In reply to comment #21)
> (From update of attachment 170490 [details])
> Why is this now a totally new layer type?  Do you plan to unify one of the other layer types with this one?
> 
> Would you mind writing up a description somewhere of how this interface will interact with the rest of the system, or does such a design doc exist already?  Without seeing the rest of the iceberg it's hard to provide solid feedback here.

I made a new layer type because there were already two separate ways to supply texture ids to the WebExternalTextureLayer, this would have been a third one. In addition, much of the functionality previously provided by the layer was mutually exclusive with this new way. I think this makes it a more clear implementation that avoids comments along the lines of "if you follow this code path, don't use the other 2 and don't call the following functions because they don't do anything with a texture provider".

I'll write up a quick doc, but in the mean time here is most of the puzzle below.

These are CC parts of the change as well as an implementation of the texture provider and how it interacts with the webkit change. I split them up to avoid patch bombing people with one massive change.

https://codereview.chromium.org/11275099
https://codereview.chromium.org/11359024

Created attachment 172583 [details]
Patch

[Antoine Labour]

Similarly to James, I would favor a version where we don't add a new layer type, but try to merge the use cases to reduce the number of paths. For example, I suspect we can merge the existing 2 paths by fixing the respective users - I think only plugins (and tests) now call setTextureId as opposed to providing a client. That would balance the added complexity of an impl-thread provider.
At a higher level, it's nice to be able to think about a layer type by what type of content it renders (e.g. a texture, a video frame, painted contents, a scrollbar, a 9-patch, ...), rather than how that content is provided. I'm comfortable with having different ways of providing the content in the same layer.

Created attachment 176275 [details]
Patch

(In reply to comment #24)
> Similarly to James, I would favor a version where we don't add a new layer type, but try to merge the use cases to reduce the number of paths. For example, I suspect we can merge the existing 2 paths by fixing the respective users - I think only plugins (and tests) now call setTextureId as opposed to providing a client. That would balance the added complexity of an impl-thread provider.
> At a higher level, it's nice to be able to think about a layer type by what type of content it renders (e.g. a texture, a video frame, painted contents, a scrollbar, a 9-patch, ...), rather than how that content is provided. I'm comfortable with having different ways of providing the content in the same layer.

Thanks for the comments, I removed the new layer with this patch.

I could also put the work of merging the existing two layer types on my todo list if you'd like, but it'll need to be a follow on patch since it requires chromium changes to not break the current path.

[Antoine Labour]

Comment on attachment 176275 [details]
Patch

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

> Source/Platform/chromium/public/WebCompositorSupport.h:85
> +    virtual WebExternalTextureLayer* createExternalTextureProviderLayer(WebExternalTextureProvider*) { return 0; }

Maybe createExternalTextureLayerWithProvider? Or, I believe WebKit allows overloading, so createExternalTextureLayer is possibly OK.

[Antoine Labour]

Overall, I'm fine with this. However I would suggest finishing the review of the chrome side before landing this, because the iterations may have some impact on the WebExternalTextureProvider::Client interface.

(In reply to comment #27)
> (From update of attachment 176275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176275&action=review
> 
> > Source/Platform/chromium/public/WebCompositorSupport.h:85
> > +    virtual WebExternalTextureLayer* createExternalTextureProviderLayer(WebExternalTextureProvider*) { return 0; }
> 
> Maybe createExternalTextureLayerWithProvider? Or, I believe WebKit allows overloading, so createExternalTextureLayer is possibly OK.

createExternalTextureLayer(Client* = 0) optionally takes a client, which would make it ambiguous between Client and Provider with NULL, so that won't work, unfortunately.

createExternalTextureLayerWithProvider sounds reasonable, I'll update.

Created attachment 176286 [details]
Patch

(In reply to comment #28)
> Overall, I'm fine with this. However I would suggest finishing the review of the chrome side before landing this, because the iterations may have some impact on the WebExternalTextureProvider::Client interface.

Agreed, I'll put up the chromium patch for review shortly.