Bug 89485 – [chromium] Create CCScopedTexture class for creating/freeing textures

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

[Dana Jansens]

[chromium] Create CCScopedTexture class for creating/freeing textures

[Dana Jansens]

I have tried to

[Dana Jansens]

I have tried to explain in the ChangeLog my intension behind proposing this class.

We currently use the TextureAllocator to create/delete textures, but even in the new prioritized texture manager, we have to explicitly manage this memory. This class will fit well into the CCPrioritizedTexture::Backing class, allowing us to be confident about texture deletion without having to code it explicitly.

More importantly, it lets us easily/safely use textures on the impl thread without relying on ManagedTexture objects.

[Dana Jansens]

Created attachment 148361 [details]
Patch

[Dana Jansens]

Created attachment 148382 [details]
Patch

added CCScopedTexureTest.CreateScopedTextureForFramebufferAttachment

[Dana Jansens]

I think it might be nice to have this implement a CCScopedResource (or whatever name) interface, and we have a bitmap implementation of something like this class as well, instead of holding on to textureIds and shm handles directly.

[Alexandre Elias]

Comment on attachment 148382 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.cpp:116
> +void CCScopedTexture::steal(CCScopedTexture& other)

Could we not include this method?  I've never liked it.  Any functionality provided by this can be done instead by the texture manager moving around CCScopedTexture* pointers between various lists, and it will be much easier to understand that way.

> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.cpp:124
> +unsigned CCScopedTexture::release()

Rename this to leak(), as it's dangerous, and does something similar to OwnPtr::leakPtr().

[Dana Jansens]

Comment on attachment 148382 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.cpp:116
>> +void CCScopedTexture::steal(CCScopedTexture& other)
> 
> Could we not include this method?  I've never liked it.  Any functionality provided by this can be done instead by the texture manager moving around CCScopedTexture* pointers between various lists, and it will be much easier to understand that way.

Sure! I added this while thinking about trying to plug this into TextureManager but I think I'd rather not at all anyway.

>> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.cpp:124
>> +unsigned CCScopedTexture::release()
> 
> Rename this to leak(), as it's dangerous, and does something similar to OwnPtr::leakPtr().

We can drop this entirely then too.

[Dana Jansens]

Created attachment 148405 [details]
Patch

[Eric Penner]

Comment on attachment 148382 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.h:66
> +    RefPtr<GraphicsContext3D> m_context;

Is it okay to keep a refptr to the context? Do we always want textures to keep contexts alive?

As we talked about offline, one concern I have with this being used in CCPrioritizedTexture is that this context pointer isn't safe on the main thread. So if we do use it, we need to be very careful with methods that access the context (in particular the destructor is tricky). Or we can just wait, and see if that class even stays on the main thread for long.

[Dana Jansens]

Comment on attachment 148382 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.h:66
>> +    RefPtr<GraphicsContext3D> m_context;
> 
> Is it okay to keep a refptr to the context? Do we always want textures to keep contexts alive?
> 
> As we talked about offline, one concern I have with this being used in CCPrioritizedTexture is that this context pointer isn't safe on the main thread. So if we do use it, we need to be very careful with methods that access the context (in particular the destructor is tricky). Or we can just wait, and see if that class even stays on the main thread for long.

My current use has been in classes that get destroyed when a context is lost, so that works well. But if you wanted to hold onto the CCScopedTexture object across the lifetime of a context loss (and just recreate the texture resource within it), then this would be hard.

I think I should move the GraphicsContext3D context from the constructor to be a parameter on create().

This keeps a RefPtr in the class still, but clear() could release its reference to the pointer, which should be equivalent of zero-ing out the textureId on context-loss in current code.

[Eric Penner]

Comment on attachment 148382 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.cpp:116
>>> +void CCScopedTexture::steal(CCScopedTexture& other)
>> 
>> Could we not include this method?  I've never liked it.  Any functionality provided by this can be done instead by the texture manager moving around CCScopedTexture* pointers between various lists, and it will be much easier to understand that way.
> 
> Sure! I added this while thinking about trying to plug this into TextureManager but I think I'd rather not at all anyway.

If we remove then ignore this.

PrioritizedTexture will have a method called ::swap(), rather than ::steal(). The reason for keeping it there was that the instances that need the swap were nested three levels deep in other classes. So to swap the instances themselves would require lots of get/set plumbing.

[Alexandre Elias]

I just talked offline with epenner and we agreed that we shouldn't pre-emptively include swap() either.  Let's try and address the problems causing the need for these by refactoring the container classes instead.

[Dana Jansens]

Nice, was just going to try figure out if we could swap PTM::Backing objects instead :) Thanks.

[Dana Jansens]

Created attachment 148412 [details]
Patch

Pass the GC3D in to ::create() and clear/deref it on ::clear().

[Eric Penner]

(In reply to comment #12)
> I just talked offline with epenner and we agreed that we shouldn't pre-emptively include swap() either.  Let's try and address the problems causing the need for these by refactoring the container classes instead.

On that thought, even with all the nested classes, it won't be hard to remove ::swap on PTM. It's ugly to do a swap at that level, so should minimally be done at a higher level container.

[Antoine Labour]

Comment on attachment 148412 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.cpp:110
> +{

Is there a good way to assert that this was called on the same thread as create() ?

[Dana Jansens]

Created attachment 148433 [details]
Patch

assert that deleteTexture() happens on the same thread as createTexture()

[Dana Jansens]

Created attachment 148669 [details]
Patch

When a texture is deleted externally or the context is lost, you may want to set the id to 0 without calling deleteTexture() on it. Adding lose() to explicitly lose the texture without freeing it.

[Adrienne Walker]

Comment on attachment 148669 [details]
Patch

Here's a collection of assorted OO design thoughts and bikesheddery:
* I don't like all the code duplication between CCScopedTexture and TrackingTextureAllocator.  Is there something wrong with TrackingTextureAllocator?
* prepare vs. create seems like an unnecessary distinction.  I think size and format are only useful for an existing id.
* create sounds too much like static PassOwnPtr<T> create().
* RefPtr<GraphicsContext3D> makes this class unable to be used on the main thread, so we have a lot of functionality that the main thread can't use.
* I don't like the way layer renderer capabilities are stored statically.

Here's a strawperson proposal to simplify this a bit.

// Bag of data holding a potentially allocated texture
// could be reused on main thread side
// could be part of FrameData and serialized safely
// Kind of like what aelias and epenner were proposing on 89667
class TextureInfo {
    id / format / size
    bytes()
}

// Deletes textures when it goes out of scope, provides stub helpers for creation.
class ScopedTexture : TextureInfo {
    // TrackingTextureAllocator already wraps capabilities and context.
    ScopedTexture(TrackingTextureAllocator*)

    // Allocate could fail if texture already allocated?
    // These are just stub functions to call tracking texture allocator's create functions
    // (May need to adjust setTextureUsageHint to be more explicit)
    bool allocateTexture(size, format)
    bool allocateTextureForFramebufferAttachment(size, format)
    void lose()
    void free()

    TrackingTextureAllocator* // not sure about lifetime issues here, but LRC owns TTA and also owns RSTextureManager which owns ScopedTexture so TTA potentially can't outlive ScopedTexture?
}

[Dana Jansens]

Created attachment 149991 [details]
Patch

[Dana Jansens]

Thanks for the proposal! This is more or less what you proposed now.

[Dana Jansens]

Created attachment 150010 [details]
Patch

[Adrienne Walker]

Comment on attachment 150010 [details]
Patch

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

Thanks for all these changes.

> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.h:41
> +class CCTexture {

Different headers for different classes, please.

> Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.h:75
> +    void free();
> +    void lose();

bikeshed: s/lose/leak/ to use the same language as smart pointer classes, in that it clears without explicitly deleting? In rereading comments above, aelias suggested this earlier and I like it a lot.

[Dana Jansens]

Created attachment 151329 [details]
Patch

[Dana Jansens]

(In reply to comment #23)
> (From update of attachment 150010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150010&action=review
> 
> Thanks for all these changes.

Thanks for reviews :)

> > Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.h:41
> > +class CCTexture {
> 
> Different headers for different classes, please.

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCScopedTexture.h:75
> > +    void free();
> > +    void lose();
> 
> bikeshed: s/lose/leak/ to use the same language as smart pointer classes, in that it clears without explicitly deleting? In rereading comments above, aelias suggested this earlier and I like it a lot.

Ya, agree with this much.

[Adrienne Walker]

Comment on attachment 151329 [details]
Patch

R=me.  Looks good.

[Dana Jansens]

Created attachment 151373 [details]
Patch for landing

[Dana Jansens]

Committed r122180: <http://trac.webkit.org/changeset/122180>