Bug 61099 - [chromium] Fix ownership of PlatformImage for ImageLayerChromiums
Summary: [chromium] Fix ownership of PlatformImage for ImageLayerChromiums
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-18 17:53 PDT by James Robinson
Modified: 2011-06-23 18:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.63 KB, patch)
2011-05-18 17:58 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (26.90 KB, patch)
2011-05-23 18:54 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (29.98 KB, patch)
2011-05-25 19:45 PDT, James Robinson
no flags Details | Formatted Diff | Diff
merged up to ToT (30.00 KB, patch)
2011-06-22 15:57 PDT, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-05-18 17:53:16 PDT
[chromium] Fix ownership of PlatformImage for ImageLayerChromiums
Comment 1 James Robinson 2011-05-18 17:58:56 PDT
Created attachment 94015 [details]
Patch
Comment 2 Adrienne Walker 2011-05-19 10:03:47 PDT
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.)
Comment 3 James Robinson 2011-05-23 11:06:47 PDT
(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.
Comment 4 James Robinson 2011-05-23 18:54:56 PDT
Created attachment 94540 [details]
Patch
Comment 5 James Robinson 2011-05-23 18:59:23 PDT
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.
Comment 6 James Robinson 2011-05-25 19:45:53 PDT
Created attachment 94905 [details]
Patch
Comment 7 James Robinson 2011-05-25 19:46:59 PDT
How 'bout now?  This should make it a lot easier to divide the responsibilities of LayerTilerChromium into main thread and compositor thread portions.
Comment 8 Adrienne Walker 2011-05-26 10:18:39 PDT
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.
Comment 9 Alok Priyadarshi 2011-05-26 12:23:53 PDT
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()
Comment 10 James Robinson 2011-05-26 13:54:38 PDT
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.
Comment 11 James Robinson 2011-06-22 15:57:40 PDT
Created attachment 98256 [details]
merged up to ToT
Comment 12 Kenneth Russell 2011-06-22 17:19:28 PDT
I'll be happy to r+ this if someone more familiar with the compositor can do the unofficial review.
Comment 13 Adrienne Walker 2011-06-22 18:29:08 PDT
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.
Comment 14 Kenneth Russell 2011-06-23 11:53:29 PDT
Comment on attachment 98256 [details]
merged up to ToT

rs=me
Comment 15 Alok Priyadarshi 2011-06-23 12:38:27 PDT
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.
Comment 16 James Robinson 2011-06-23 12:47:19 PDT
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
Comment 17 Alok Priyadarshi 2011-06-23 13:10:17 PDT
(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?
Comment 18 James Robinson 2011-06-23 13:48:16 PDT
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.
Comment 19 James Robinson 2011-06-23 18:17:53 PDT
Committed r89647: <http://trac.webkit.org/changeset/89647>