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

commit 60d359fd3bd0bbe1089cf24c196187b3c03fcd65
Author: Eric Karl <ericrk@chromium.org>
Date: Tue Aug 21 22:59:23 2018

Android OOP-D: DisplayScheduler should only report draw-inducing damage

Currently DisplayScheduler reports damage to SurfaceManager in any case
where a damaged surface ID damages the display. While these cases will
trigger a draw when visible, they don't necessarily trigger a draw
in any defined timeframe. This can lead to issues. Consider:

1) Display 1 is not visible, embeds surface A.
2) Surface A sends CF 1, we don't immediately ack as we will wait for
   Display 1 to draw.
3) Display 2 becomes visible, embeds surface A with a new surface ID.
4) Display 2 will never update, as no new frames will be produced for
   surface A as the renderer is continually waiting on the ack for
   CF 1.

By having DisplayScheduler report no damage if invisible, we always
ack frames in a timely manner. This may allow a renderer to produce
some extra frames when its display is invisible, but this should be
limited as the renderer itself receives visibility notifications and
won't get additional BeginFrames once visibility is fully processed.

Bug:  875895 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I272de950d8e4d4c819c445dc8162ac360afb80a1
Reviewed-on: https://chromium-review.googlesource.com/1182862
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584917}
[modify] https://crrev.com/60d359fd3bd0bbe1089cf24c196187b3c03fcd65/components/viz/service/display/display_scheduler.cc
[modify] https://crrev.com/60d359fd3bd0bbe1089cf24c196187b3c03fcd65/components/viz/service/display/display_scheduler_unittest.cc
[modify] https://crrev.com/60d359fd3bd0bbe1089cf24c196187b3c03fcd65/components/viz/service/surfaces/surface_observer.h

Is it not possible for the display to go invisible after it defers ack? Can we have flaky behaviour due to this?

What do you mean by "defers ack"? Probably missing something obvious - can you point to the code?

The draw callback. Saman maybe has a point! Suppose we decide that the display is visible and there is damage that'll schedule a DrawAndSwap.

In the meantime, the display is no longer visible, and so we never trigger the DrawAndSwap and thus we never call the draw callback. That can happen, right?

These are all good points, but realized that I think none of this matters after a subsequent fix I put in: https://chromium-review.googlesource.com/c/chromium/src/+/1184376

We now tear down the Display when a compositor goes invisible, which will force any pending frames to fail/resolve.

Given that you make a good point about the above CL being not-quite-right, let's just revert it for now and won't-fix this.

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

commit c027fd37832ab4948ecf371d5cab312bfa301f29
Author: Eric Karl <ericrk@chromium.org>
Date: Thu Sep 20 23:46:17 2018

Revert "Android OOP-D: DisplayScheduler should only report draw-inducing damage"

This reverts commit 60d359fd3bd0bbe1089cf24c196187b3c03fcd65.

Reason for revert: This logic could still be flaky (what if we go invisible after deferring commits), and isn't needed after https://chromium-review.googlesource.com/c/chromium/src/+/1184376, so let's revert.

Original change's description:
> Android OOP-D: DisplayScheduler should only report draw-inducing damage
> 
> Currently DisplayScheduler reports damage to SurfaceManager in any case
> where a damaged surface ID damages the display. While these cases will
> trigger a draw when visible, they don't necessarily trigger a draw
> in any defined timeframe. This can lead to issues. Consider:
> 
> 1) Display 1 is not visible, embeds surface A.
> 2) Surface A sends CF 1, we don't immediately ack as we will wait for
>    Display 1 to draw.
> 3) Display 2 becomes visible, embeds surface A with a new surface ID.
> 4) Display 2 will never update, as no new frames will be produced for
>    surface A as the renderer is continually waiting on the ack for
>    CF 1.
> 
> By having DisplayScheduler report no damage if invisible, we always
> ack frames in a timely manner. This may allow a renderer to produce
> some extra frames when its display is invisible, but this should be
> limited as the renderer itself receives visibility notifications and
> won't get additional BeginFrames once visibility is fully processed.
> 
> Bug:  875895 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
> Change-Id: I272de950d8e4d4c819c445dc8162ac360afb80a1
> Reviewed-on: https://chromium-review.googlesource.com/1182862
> Reviewed-by: Fady Samuel <fsamuel@chromium.org>
> Commit-Queue: Eric Karl <ericrk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#584917}

TBR=fsamuel@chromium.org,ericrk@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  875895 
Change-Id: Idefad0b80d08b3c9fecf655b27b3c6ffb85eca10
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1232335
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593003}
[modify] https://crrev.com/c027fd37832ab4948ecf371d5cab312bfa301f29/components/viz/service/display/display_scheduler.cc
[modify] https://crrev.com/c027fd37832ab4948ecf371d5cab312bfa301f29/components/viz/service/display/display_scheduler_unittest.cc
[modify] https://crrev.com/c027fd37832ab4948ecf371d5cab312bfa301f29/components/viz/service/surfaces/surface_observer.h