Display skips swapping if overlays draw the damage

After scheduling overlay planes it's possible that the damaged region
is completely drawn by overlays, and drawing to the surface is skipped.
This will commonly occur during fullscreen video playback where the video
is in an overlay. In this case, it's much more power efficient to skip 
SwapBuffers for the native surface. This CL updates Renderer::DrawFrame to
return whether it skipped drawing because of overlays, and Display to
skip the Swap in that case.

BUG=533630
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[watk, 2015-11-19 05:16:52]

Description was changed from

==========
OutputSurfaceWithoutParent skips SwapBuffers when undamaged

After scheduling overlay planes it's now possible that the damaged region
is completely drawn by overlays, and drawing to the surface is skipped.
This will commonly occur during fullscreen video playback where the video
is in an overlay. In that case, it's much more power efficient on Android
to avoid calling eglSwapBuffers for the native surface. This CL updates
OutputSurfaceWithoutParent to skip the real Swap if the damage rect is
empty.

BUG=533630
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
After scheduling overlay planes it's possible that the damaged region
is completely drawn by overlays, and drawing to the surface is skipped.
This will commonly occur during fullscreen video playback where the video
is in an overlay. In this case, it's much more power efficient to skip 
SwapBuffers for the native surface. This CL updates Renderer::DrawFrame to
return whether it skipped drawing because of overlays, and Display to
skip the Swap in that case.

BUG=533630
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[watk, 2015-11-19 06:05:37]

Patchset #2 (id:20001) has been deleted

[watk, 2015-11-19 06:09:58]

Patchset #2 (id:40001) has been deleted

[watk, 2015-11-19 06:15:42]

watk@chromium.org changed reviewers:
+ ccameron@chromium.org

[watk, 2015-11-19 06:15:42]

Hey ccameron, I tried two different methods to achieve this. I'm pretty sure
patch set 2 is the better one, though. I think it might be cleanest for the gpu
side surface to be responsible for skipping the Swap when possible but it's not
clear how to get the information over there when postSubBuffers isn't supported.
Otherwise a PostSubBuffers of an empty rect would indicate this situation. WDYT?

[ccameron, 2015-11-19 09:28:03]

ccameron@chromium.org changed reviewers:
+ piman@chromium.org

[ccameron, 2015-11-19 09:28:03]

I like PS1 a bit better, but I think the best approach would be to add something
like "Capabilities().allow_empty_swap". In computing the damage rect in
DirectRenderer::DrawFrame, we can initially always compute the damage rect, and
then,
* if we support don't have "use_partial_swap" or "allow_empty_swap", then force
the damage rect to be the full rect
* if we support don't have "use_partial_swap" but do have "allow_empty_swap",
then force the damage rect to be either empty or be the full rect
* if we support "use_partial_swap" then leave the damage rect as computed

piman may also have opinions on this

[piman, 2015-11-19 21:14:14]

I don't understand either approach. ScheduleOverlayPlane doesn't do anything
until Swap/PostSubBuffer - otherwise you can't have atomic changes! While I
agree that you may want to skip the swap, you still need a form of
CommitOverlayPlanes() or something.

[piman, 2015-11-19 21:22:04]

https://codereview.chromium.org/1454323002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1454323002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2612: // choose whether to do a full swap or partial
swap.
If we do that we need to fix all the OutputSurface implementations. See e.g.
GpuBrowserCompositorOutputSurface::SwapBuffers

[watk, 2015-11-19 21:22:08]

On 2015/11/19 21:14:14, piman (slow to review) wrote:
> I don't understand either approach. ScheduleOverlayPlane doesn't do anything
> until Swap/PostSubBuffer - otherwise you can't have atomic changes! While I
> agree that you may want to skip the swap, you still need a form of
> CommitOverlayPlanes() or something.

I know what you mean. It's non-obvious that this should work. The reason it
should be ok in practice, and correct me if I'm missing something, is because
the only configuration where we'll skip the Swap is on Android for fullscreen
video, and in that case we don't need any atomic changes. If you're skipping the
swap it's because there's nothing that could be updated atomically with the
overlay: only the overlay is visible and the overlay will be drawn in
ScheduleOverlayPlane instead of waiting for Swap.

WDYT?

[piman, 2015-11-19 21:25:54]

On 2015/11/19 21:22:08, watk wrote:
> On 2015/11/19 21:14:14, piman (slow to review) wrote:
> > I don't understand either approach. ScheduleOverlayPlane doesn't do anything
> > until Swap/PostSubBuffer - otherwise you can't have atomic changes! While I
> > agree that you may want to skip the swap, you still need a form of
> > CommitOverlayPlanes() or something.
> 
> I know what you mean. It's non-obvious that this should work. The reason it
> should be ok in practice, and correct me if I'm missing something, is because
> the only configuration where we'll skip the Swap is on Android for fullscreen
> video, and in that case we don't need any atomic changes. If you're skipping
the
> swap it's because there's nothing that could be updated atomically with the
> overlay: only the overlay is visible and the overlay will be drawn in
> ScheduleOverlayPlane instead of waiting for Swap.
> 
> WDYT?

I think that we should add a CommitOverlayPlanes and keep things consistent.

[piman, 2015-11-19 21:27:02]

On 2015/11/19 21:25:54, piman (slow to review) wrote:
> On 2015/11/19 21:22:08, watk wrote:
> > On 2015/11/19 21:14:14, piman (slow to review) wrote:
> > > I don't understand either approach. ScheduleOverlayPlane doesn't do
anything
> > > until Swap/PostSubBuffer - otherwise you can't have atomic changes! While
I
> > > agree that you may want to skip the swap, you still need a form of
> > > CommitOverlayPlanes() or something.
> > 
> > I know what you mean. It's non-obvious that this should work. The reason it
> > should be ok in practice, and correct me if I'm missing something, is
because
> > the only configuration where we'll skip the Swap is on Android for
fullscreen
> > video, and in that case we don't need any atomic changes. If you're skipping
> the
> > swap it's because there's nothing that could be updated atomically with the
> > overlay: only the overlay is visible and the overlay will be drawn in
> > ScheduleOverlayPlane instead of waiting for Swap.
> > 
> > WDYT?
> 
> I think that we should add a CommitOverlayPlanes and keep things consistent.

(there's perfectly valid reasons to skip the swap on other platforms too if
damage is covered by overlays)

[watk, 2015-11-19 21:28:42]

On 2015/11/19 21:27:02, piman (slow to review) wrote:
> On 2015/11/19 21:25:54, piman (slow to review) wrote:
> > On 2015/11/19 21:22:08, watk wrote:
> > > On 2015/11/19 21:14:14, piman (slow to review) wrote:
> > > > I don't understand either approach. ScheduleOverlayPlane doesn't do
> anything
> > > > until Swap/PostSubBuffer - otherwise you can't have atomic changes!
While
> I
> > > > agree that you may want to skip the swap, you still need a form of
> > > > CommitOverlayPlanes() or something.
> > > 
> > > I know what you mean. It's non-obvious that this should work. The reason
it
> > > should be ok in practice, and correct me if I'm missing something, is
> because
> > > the only configuration where we'll skip the Swap is on Android for
> fullscreen
> > > video, and in that case we don't need any atomic changes. If you're
skipping
> > the
> > > swap it's because there's nothing that could be updated atomically with
the
> > > overlay: only the overlay is visible and the overlay will be drawn in
> > > ScheduleOverlayPlane instead of waiting for Swap.
> > > 
> > > WDYT?
> > 
> > I think that we should add a CommitOverlayPlanes and keep things consistent.
> 
> (there's perfectly valid reasons to skip the swap on other platforms too if
> damage is covered by overlays)

Ok, this certainly sounds like the right solution, thanks. I can start looking
at how we'd do it.