One way to fix this is by making the embedding renderer notify the embedee of its visibility status. This has two main problems:

- Can result in a lot of IPC messages. A page can have many OOPIFs and scrolling is a very common interaction with the web page.

- When the OOPIF is about to get visible, if its renderer process is slow to produce a frame we might end up showing old content. We can use surface sync to improve synchronization but then the scroll gets janky.

Another approach is rate-limiting off-screen clients in Viz. Viz knows which clients are offscreen. So one option is to send BeginFrames less frequently to those clients. Not only is this approach easier to implement, but also it will never result in more IPC messages than we have today, and also we won't have the problem of showing content that is too old.

This won't reduce GPU memory usage though.

This does not solve the same problem as frame eviction, and generally I think evicting frames that are simply scrolled away is too risky.

Why is it too risky? I think we need to solve this problem.

Because of flashes when they get visible. I will collect metrics to see if evicting offscreen OOPIFs is justified. Generally this idea should not replace frame eviction and frame eviction should not replace this idea; we can rate-limit aggressively but eviction should happen cautiously.

This sounds like two parts to the same problem: offscreen Renderers are participating in compositing.
  1) These offscreen renderers are continuously submitting CompositorFrames
  2) We are using up GPU memory for these offscreen renderers.

We likely will need to strike some balance based on how close these renderers are to the screen's visible rect. 

The north star here is site isolation should not increase the amount of GPU memory we use in total. It should not have a negative impact on performance either.

At a high level, a single-process renderer already has to make a very similar tradeoff wrt tiles:
- if we only render visible tiles on demand, we checkerboard/flash when scrolling
- rendering the whole page uses too many resources (memory and CPU/GPU time), so we pre-render with a limit how much we do (with a notion of "soon" tiles that extend pass the visible viewport, which is also skewed when scrolling), and scale back (dropping pre-rendered tiles) when the whole page goes invisible
- depending on the operations that change the set of tiles that need to be rendered, we either flash (when smoothness is more important, e.g. scrolling) or block (when correctness is more important, e.g. commit).

All that to say, I think we should aim to follow similar principles for OOPIFs, if not outright unifying the logic (there's likely issues with that), but possibly at least try to tune it/tradeoff in a similar way.

To stop the offscreen clients submitting compositor-frames, as samans@ suggested in comment #1, we could send BeginFrames less frequently to them. Could another option be to not send the begin-frame-ack to the offscreen clients? Would something like this be too simplistic: https://chromium-review.googlesource.com/c/chromium/src/+/1278535? This means the offscreen surfaces will always flash at least one frame of old content when it becomes visible. So it's probably not great ... but how bad would this be? This would be somewhat equivalent to switching to a non-discarded background tab, I think?

For addressing the GPU memory issue: the client could exclude the offscreen embedded-surface in |referenced_surfaces| if the embedded-surface is too far from the viewport (somewhere around [1]). Would that be sufficient to free up the gpu memory, or are there additional work that needs to happen? Once the embedder-surface starts getting closer to the viewport, it would start getting included in referenced_surfaces again. Although, maybe the offscreen surface would keep re-allocating + freeing the gpu memory?

[1] https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?type=cs&sq=package:chromium&g=0&l=1949

Could we perhaps be slightly more lax than your patch sadrul@ ?

For example, could we send the begin-frame-ack once the offscreen clients are within a region which could scroll into view within a few frames? Allowing them to prepare updated content. Avoiding the frame of old content you are concerned about?

Just so we're clear, I think you mean DidReceiveCompositorFrameAck not BeginFrameAck. BeginFrameAck goes from client => viz in response to a BeginFrame.

DidReceiveCompositorFrameAck goes from Viz => client in response to a SubmitCompositorFrame.

With that said, I like the direction Sadrul's patch is going in but I don't think it'll actually work in all cases.

We send an early draw callback here:

https://cs.chromium.org/chromium/src/components/viz/service/surfaces/surface_manager.cc?sq=package:chromium&g=0&l=576

I think we do want a prepaint skirt. Within that region we start pumping out frames regularly. Outside that region, we stop AND we evict GPU resources.

This needs to apply recursively. Hence the need for a direct client to client channel that Jon has started work on.

I meant that other ack, yes, sorry!

> We send an early draw callback here: ...

We could change that so it doesn't run the draw-callback if the surface-id is in |surfaces_to_ack_on_next_draw_|, for example?

#9: It's worth experimenting with your CL. Can you try the page at #0 and scroll the video in and out of the viewport quickly? Even if something like this is not doable we can still make improvements by making a certain percentage of BeginFrames animate-only.
It's not enough to drop the surface from referenced_surfaces. We need to also send a message to the child to set its visibility to false, otherwise a visible client will never get rid of its resources.

I made some additional changes to the CL. When I try with the video and scroll up and down, I don't really see a difference (with the caveat that I don't really have a good eye for such issues anyway).

Trace with tot: http://springfield.wat.corp.google.com/stuff/traces/trace_oopif-youtube-tot.json.gz

Trace with the fix: http://springfield.wat.corp.google.com/stuff/traces/trace_oopif-youtube-with-fix.json.gz

In the traces, the video is playing onscreen for first few seconds, and then I page-down to make the video offscreen (still playing). Some notes:
 . The viz-thread and main-thread in gpu-process does a lot less work with the CL for when the video is offscreen. This is expected.
 . The compositor-thread in the youtube.com process starts receiving begin-frames while the video is hidden with the CL, but it doesn't seem to get the begin-frames in ToT. This I think is a bit weird? I think it wouldn't be too hard to stop sending begin-frames to youtube.com, but I can't explain why it doesn't seem to get the begin-frames in ToT.

Trace with no-begin frame sent: http://springfield.wat.corp.google.com/stuff/traces/trace_oopif-youtube-with-fix-no-begin-frame.json.gz

(I have sent out the CL for review)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4dea3aba8069abf0688bef434173cc620ce87139

commit 4dea3aba8069abf0688bef434173cc620ce87139
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Fri Nov 02 16:59:56 2018

viz: Track the undrawn surfaces after aggregation.

Track the undrawn surfaces after aggregating all the surfaces for
display. This will be used to throttle begin-frames to undrawn
surfaces.

BUG=884876

Change-Id: I30fca08d4a8b5c773bc8f29111aad272032e14f9
Reviewed-on: https://chromium-review.googlesource.com/c/1315527
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604961}
[modify] https://crrev.com/4dea3aba8069abf0688bef434173cc620ce87139/components/viz/service/display/surface_aggregator.cc
[modify] https://crrev.com/4dea3aba8069abf0688bef434173cc620ce87139/components/viz/service/display/surface_aggregator.h
[modify] https://crrev.com/4dea3aba8069abf0688bef434173cc620ce87139/components/viz/service/display/surface_aggregator_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b04ad36ce3a18b0cc4c0f01406117566efccd4cb

commit b04ad36ce3a18b0cc4c0f01406117566efccd4cb
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Tue Nov 06 01:19:25 2018

viz: Throttle begin-frames to offscreen clients.

If a surface is offscreen, then stop sending it OnBeginFrame messages.
This avoids doing unnecessary work in the client. If an OOPIF, video,
etc. are offscreen because the main-page has been scrolled too far (or
not enough, or for some other reason), then they don't need to be
submitting new frames. Not sending the begin-frame messages to the
oopif etc. stops it from continuing to generate and submit new frames.
The side effect of this is that once the oopif becomes visible again,
its first frame can still be old content. This can resolved (e.g. by
throttling the messages, instead of stopping them outright), but I
think it's worth experimenting with this approach first. I think of
this as being a bit similar to how we do not trigger rAF callbacks on
background tabs.

One exception is that a begin-frame is always sent to a surface when
it is first embedded, even if it is offscreen. This allows the clients
to determine correctly when their embedding is complete.

BUG=884876

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I5c58a0fc956338b470b2fbfe30079d661927c894
Reviewed-on: https://chromium-review.googlesource.com/c/1278535
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605554}
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/display/display.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/display/display.h
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/display/display_scheduler.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/display/display_scheduler.h
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/display/display_scheduler_unittest.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/surfaces/surface.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/surfaces/surface.h
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/b04ad36ce3a18b0cc4c0f01406117566efccd4cb/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed198292670dd090c5092c2318aee7a6855e0164

commit ed198292670dd090c5092c2318aee7a6855e0164
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Tue Nov 06 07:19:07 2018

Revert "viz: Throttle begin-frames to offscreen clients."

This reverts commit b04ad36ce3a18b0cc4c0f01406117566efccd4cb.

Reason for revert: Made WebViewSizeTest.Shim_TestResizeWebviewWithDisplayNoneResizesContent flaky. crbug.com/902224

Original change's description:
> viz: Throttle begin-frames to offscreen clients.
> 
> If a surface is offscreen, then stop sending it OnBeginFrame messages.
> This avoids doing unnecessary work in the client. If an OOPIF, video,
> etc. are offscreen because the main-page has been scrolled too far (or
> not enough, or for some other reason), then they don't need to be
> submitting new frames. Not sending the begin-frame messages to the
> oopif etc. stops it from continuing to generate and submit new frames.
> The side effect of this is that once the oopif becomes visible again,
> its first frame can still be old content. This can resolved (e.g. by
> throttling the messages, instead of stopping them outright), but I
> think it's worth experimenting with this approach first. I think of
> this as being a bit similar to how we do not trigger rAF callbacks on
> background tabs.
> 
> One exception is that a begin-frame is always sent to a surface when
> it is first embedded, even if it is offscreen. This allows the clients
> to determine correctly when their embedding is complete.
> 
> BUG=884876
> 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
> Change-Id: I5c58a0fc956338b470b2fbfe30079d661927c894
> Reviewed-on: https://chromium-review.googlesource.com/c/1278535
> Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
> Reviewed-by: Fernando Serboncini <fserb@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#605554}

TBR=sadrul@chromium.org,kenrb@chromium.org,fsamuel@chromium.org,fserb@chromium.org,piman@chromium.org

Change-Id: I383fb0ba353074db91ae0bdc3f204005d59551be
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 884876, 902224
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/c/1319402
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605622}
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/display/display.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/display/display.h
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/display/display_scheduler.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/display/display_scheduler.h
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/display/display_scheduler_unittest.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/surfaces/surface.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/surfaces/surface.h
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/ed198292670dd090c5092c2318aee7a6855e0164/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

I thought I had asked a question on this bug a while ago but apparently it never went out: Did anyone look at why the OOPIF is not getting throttled when offscreen? Render throttling should kick in when an OOPIF's viewport intersection rect is empty -- i.e. LocalFrameView::ShouldThrottleRendering() will return true -- and that should prevent compositor frames from submitting.

There might be some bad interaction with the media code (the OOPIFs in #0 are videos). I'm not very familiar with the throttling code. Do you have time to take a look yourself?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d48953f8e42acdb29405aa34bd4e7757ee036aa

commit 6d48953f8e42acdb29405aa34bd4e7757ee036aa
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Wed Jan 09 18:57:48 2019

Reland "viz: Throttle begin-frames to offscreen clients."

This relands crrev.com/605554. The fix since the original change:
 . Instead of completely blocking begin-frames to offscreen clients,
   send at most one begin-frame every second to offscreen clients. This
   ensures that the content of the offscreen clients do not lag behind
   too far.

Original CL description
=======================
If a surface is offscreen, then stop sending it OnBeginFrame messages.
This avoids doing unnecessary work in the client. If an OOPIF, video,
etc. are offscreen because the main-page has been scrolled too far (or
not enough, or for some other reason), then they don't need to be
submitting new frames. Not sending the begin-frame messages to the
oopif etc. stops it from continuing to generate and submit new frames.
The side effect of this is that once the oopif becomes visible again,
its first frame can still be old content. This can resolved (e.g. by
throttling the messages, instead of stopping them outright), but I
think it's worth experimenting with this approach first. I think of
this as being a bit similar to how we do not trigger rAF callbacks on
background tabs.

One exception is that a begin-frame is always sent to a surface when
it is first embedded, even if it is offscreen. This allows the clients
to determine correctly when their embedding is complete.

BUG=884876, 902224

Change-Id: If41cdd703fda4fe7a09171f862dae8771b2ab3a6
Reviewed-on: https://chromium-review.googlesource.com/c/1320199
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621246}
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/display/display.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/display/display.h
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/display/display_scheduler.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/display/display_scheduler.h
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/display/display_scheduler_unittest.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/frame_sinks/compositor_frame_sink_support.h
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/surfaces/surface.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/surfaces/surface.h
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/components/viz/service/surfaces/surface_manager.cc
[modify] https://crrev.com/6d48953f8e42acdb29405aa34bd4e7757ee036aa/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc