Free temporary GPU and memory resources held by inactive or hidden 2D canvases

Free temporary GPU and memory resources held by inactive or hidden 2D canvases

BUG=272374
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165966

[eseidel, 2013-12-19 02:56:32]

https://codereview.chromium.org/117703004/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/117703004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.h:130: virtual bool hidden() const OVERRIDE {
return document().hidden(); }
Why do these individual elements know if they're hidden or not?

Shouldn't we just be signing up for some sort of "left the viewport" or "page
visibility chagned" notifications by things which care and then telling them via
some event/callback?

https://codereview.chromium.org/117703004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/117703004/diff/20001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.cpp:45: const size_t MaxSize =
~static_cast<size_t>(0);
std::limits?

https://codereview.chromium.org/117703004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerManager.cpp (right):

https://codereview.chromium.org/117703004/diff/20001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerManager.cpp:185: if
(currentLayer->hidden()) {
I'm surprised youw ant to check this "hidden" method everywhere instead of just
sending a didHide event to the various things which need to free memory.

https://codereview.chromium.org/117703004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerManager.h (right):

https://codereview.chromium.org/117703004/diff/20001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerManager.h:46: void didHidePage();
The page concept is largely leaving blink as in the out of process iframe world
blink may only know about some of teh Frames in a single page.  It's possible
didHidePage is still the right name for this however.

[Justin Novosad, 2013-12-19 14:00:06]

On 2013/12/19 02:56:32, eseidel wrote:
> Shouldn't we just be signing up for some sort of "left the viewport" or "page
> visibility chagned" notifications by things which care and then telling them
via
> some event/callback?
> 

I guess I was being shy about making significant changes to
node/page/document...
The way I see this happening is as follows:
Visibility change notifications are already propagated down to the document.
From there, the document could maintain a collection of nodes that are
interested in visibility change notifications, and propagate the message to just
those nodes.  

By the way, the strategy here is to track the document's visibility and not the
canvas's visibility because a lot of apps use offscreen canvases for preliminary
rendering tasks. I do not want this patch to penalize those offscreen canvas use
cases.

[eseidel, 2013-12-19 19:41:38]

We currently don't have any real viewporting support in Blink. We need to grow
some, desperately.

This fix is less about viewporting specifically and more about just whether the
tab is even visible.

I think we could either have a full document crawl, sending a "btw this documetn
just got hidden" notification to the entire DOM/rendering tree, or we could have
Nodes sign up for these notifications.  Either is fine with me.

It just seemed awkward that this change was about polling for this state instead
of being told about it.  But maybe I misunderstood.

[Justin Novosad, 2014-01-24 18:13:13]

Done. new patch.

The new patch uses an observer pattern to propagate visibility state changed
from the document down to the Canvas2DLayerBridge.  So no polling, and no DOM
traversal.
I also made some adjustments to the resource management strategy based on some
testing I performed locally.

[Stephen White, 2014-01-24 18:52:34]

https://codereview.chromium.org/117703004/diff/100001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/117703004/diff/100001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.h:34: #include
"platform/graphics/Canvas2DLayerBridge.h"
Is this still necessary? I see DocumentVisibilityObserver and friends below, but
nothing bridge-dependent.

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:152:
freeMemoryIfPossible(MaxSize);
Nit: Maybe there should just be a freeAllMemoryIfPossible() entry  point? or
freeAllMemory()? I see this problem goes all the way down to  SkBitmapHeap,
though.

Could we just do freeMemoryIfPossible(bytesAllocated()), if that's the same
thing?

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:254: return
m_releasedMailboxInfo && m_framesSinceMailboxRelease > 2;
A comment to explain this heuristic might be useful here.

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:368: // With hidden canvases,
we release the SkImage immediately because
I don't understand this comment. This seems to be the !isHidden() case?

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerManager.cpp (right):

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerManager.cpp:89: layer = layer->next();
Could you add a comment here explaining why we have to increment layers before
we limit pending frames? Seems a little subtle.

https://codereview.chromium.org/117703004/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/117703004/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:3929: bool visible = visibilityState ==
WebPageVisibilityStateVisible;
Don't see the need for this change; maybe a remnant of an older version (or I'm
being dumb).

https://codereview.chromium.org/117703004/diff/100001/Source/web/tests/Canvas...
File Source/web/tests/Canvas2DLayerManagerTest.cpp (right):

https://codereview.chromium.org/117703004/diff/100001/Source/web/tests/Canvas...
Source/web/tests/Canvas2DLayerManagerTest.cpp:183: EXPECT_EQ((size_t)0,
layer->bytesAllocated());
Could we expect that bytesAllocated() is non-zero at some point before the set
hidden call?

[Justin Novosad, 2014-01-27 18:43:25]

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/117703004/diff/100001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:368: // With hidden canvases,
we release the SkImage immediately because
On 2014/01/24 18:52:35, Stephen White wrote:
> I don't understand this comment. This seems to be the !isHidden() case?

Holy s***. This is a major bug. Thanks for spotting it.  The condition is
inverted which inserts a race condition, and fails to fix unwanted
double-buffering under certain conditions.  I fixed this locally, and GPU memory
consumption is even better now!

[Justin Novosad, 2014-01-27 18:47:11]

New Patch.

[Stephen White, 2014-01-27 18:55:13]

Canvas LGTM. eseidel@, could you weigh in on the DOM parts?

https://codereview.chromium.org/117703004/diff/290001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/117703004/diff/290001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:251: // acitvely presented by
the compositor, suggesting that it does
Nit: acitvely -> actively

Perhaps we should explain what the heuristic is; ie., the canvas hasn't been
drawn to in the last two frames (if that's what it is.)

[Justin Novosad, 2014-01-27 20:23:13]

Comment fixed. 
eseidel@: anything to add?

[eseidel, 2014-01-28 00:02:27]

I'm OK with the general approach.  Are these resources held by the renderer or
the browser?  If they're held by the browser, why does it need to go through the
renderer to release them?  The browser knows when a page loses visibility (it
told it to!).

But overall if this looks OK to canvas folks, I'm OK with the overall design.

[Justin Novosad, 2014-01-28 16:21:02]

On 2014/01/28 00:02:27, eseidel wrote:
> I'm OK with the general approach.  Are these resources held by the renderer or
> the browser?  If they're held by the browser, why does it need to go through
the
> renderer to release them?  The browser knows when a page loses visibility (it
> told it to!).
> 
> But overall if this looks OK to canvas folks, I'm OK with the overall design.

All the resources in question are held by the renderer. GPU textures are
technically held by the GPU process, but it is the renderer that holds them by
proxy because their scope is tied to objects that live in the renderer.

[commit-bot: I haz the power, 2014-01-28 16:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/117703004/330001

[commit-bot: I haz the power, 2014-01-28 16:38:05]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Justin Novosad, 2014-01-28 18:23:46]

Oops, missing approval from a Source/web OWNER for a... unit test!  This is
ridiculous.

+aelias

[aelias_OOO_until_Jul13, 2014-01-28 18:54:53]

On 2014/01/28 18:23:46, junov wrote:
> Oops, missing approval from a Source/web OWNER for a... unit test!  This is
> ridiculous.
> 
> +aelias

lgtm

[commit-bot: I haz the power, 2014-01-28 19:07:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/117703004/330001

[commit-bot: I haz the power, 2014-01-28 21:47:04]

Change committed as 165966

[commit-bot: I haz the power, 2014-01-28 21:49:28]

CQ bit was unchecked on CL. Ignoring.