Bug 91177 – [chromium] Add 'self-managed' option to CCPrioritizedTexture to enable render-surface and canvas use cases.

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Penner]

[chromium] Add 'self-managed' option to CCPrioritizedTexture to enable render-surface and canvas use cases.

[Eric Penner]

Created attachment 152118 [details]
Patch

[Dana Jansens]

Comment on attachment 152118 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:500
> +size_t CCLayerTreeHost::prioritizeTextures(const LayerList& updateList)

Can we make the size_t a reference argument instead of a return value? That way it can get a descriptive name in the method definition.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
> +    // Create a memory placeholder texture to represent all surfaces,
> +    // since contents and surfaces aren't managed together (yet).

What will it mean to be "managed together"?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:54
> +    PassOwnPtr<CCPrioritizedTexture> createMemoryPlaceholder(size_t bytes)

could/should this be static?

[Eric Penner]

Comment on attachment 152118 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:500
>> +size_t CCLayerTreeHost::prioritizeTextures(const LayerList& updateList)
> 
> Can we make the size_t a reference argument instead of a return value? That way it can get a descriptive name in the method definition.

Sure sounds good.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
>> +    // since contents and surfaces aren't managed together (yet).
> 
> What will it mean to be "managed together"?

I meant that currently they are self managed via scoped textures. When all the surfaces are just prioritized textures themselves (if that makes sense once on the same thread) then we don't need a placeholder.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:54
>> +    PassOwnPtr<CCPrioritizedTexture> createMemoryPlaceholder(size_t bytes)
> 
> could/should this be static?

These functions create textures for the manager instance, so they can't be static. The CCPrioritizedTexture create functions are static though.

[Dana Jansens]

Comment on attachment 152118 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
>>> +    // since contents and surfaces aren't managed together (yet).
>> 
>> What will it mean to be "managed together"?
> 
> I meant that currently they are self managed via scoped textures. When all the surfaces are just prioritized textures themselves (if that makes sense once on the same thread) then we don't need a placeholder.

Oh okay, I think that wont make sense in ubercomp. The renderer reduces memory use for contents. But the host is the one allocating, so itll be a different process entirely. I think maybe this comment could just go away then?

>>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:54
>>> +    PassOwnPtr<CCPrioritizedTexture> createMemoryPlaceholder(size_t bytes)
>> 
>> could/should this be static?
> 
> These functions create textures for the manager instance, so they can't be static. The CCPrioritizedTexture create functions are static though.

Oh, createTexture(). Okay!

[Eric Penner]

Comment on attachment 152118 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
>>>> +    // since contents and surfaces aren't managed together (yet).
>>> 
>>> What will it mean to be "managed together"?
>> 
>> I meant that currently they are self managed via scoped textures. When all the surfaces are just prioritized textures themselves (if that makes sense once on the same thread) then we don't need a placeholder.
> 
> Oh okay, I think that wont make sense in ubercomp. The renderer reduces memory use for contents. But the host is the one allocating, so itll be a different process entirely. I think maybe this comment could just go away then?

But the child controls the host indirectly via not creating content for them right?  You think we should just keep doing this then?  Works by me. Anyway I'll remove the very likely incorrect comment.

[Dana Jansens]

Comment on attachment 152118 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
>>>>> +    // since contents and surfaces aren't managed together (yet).
>>>> 
>>>> What will it mean to be "managed together"?
>>> 
>>> I meant that currently they are self managed via scoped textures. When all the surfaces are just prioritized textures themselves (if that makes sense once on the same thread) then we don't need a placeholder.
>> 
>> Oh okay, I think that wont make sense in ubercomp. The renderer reduces memory use for contents. But the host is the one allocating, so itll be a different process entirely. I think maybe this comment could just go away then?
> 
> But the child controls the host indirectly via not creating content for them right?  You think we should just keep doing this then?  Works by me. Anyway I'll remove the very likely incorrect comment.

Ya I think its exactly what we want. Then the host can create surfaces at will, because they fit within the budget of the renderer providing the quads.

[Eric Penner]

Created attachment 152310 [details]
Patch

[Adrienne Walker]

Comment on attachment 152310 [details]
Patch

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

Can you add asserts about things with backings not being self-managed textures, e.g. CCPrioritizedTextureManager::returnBacking/createBacking/destroyBacking/assertInvariants?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:500
> -void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList)
> +void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList, size_t& memoryForRenderSurfaces)

This feels overloaded.  Can you split this into two functions? e.g. "size_t calculateMemoryForRenderSurfaces()" and "void prioritizeTextures(const LayerList&)" and maybe have paintContents add the managed textures?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:545
> +    // Create memory place-holder for surfaces (which are managed independantly) but should
> +    // reduce memory available for contents.
> +    OwnPtr<CCPrioritizedTexture> surfacePlaceholder = m_contentsTextureManager->createMemoryPlaceholder(memoryForRenderSurfaces);
> +    surfacePlaceholder->setRequestPriority(CCPriorityCalculator::renderSurfacePriority());

...and then maybe move this code into paintLayerContents. It's a little sketchy to me that the surfacePlaceholder has such a transient lifetime; should it be owned by the CCLTH?  If you create a self-managed texture, shouldn't the memory available immediately go down and then go back up if and when it gets deleted?

> Source/WebKit/chromium/tests/CCPrioritizedTextureTest.cpp:324
> +

Boo random whitespace.

[Eric Penner]

Created attachment 152326 [details]
Patch

[Eric Penner]

(In reply to comment #8)
> (From update of attachment 152310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152310&action=review
> 
> Can you add asserts about things with backings not being self-managed textures, e.g. CCPrioritizedTextureManager::returnBacking/createBacking/destroyBacking/assertInvariants?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:500
> > -void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList)
> > +void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList, size_t& memoryForRenderSurfaces)
> 
> This feels overloaded.  Can you split this into two functions? e.g. "size_t calculateMemoryForRenderSurfaces()" and "void prioritizeTextures(const LayerList&)" and maybe have paintContents add the managed textures?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:545
> > +    // Create memory place-holder for surfaces (which are managed independantly) but should
> > +    // reduce memory available for contents.
> > +    OwnPtr<CCPrioritizedTexture> surfacePlaceholder = m_contentsTextureManager->createMemoryPlaceholder(memoryForRenderSurfaces);
> > +    surfacePlaceholder->setRequestPriority(CCPriorityCalculator::renderSurfacePriority());
> 
> ...and then maybe move this code into paintLayerContents. It's a little sketchy to me that the surfacePlaceholder has such a transient lifetime; should it be owned by the CCLTH?  If you create a self-managed texture, shouldn't the memory available immediately go down and then go back up if and when it gets deleted?
> 
> > Source/WebKit/chromium/tests/CCPrioritizedTextureTest.cpp:324
> > +
> 
> Boo random whitespace.

I did most of what you asked but prioritizeTextures needs to make the placeholder after clearPriorities().

Regarding the temporary request. It just needs to exist during PrioritizedTextureManager::prioritizeTextures(). The model is that everything is cleared, requested and granted permission within CCLayerTreeHost::prioritizedTextures(), so the placeholder isn't needed outside of that function. For other placeholders like Canvas I imagine they will be persistent objects but in this case I thought it was nicer to keep it as a temp where it is needed.

[Eric Penner]

Comment on attachment 152310 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:95
> +    ASSERT(!m_isSelfManaged);

I added this ASSERT which should prevent anything to do with allocating memory. However I'll move this into the manager and add some extra ones to be safe.

[Adrienne Walker]

(In reply to comment #10)

> I did most of what you asked but prioritizeTextures needs to make the placeholder after clearPriorities().

Why?

> Regarding the temporary request. It just needs to exist during PrioritizedTextureManager::prioritizeTextures(). The model is that everything is cleared, requested and granted permission within CCLayerTreeHost::prioritizedTextures(), so the placeholder isn't needed outside of that function. For other placeholders like Canvas I imagine they will be persistent objects but in this case I thought it was nicer to keep it as a temp where it is needed.

I figured that's how other objects would be used, so it's odd to me that this would behave differently.

It still seems wrong to me that if somebody creates a placeholder after prioritizeTextures, then things like reduceMemory or requestLate will behave incorrectly.  Is there a reason that register/unregister a self-managed texture doesn't immediately change the available memory, as if it had acquired a backing?

[Adrienne Walker]

(In reply to comment #12)

> It still seems wrong to me that if somebody creates a placeholder after prioritizeTextures, then things like reduceMemory or requestLate will behave incorrectly.  Is there a reason that register/unregister a self-managed texture doesn't immediately change the available memory, as if it had acquired a backing?

Ok, scratch that.  This doesn't make any sense.  It's no good for anybody to create textures, even self-managed ones, *after* prioritizeTextures and expect any guarantees to be held.

[Eric Penner]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > It still seems wrong to me that if somebody creates a placeholder after prioritizeTextures, then things like reduceMemory or requestLate will behave incorrectly.  Is there a reason that register/unregister a self-managed texture doesn't immediately change the available memory, as if it had acquired a backing?
> 
> Ok, scratch that.  This doesn't make any sense.  It's no good for anybody to create textures, even self-managed ones, *after* prioritizeTextures and expect any guarantees to be held.

I was already mid comment, so here goes anyway:

Since textures are granted permission during prioritizeTextures(), we won't give more textures permission to allocate even if we were to increase the memory available after prioritizeTextures(). The primary benefit of releasing backings etc. when a texture is deleted is to allow those backings to be recycled. Since self-managed textures aren't recycled there is marginal benefit to changing the memory limit around.

Anyway, since that 'could' be the case that memory limits are adjusted, I see your point about persisting the object rather than having it be temporary. So I can do that if you want, to make it more independent of the manager implementation.

As for actually adjusting the memory available on unregister, I think that will have marginal benefit and possibly also be error prone since the user can change the selfManaged flag whenever they want.

[Dana Jansens]

IMO the temporary object is nice, but a comment saying it is only needed for prioritization might be nice.

[Dana Jansens]

saying why it is*

[Eric Penner]

(In reply to comment #12)
> (In reply to comment #10)
> 
> > I did most of what you asked but prioritizeTextures needs to make the placeholder after clearPriorities().
> 
> Why?
> 

It needs to set priority (and be created if it's a temp) after clearPriorities but before prioritizeTextures().

[Dana Jansens]

Can we do in CCLTH something like this?

m_pTM->clearPriorities()
surfaceMem = prioritizeSurfaceTextures()
prioritizeLayerTextures()

[Adrienne Walker]

(In reply to comment #17)
> (In reply to comment #12)
> > (In reply to comment #10)
> > 
> > > I did most of what you asked but prioritizeTextures needs to make the placeholder after clearPriorities().
> > 
> > Why?
> > 
> 
> It needs to set priority (and be created if it's a temp) after clearPriorities but before prioritizeTextures().

Because clearPriorities sets its priority? If so, maybe it shouldn't do that for self-managed textures.  How will that work for canvas self-managed textures?  Do you expect them to be re-set every frame like we do for other textures?

[Eric Penner]

Created attachment 152346 [details]
add_better_comment

[Eric Penner]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > 
> > > > I did most of what you asked but prioritizeTextures needs to make the placeholder after clearPriorities().
> > > 
> > > Why?
> > > 
> > 
> > It needs to set priority (and be created if it's a temp) after clearPriorities but before prioritizeTextures().
> 
> Because clearPriorities sets its priority? If so, maybe it shouldn't do that for self-managed textures.  How will that work for canvas self-managed textures?  Do you expect them to be re-set every frame like we do for other textures?

Yeah because clearPriorities changes the priority, so all priorities need to be set after that to get allocated. I expect that canvases will set the priority in ::setTexturePriorities like other layers?

[Eric Penner]

(In reply to comment #18)
> Can we do in CCLTH something like this?
> 
> m_pTM->clearPriorities()
> surfaceMem = prioritizeSurfaceTextures()
> prioritizeLayerTextures()

How is it better than currently? Passing a parameter to prioritizeTextures is bad?  I think taking clearPriorities() out of prioritizeTextures() is as bad as passing a parameter to it.

[Dana Jansens]

Comment on attachment 152346 [details]
add_better_comment

Sorry, to clarify and actually read the code again.. something like..

void CCLTH::prioritizeTextures(OverdrawMetrics metrics) {
  ptm->clearPriorities()

  setPrioritiesForSurfaces(surfaceMemoryBytes)
  setPrioritiesForLayers()
  [in future setPrioritiesForOtherThingsLikeCanvas()]

  ptm->prioritizeTextures()
  metrics.didUseBlah..(surfaceMemoryBytes)
}

I'm not sure it's any help though, what you've got makes sense too.

[Eric Penner]

(In reply to comment #23)
> (From update of attachment 152346 [details])
> Sorry, to clarify and actually read the code again.. something like..
> 
> void CCLTH::prioritizeTextures(OverdrawMetrics metrics) {
>   ptm->clearPriorities()
> 
>   setPrioritiesForSurfaces(surfaceMemoryBytes)
>   setPrioritiesForLayers()
>   [in future setPrioritiesForOtherThingsLikeCanvas()]
> 
>   ptm->prioritizeTextures()
>   metrics.didUseBlah..(surfaceMemoryBytes)
> }
> 
> I'm not sure it's any help though, what you've got makes sense too.

Ahh! I get it. That might be better but I was thinking canvases would each have their own self-managed texture which sets it's priority with other contents.

Enne, I'm happy to implement any of the additional suggestions if you still think they are better:
- Different behavior for clearPriorities of self-managed textures?
- Use a member variable for the placeholder in CCLTH. 

Just let me know what you prefer. In that case I'll add in Dana's suggestion too. IMO the current implementation has equal merits on the remaining issues mentioned.

[Adrienne Walker]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 152346 [details] [details])
> > Sorry, to clarify and actually read the code again.. something like..
> > 
> > void CCLTH::prioritizeTextures(OverdrawMetrics metrics) {
> >   ptm->clearPriorities()
> > 
> >   setPrioritiesForSurfaces(surfaceMemoryBytes)
> >   setPrioritiesForLayers()
> >   [in future setPrioritiesForOtherThingsLikeCanvas()]
> > 
> >   ptm->prioritizeTextures()
> >   metrics.didUseBlah..(surfaceMemoryBytes)
> > }
> > 
> > I'm not sure it's any help though, what you've got makes sense too.
> 
> Ahh! I get it. That might be better but I was thinking canvases would each have their own self-managed texture which sets it's priority with other contents.
> 
> Enne, I'm happy to implement any of the additional suggestions if you still think they are better:
> - Different behavior for clearPriorities of self-managed textures?
> - Use a member variable for the placeholder in CCLTH. 

I like what Dana suggests above.  Use a member variable to persist the placeholder and set its priority between clear and prioritize.  Don't treat self-managed textures differently in clearPriorities, since that won't make sense for canvases.

[Eric Penner]

Created attachment 152387 [details]
rebase_address_feedback

[Eric Penner]

Created attachment 152389 [details]
rebase_address_feedback

[Eric Penner]

Created attachment 152390 [details]
rebase_address_feedback

[Eric Penner]

Comment on attachment 152390 [details]
rebase_address_feedback

Arg, can't get chrome at ToT to run due to an unrelated crash.  I'll request CQ again once I can run in chrome.

[Adrienne Walker]

Comment on attachment 152390 [details]
rebase_address_feedback

R=me.  Thanks for those changes.  I find that to be way more intuitive.

[Eric Penner]

Comment on attachment 152390 [details]
rebase_address_feedback

NP, it's more clear now I agree. This works when rebased against gclient branch so I think it's GTG.

[WebKit Review Bot]

Comment on attachment 152390 [details]
rebase_address_feedback

Clearing flags on attachment: 152390

Committed r122658: <http://trac.webkit.org/changeset/122658>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

Comment on attachment 152390 [details]
rebase_address_feedback

+1 nice job