Bug 66083 – [chromium] Split LayerTextureUpdater into main thread / compositor thread versions

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Iain Merrick]

Split LayerTextureUpdater into main thread / compositor thread versions.

[Iain Merrick]

Created attachment 103654 [details]
Snapshot of in-progress work.

[Iain Merrick]

Created attachment 103655 [details]
How about if I include the new files as well? That would help.

This compiles but needs a lot of work still. Also, I need to add "Impl"s to those new classes to fit our naming scheme.

[Adrienne Walker]

I think this is shaping up in the right direction.  :)

I'm not quite sure that I'm comfortable with m_client pointers across threads.  I'm guessing you're doing this to safely push properties via virtual dispatch rather than static_cast, but it also means that if a LayerChromium gets deleted from the tree, there's a CCLayer with a bogus pointer that will (hopefully) get cleaned up on the next tree sync.

If you're worried about type safety in pushing from one layer to another, I'd rather see some sort of virtual dispatch where you don't hold a member pointer to something on another thread and instead do it on a function arg somewhere in the tree synchronizing callstack. (That's just my thought on how to handle that.)

Also, I think the CCLayerTextureUpdaterImpl will eventually need to live on the CCLayerImpl instead of also on the LayerChromium, but I think might be better done as a part of bug 66074, given which layer tree currently gets walked to do the actual upload.

alokp: Are there any accelerated drawing tests that we can run to make sure refactorings here and in the tiler haven't broken things for you?

[James Robinson]

why does ContentLayerChromium own the CCLayerTextureUpdaterImpl?  That seems quite wrong.

[Iain Merrick]

James: that's a placeholder, I hadn't yet figured out where to move it to. I guess it should be CCLayerImpl?

[Adrienne Walker]

(In reply to comment #4)
> why does ContentLayerChromium own the CCLayerTextureUpdaterImpl?  That seems quite wrong.

See bug 66074.  LayerTilerChromium::updateRect(LayerTextureUpdater*) is called via LayerRendererChromium::updateCompositorResources which iterates over the LayerChromium tree.  So, until that pass is changed to iterate over the CC layer tree, it can't be owned by the CC layer.

I personally think that change can be deferred.  There's enough in flux here as it is.

[James Robinson]

It should be whoever takes ownership of the uploader on the CCLayerImpl side.  Hopefully CCTiledLayerImpl could own it, assuming that we can share this somehow across the various TiledLayerChromium subclasses (ContentLayerChromium and ImageLayerChromium) and configurations (software painting vs accelerated painting).  Otherwise you can create the appropriate subclasses of CCTiledLayerImpl.

[Iain Merrick]

To Enne's earlier reply: yes, the Client pointers are intended to keep things typesafe. But you're right, that leads to some hairy pointer-sharing across threads.

Thinking about it, m_client is only ever used in response to a call from the main thread. So if we're consistent about that, it should be safe. But maybe that isn't any better than just holding the base class pointer elsewhere and casting it.

[James Robinson]

(In reply to comment #6)
> (In reply to comment #4)
> > why does ContentLayerChromium own the CCLayerTextureUpdaterImpl?  That seems quite wrong.
> 
> See bug 66074.  LayerTilerChromium::updateRect(LayerTextureUpdater*) is called via LayerRendererChromium::updateCompositorResources which iterates over the LayerChromium tree.  So, until that pass is changed to iterate over the CC layer tree, it can't be owned by the CC layer.
> 
> I personally think that change can be deferred.  There's enough in flux here as it is.

Hm, I thought the plan was to change uploading as well?  I'm OK with doing that separately, but then the naming is wrong.  The current model is that updateCompositorResources() operates on the main thread data structures (LayerChromium tree).  So the plan is to split up the uploading into multiple classes while leaving it on the LayerChromium tree, and then later move part of the uploading to the CCLayerImpl side?  If so I'd suggest that we don't use the CC*Impl naming convention until the second step actually happens.

[James Robinson]

After some in person discussion, I don't think we want to do this split yet.  It's an optimization, and potentially a very useful one, but it's not strictly necessary for our current goals.  If profiling shows that this is a real hotspot we can revisit.

[Iain Merrick]

Clearing off an obsolete bug.