Bug 61099 – [chromium] Fix ownership of PlatformImage for ImageLayerChromiums

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Fix ownership of PlatformImage for ImageLayerChromiums

[James Robinson]

Created attachment 94015 [details]
Patch

[Adrienne Walker]

Strawman proposal: if the lifetimes of the layer, tiler, and updater are too entangled, would it make sense to go ahead and move the ownership of the updater to the layer at this point?

That would be a world where the layer clearly owned both the updater and the tiler, and one where the updater owned the PlatformImage.  The tiler would then just take a pointer to an updater during the prepare and update functions, which would make it clear that it had no ownership.  You could then skip all the RefPtr changes and leave them as OwnPtr.

(I think there are plenty of use cases for RefPtr, but I'm always reluctant to introduce more uses of it.  It makes it much harder to reason about and debug where and when destruction occurs.)

[James Robinson]

(In reply to comment #2)
> Strawman proposal: if the lifetimes of the layer, tiler, and updater are too entangled, would it make sense to go ahead and move the ownership of the updater to the layer at this point?
> 
> That would be a world where the layer clearly owned both the updater and the tiler, and one where the updater owned the PlatformImage.  The tiler would then just take a pointer to an updater during the prepare and update functions, which would make it clear that it had no ownership.  You could then skip all the RefPtr changes and leave them as OwnPtr.
> 
> (I think there are plenty of use cases for RefPtr, but I'm always reluctant to introduce more uses of it.  It makes it much harder to reason about and debug where and when destruction occurs.)

That does sound better.

[James Robinson]

Created attachment 94540 [details]
Patch

[James Robinson]

How about this?  I expect heavy conflicts with alokp's patch :)

This patch makes it more obvious that draw() depends on the texture updater, which is definitely a design issue.

[James Robinson]

Created attachment 94905 [details]
Patch

[James Robinson]

How 'bout now?  This should make it a lot easier to divide the responsibilities of LayerTilerChromium into main thread and compositor thread portions.

[Adrienne Walker]

Comment on attachment 94905 [details]
Patch

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

This looks great to me.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:141
> +        ImageLayerTextureUpdater* imageTextureUpdater = static_cast<ImageLayerTextureUpdater*>(m_textureUpdater.get());

The static_cast makes me uncomfortable, but we can fix that when we finally divorce ImageLayerChromium from ContentLayerChromium and they each have their own OwnPtr to a derived texture updater class.

[Alok Priyadarshi]

Comment on attachment 94905 [details]
Patch

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

I think moving the ownership of texture-updater to the content-layers make the design more complicated.
- Now Image layers have to update both tiler and texture-updater and do it in correct order
- Pass texture-updater to the tiler in all calls (even draw) under the implicit understanding that the texture-updater instance remains the same for all these calls.

I think part of the complication is arising from the fact that ImageLayer derives from ContentLayer. Would it help if we divorce ImageLayers before making this change?

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:135
>  {

m_textureUpdater needs to be cleared in ContentLayerChromium::cleanupResources()

[James Robinson]

Splitting Image/Content apart will definitely clear this up.  Right now the main reason they are entangled is to share the implementation of draw().  I'd like to move the draw() logic in a CCTiledLayerImpl at which point it should be pretty easy to split the classes up completely.  In order to do that, however, I need to disentangle the uploading part of LayerTilerChromium from the rendering part which is where this patch comes in.

Logically I don't think that the LayerTilerChromium should be responsible for updating itself, it's up to the layer to manage passing contents in and the tiler should just take care of managing the textures, shaders, and rendering.

[James Robinson]

Created attachment 98256 [details]
merged up to ToT

[Kenneth Russell]

I'll be happy to r+ this if someone more familiar with the compositor can do the unofficial review.

[Adrienne Walker]

Comment on attachment 98256 [details]
merged up to ToT

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

Unofficially, this looks good to me, other than that one nit.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:141
> +        ImageLayerTextureUpdater* imageTextureUpdater = static_cast<ImageLayerTextureUpdater*>(m_textureUpdater.get());

Could you maybe add a // FIXME here that this static_cast can be removed once ImageLayerChromium no longer derives from ContentLayerChromium? I really dislike static_cast, so I'd feel better if it was explicit that it was temporary.

[Kenneth Russell]

Comment on attachment 98256 [details]
merged up to ToT

rs=me

[Alok Priyadarshi]

Comment on attachment 98256 [details]
merged up to ToT

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:49
> +    static PassOwnPtr<LayerTilerChromium> create(LayerRendererChromium*, const IntSize& tileSize, BorderTexelOption);

I think it is fine to move the ownership of LayerTextureUpdater from LayerTilerChromium to the layer itself. I do not quite like passing the updater in these functions - prepareToUpdate(), updateRect(), and most importantly draw(). This is based on an IMPLICIT assumption that LayerTextureUpdater does not change. Can we still keep a pointer to LayerTextureUpdater in the tiler but not own it. It is not super elegant, but at least clears the assumption.

[James Robinson]

That would be worse, not better. The real problem is that the draw() implementation needs the updater at all, which I'll fix shortly. One thing at a time

[Alok Priyadarshi]

(In reply to comment #16)
> That would be worse, not better. The real problem is that the draw() implementation needs the updater at all, which I'll fix shortly.

how are you planning to fix that?

[James Robinson]

the only think draw() needs the updater for is to check the orientation of the tile textures, which IMO should be state on the tiler anyway.

[James Robinson]

Committed r89647: <http://trac.webkit.org/changeset/89647>