Bug 89485 - [chromium] Create CCScopedTexture class for creating/freeing textures
Summary: [chromium] Create CCScopedTexture class for creating/freeing textures
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: Dana Jansens
URL:
Keywords:
Depends on:
Blocks: 89474 89667 90841
  Show dependency treegraph
 
Reported: 2012-06-19 10:54 PDT by Dana Jansens
Modified: 2012-07-09 18:45 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.22 KB, patch)
2012-06-19 11:01 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (24.14 KB, patch)
2012-06-19 11:48 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (22.76 KB, patch)
2012-06-19 13:21 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (22.37 KB, patch)
2012-06-19 13:53 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (22.67 KB, patch)
2012-06-19 14:55 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (23.61 KB, patch)
2012-06-20 15:40 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (15.46 KB, patch)
2012-06-28 13:14 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (15.96 KB, patch)
2012-06-28 14:10 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (20.13 KB, patch)
2012-07-09 15:13 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (20.14 KB, patch)
2012-07-09 17:57 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-06-19 10:54:36 PDT
[chromium] Create CCScopedTexture class for creating/freeing textures
Comment 1 Dana Jansens 2012-06-19 10:59:53 PDT
I have tried to
Comment 2 Dana Jansens 2012-06-19 11:01:43 PDT
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.
Comment 3 Dana Jansens 2012-06-19 11:01:50 PDT
Created attachment 148361 [details]
Patch
Comment 4 Dana Jansens 2012-06-19 11:48:15 PDT
Created attachment 148382 [details]
Patch

added CCScopedTexureTest.CreateScopedTextureForFramebufferAttachment
Comment 5 Dana Jansens 2012-06-19 12:50:24 PDT
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.
Comment 6 Alexandre Elias 2012-06-19 13:09:19 PDT
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().
Comment 7 Dana Jansens 2012-06-19 13:19:30 PDT
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.
Comment 8 Dana Jansens 2012-06-19 13:21:39 PDT
Created attachment 148405 [details]
Patch
Comment 9 Eric Penner 2012-06-19 13:39:02 PDT
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.
Comment 10 Dana Jansens 2012-06-19 13:43:49 PDT
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.
Comment 11 Eric Penner 2012-06-19 13:45:48 PDT
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.
Comment 12 Alexandre Elias 2012-06-19 13:50:31 PDT
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.
Comment 13 Dana Jansens 2012-06-19 13:51:26 PDT
Nice, was just going to try figure out if we could swap PTM::Backing objects instead :) Thanks.
Comment 14 Dana Jansens 2012-06-19 13:53:49 PDT
Created attachment 148412 [details]
Patch

Pass the GC3D in to ::create() and clear/deref it on ::clear().
Comment 15 Eric Penner 2012-06-19 14:06:20 PDT
(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.
Comment 16 Antoine Labour 2012-06-19 14:39:27 PDT
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() ?
Comment 17 Dana Jansens 2012-06-19 14:55:54 PDT
Created attachment 148433 [details]
Patch

assert that deleteTexture() happens on the same thread as createTexture()
Comment 18 Dana Jansens 2012-06-20 15:40:53 PDT
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.
Comment 19 Adrienne Walker 2012-06-21 13:52:25 PDT
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?
}
Comment 20 Dana Jansens 2012-06-28 13:14:56 PDT
Created attachment 149991 [details]
Patch
Comment 21 Dana Jansens 2012-06-28 13:15:22 PDT
Thanks for the proposal! This is more or less what you proposed now.
Comment 22 Dana Jansens 2012-06-28 14:10:25 PDT
Created attachment 150010 [details]
Patch
Comment 23 Adrienne Walker 2012-07-09 10:44:57 PDT
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.
Comment 24 Dana Jansens 2012-07-09 15:13:36 PDT
Created attachment 151329 [details]
Patch
Comment 25 Dana Jansens 2012-07-09 15:14:25 PDT
(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.
Comment 26 Adrienne Walker 2012-07-09 15:18:41 PDT
Comment on attachment 151329 [details]
Patch

R=me.  Looks good.
Comment 27 Dana Jansens 2012-07-09 17:57:58 PDT
Created attachment 151373 [details]
Patch for landing
Comment 28 Dana Jansens 2012-07-09 18:43:47 PDT
Committed r122180: <http://trac.webkit.org/changeset/122180>