Bug 76715 – Refactor plugin drawing to be more data driven

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tim Dresser]

From bug 73059:

CCLayerImpl-derived classes all currently handle drawing themselves internally, making direct GL calls.  This is problematic for several reasons.  Testing these layers is tricky because testing must be done at a mock GL interface level rather than at a higher abstract level.  It makes culling closely dependent on how each layer draws.  It also makes some optimizations like layer squashing impossible.

To fix this, CCLayerImpl-derived classes should produce a set of quads to render with some basic information in them about how they are to be drawn.  This list can then be processed and drawn in a more data-driven and abstract manner.


CCPluginLayerImpl still handles drawing internally and should be transitioned to the data-driven approach.
This builds on the initial refactor from bug 76274 and is closely related to bug 76635, which refactors CCCanvasLayerImpl.

[Tim Dresser]

Created attachment 123343 [details]
Patch

[Adrienne Walker]

Comment on attachment 123343 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:682
> +struct PluginProgramBinding {

Whoa, I never knew this code existed.  Can you combine all four of these shader permutations? (If you want to make it a separate patch for clarity, feel welcome to.) It looks like PosTexStretch is PosTexTransform but with an identical translation and stretch for x and y.  And, "flip" could be considered just a change in tex transform, so there's no need for a separate shader for it or any of this boilerplate uniform-related template code.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:723
> +        ASSERT(extensions->supports("GL_CHROMIUQUAD->iosurface"));

Search/replace typo?

> Source/WebCore/platform/graphics/chromium/cc/CCPluginDrawQuad.h:63
> +    CCPluginLayerImpl* m_layer;

From a design perspective, I don't think quads should have backpointers back to their originating layers if it can be helped.  Architecturally, I'd be fine with the assumption that we call appendQuads prior to any draw call.  That would let you push all the io surface handling back into plugin layer, and simply give the quad a texture id.

[Tim Dresser]

> From a design perspective, I don't think quads should have backpointers back to their originating layers if it can be helped.  Architecturally, I'd be fine with the assumption that we call appendQuads prior to any draw call.  That would let you push all the io surface handling back into plugin layer, and simply give the quad a texture id.

It definitely would be a lot nicer if that backpointer wasn't there.

In appendQuads, I have no access to a GraphicsContext3D, so if the layer's ioSurface doesn't have a texture, I can't create one.

The ioSurface's textureId needs to be shared between all the quads. I don't see how I can share it without the backpointer, in the case where no texture exists when appendQuads is called.

I can move the ioSurfaceChanged logic into appendQuads, which is an improvement.

[Adrienne Walker]

(In reply to comment #3)
> > From a design perspective, I don't think quads should have backpointers back to their originating layers if it can be helped.  Architecturally, I'd be fine with the assumption that we call appendQuads prior to any draw call.  That would let you push all the io surface handling back into plugin layer, and simply give the quad a texture id.
> 
> It definitely would be a lot nicer if that backpointer wasn't there.
> 
> In appendQuads, I have no access to a GraphicsContext3D, so if the layer's ioSurface doesn't have a texture, I can't create one.

You could create one during PluginLayerChromium::updateCompositorResources, store it on the PluginLayerChromium, and sync it over to the CCPluginLayerChromium.  That's the path where all the other texture ids gets allocated.

[Tim Dresser]

Created attachment 123578 [details]
Patch

[Adrienne Walker]

Comment on attachment 123578 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:66
> +        if (!m_ioSurfaceTextureId)
> +            m_ioSurfaceTextureId = rendererContext->createTexture();

If you're going to store the texture ID on the main thread, please use a ManagedTexture like other layer types.  Otherwise, we can't track this GPU memory or make sure we're not using a bogus texture id when the context is lost.  Additionally, you'll need to reserve it each frame and set m_ioSurfaceChanged if it stops being valid because the texture manager has evicted it.  VideoLayerChromium is a good model to use here.

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:78
> +        extensions->texImageIOSurface2DCHROMIUM(Extensions3D::TEXTURE_RECTANGLE_ARB,
> +                                                m_ioSurfaceWidth,
> +                                                m_ioSurfaceHeight,
> +                                                m_ioSurfaceId,
> +                                                0);

I'm a little dubious about this texImage, since the texture id could be in use on the impl thread.  reveman: If this texture id comes from a managed texture, is that sufficient to work with atomic updates? Does this also need to get wrapped in a texture updater?

[Tim Dresser]

Created attachment 123808 [details]
Patch

[David Reveman]

(In reply to comment #6)
> (From update of attachment 123578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123578&action=review
> 
> > Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:66
> > +        if (!m_ioSurfaceTextureId)
> > +            m_ioSurfaceTextureId = rendererContext->createTexture();
> 
> If you're going to store the texture ID on the main thread, please use a ManagedTexture like other layer types.  Otherwise, we can't track this GPU memory or make sure we're not using a bogus texture id when the context is lost.  Additionally, you'll need to reserve it each frame and set m_ioSurfaceChanged if it stops being valid because the texture manager has evicted it.  VideoLayerChromium is a good model to use here.
> 
> > Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:78
> > +        extensions->texImageIOSurface2DCHROMIUM(Extensions3D::TEXTURE_RECTANGLE_ARB,
> > +                                                m_ioSurfaceWidth,
> > +                                                m_ioSurfaceHeight,
> > +                                                m_ioSurfaceId,
> > +                                                0);
> 
> I'm a little dubious about this texImage, since the texture id could be in use on the impl thread.  reveman: If this texture id comes from a managed texture, is that sufficient to work with atomic updates? Does this also need to get wrapped in a texture updater?

It's not sufficient for guaranteeing that these plugin layer updates are atomic. You'll need to use the "deleteTextureAfterCommit(texture->steal())" before reserving the texture for that. Btw, I'm failing to see where in this patch the texture is reserved.

It doesn't need to get wrapped in the texture updater right now. The patches I have in flight requires that but you don't need to worry about the yet.

[Tim Dresser]

On further testing, I found a reproducible crash on OSX.
It occurs using my most recent patch, both with and without reveman's suggestion.

I'll fix it, and submit a new patch with the bug fixed and reveman's suggestion incorporated.

[Tim Dresser]

Would it be preferable to create the layer's ioSurfaceTexture in the CCLayerImpl::willDraw function discussed in bug 76720? 

With that approach, all the texture management would be in the impl thread, which seems like it might be better. Would it be cleaner to use a ManagedTexture in that case, or not?

[David Reveman]

(In reply to comment #10)
> Would it be preferable to create the layer's ioSurfaceTexture in the CCLayerImpl::willDraw function discussed in bug 76720? 
> 
> With that approach, all the texture management would be in the impl thread, which seems like it might be better. Would it be cleaner to use a ManagedTexture in that case, or not?

I think that works well right now. The only problem I'm aware of is that in a future unified/out-of-process compositor we can't pass pointers around so textures and serialized properties set in pushPropertiesTo are likely the only way you can transfer data from a layer in the renderer to a layer in the compositor.

[Tim Dresser]

(In reply to comment #8)
> It's not sufficient for guaranteeing that these plugin layer updates are atomic. You'll need to use the "deleteTextureAfterCommit(texture->steal())" before reserving the texture for that. Btw, I'm failing to see where in this patch the texture is reserved.

It looks like deleteTextureAfterCommit doesn't do anything at the moment.
It appends the texture to the CCLayerTreeHost's m_deleteTextureAfterCommitList, but that list is never used...

Will this approach be adequate to ensure that the texImageIOSurface2DCHROMIUM call is safe?

[Dana Jansens]

The list is used to hold a reference to a texture during a commit, so that the texture doesn't disappear on draw/impl side and it can continue to draw with it, until the commit is finished and it isn't needed anymore.

[Tim Dresser]

When reserving the ioSurfaceTexture, I believe that I should be passing Extensions3D::TEXTURE_RECTANGLE_ARB as the format. With this format, the call to ManagedTexture::reserve reaches GraphicsContext3D::computeFormatAndTypeParameters and then chokes, because TEXTURE_RECTANGLE_ARB isn't expected.

I've tried just adding TEXTURE_RECTANGLE_ARB to the switch statement in computeFormatAndTypeParameters, but I end up with a GL_INVALID_ENUM error that I'm not sure how to handle.

Should I be trying to reserve the texture formatted TEXTURE_RECTANGLE_ARB, or am I approaching this incorrectly?

[David Reveman]

(In reply to comment #14)
> When reserving the ioSurfaceTexture, I believe that I should be passing Extensions3D::TEXTURE_RECTANGLE_ARB as the format. With this format, the call to ManagedTexture::reserve reaches GraphicsContext3D::computeFormatAndTypeParameters and then chokes, because TEXTURE_RECTANGLE_ARB isn't expected.
> 
> I've tried just adding TEXTURE_RECTANGLE_ARB to the switch statement in computeFormatAndTypeParameters, but I end up with a GL_INVALID_ENUM error that I'm not sure how to handle.
> 
> Should I be trying to reserve the texture formatted TEXTURE_RECTANGLE_ARB, or am I approaching this incorrectly?

Sorry, I should have looked more closely at this patch. TextureManager doesn't really support foreign texture formats and you can't really allocate the texture on your own when using a ManagedTexture.

I think you'll have to do your own double buffering for it to work correctly with incremental updates. Use two ioSurfaceTextureIds and flip between them for each update.

[Tim Dresser]

(In reply to comment #15)
> I think you'll have to do your own double buffering for it to work correctly with incremental updates. Use two ioSurfaceTextureIds and flip between them for each update.

From my understanding, this additional complexity has been introduced because we moved the texture creation code off the compositor thread.

It feels like things could be simplified significantly by moving everything back onto the compositor thread. 

Is there any advantage to allocating the ioSurfaceTexture on the main thread?

If I was to leave the allocation of the ioSurfaceTexture on the compositor thread, could I avoid the additional overhead of the ManagedTexture and double buffering?

[Adrienne Walker]

(In reply to comment #16)

> If I was to leave the allocation of the ioSurfaceTexture on the compositor thread, could I avoid the additional overhead of the ManagedTexture and double buffering?

Definitely.  You could do something similar to the video layer here, where you call something before appendQuads to create and upload the texture.

[David Reveman]

(In reply to comment #16)
> (In reply to comment #15)
> > I think you'll have to do your own double buffering for it to work correctly with incremental updates. Use two ioSurfaceTextureIds and flip between them for each update.
> 
> From my understanding, this additional complexity has been introduced because we moved the texture creation code off the compositor thread.
> 
> It feels like things could be simplified significantly by moving everything back onto the compositor thread. 
> 
> Is there any advantage to allocating the ioSurfaceTexture on the main thread?

The textures are always allocated on the compositor thread. It's just the management of textures that can be done on the main thread. As you can't use a ManagedTexture for these ioSurfaces, all texture handling has to happen on the compositor thread.

> 
> If I was to leave the allocation of the ioSurfaceTexture on the compositor thread, could I avoid the additional overhead of the ManagedTexture and double buffering?

You shouldn't use ManagedTexture but you still need double buffering to not get artifacts when incremental updates are enabled.

[Tim Dresser]

(In reply to comment #18)
> You shouldn't use ManagedTexture but you still need double buffering to not get artifacts when incremental updates are enabled.

It feels like double buffering is a separate issue from this refactor, should it be left as a separate bug?

[David Reveman]

(In reply to comment #19)
> (In reply to comment #18)
> > You shouldn't use ManagedTexture but you still need double buffering to not get artifacts when incremental updates are enabled.
> 
> It feels like double buffering is a separate issue from this refactor, should it be left as a separate bug?

Yes, that's fine with me. Just make sure you create a new bug for it before you close this one so we don't forget.

[Tim Dresser]

Created attachment 124560 [details]
Patch

[Dana Jansens]

Comment on attachment 124560 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:229
> +            layer->willDraw(m_layerRenderer.get());

willDraw() will be called on the layer both when it represents itself and when it represents a contributing RenderSurface. Is that what you want here?

[Tim Dresser]

Created attachment 124561 [details]
Patch

[Adrienne Walker]

Comment on attachment 124561 [details]
Patch

This looks good to me.  Thanks for taking the time to remove the layer backpointer.  :)

[James Robinson]

Comment on attachment 124561 [details]
Patch

Awesome, R=me

[WebKit Review Bot]

Comment on attachment 124561 [details]
Patch

Clearing flags on attachment: 124561

Committed r106607: <http://trac.webkit.org/changeset/106607>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.