Bug 76295 – [Chromium] Replace TextureManager eviction list with deferred texture allocator.

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

[David Reveman]

Instead of implicitly deferring texture creation/deletion inside TextureManager add an explicit deferred texture allocator that uses fake texture IDs and makes it possible to defer texture creation until use on impl thread.

[David Reveman]

Created attachment 123410 [details]
Patch

[David Reveman]

Created attachment 123571 [details]
Patch

[David Reveman]

This eliminates the need for:
https://bugs.webkit.org/show_bug.cgi?id=72192

[WebKit Review Bot]

Comment on attachment 123571 [details]
Patch

Attachment 123571 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11336204

[Adrienne Walker]

Comment on attachment 123571 [details]
Patch

I know you mentioned this idea offhand to me a week or few ago, but could you explicitly restate your motivation here? I'm not totally sure that I understand what this texture deferral is trying to solve.

[David Reveman]

(In reply to comment #5)
> (From update of attachment 123571 [details])
> I know you mentioned this idea offhand to me a week or few ago, but could you explicitly restate your motivation here? I'm not totally sure that I understand what this texture deferral is trying to solve.

The main reason for this is to be able to create and delete textures on the renderer thread without the need for a GraphicsContext3D. Asynchronous commits [1] as well as threaded parallel paint/upload [2] requires the ability to refer to persistent textures prior to the commit happening on the impl thread. My previous attempt [3] at solving this allowed initialization of compositor resources prior to starting an update by making a round trip to the impl thread. I think this approach is much better. Other than the obvious benefit of not requiring an unnecessary round trip to the impl thread, this approach provides some other improvements:

A) Cleans up the awkward eviction list in the TextureManager that is a side effect of making a texture manager designed for a single threaded environment work with multiple threads.

B) Improves resource usage as it allows the TextureManager to be used without delayed texture deletion. E.g. for managing render surfaces on the impl thread or content textures in single threaded mode.

C) Decision about if a texture needs to be allocated can be pushed to the impl thread. Eventually we might want to delay some texture allocations and uploads until the impl side as determined that they are useful. E.g. not yet visible pre-paint tiles.


Main reason is still the ability to do asynchronous commits and parallel paint/uploads.

[1] https://bugs.webkit.org/show_bug.cgi?id=73279
[2] https://bugs.webkit.org/show_bug.cgi?id=74113
[3] https://bugs.webkit.org/show_bug.cgi?id=72192

[Dana Jansens]

Comment on attachment 123571 [details]
Patch

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

Please excuse any naive comments.. The CL is somewhat overwhelming to read. Do you have these as separate patches in a branch locally, that I could read it in steps?

> Source/WebCore/platform/graphics/chromium/DeferredTextureAllocator.cpp:64
> +    unsigned textureId = m_allocator->createTexture(size, format);

the whole "texture" vs "textureId" thing is not super clear. I think "texture" comes from the main thread and "textureId" from the impl side?

> Source/WebCore/platform/graphics/chromium/DeferredTextureAllocator.h:59
> +class DeferredTextureAllocatorBinding : public TextureAllocatorBinding {

I was wondering if you think this could be done without a new binding class. What if the Allocator has some interface it implements, which is passed to the Proxy. Or vice-versa, the proxy implements some interface for the Allocator..? A lot of this diff is just changing allocators to bindings, and moving the bind from updateTextureRect out to updateRect. Or maybe it could be split up into steps?

> Source/WebCore/platform/graphics/chromium/DeferredTextureAllocator.h:81
> +    RefPtr<TextureAllocator> m_allocator;

IIUC this is binding the main thread allocator to the impl side allocator. I think m_allocator refers to the impl side allocator, but the name could be more clear. Perhaps m_textures as well.

> Source/WebCore/platform/graphics/chromium/FrameBufferSkPictureCanvasLayerTextureUpdater.cpp:44
> +    unsigned textureId = binding->textureId(texture->textureId());

There are 3 meanings of "textureId" in one line. If the texture has a textureId, and the binding binds it to another textureId.. some names could help a lot here.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:60
> +            context->bindTexture(GraphicsContext3D::TEXTURE_2D, binding->textureId(texture()->textureId()));

isn't this done in ManagedTexture::bindTexture() ?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:680
> +    Platform3DObject textureId = quad->doubleBuffered() ? contentsTextureAllocatorBinding()->textureId(quad->textureId()) : quad->textureId();

This line is super obscure to read..

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:480
> +    // Delete textures no longer used after commit finished.

a method on the allocator?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:568
> +    OwnPtr<DeferredTextureAllocatorBinding> contentsTextureAllocatorBinding = DeferredTextureAllocatorBinding::create(contentsTextureAllocator);

one is Ref, one is Own, which made me wonder who else can ref++ the contentsTextureAllocator other than LayerRendererChromium..

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:575
> +            contentsTextureAllocator->setUseTextureStorageExt(true);

why is this in here?

[David Reveman]

Thanks for looking at this patch!

(In reply to comment #7)
> (From update of attachment 123571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123571&action=review
> 
> Please excuse any naive comments.. The CL is somewhat overwhelming to read. Do you have these as separate patches in a branch locally, that I could read it in steps?

The patch is large because has to touch a huge amount of files. A lot of it is simple stuff like s/TextureAllocator/TextureAllocatorBinding/. This is one big commit in my tree. There's no obvious way to split it into multiple functional commits but I'll look into it.

> 
> > Source/WebCore/platform/graphics/chromium/DeferredTextureAllocator.cpp:64
> > +    unsigned textureId = m_allocator->createTexture(size, format);
> 
> the whole "texture" vs "textureId" thing is not super clear. I think "texture" comes from the main thread and "textureId" from the impl side?

ManagedTexture is used for texture management. Texture IDs can be shared between threads. ManagedTextures can't be shared between threads.

> 
> > Source/WebCore/platform/graphics/chromium/DeferredTextureAllocator.h:59
> > +class DeferredTextureAllocatorBinding : public TextureAllocatorBinding {
> 
> I was wondering if you think this could be done without a new binding class. What if the Allocator has some interface it implements, which is passed to the Proxy. Or vice-versa, the proxy implements some interface for the Allocator..? A lot of this diff is just changing allocators to bindings, and moving the bind from updateTextureRect out to updateRect. Or maybe it could be split up into steps?

Yes, we could move this functionality into the existing texture allocator but I'm worried that would make it more confusing. How about making the TextureAllocatorBinding type derived from the TextureAllocator class? Maybe also change do these name changes:

TextureAllocator -> TextureIdGenerator
TextureAllocatorBinding -> TextureAllocator

not sure that make things more clear.

An alternative that I've considered is adding support for foreign textures to ManagedTexture/TextureManager. That approach would move the TextureAllocatorBinding functionality into the TextureManager.

> 
> > Source/WebCore/platform/graphics/chromium/DeferredTextureAllocator.h:81
> > +    RefPtr<TextureAllocator> m_allocator;
> 
> IIUC this is binding the main thread allocator to the impl side allocator. I think m_allocator refers to the impl side allocator, but the name could be more clear. Perhaps m_textures as well.

m_allocator refers to the allocator that texture allocations are deferred to. You're right that this is currently the impl side allocator but there is no such constraint from an encapsulation and data hiding perspective. Would something like m_targetAllocator and TextureMap m_sourceToTarget be better?

> 
> > Source/WebCore/platform/graphics/chromium/FrameBufferSkPictureCanvasLayerTextureUpdater.cpp:44
> > +    unsigned textureId = binding->textureId(texture->textureId());
> 
> There are 3 meanings of "textureId" in one line. If the texture has a textureId, and the binding binds it to another textureId.. some names could help a lot here.

There's the main thread textureId and the impl thread textureId. I thought about using function names like realTextureId and fakeTextureId but that's confusing as the system allows the fakeTextureId to be a real textures in some situations. E.g. each ManagedTexture on the impl side holds real textureIds.

> 
> > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:60
> > +            context->bindTexture(GraphicsContext3D::TEXTURE_2D, binding->textureId(texture()->textureId()));
> 
> isn't this done in ManagedTexture::bindTexture() ?

You can't access the ManagedTexture here as you're on the impl thread.

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:680
> > +    Platform3DObject textureId = quad->doubleBuffered() ? contentsTextureAllocatorBinding()->textureId(quad->textureId()) : quad->textureId();
> 
> This line is super obscure to read..

If quad is double buffered then texture Id is a managed texture that needs to be allocated and translated into an impl side texture Id. If it's not double buffered then texture Id is a real Id and already allocated. I know, this is confusing. I'll look at how this can be made more clear. Not sure a comment is enough.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:480
> > +    // Delete textures no longer used after commit finished.
> 
> a method on the allocator?

Do you mean adding a TextureAllocatorBinding delete method that takes a vector? I guess that could make sense.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:568
> > +    OwnPtr<DeferredTextureAllocatorBinding> contentsTextureAllocatorBinding = DeferredTextureAllocatorBinding::create(contentsTextureAllocator);
> 
> one is Ref, one is Own, which made me wonder who else can ref++ the contentsTextureAllocator other than LayerRendererChromium..

TextureAllocatorBinding instances.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:575
> > +            contentsTextureAllocator->setUseTextureStorageExt(true);
> 
> why is this in here?

As the instantiation of the contentsTextureAllocator move here I also moved this initialization part.

[David Reveman]

Created attachment 125893 [details]
Patch

[David Reveman]

Comment on attachment 125893 [details]
Patch

rebase

[Eric Seidel (no email)]

Comment on attachment 125893 [details]
Patch

Cleared review? from attachment 125893 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).