[Webkit-unassigned] [Bug 93010] Add WebExternalTextureProvider; an alternate means for providing textures to a TextureLayerChromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93010





--- Comment #6 from Sean Hunt <scshunt at csclub.uwaterloo.ca>  2012-08-07 08:01:37 PST ---
> --- Comment #5 from James Robinson <jamesr at chromium.org>  2012-08-03 22:07:25 PST ---
> (From update of attachment 156503)
> 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.