[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 #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?

> 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

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