Bug 89045 – [chromium] LayerRendererChromium is not getting visibility messages in single threaded compositing mode.

• Component: New Bugs
• Status: RESOLVED FIXED

[Michal Mocny]

[chromium] LayerRendererChromium is not getting visibility messages in single threaded compositing mode.

[Michal Mocny]

Created attachment 147431 [details]
Patch

[Michal Mocny]

Comment on attachment 147431 [details]
Patch

Some webkit unittests are failing locally.  Going to remove cq while I take a look.

[Dana Jansens]

Comment on attachment 147431 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:262
>      // This proxy doesn't block commits when not visible so use a normal commit.

This comment should be changed to reflect what it's doing and why.

[Vangelis Kokkevis]

On my local build I had to add an early out before calling doCommit()

if (!m_layerRendererInitialized)
  return;

Without it content fails to load in foregrounded tabs and tabs that load in the background crash.  Did you not need it?

[WebKit Review Bot]

Comment on attachment 147431 [details]
Patch

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

New failing tests:
compositing/video-page-visibility.html
CCLayerTreeHostTestVisibilityAndAllocationControlDrawing.runSingleThread

[WebKit Review Bot]

Created attachment 147519 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

If we're going to do a commit, we should probably do everything that CCSingleThreadProxy::commitAndComposite() does, except the composite. Otherwise we're going to be having unexpected weirdness I think.

[James Robinson]

(In reply to comment #7)
> If we're going to do a commit, we should probably do everything that CCSingleThreadProxy::commitAndComposite() does, except the composite. Otherwise we're going to be having unexpected weirdness I think.

I don't think so, we definitely do not want to paint etc if we aren't visible.

[Dana Jansens]

(In reply to comment #8)
> (In reply to comment #7)
> > If we're going to do a commit, we should probably do everything that CCSingleThreadProxy::commitAndComposite() does, except the composite. Otherwise we're going to be having unexpected weirdness I think.
> 
> I don't think so, we definitely do not want to paint etc if we aren't visible.

Hm, I don't like having more than one type of commit though, unless we're being somehow more explicit about it.. In this case we're commiting a background memory allocation of 0, which means there will be nothing to paint anyhow. In threaded mode, we always commit the same way, and we could paint while not visible if we got a bigger background memory allocation (but that's good isn't it, with the goal of a larger background allocation being making the tab come back to life faster?)

[James Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > If we're going to do a commit, we should probably do everything that CCSingleThreadProxy::commitAndComposite() does, except the composite. Otherwise we're going to be having unexpected weirdness I think.
> > 
> > I don't think so, we definitely do not want to paint etc if we aren't visible.
> 
> Hm, I don't like having more than one type of commit though, unless we're being somehow more explicit about it.. In this case we're commiting a background memory allocation of 0, which means there will be nothing to paint anyhow. In threaded mode, we always commit the same way, and we could paint while not visible if we got a bigger background memory allocation (but that's good isn't it, with the goal of a larger background allocation being making the tab come back to life faster?)

Except that you typically paint different content when not visible (i.e. not focused), so I'm not sure how useful it really is.

[Dana Jansens]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > If we're going to do a commit, we should probably do everything that CCSingleThreadProxy::commitAndComposite() does, except the composite. Otherwise we're going to be having unexpected weirdness I think.
> > > 
> > > I don't think so, we definitely do not want to paint etc if we aren't visible.
> > 
> > Hm, I don't like having more than one type of commit though, unless we're being somehow more explicit about it.. In this case we're commiting a background memory allocation of 0, which means there will be nothing to paint anyhow. In threaded mode, we always commit the same way, and we could paint while not visible if we got a bigger background memory allocation (but that's good isn't it, with the goal of a larger background allocation being making the tab come back to life faster?)
> 
> Except that you typically paint different content when not visible (i.e. not focused), so I'm not sure how useful it really is.

Hm.. if it's not useful we could assume the memory manager would just leave background allocations at 0 then? Could we just do a full commit while backgrounded then?

[Michal Mocny]

Created attachment 147643 [details]
Patch

[James Robinson]

Comment on attachment 147643 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:263
> +    compositeImmediately();

I don't think it's a great idea to composite when going visible -> not visible - it's bypassing all of our rate limiting, flow control, etc.

One particular problem is this is going to produce a frame without firing requestAnimationFrame callbacks, which is bad.

[Dana Jansens]

Comment on attachment 147643 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:263
>> +    compositeImmediately();
> 
> I don't think it's a great idea to composite when going visible -> not visible - it's bypassing all of our rate limiting, flow control, etc.
> 
> One particular problem is this is going to produce a frame without firing requestAnimationFrame callbacks, which is bad.

FWIW doComposite() early-outs if (!m_layerTreeHostImpl->visible()). Does that alleviate the problem?

[James Robinson]

Not if we're doing this when first becoming visible.

[Dana Jansens]

(In reply to comment #15)
> Not if we're doing this when first becoming visible.

Are you worried, in that case, about drawing a frame with a 0 allocation? That should also be prevented by checking canDraw() in doComposite(). compositeImmediately() will only actually draw a frame if (visible && canDraw()). I think/hope these cases should be covered by unit tests.

[Dana Jansens]

I do see a potential problem of bypassing the rate limiting, but since this only happens when not visible, we should be doing commits without actually drawing frames anyhow. And presumably the mem manager isn't spamming us with differing memory allocations at a problematic rate.

Is it bad to not rate-limit visiblity changes of the tab?

[James Robinson]

I'm worried about when we _are_ visible compositing a frame without calling RAF callbacks.

In general, anything that tries to actually composite a frame while bypassing the rest of the control flow is deeply suspicious.

[James Robinson]

void CCLayerTreeHost::setVisible(bool visible)
{
    if (m_visible == visible)
        return;

    m_visible = visible;

    // FIXME: Remove this stuff, it is here just for the m20 merge.
    if (!m_visible && m_layerRendererInitialized) {
        if (m_proxy->layerRendererCapabilities().contextHasCachedFrontBuffer)
            setContentsMemoryAllocationLimitBytes(0);
        else
            setContentsMemoryAllocationLimitBytes(m_contentsTextureManager->preferredMemoryLimitBytes());
    }

    setNeedsForcedCommit();
}

we're calling setNeedsForcedCommit() when visible=true

[Dana Jansens]

(In reply to comment #18)
> I'm worried about when we _are_ visible compositing a frame without calling RAF callbacks.

Oh okay, I don't think it would be possible with the current use of setNeedsForcedCommit() (when we become visible we have a 0 memory allocation I think always right now since we set it to 0 when we become non-visible) but the API certainly opens up the possibility of it in the future, so it's not very safe.

> In general, anything that tries to actually composite a frame while bypassing the rest of the control flow is deeply suspicious.

What do you think of a scheduleComposite(bool forced) on the CCLTHClient, or maybe a scheduleForcedComposite() or the like?

[James Robinson]

I don't think we want a composite at _all_ in this case.  We just want to do a commit to propagate the visibility state to CCLTHI, we don't want anything else in the composite path.

[Dana Jansens]

(In reply to comment #21)
> I don't think we want a composite at _all_ in this case.  We just want to do a commit to propagate the visibility state to CCLTHI, we don't want anything else in the composite path.

Ah, yeh, I'm conflating commit with composite, my bad. >.<

I feel like I had asked this but I can't find it anywhere in the bug.. Are you okay with the commit happening solely inside the CCSingleThreadProxy (ie skipping the rest of the control flow in RenderWidget etc)?

[James Robinson]

Yeah, commits are (or should be) a compositor-internal thing.  We have to make sure that the function we call does exactly what we want and nothing more, but the proxy is supposed to decide when/how we synchronize state between main thread and impl thread data structures so IMO it's appropriate for it to handle that.

[Michal Mocny]

Created attachment 147672 [details]
Patch

[Michal Mocny]

James, as discussed, this will not just commit without composite.  I could pull out the common code to avoid duplication.

The unittests diverged behaviour between single/multi threaded mode since we end up forcing an additional commit before composite when single threaded sometimes.

[Dana Jansens]

Comment on attachment 147672 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        actually composite immediately.  This is needed to push visibility changes to LayerRendererChromium when

"should commit immediately"?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:82
> -    ASSERT(CCProxy::isImplThread());
> +    /ASSERT(CCProxy::isImplThread());

typo?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1014
> +            // Give not visibile memory allocation, still should not draw.

typo: "not visibile" -> "non-visible"

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1075
> +        case 7:
> +            EXPECT_TRUE(impl->visible());
> +            EXPECT_TRUE(impl->sourceFrameCanBeDrawn());
> +            break;

This is after the test is over isn't it? Do you need this case?

[Michal Mocny]

Comment on attachment 147672 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        actually composite immediately.  This is needed to push visibility changes to LayerRendererChromium when
> 
> "should commit immediately"?

thanks, old commit message still.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:82
>> +    /ASSERT(CCProxy::isImplThread());
> 
> typo?

yep thanks.

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1075
>> +            break;
> 
> This is after the test is over isn't it? Do you need this case?

This runs before the test ends.

[James Robinson]

(In reply to comment #25)
> James, as discussed, this will not just commit without composite.  I could pull out the common code to avoid duplication.
> 

Will, or will not?

> The unittests diverged behaviour between single/multi threaded mode since we end up forcing an additional commit before composite when single threaded sometimes.

[Dana Jansens]

Comment on attachment 147672 [details]
Patch

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

>>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1075
>>> +            break;
>> 
>> This is after the test is over isn't it? Do you need this case?
> 
> This runs before the test ends.

Ok I see, this is the result from step 6 in didCommitAndDrawFrame, thanks!

[Michal Mocny]

Created attachment 147677 [details]
Patch

[Michal Mocny]

(In reply to comment #28)
> (In reply to comment #25)
> > James, as discussed, this will not just commit without composite.  I could pull out the common code to avoid duplication.
> > 
> 
> Will, or will not?
Will *now*  (sorry, typo).
> 
> > The unittests diverged behaviour between single/multi threaded mode since we end up forcing an additional commit before composite when single threaded sometimes.

[James Robinson]

Comment on attachment 147677 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:265
> +    CCTextureUpdater updater;

Do we actually want to do uploads in this case?

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:267
> +    if (!m_layerTreeHost->updateLayers(updater))

it feels really weird to call updateLayers in this case if we aren't actually going do do any updates. What logic from this function are we trying to run in this case?

[Michal Mocny]

Comment on attachment 147677 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:265
>> +    CCTextureUpdater updater;
> 
> Do we actually want to do uploads in this case?

doCommit requires an updater, is there another implementation of one that does nothing?  Should I dissect doCommit to run a subset of it?

>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:267
>> +    if (!m_layerTreeHost->updateLayers(updater))
> 
> it feels really weird to call updateLayers in this case if we aren't actually going do do any updates. What logic from this function are we trying to run in this case?

I was just trying to mirror behaviour of commitAndComposite minus the composite.  updateLayers does things like initialize LRC, but I could just guard against that here.

[James Robinson]

(In reply to comment #33)
> (From update of attachment 147677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147677&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:265
> >> +    CCTextureUpdater updater;
> > 
> > Do we actually want to do uploads in this case?
> 
> doCommit requires an updater, is there another implementation of one that does nothing?  Should I dissect doCommit to run a subset of it?
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:267
> >> +    if (!m_layerTreeHost->updateLayers(updater))
> > 
> > it feels really weird to call updateLayers in this case if we aren't actually going do do any updates. What logic from this function are we trying to run in this case?
> 
> I was just trying to mirror behaviour of commitAndComposite minus the composite.  updateLayers does things like initialize LRC, but I could just guard against that here.

I know and I'm saying that is extremely suspect.  Let's start with what you _do_ want to happen and work back from there.

[James Robinson]

Here's a concrete proposal:

1.) Kill CCProxy::setNeedsForcedCommit() and never add it back.  It's a mistake.

2.) Add (back) CCProxy::setVisible. Have the implementation always make a blocking call to the impl thread to propagate the current visibility information.  On the impl thread, call deleteContentsTexturesOnImplThread().

3.) Have CCLayerTreeHost::setVisible do everything it wants to do in response to a visibility change (i.e. update the contentsTextureManager's limit) directly, then call CCProxy::setVisible()

4.) Have CCLayerTreeHostImpl::setVisible drop impl thread textures as needed - it appears this already happens for render surface textures and setVisiblityCHROMIUM()

et voila

GPU memory callbacks do not need any special proxy treatment, they come in on the correct thread already (the thread owning the GL context) so they can already issue whatever GL calls they want to free up resources.

[Dana Jansens]

There were a couple design decisions that led us to this point.

1. Eliminating alternate communication channels between threads and make everything a commit. See https://bugs.webkit.org/show_bug.cgi?id=85108#c33 onward.
2. Decoupling visibility from gpu memory allocations entirely so they come in 2 separate messages, and neither one controls the other state.

I think you're saying that 1 was a mistake, fair enough.

Is 2 also a mistake? Can you comment on 2 especially in relation to enne's comment #c33 (link above)?

(Maybe we could have a lil VC about this?)

[Vangelis Kokkevis]

(In reply to comment #36)
> There were a couple design decisions that led us to this point.
> 
> 1. Eliminating alternate communication channels between threads and make everything a commit. See https://bugs.webkit.org/show_bug.cgi?id=85108#c33 onward.
> 2. Decoupling visibility from gpu memory allocations entirely so they come in 2 separate messages, and neither one controls the other state.

Looking at the code, they don't seem to be too decoupled.  The contents texture manager only gets its limits updated when the visibility matches the limit with a somewhat questionable check (that assumes that !visible == 0 texture allocation) in CCLayerTreeHost::updateLayers().

I think the visibility / memory management code needs some serious clean up.  There's lots of scary FIXME's in the code. 

However, not freeing up textures and backbuffers on background tabs is a serious regression that affects stability and has also been merged into M20 which is about to go to Stable. We'll need to come up with the safest possible patch for now and then proceed to get the right fix in place. In my mind, the only safe fix at the moment is to allow a full commit to happen since that's where all the management logic currently is. This is what the threaded proxy does which presumably works ok.  We can either force it to happen right away or trigger it on a timer.


> 
> I think you're saying that 1 was a mistake, fair enough.
> 
> Is 2 also a mistake? Can you comment on 2 especially in relation to enne's comment #c33 (link above)?
> 
> (Maybe we could have a lil VC about this?)

[James Robinson]

There are multiple issues.

Visibility is currently not decoupled at all from memory management.  If it was, we may not be having these issues.  The other issue is that visibility is deeply coupled with proxy/scheduling behavior - especially since the single thread proxy relies on render_widget for scheduling.  Given that, I don't think it makes sense to try to pretend it's separate.

Longer-term, the GPU memory allocation callbacks always come in on the thread owning the GL context, so we can make any resource allocation decisions we want directly when receiving the message without involving the proxy at all.  The content texture limit only matters when we want to do the next commit and we should just let the normal flow control mechanism decide when that is.

[Michal Mocny]

Created attachment 147923 [details]
Patch

[Michal Mocny]

This is a quick update with new direction for early feedback.

CCLayerTreeHostTestVisibilityAndAllocationControlDrawing is failing (expected) and will need to be updated.

Right now, m_contentsTexturesWerePurgedOnImpl is set assuming it had happened upon receiving a 0 allocation (current true), but I will update to make this explicitly set.

Also, while unittests pass and both single/multi thread compositing seem to be behaving, I am getting an error (../../gpu/command_buffer/client/share_group.cc(74): GPU_DCHECK(id == 0 || id_allocator_.InUse(id) (0)) failed.) occasionally and will need to track that down, assuming it doesn't happen on ToT without this patch.

[WebKit Review Bot]

Attachment 147923 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 147923 [details]
Patch

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

If I understand the situation before this patch (broken as it is) is:

Before: CCLayerTreeHost::setVisible() reduces the contents memory limit, then tries to do a forced commit to delete contents textures either down to zero or the preferred minimum limit (based on contextHasCachedFrontBuffer). The forced commit doesn't work in the single threaded proxy currently but that aside it's attempting to do the texture deletions in response to setVisible.  CCLayerTreeHost::setVisible propagates a visibility bit to the impl side, which calls into LRC::setVisible which drops render surface textures and calls setVisibilityCHROMIUM()

After: Contents textures are deleted by the onGpuMemoryAllocationChanged callback, visibility and framebuffer stuff works the same.

This is a deliberate change, right?  Are we sure that using the GpuMemory.... callback instead of setVisible() for contents textures is going to work reliably?  If we have to merge a fix back, on which branches can we rely on these callbacks?

Given this, why are you still calling setContentsMemoryAllocationLimitBytes() inside CCLayerTreeHost::setVisible()? it seems like this value is just going to be clobbered by the call from the GPU memory manager.  It makes me really uneasy that you still have two systems trying to fight over that value in asynchronous ways.  I think you need to pick one system - either the CCLTH::setVisible() call or the gpu memory callback system - and have it be in charge of setContentsMemoryAllocationLimitBytes().  There's even a FIXME in the CCLTH::setVisible() indicating that some lines of code are just for some m20 merge.  It's m21 branch time already!

There's also _another_ set of ugly in CCLayerTreeHost::updateLayers() that has to do with m_frameIsForDisplay and different systems fighting over visibility state.  It feels like this needs to go away too.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:229
> +        // Purge all memory without requiring a commit.

it's weird to touch all of these internals on m_layerRenderer here - why not just call a function on LayerRendererChromium and have it do the right thing?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:232
> +            m_layerRenderer->m_client->setSourceFrameCanBeDrawn(false);

poor Demeter :(

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.cpp:110
> +void TrackingTextureAllocator::deleteAllTextures()
> +{
> +    // Copy so as not to invalidate iteration
> +    std::map< unsigned, std::pair<IntSize, GC3Denum> > textures = m_textures;
> +    for (std::map< unsigned, std::pair<IntSize, GC3Denum> >::iterator it = textures.begin(); it != textures.end(); ++it)
> +        deleteTexture(it->first, it->second.first, it->second.second);

doing a map walk + element-by-element delete seems unnecessary.  Why not iterate over the list of texture IDs + call m_context->deleteTexture() then zero out m_currentMemoryUseBytes ?

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.h:63
> +    std::map< unsigned, std::pair<IntSize, GC3Denum> > m_textures;

we don't use std::map<> in WebKit code - see HashMap.

I think all you really need is a set here, however (i.e. HashSet)

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:209
> +void CCThreadProxy::setVisible(bool visible)
> +{
> +    TRACE_EVENT0("cc", "CCThreadProxy::setVisible");
> +    CCProxy::implThread()->postTask(createCCThreadTask(this, &CCThreadProxy::setVisibleOnImplThread, visible));
> +}

I think this needs to be blocking (i.e. use a completion, etc)

[Michal Mocny]

Created attachment 147978 [details]
Patch

[Michal Mocny]

James, I think the deliberate direction is to have onGpuMemoryAllocationChanged callback handle all the memory allocation related work, and ideally not need setVisible to do anything memory related.


Current patch exhibits flashing black regions of tab contents when rapid tab switching w/ 2 tabs when using threaded compositing.  My hypothesis is that I am not sufficiently guarding the deletion of content textures in LRC::setGpuMemoryAllocation against them still being used, (I working on confirming this happens when getting 0 allocation when between begin/finish frame).

Also, not sure what you meant by your comment "poor Demeter".

[James Robinson]

Comment on attachment 147978 [details]
Patch

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

Re: Demeter: http://en.wikipedia.org/wiki/Law_of_Demeter.  It's not something we follow terribly religiously, but reaching through many layers of pointers to do a function call is a red flag.

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.cpp:107
> +    // Copy so as not to invalidate iteration

comment stale

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.cpp:108
> +    for (WTF::HashSet< unsigned >::const_iterator it = m_textureIds.begin(); it != m_textureIds.end(); ++it)

no WTF::, no spaces around template parameter

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.h:62
> +    WTF::HashSet< unsigned > m_textureIds;

no WTF::

no spaces around the type in the template parameter

a better name would be useful here to indicate which textures this is referring to

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:415
> +    m_contentsTexturesWerePurgedOnImpl = !bytes;

This seems a bit dodgy - the concepts of "here is your allocation" and "I deleted all your textures" are separate things.  Imagine:

1.) Tab becomes invisible
2.) GPU memory manager passes contents allocation of 0
3.) LRC deletes all contents textures
4.) Tab becomes visible
5.) GPU memory manager passes non-zero contents allocation
6.) Commit requested with non-zero allocation

if there aren't any commits between steps 2-6 (seems perfectly reasonable) then this code will see a non-zero allocation and not realize that all of the contents textures are actually gone.

I think "contentsTexturesWerePurged" is a separate bit of state from "here is your contents memory allocation" and this might explain the flashing you're seeing.  I think you need to pass both of these pieces of state.

Why isn't the contents texture memory limit set as part of the commit flow (i.e. in beginFrame() in threaded mode, in commitAndComposite in single threaded mode) ?  I see various "postSet..." for limits but I don't understand when you would want to know the limits on the main thread unless you were planning to do a commit

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:463
> +    if (m_contentsTexturesWerePurgedOnImpl) {
> +        m_contentsTextureManager->evictAndRemoveAllDeletedTextures();
> +        m_contentsTexturesWerePurgedOnImpl = false;

why are you storing a memory bool to keep track of whether the contents textures were purged only to do the actual logic here? this seems like another opportunity for bugs and races if the state gets clobbered.  As soon as you know in CCLTH that the contents textures were purged, just tell the manager.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:466
> +    m_frameIsForDisplay = m_memoryAllocationBytes;

when do we expect to get a commit with a memory budget of zero? it seems like that'd be a bug somewhere else in the system

[Michal Mocny]

Created attachment 148153 [details]
Patch

[Michal Mocny]

Comment on attachment 147978 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:415
>> +    m_contentsTexturesWerePurgedOnImpl = !bytes;
> 
> This seems a bit dodgy - the concepts of "here is your allocation" and "I deleted all your textures" are separate things.  Imagine:
> 
> 1.) Tab becomes invisible
> 2.) GPU memory manager passes contents allocation of 0
> 3.) LRC deletes all contents textures
> 4.) Tab becomes visible
> 5.) GPU memory manager passes non-zero contents allocation
> 6.) Commit requested with non-zero allocation
> 
> if there aren't any commits between steps 2-6 (seems perfectly reasonable) then this code will see a non-zero allocation and not realize that all of the contents textures are actually gone.
> 
> I think "contentsTexturesWerePurged" is a separate bit of state from "here is your contents memory allocation" and this might explain the flashing you're seeing.  I think you need to pass both of these pieces of state.
> 
> Why isn't the contents texture memory limit set as part of the commit flow (i.e. in beginFrame() in threaded mode, in commitAndComposite in single threaded mode) ?  I see various "postSet..." for limits but I don't understand when you would want to know the limits on the main thread unless you were planning to do a commit

You are right, they are separate concepts.  However, in this previous patch I didnt set memory allocation limit only during a commit.  The limit was updated (via the postSet..) every time it changed and so the situation you describe would still have caused an eviction of all textures.  (I mentioned in an earlier patch that it would be good to be explicit but wasn't necessary)

However, the suggestion to update memory limit only during commit is a good one and the new patch does this, so I now it is necessary to track if textures were purged with separate state, which I do in CCLTHImpl.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:463
>> +        m_contentsTexturesWerePurgedOnImpl = false;
> 
> why are you storing a memory bool to keep track of whether the contents textures were purged only to do the actual logic here? this seems like another opportunity for bugs and races if the state gets clobbered.  As soon as you know in CCLTH that the contents textures were purged, just tell the manager.

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:466
>> +    m_frameIsForDisplay = m_memoryAllocationBytes;
> 
> when do we expect to get a commit with a memory budget of zero? it seems like that'd be a bug somewhere else in the system

When returning from visible(false) we have a 0 allocation and still commit.  It would be interesting to prevent scheduling a commit until we have a non 0 allocation, but I really think that should be a follow on patch.  This logic was simplified significantly in recent patch, but we still protect against drawing a frame when we have 0 allocation.

[Michal Mocny]

New patch is another work in progress (still working on failing tests, etc), but updating early for feedback.

Currently adjust memory allocation only when already doing a commit, so adjusting memory allocation in CCLTH no longer schedules a commit, and we don't need to protect against memory allocation changes between commits.

The patch fundamentally assumes it is safe to delete all content textures on impl thread, which I don't quite think is true as the patch stands right now.  After beginning a frame, I think we could switch visibility and get a 0 allocation which textures are still in use by main thread.  I will need to find a way to delay deleting until we are finished with them.

[Dana Jansens]

Comment on attachment 148153 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1273
> +    if (m_visible != allocationIsForForeground)
> +        return;

Does this mean the GPU mem manager will also spam foreground allocations? (What if the GPU MemMgr is faster getting its message to LRC than the main thread's visibility state? Is that possible?)

> Source/WebCore/platform/graphics/chromium/TextureManager.h:91
> +    void evictAndRemoveAllDeletedTextures();

bikeshed: Is this name clear? "RemoveAllDeletedTextures" sounds to me like the TM is tracking deleted textures, but this is informing it that all textures are in fact deleted.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:400
> -void CCLayerTreeHost::setContentsMemoryAllocationLimitBytes(size_t bytes)
> +void CCLayerTreeHost::setMemoryAllocationLimitBytes(size_t bytes)

How come removing Contents from this name, but using Content in many of the new name? Is this limit meaning something new/different?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:238
> +    void evictAllContentTextures();

This name could be better I think. It's not evicting, it's informing that they are deleted.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:125
> +    , m_contentTexturesWerePurgedSinceLastCommit(false)

nit: contentsTextures (we use "contents" plural-like everywhere, I believe)
nit: would "WereDeleted" make sense for this? Purged is not a term we've used elsewhere before.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:420
> +    // Every time we get a new non 0 memory allocation, we must schedule a commit.

This comment just says what the next line does, probably shouldn't need to be here.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:347
> +    if (m_layerTreeHostImpl->contentTexturesWerePurgedSinceLastCommit())
> +        m_layerTreeHost->evictAllContentTextures();
> +    m_layerTreeHost->setMemoryAllocationLimitBytes(m_layerTreeHostImpl->memoryAllocationLimitBytes());

Would this make sense as one function call (pass the werePurged flag in while setting the memory limit)?

[Michal Mocny]

Comment on attachment 148153 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1273
>> +        return;
> 
> Does this mean the GPU mem manager will also spam foreground allocations? (What if the GPU MemMgr is faster getting its message to LRC than the main thread's visibility state? Is that possible?)

GpuMemoryManager can certainly send multiple foreground allocations (they change over time).  Regarding spam, there is a patch landing to limit the rate at which they are sent, but if you question is actually about possible races, right now we actually send allocation in response to visibility changes in the renderer -- but that will change.

Once that change happens, you are right, we may drop a non 0 allocation and thus fail to start drawing.

>> Source/WebCore/platform/graphics/chromium/TextureManager.h:91
>> +    void evictAndRemoveAllDeletedTextures();
> 
> bikeshed: Is this name clear? "RemoveAllDeletedTextures" sounds to me like the TM is tracking deleted textures, but this is informing it that all textures are in fact deleted.

Yes, I should be more clear, I meant to imply that Textures needed to be deleted already.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:400
>> +void CCLayerTreeHost::setMemoryAllocationLimitBytes(size_t bytes)
> 
> How come removing Contents from this name, but using Content in many of the new name? Is this limit meaning something new/different?

The memory allocation is a global limit for content+render surface etc, so I changed to name to reflect that.  However, in some places we change the actual content texture manager limit based on this global number.  The exact way we divide up the allocation is not perfect (see previous design docs regarding splitting content/render texture allocation), but it is what it is.  Maybe I didn't get all the naming right, so I'll double check.

[James Robinson]

(In reply to comment #47)
> When returning from visible(false) we have a 0 allocation and still commit.  It would be interesting to prevent scheduling a commit until we have a non 0 allocation, but I really think that should be a follow on patch.  This logic was simplified significantly in recent patch, but we still protect against drawing a frame when we have 0 allocation.

What's the point of doing a commit with a 0 allocation? We won't be able to paint anything at all, so why bother?

[James Robinson]

Comment on attachment 148153 [details]
Patch

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

In threaded mode, I think you can call setCanBeginFrame() on the scheduler whenever our memory allocation is zero.  There's no reason to commit with a zero allocation, there's nothing we can do on the main thread.

In single threaded mode we don't have as much control over the scheduler so I can see that we might get a call to render after becoming visible but before we get a GPU memory allocation callback - is that the case we're worried about?  In that case, we definitely need to bail on the frame before issuing a swap - so the question is where to do this.  How does the "SourceFrameCanBeDrawn" mechanism work in single threaded mode?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:250
> +    // We shouldn't draw the frame if the current memory allocation is 0.
> +    hostImpl->setSourceFrameCanBeDrawn(!!m_contentsTextureManager->maxMemoryLimitBytes());

I think this can be solved in a better way

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:397
> +    setNeedsCommit();

why do we need a commit here?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:405
> +    m_memoryAllocationLimitBytes = bytes;

why doesn't this call m_contentsTextureManager->setMemoryAllocationLimitBytes() - why wait until updateLayers() ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:305
> +    size_t m_memoryAllocationLimitBytes;

I don't think you need this

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:415
> +        static_cast<TrackingTextureAllocator*>(contentsTextureAllocator())->deleteAllTextures();

and what if the texture allocator isn't a TrackingTextureAllocator?

seems like deleteAllTextures() is a reasonable API to have exposed on the virtual base class and implemented by TrackingTextureAllocator.  That we could have a chance of getting a runtime ASSERT() or something of that nature if someone got the type wrong instead of silently casting to the wrong type

[James Robinson]

Comment on attachment 148153 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-338
> -void CCLayerTreeHost::setNeedsForcedCommit()

please delete the scheduler code for Forced Commit as well while you're in here.

[Dana Jansens]

Comment on attachment 148153 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-338
>> -void CCLayerTreeHost::setNeedsForcedCommit()
> 
> please delete the scheduler code for Forced Commit as well while you're in here.

The thread proxy/scheduler has had a forcedCommit prior to these changes in order to implement compositeAndReadback. We should probably just revert the changes from my CL wrt forced commit?

[James Robinson]

Ah, darned compositeAndReadback. Let's address that separately, then.

[Michal Mocny]

Comment on attachment 148153 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:250
>> +    hostImpl->setSourceFrameCanBeDrawn(!!m_contentsTextureManager->maxMemoryLimitBytes());
> 
> I think this can be solved in a better way

If we don't commit a 0 allocation, or bail early whenever we have a 0 allocation, then we won't need this at all.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:405
>> +    m_memoryAllocationLimitBytes = bytes;
> 
> why doesn't this call m_contentsTextureManager->setMemoryAllocationLimitBytes() - why wait until updateLayers() ?

This is for bootstrapping, the first time we set allocation limit the layer renderer/contents texture manager are not initialized yet.  I store here and use it in update layers after initialization.  If updateLayers had access to CCLTHImpl then we could just read the value directly from hostImpl.  Same goes for evictAllContentTextures, if we had access to hostImpl we wouldn't need to modify the Proxy behaviour.  Could I add that?

[Michal Mocny]

Comment on attachment 148153 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:494
> +    m_layerTreeHost->setMemoryAllocationLimitBytes(m_layerTreeHostImpl->memoryAllocationLimitBytes());

Was investigating if I can give updateLayer access to hostImpl, but updateLayers is run on main without blocked impl.  Its actually not safe to read hostImpl here directly either (I believe).  Seems there is a scheduledActionBeginFrame which uses a BeginFrameAndCommitState to post data to beginFrame for situations like this.

[James Robinson]

No, you cannot access CCLayerTreeHostImpl during updateLayers() nor do you need to - just create the contents texture manager with some default allocation and then give it a "real" allocation on the first beginFrame().  We won't hit updateLayers() before going through a commit.

[James Robinson]

Comment on attachment 148153 [details]
Patch

Needless to say, we also need to figure out the unit test situation before landing.

[James Robinson]

How about this for the !visible -> visible transition in single threaded mode: Whenever we transition from !visible -> visible, we produce a synthetic memory allocation (4x viewport or whatever) and use that until we get the first visible allocation from the manager.  Then we can happily produce frames as soon as we become visible and then adjust to hit the manager-set limit once we have it.

[Michal Mocny]

(In reply to comment #60)
> How about this for the !visible -> visible transition in single threaded mode: Whenever we transition from !visible -> visible, we produce a synthetic memory allocation (4x viewport or whatever) and use that until we get the first visible allocation from the manager.  Then we can happily produce frames as soon as we become visible and then adjust to hit the manager-set limit once we have it.

I'm confused regarding the purpose of this suggestion.  Is it just to decrease latency to produce the first frame?  And why is this suggestion specific to single thread mode, because we rely on render widget for scheduling?

Anyway, I believe I already accomplish what you suggest by initializing m_memoryAllocationLimitBytes in CCLTHImpl to non-0 (see constructor).  Then, on the first commit, we push that number over.  Ideally we wouldn't need to default to non-0 and we could delay the first frame until we get an allocation -- but I erred on the side of caution for now.

Regarding access to CCLayerTreeHostImpl during updateLayers: I'm just going to push the state of the two values (memoryAllocationLimitBytes and contentsTexturesWereDeletedSinceLastCommit) over using the m_pendingBeginFrameRequest in scheduledActionBeginFrame, then that we don't access hostImpl in beginFrame and can still set CCLTH values before calling updateLayers.

I think I've tracked the current crasher to the fact that we can get a 0 allocation while not visible but main thread is in the middle of beginFrame which started with a non 0 allocation since the impl thread is not blocked during this time.  I'm working delaying deleting contents textures until commitComplete.

[James Robinson]

(In reply to comment #61)
> (In reply to comment #60)
> > How about this for the !visible -> visible transition in single threaded mode: Whenever we transition from !visible -> visible, we produce a synthetic memory allocation (4x viewport or whatever) and use that until we get the first visible allocation from the manager.  Then we can happily produce frames as soon as we become visible and then adjust to hit the manager-set limit once we have it.
> 
> I'm confused regarding the purpose of this suggestion.  Is it just to decrease latency to produce the first frame?  And why is this suggestion specific to single thread mode, because we rely on render widget for scheduling?
> 
> Anyway, I believe I already accomplish what you suggest by initializing m_memoryAllocationLimitBytes in CCLTHImpl to non-0 (see constructor).  Then, on the first commit, we push that number over.  Ideally we wouldn't need to default to non-0 and we could delay the first frame until we get an allocation -- but I erred on the side of caution for now.
> 
> Regarding access to CCLayerTreeHostImpl during updateLayers: I'm just going to push the state of the two values (memoryAllocationLimitBytes and contentsTexturesWereDeletedSinceLastCommit) over using the m_pendingBeginFrameRequest in scheduledActionBeginFrame, then that we don't access hostImpl in beginFrame and can still set CCLTH values before calling updateLayers.
> 
> I think I've tracked the current crasher to the fact that we can get a 0 allocation while not visible but main thread is in the middle of beginFrame which started with a non 0 allocation since the impl thread is not blocked during this time.

If we aren't visible, how is the main thread in the middle of a beginFrame ?

>  I'm working delaying deleting contents textures until commitComplete.

[Michal Mocny]

(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > How about this for the !visible -> visible transition in single threaded mode: Whenever we transition from !visible -> visible, we produce a synthetic memory allocation (4x viewport or whatever) and use that until we get the first visible allocation from the manager.  Then we can happily produce frames as soon as we become visible and then adjust to hit the manager-set limit once we have it.
> > 
> > I'm confused regarding the purpose of this suggestion.  Is it just to decrease latency to produce the first frame?  And why is this suggestion specific to single thread mode, because we rely on render widget for scheduling?
> > 
> > Anyway, I believe I already accomplish what you suggest by initializing m_memoryAllocationLimitBytes in CCLTHImpl to non-0 (see constructor).  Then, on the first commit, we push that number over.  Ideally we wouldn't need to default to non-0 and we could delay the first frame until we get an allocation -- but I erred on the side of caution for now.
> > 
> > Regarding access to CCLayerTreeHostImpl during updateLayers: I'm just going to push the state of the two values (memoryAllocationLimitBytes and contentsTexturesWereDeletedSinceLastCommit) over using the m_pendingBeginFrameRequest in scheduledActionBeginFrame, then that we don't access hostImpl in beginFrame and can still set CCLTH values before calling updateLayers.
> > 
> > I think I've tracked the current crasher to the fact that we can get a 0 allocation while not visible but main thread is in the middle of beginFrame which started with a non 0 allocation since the impl thread is not blocked during this time.
> 
> If we aren't visible, how is the main thread in the middle of a beginFrame ?

Based on traces, I think:
1. CCThreadProxy::scheduledActionBeginFrame on lmpl, main not blocked
2. CCLTH::setVisible(false) on main
3. CCLTHImpl::setVisible(false)
4. CCThreadProxy::beginFrame() on main, impl not blocked
5. CCLTH::updateLayers() on main, impl not blocked
5. CCLTHImpl::setMemoryAllocationLimitBytes(0) on impl, in reaction to (3)

> 
> >  I'm working delaying deleting contents textures until commitComplete.

[James Robinson]

I've got an update in progress.  Here are the behaviors that I'm working towards, please go through and check carefully if these will or won't work.

Invariants:

1.) We never commit (paint, animate, any of it) when not visible on the main thread -except- for compositeAndReadback, regardless of threaded vs non-threaded mode
2.) CCLayerTreeHost::m_contentsTextureManager's memory budget is only set by updateLayers() when we are going to make a frame and is always set to a non-zero value
3.) Zero-sized allocations from the GPU process are always serviced immediately on the impl thread.  Non-zero allocations are met in the next frame, whenever we would produce that frame according to our usual frame scheduling logic.
4.) The impl thread always knows the set of currently-allocated managed texture IDs and can delete them all whenever it likes without needing the main thread to be responsive.

Details:

There are two main changes - tweaking how the contents texture manager's budget is handled and tweaking frame scheduling for the !visible case.

The scheduling change is a bit more subtle but it unifies the single and multi threaded paths and is really important.  Except for compositeAndReadback (which I'll talk about below), we simply won't produce frames when not visible.  This already happens in the single threaded path thanks to render_widget so the only change is to the threaded path.  The difficulty here is we might post a beginFrame task from the impl thread and then get a setVisible(false) call on the main thread before the beginFrame task runs.  Since I'm making the setVisible() call a blocking call from main thread -> impl thread, when the beginFrame task eventually does run on the main thread we can know that the impl thread's notion of visibility is in sync with the main threads.  Thus I'm planning to simply abort the frame before doing any processing on the main thread.  The scheduler will know if it gets a beginFrameComplete and its visibility state is false then the frame is aborted and it can go back to COMMIT_STATE_IDLE.

compositeAndReadback is special - this call currently does come in when we aren't visible (in single and threaded mode) and we need to service it.  In particular, we need to send a beginFrame over and have it not be ignored on the main thread.  For this I'm thinking of having the proxy keep track of whether it's servicing a compositeAndReadback() and use that bit on the main thread to know to process the beginFrame normally.  On the impl side, we need a few changes.  First, we have to allocate a default framebuffer (ensureFramebufferCHROMIUM) even if we've dropped it previously and remember to discard it after the readPixels().  Second, we have to provide a non-zero contents texture allocation on the beginFrame message, and again remember to delete the textures after the readPixels().  Third, we have to know that the beginFrame is a forced frame so when we get the beginFrameComplete we go ahead with the rest of the frame.  For this, I think I'll have to add ACTION_BEGIN_FORCED_FRAME and a corresponding COMMIT_STATE_FORCED_FRAME_IN_PROGRESS so the scheduler can keep track of the magicness of this frame, and then add some logic after the readpixels call to drop resources after the readback.  It's probably a good time to stop swapping on readbacks too....

The contents texture manager's budget is only relevant when we want to make a frame, so it's now passed in on the updateLayers().  Since we only make frames when we are visible and we never have a zero allocation when visible (thanks to the frame scheduling changes above), this value is always non-zero.  The other thing the texture manager needs to know about is if we've killed all of the underlying textures from the impl thread - this bit is passed in by the proxy before the updateLayers() call.  This means if we're running while visible and the manager wants to decrease our budget to something other than zero, we'll get a new (non-zero) allocation on the impl thread, schedule a frame, then when it's time to make the frame pass the new lower limit in to updateLayers(), then have the contents texture manager evict down to our new limit and make a frame with the new budget.  When the commit completes we'll get notified on the impl thread of which textures the contents texture manager decided to evict and issue the deleteTexture() calls on them.

The texture budget we pass in will be based on the most recent non-zero memory allocation we received from the GPU memory manager, or some default value I'll pull out my ass if we haven't heard anything yet.  On compositor initialization, we can't afford to wait for a round-trip through the GPU process to get a budget for the first frame.  I don't think handling a decrease to a non-zero budget on a visible tab needs to be terribly urgent - we can get to it when we get to making the next frame.  If we wanted to satisfy reduced texture budgets directly from the impl thread, we could keep a priority-list ordered set of textures once we have priorities and delete based on that.  Let's worry about that later.

The last bit of state to keep around is the desired framebuffer status.  Currently we discard the framebuffer if the manager tells us to, but since we always ensureFramebuffer when we beginDrawingFrame() if we draw for any reason after getting a discard hint (say due to compositeAndReadback) the framebuffer just sits around forever.  This is no good!  The manager's allocation messages may be arbitrarily behind our visibility notion, so if we simply blindly discard the framebuffer when we get an allocation we might end up discarding it when visible and have to recreate it unnecessarily and risk flashing.  Instead, I think I'll keep track of the most recent thing that the manager suggested and use that as a hint.  Specifically, if we are told to drop the framebuffer and we know that we aren't currently visible we'll go ahead and drop it - but if we get told to discard the framebuffer when we are visible we'll assume the allocations are just behind and ignore it.  If we have to make a framebuffer while not visible due to compositeAndReadback, we need to store the fact that the framebuffer was gone and discard it again after the readback.  I can't think of any situations where we would gain useful data by the memory manager telling us that we _do_ need a framebuffer - we definitely don't want or need to allocate one before we are ready to produce a frame - so I'm imaging that will just go into a black hole.  The FIXME in beginDrawingFrame() saying "FIXME: Remove this once framebuffer is automatically recreated on first use" seems confused - I'm pretty sure the code is doing exactly what the FIXME suggests it should do.

[Michal Mocny]

(In reply to comment #64)
> I've got an update in progress.  Here are the behaviors that I'm working towards, please go through and check carefully if these will or won't work.
> 
> Invariants:
> 
> 1.) We never commit (paint, animate, any of it) when not visible on the main thread -except- for compositeAndReadback, regardless of threaded vs non-threaded mode
> 2.) CCLayerTreeHost::m_contentsTextureManager's memory budget is only set by updateLayers() when we are going to make a frame and is always set to a non-zero value
> 3.) Zero-sized allocations from the GPU process are always serviced immediately on the impl thread.  Non-zero allocations are met in the next frame, whenever we would produce that frame according to our usual frame scheduling logic.
> 4.) The impl thread always knows the set of currently-allocated managed texture IDs and can delete them all whenever it likes without needing the main thread to be responsive.
> 
> Details:
> 
> There are two main changes - tweaking how the contents texture manager's budget is handled and tweaking frame scheduling for the !visible case.
> 
> The scheduling change is a bit more subtle but it unifies the single and multi threaded paths and is really important.  Except for compositeAndReadback (which I'll talk about below), we simply won't produce frames when not visible.  This already happens in the single threaded path thanks to render_widget so the only change is to the threaded path.  The difficulty here is we might post a beginFrame task from the impl thread and then get a setVisible(false) call on the main thread before the beginFrame task runs.  Since I'm making the setVisible() call a blocking call from main thread -> impl thread, when the beginFrame task eventually does run on the main thread we can know that the impl thread's notion of visibility is in sync with the main threads.  Thus I'm planning to simply abort the frame before doing any processing on the main thread.  The scheduler will know if it gets a beginFrameComplete and its visibility state is false then the frame is aborted and it can go back to COMMIT_STATE_IDLE.
> 
> compositeAndReadback is special - this call currently does come in when we aren't visible (in single and threaded mode) and we need to service it.  In particular, we need to send a beginFrame over and have it not be ignored on the main thread.  For this I'm thinking of having the proxy keep track of whether it's servicing a compositeAndReadback() and use that bit on the main thread to know to process the beginFrame normally.  On the impl side, we need a few changes.  First, we have to allocate a default framebuffer (ensureFramebufferCHROMIUM) even if we've dropped it previously and remember to discard it after the readPixels().  Second, we have to provide a non-zero contents texture allocation on the beginFrame message, and again remember to delete the textures after the readPixels().  Third, we have to know that the beginFrame is a forced frame so when we get the beginFrameComplete we go ahead with the rest of the frame.  For this, I think I'll have to add ACTION_BEGIN_FORCED_FRAME and a corresponding COMMIT_STATE_FORCED_FRAME_IN_PROGRESS so the scheduler can keep track of the magicness of this frame, and then add some logic after the readpixels call to drop resources after the readback.  It's probably a good time to stop swapping on readbacks too....
> 
> The contents texture manager's budget is only relevant when we want to make a frame, so it's now passed in on the updateLayers().  Since we only make frames when we are visible and we never have a zero allocation when visible (thanks to the frame scheduling changes above), this value is always non-zero.  The other thing the texture manager needs to know about is if we've killed all of the underlying textures from the impl thread - this bit is passed in by the proxy before the updateLayers() call.  This means if we're running while visible and the manager wants to decrease our budget to something other than zero, we'll get a new (non-zero) allocation on the impl thread, schedule a frame, then when it's time to make the frame pass the new lower limit in to updateLayers(), then have the contents texture manager evict down to our new limit and make a frame with the new budget.  When the commit completes we'll get notified on the impl thread of which textures the contents texture manager decided to evict and issue the deleteTexture() calls on them.
> 
> The texture budget we pass in will be based on the most recent non-zero memory allocation we received from the GPU memory manager, or some default value I'll pull out my ass if we haven't heard anything yet.  On compositor initialization, we can't afford to wait for a round-trip through the GPU process to get a budget for the first frame.  I don't think handling a decrease to a non-zero budget on a visible tab needs to be terribly urgent - we can get to it when we get to making the next frame.  If we wanted to satisfy reduced texture budgets directly from the impl thread, we could keep a priority-list ordered set of textures once we have priorities and delete based on that.  Let's worry about that later.
> 
> The last bit of state to keep around is the desired framebuffer status.  Currently we discard the framebuffer if the manager tells us to, but since we always ensureFramebuffer when we beginDrawingFrame() if we draw for any reason after getting a discard hint (say due to compositeAndReadback) the framebuffer just sits around forever.  This is no good!  The manager's allocation messages may be arbitrarily behind our visibility notion, so if we simply blindly discard the framebuffer when we get an allocation we might end up discarding it when visible and have to recreate it unnecessarily and risk flashing.  Instead, I think I'll keep track of the most recent thing that the manager suggested and use that as a hint.  Specifically, if we are told to drop the framebuffer and we know that we aren't currently visible we'll go ahead and drop it - but if we get told to discard the framebuffer when we are visible we'll assume the allocations are just behind and ignore it.  If we have to make a framebuffer while not visible due to compositeAndReadback, we need to store the fact that the framebuffer was gone and discard it again after the readback.  I can't think of any situations where we would gain useful data by the memory manager telling us that we _do_ need a framebuffer - we definitely don't want or need to allocate one before we are ready to produce a frame - so I'm imaging that will just go into a black hole.  The FIXME in beginDrawingFrame() saying "FIXME: Remove this once framebuffer is automatically recreated on first use" seems confused - I'm pretty sure the code is doing exactly what the FIXME suggests it should do.

Regarding the fixme, the official discardFramebufferEXT extension will automatically recreate the framebuffer upon next use (at the GL level) without need for an explicit "ensureFramebuffer" call.  Our extension doesn't do that yet, hence the FIXME.

[Dana Jansens]

(In reply to comment #64)
> I've got an update in progress.  Here are the behaviors that I'm working towards, please go through and check carefully if these will or won't work.
> 
> Invariants:
> 
> 1.) We never commit (paint, animate, any of it) when not visible on the main thread -except- for compositeAndReadback, regardless of threaded vs non-threaded mode
> 2.) CCLayerTreeHost::m_contentsTextureManager's memory budget is only set by updateLayers() when we are going to make a frame and is always set to a non-zero value
> 3.) Zero-sized allocations from the GPU process are always serviced immediately on the impl thread.  Non-zero allocations are met in the next frame, whenever we would produce that frame according to our usual frame scheduling logic.
> 4.) The impl thread always knows the set of currently-allocated managed texture IDs and can delete them all whenever it likes without needing the main thread to be responsive.
> 
> Details:
> 
> There are two main changes - tweaking how the contents texture manager's budget is handled and tweaking frame scheduling for the !visible case.
> 
> The scheduling change is a bit more subtle but it unifies the single and multi threaded paths and is really important.  Except for compositeAndReadback (which I'll talk about below), we simply won't produce frames when not visible.  This already happens in the single threaded path thanks to render_widget so the only change is to the threaded path.  The difficulty here is we might post a beginFrame task from the impl thread and then get a setVisible(false) call on the main thread before the beginFrame task runs.  Since I'm making the setVisible() call a blocking call from main thread -> impl thread, when the beginFrame task eventually does run on the main thread we can know that the impl thread's notion of visibility is in sync with the main threads.  Thus I'm planning to simply abort the frame before doing any processing on the main thread.  The scheduler will know if it gets a beginFrameComplete and its visibility state is false then the frame is aborted and it can go back to COMMIT_STATE_IDLE.

This sounds legit to me, though a bit magical. The fact that beginFrame is posted to the main thread non-blocking makes it hard to make it fail explicitly back to the scheduler though, so maybe the magic must be. What if beginFrameCompleteOnImplThread (and CCSchedulder::beginFrameComplete) took a success flag? Would this remove some of the beginFrameComplete special-casing for compositeAndReadback?

> compositeAndReadback is special - this call currently does come in when we aren't visible (in single and threaded mode) and we need to service it.  In particular, we need to send a beginFrame over and have it not be ignored on the main thread.  For this I'm thinking of having the proxy keep track of whether it's servicing a compositeAndReadback() and use that bit on the main thread to know to process the beginFrame normally.  On the impl side, we need a few changes.  First, we have to allocate a default framebuffer (ensureFramebufferCHROMIUM) even if we've dropped it previously and remember to discard it after the readPixels().  Second, we have to provide a non-zero contents texture allocation on the beginFrame message, and again remember to delete the textures after the readPixels().  Third, we have to know that the beginFrame is a forced frame so when we get the beginFrameComplete we go ahead with the rest of the frame.  For this, I think I'll have to add ACTION_BEGIN_FORCED_FRAME and a corresponding COMMIT_STATE_FORCED_FRAME_IN_PROGRESS so the scheduler can keep track of the magicness of this frame, and then add some logic after the readpixels call to drop resources after the readback.  It's probably a good time to stop swapping on readbacks too....

Yes, with bug #84803 in, dropping the swap should be good to go!

The allocation used for this case is the last non-zero allocation that was given to the renderer?

> The contents texture manager's budget is only relevant when we want to make a frame, so it's now passed in on the updateLayers().  Since we only make frames when we are visible and we never have a zero allocation when visible (thanks to the frame scheduling changes above), this value is always non-zero.  The other thing the texture manager needs to know about is if we've killed all of the underlying textures from the impl thread - this bit is passed in by the proxy before the updateLayers() call.  This means if we're running while visible and the manager wants to decrease our budget to something other than zero, we'll get a new (non-zero) allocation on the impl thread, schedule a frame, then when it's time to make the frame pass the new lower limit in to updateLayers(), then have the contents texture manager evict down to our new limit and make a frame with the new budget.  When the commit completes we'll get notified on the impl thread of which textures the contents texture manager decided to evict and issue the deleteTexture() calls on them.

Why not pass in the "allTexturesWereDeleted" flag to updateLayers() directly instead of passing it in separately from the proxy?

Thinking aloud: So canDraw() is going to be false between deleting textures on impl and getting the next frame from the main thread.

What is the story for when receive impl::setVisible(false) during COMMIT_STATE_UPDATING_RESOURCES or COMMIT_STATE_READY_TO_COMMIT? The textures are deleted immediately? How will the scheduler be modified for this? Or is this somehow not possible?

[James Robinson]

(In reply to comment #65)
> Regarding the fixme, the official discardFramebufferEXT extension will automatically recreate the framebuffer upon next use (at the GL level) without need for an explicit "ensureFramebuffer" call.  Our extension doesn't do that yet, hence the FIXME.

OK, good to know.  I'll update the comment - this means that we'll never call ensureFramebuffer, right?

[James Robinson]

(In reply to comment #66)
> > The scheduling change is a bit more subtle but it unifies the single and multi threaded paths and is really important.  Except for compositeAndReadback (which I'll talk about below), we simply won't produce frames when not visible.  This already happens in the single threaded path thanks to render_widget so the only change is to the threaded path.  The difficulty here is we might post a beginFrame task from the impl thread and then get a setVisible(false) call on the main thread before the beginFrame task runs.  Since I'm making the setVisible() call a blocking call from main thread -> impl thread, when the beginFrame task eventually does run on the main thread we can know that the impl thread's notion of visibility is in sync with the main threads.  Thus I'm planning to simply abort the frame before doing any processing on the main thread.  The scheduler will know if it gets a beginFrameComplete and its visibility state is false then the frame is aborted and it can go back to COMMIT_STATE_IDLE.
> 
> This sounds legit to me, though a bit magical. The fact that beginFrame is posted to the main thread non-blocking makes it hard to make it fail explicitly back to the scheduler though, so maybe the magic must be. What if beginFrameCompleteOnImplThread (and CCSchedulder::beginFrameComplete) took a success flag? Would this remove some of the beginFrameComplete special-casing for compositeAndReadback?

That's a good idea!  I'll do that.

> 
> > compositeAndReadback is special - this call currently does come in when we aren't visible (in single and threaded mode) and we need to service it.  In particular, we need to send a beginFrame over and have it not be ignored on the main thread.  For this I'm thinking of having the proxy keep track of whether it's servicing a compositeAndReadback() and use that bit on the main thread to know to process the beginFrame normally.  On the impl side, we need a few changes.  First, we have to allocate a default framebuffer (ensureFramebufferCHROMIUM) even if we've dropped it previously and remember to discard it after the readPixels().  Second, we have to provide a non-zero contents texture allocation on the beginFrame message, and again remember to delete the textures after the readPixels().  Third, we have to know that the beginFrame is a forced frame so when we get the beginFrameComplete we go ahead with the rest of the frame.  For this, I think I'll have to add ACTION_BEGIN_FORCED_FRAME and a corresponding COMMIT_STATE_FORCED_FRAME_IN_PROGRESS so the scheduler can keep track of the magicness of this frame, and then add some logic after the readpixels call to drop resources after the readback.  It's probably a good time to stop swapping on readbacks too....
> 
> Yes, with bug #84803 in, dropping the swap should be good to go!
> 
> The allocation used for this case is the last non-zero allocation that was given to the renderer?

Correct.

> 
> > The contents texture manager's budget is only relevant when we want to make a frame, so it's now passed in on the updateLayers().  Since we only make frames when we are visible and we never have a zero allocation when visible (thanks to the frame scheduling changes above), this value is always non-zero.  The other thing the texture manager needs to know about is if we've killed all of the underlying textures from the impl thread - this bit is passed in by the proxy before the updateLayers() call.  This means if we're running while visible and the manager wants to decrease our budget to something other than zero, we'll get a new (non-zero) allocation on the impl thread, schedule a frame, then when it's time to make the frame pass the new lower limit in to updateLayers(), then have the contents texture manager evict down to our new limit and make a frame with the new budget.  When the commit completes we'll get notified on the impl thread of which textures the contents texture manager decided to evict and issue the deleteTexture() calls on them.
> 
> Why not pass in the "allTexturesWereDeleted" flag to updateLayers() directly instead of passing it in separately from the proxy?
> 
> Thinking aloud: So canDraw() is going to be false between deleting textures on impl and getting the next frame from the main thread.
> 
> What is the story for when receive impl::setVisible(false) during COMMIT_STATE_UPDATING_RESOURCES or COMMIT_STATE_READY_TO_COMMIT? The textures are deleted immediately? How will the scheduler be modified for this? Or is this somehow not possible?

Can't happen - the main thread is blocked during these commit states (more specifically, from the time it starts processing the beginFrame message until the commit is complete).

[Michal Mocny]

(In reply to comment #67)
> (In reply to comment #65)
> > Regarding the fixme, the official discardFramebufferEXT extension will automatically recreate the framebuffer upon next use (at the GL level) without need for an explicit "ensureFramebuffer" call.  Our extension doesn't do that yet, hence the FIXME.
> 
> OK, good to know.  I'll update the comment - this means that we'll never call ensureFramebuffer, right?

To be clear, we'll not need to call ensureFramebuffer if we ever implement the extension to spec, which is not currently high priority.  We should absolutely continue to call it now.

[Dana Jansens]

(In reply to comment #68)
> > What is the story for when receive impl::setVisible(false) during COMMIT_STATE_UPDATING_RESOURCES or COMMIT_STATE_READY_TO_COMMIT? The textures are deleted immediately? How will the scheduler be modified for this? Or is this somehow not possible?
> 
> Can't happen - the main thread is blocked during these commit states (more specifically, from the time it starts processing the beginFrame message until the commit is complete).

Oh, yes, I'm mixing allocation and visible. <rephasing>

It's not clear to me in this description, or I missed it, what happens if impl receives a 0 allocation while it considers itself to be visible. Does it:

- Delete textures immediately? Then I'm worried about being in the middle of COMMIT_STATE_UPDATING_RESOURCES
- Save the 0 allocation until it becomes non-visible, then delete immediately?
- Other..

[James Robinson]

Ignores it completely.

[Dana Jansens]

(In reply to comment #64)
> 3.) Zero-sized allocations from the GPU process are always serviced immediately on the impl thread.  Non-zero allocations are met in the next frame, whenever we would produce that frame according to our usual frame scheduling logic.

(In reply to comment #71)
> Ignores it completely.

Ok! So reiterating for my own understanding.

Tab is visible:
3a.) Zero-sized allocations are ignored. Non-zero allocations are met in the next frame, whenever we would produce that frame according to our usual frame scheduling logic.

Tab is not visible:
3b.) Zero-size allocations are serviced immediately by the impl thread. Non-zero allocations are met in the next frame, whenever we would produce that frame according to our usual frame scheduling logic. But in the future, non-zero allocations may be serviced by the impl thread and it would communicate which textures were deleted when producing the next frame.

[James Robinson]

Yup!

[James Robinson]

Created attachment 148638 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 148638 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1269
> +    if (m_visible && !allocation.gpuResourceSizeInBytes)

Does this mean that if for some reason we end up getting setVisible(false) after the memory allocation that asks us to drop the framebuffer and textures we'll never actually drop them?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-407
> -            setContentsMemoryAllocationLimitBytes(0);

Do we not have a distinction anymore on whether the context has a front buffer or not?  We don't want to be deleting all contents textures if we don't have a cached front buffer.

[James Robinson]

(In reply to comment #75)
> (From update of attachment 148638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148638&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1269
> > +    if (m_visible && !allocation.gpuResourceSizeInBytes)
> 
> Does this mean that if for some reason we end up getting setVisible(false) after the memory allocation that asks us to drop the framebuffer and textures we'll never actually drop them?

The setVisible() call in the renderer always happens-before the associated memory allocation (since the gpu process is notified of the visibility change from the setVisibilityCHROMIUM call).  It's possible that the allocation associated with a given setVisible(false) call could happen significantly later than the setVisible() call but never before.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-407
> > -            setContentsMemoryAllocationLimitBytes(0);
> 
> Do we not have a distinction anymore on whether the context has a front buffer or not?  We don't want to be deleting all contents textures if we don't have a cached front buffer.

Interesting question.  I'm not sure if the service side is doing anything special with the hasCachedFramebuffer.  I could check that capability before issuing deletes for contents textures if that's what we want - but it seems like the gpu memory manager should be keeping track of this sort of thing when it's handing out allocations.  I will investigate - it doesn't seem safe to assume that any of this is doing what we want currently.

[Eric Penner]

> Invariants:
> 
> 1.) We never commit (paint, animate, any of it) when not visible on the main thread -except- for compositeAndReadback, regardless of threaded vs non-threaded mode

Sorry if this was already explained in other comments, but can you describe the motivation behind this invariant? What bad condition(s) does it prevent?

It's not ideal, but currently the main thread controls what textures should be kept alive when the memory limit changes, so it was assumed the main thread would always be in the loop. If it helps, we no longer need to do any painting/animating, or anything, to decide which textures we should keep when changing the memory limit (with the new texture manager). 

In addition to the concern about when we don't have a cachedFrontBuffer, deleting all textures in that allocator assumes that all textures can be deleted safely. I think that might just be okay right now (canvas rings a bell but I think that is done differently now).

In any case, going forward it sounds like all texture management will be moved to the impl side and then this won't be an issue.

[James Robinson]

Two main reasons:

1.) Save cpu/power/etc - there's no point in doing work on a frame that we aren't going to put on screen.
2.) We need to avoid calling requestAnimationFrame callbacks when !visible to be spec compliant and avoid busting web pages and I'd prefer not to have to special case this.

I also think it's a lot easier to reason about the system if the "make a frame" logic on the main thread only happens when we want to actually make a frame.  It's a bit silly to try to invoke some of the logic just to do memory management.  We also want to be able to be somewhat responsive to memory allocation changes - it's not a hard realtime system, but faster is better, so leaving the main thread out of the calculation helps quite a bit.

[Adrienne Walker]

Comment on attachment 148638 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1398
> +        discardFramebuffer();

Should we only do this if the last allocation said to? (Or if we recreated it for the purpose of compositeAndReadback?)

[James Robinson]

Maybe, that's the state tracking bit I was mentioning to you earlier - I could keep a bit on LayerRendererChromium (like m_framebufferDiscardRequestedOnLastAllocation, but with a hopefully better name) and decide whether or not to discard after the readback. It seems like a theoretical concern today (we appear to always be told to discard when in the background) but not too hard to add.

[James Robinson]

Created attachment 148695 [details]
rebased, real changelogs

[James Robinson]

This can make it through basic browsing (create/close/switch tabs) with composited and video content in single and threaded mode without hitting any ASSERT()s and passes unit / layout tests.  I'm going to do more manual testing to make sure we're actually freeing the resources we mean to on the service side.  I think we're good here as far as calls we're issuing into the context but there seem to be some backwards flushes and possibly other badness.

Unresolved feedback:
1.) If the allocation callback says 0, we drop to 0 even if we don't have a cached frame buffer.  I could change this to not drop contents textures if the frame buffer isn't cached, but this feels a bit fishy - I'm basically ignoring the global texture manager's suggestion based on local information - so it can't be right.  Maybe we want to ignore the allocation in the short term, maybe not.

2.) I'm not trying to be careful about restoring to the previous state after a readback-induced allocation when not visible.  It's not too much work to try to track a bit more state here, I just want to be careful when doing so to not accidentally end up not freeing anything again.  I'd rather address that in a separate patch since it is an optimization - dropping the world will always produce the correct result even if it's a bit slower than we want.  Messing that optimization up will result in a lot more badness (memory allocated and left around for all backgrounded tabs in a session restore situation) than just deleting will, IMO.

[James Robinson]

Created attachment 148710 [details]
add flushes after deleteTexture calls

[James Robinson]

compositing/video-page-visibility.html is somewhat interesting. It does a number of testRunner.display() calls, some while the page isn't visible.  With this patch we appear to actually render on each call (since that's what compositeAndReadback does) but we don't have any damage so every frame after the first simply puts another 50% gray layer on top, so the end result is a very dim but otherwise correct rendering.  I believe without this patch the setNeedsForcedCommit() in CCLayerTreeHost::setVisible() caused us to do full frame damage when switching visibility, so DRT would think that the last frame we produced was a full repaint.

[WebKit Review Bot]

Comment on attachment 148710 [details]
add flushes after deleteTexture calls

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

New failing tests:
compositing/video-page-visibility.html

[WebKit Review Bot]

Created attachment 148732 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Michal Mocny]

Comment on attachment 148710 [details]
add flushes after deleteTexture calls

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1402
> +        discardFramebuffer();

This assumes we have no framebuffer and no allocation when not visible, which may not always be the case.  What if we just cache the last recieved GpuMemoryAllocationCHROMIUM and call setGpuMemoryAllocation here?  This also solves code duplication.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:504
> +    if (!m_inCompositeAndReadback && !m_layerTreeHost->visible()) {

Will aborting frame here mean we won't signal RAF?

[James Robinson]

(In reply to comment #87)
> (From update of attachment 148710 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148710&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1402
> > +        discardFramebuffer();
> 
> This assumes we have no framebuffer and no allocation when not visible, which may not always be the case.  What if we just cache the last recieved GpuMemoryAllocationCHROMIUM and call setGpuMemoryAllocation here?  This also solves code duplication.
> 

Correct - see comment #82 point (2) 

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:504
> > +    if (!m_inCompositeAndReadback && !m_layerTreeHost->visible()) {
> 
> Will aborting frame here mean we won't signal RAF?

Correct - RAF is triggered by the updateAnimations() call below.  This is intentional.

[Dana Jansens]

Comment on attachment 148710 [details]
add flushes after deleteTexture calls

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

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.cpp:110
> +void TrackingTextureAllocator::deleteAllTextures()
> +{
> +    for (HashSet<unsigned>::const_iterator it = m_allocatedTextureIds.begin(); it != m_allocatedTextureIds.end(); ++it)
> +        GLC(m_context.get(), m_context->deleteTexture(*it));
> +    m_currentMemoryUseBytes = 0;
> +    m_allocatedTextureIds.clear();
>  }

I don't think I like using TextureAllocator as another texture manager on impl thread. The tracking texture allocator's purpose today is to track memory use within a category. Our current categories are "contents" and "render passes".

This is why video is using the contents allocator, as it's allocating textures that contain stuff that will be drawn to RenderPasses, and are not temporary for the current frame.

Adding this to the TextureAllocator now makes it also a manager of texture memory, and ties it more intimately to an instance of a TextureManager. If this behaviour is so tied to a TextureManager, I wish it could be closer to the TM, and not pull this whole class and it's interface with it.

If we're going to stop allocating video layer textures as "contents", then what category do they fit in? Do we need to add yet another TextureAllocator to LRC?

[Dana Jansens]

Comment on attachment 148710 [details]
add flushes after deleteTexture calls

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

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:304
> +    GraphicsContext3D* context3D = context->context3D();
> +    if (!context3D)
> +        return false;
> +
> +    GLC(context3D, textureId = context3D->createTexture());
> +    GLC(context3D, context3D->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId));
> +    // Do basic linear filtering on resize.
> +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::LINEAR));
> +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, GraphicsContext3D::LINEAR));
> +    // NPOT textures in GL ES only work when the wrap mode is set to GraphicsContext3D::CLAMP_TO_EDGE.
> +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_S, GraphicsContext3D::CLAMP_TO_EDGE));
> +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_T, GraphicsContext3D::CLAMP_TO_EDGE));
> +
> +    GLC(context3D, context3D->texImage2DResourceSafe(GraphicsContext3D::TEXTURE_2D, 0, format, size.width(), size.height(), 0, format, GraphicsContext3D::UNSIGNED_BYTE));

This code really needs a common home, if it's not going to be TrackingTextureAllocator for everyone. :/

This ties to enne's question on https://bugs.webkit.org/show_bug.cgi?id=89485#c19 "Is there something wrong with TrackingTextureAllocator?" and why I'm imagining CCScopedTexture as a way to create textures. Let's figure out how textures should be created and have a class that does only that?

[James Robinson]

Video using the contents allocator is completely bogus and I fixed that in this patch.  It shouldn't use one at all.

A TextureManager is responsible for managing a set of resources given a budget.  It's actually dealing in promises - it's one layer removed from actual GL textures.  A TextureAllocator is a way to allocate/free to actual GL textures.  I think it's perfectly reasonable for a TextureAllocator to know what GL textures it's given out and for someone who is using a TextureManager to let them know that things have changed w.r.t. the textures beneath it.

[James Robinson]

(In reply to comment #90)
> (From update of attachment 148710 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148710&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:304
> > +    GraphicsContext3D* context3D = context->context3D();
> > +    if (!context3D)
> > +        return false;
> > +
> > +    GLC(context3D, textureId = context3D->createTexture());
> > +    GLC(context3D, context3D->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId));
> > +    // Do basic linear filtering on resize.
> > +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::LINEAR));
> > +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, GraphicsContext3D::LINEAR));
> > +    // NPOT textures in GL ES only work when the wrap mode is set to GraphicsContext3D::CLAMP_TO_EDGE.
> > +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_S, GraphicsContext3D::CLAMP_TO_EDGE));
> > +    GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_T, GraphicsContext3D::CLAMP_TO_EDGE));
> > +
> > +    GLC(context3D, context3D->texImage2DResourceSafe(GraphicsContext3D::TEXTURE_2D, 0, format, size.width(), size.height(), 0, format, GraphicsContext3D::UNSIGNED_BYTE));
> 
> This code really needs a common home, if it's not going to be TrackingTextureAllocator for everyone. :/

It's 6 lines of code.  We can make a utility function somewhere but it's not really necessary for 6 lines.

[Adrienne Walker]

Comment on attachment 148710 [details]
add flushes after deleteTexture calls

R=me, assuming you'll rebaseline that test after landing.

[James Robinson]

Committed r120982: <http://trac.webkit.org/changeset/120982>

[WebKit Review Bot]

Re-opened since this is blocked by 89740

[James Robinson]

Some aura unit tests failed the new ASSERT()s in TrackingTextureAllocator since they used a TestWebGraphicsContext3D that always returned 1 from createTexture().  Fix for that up here: https://chromiumcodereview.appspot.com/10635024, once that lands I'll reland this patch as is.

[James Robinson]

Committed r121076: <http://trac.webkit.org/changeset/121076>