Always set damage rect to output rect if 3D context was reshaped.

Always set damage rect to output rect if 3D context was reshaped.

The first draw after a reshape needs to draw the entire contents of the
window, so ensure the overlay processor doesn't accidentally shrink the
damage rect.

BUG=721131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2881483002
Cr-Commit-Position: refs/heads/master@{#471244}
Committed: https://chromium.googlesource.com/chromium/src/+/2f1f8dd5a7f7d15da2c24fc2ffa27ef48c7441a1

[jbauman, 2017-05-11 02:15:04]

Description was changed from

==========
Always set damage rect to output rect if 3D context was reshaped.

The first draw after a reshape needs to draw the entire contents of the
window, so ensure the overlay processor doesn't accidentally shrink the
damage rect.

BUG=721131
==========

to

==========
Always set damage rect to output rect if 3D context was reshaped.

The first draw after a reshape needs to draw the entire contents of the
window, so ensure the overlay processor doesn't accidentally shrink the
damage rect.

BUG=721131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jbauman, 2017-05-11 02:15:09]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 02:16:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 03:50:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 03:50:14]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-05-11 04:04:03]

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

[ccameron, 2017-05-11 08:49:18]

I'm a bit worried that this is plastering over a real bug in damage tracking.
Because of that fear, I went and tried to re-verify all of the damage tracking
code, and it was a bit hard to keep track of it.

I think it would be simpler if we were to break the Overlay and Underlay
strategies into two functions and re-use the OverlayProcessor variables for
overlay and underlay tracking.

So, would it be possible to do that split-up before making more changes to
damage tracking code?

If that's a giant pain, then I can do it when I get back to my Windows box later
this week.

And, if you're certain that this isn't an underlying bug in the damage tracking,
then lgtm to unblock you.

https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay.cc
File cc/output/dc_layer_overlay.cc (right):

https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay....
cc/output/dc_layer_overlay.cc:125: DCLayerOverlayList* ca_layer_overlays) {
This function is doing both the "overlay" strategy (which is pretty simple) and
the "underlay" strategy (which is pretty subtle).

Do you think we could internally split this into two functions -- bool
ProcessForOverlays and bool ProcessForUnderlays, and have this function do
  if (ProcessForOverlays(...))
    ProcessForUnderlays(...);

https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay....
cc/output/dc_layer_overlay.cc:178: !was_reshaped) {
Drive-by comment -- is this if missing a
  else {
    damage_rect->Union(quad_rectangle);
  }
?

https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay.h
File cc/output/dc_layer_overlay.h (right):

https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay....
cc/output/dc_layer_overlay.h:95: 
It may be preferable to use OverlayProcessor::previous_frame_underlay_rect_
directly (by pointer, like |overlay_damage_rect|) here.

https://codereview.chromium.org/2881483002/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2881483002/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:285: was_reshaped = true;
Would it be easier call something like
  overlay_processor_->OutputSurfaceWasReshaped()
here, instead of tracking the state?

[jbauman, 2017-05-11 22:45:51]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 22:46:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2017-05-11 23:16:14]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 23:16:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2017-05-11 23:24:42]

PTAL. I just redid the patch because it's not really connected to overlays
(though the way the overlay processor changes damage rects triggers it) but
rather to SetDrawRectangle.

On 2017/05/11 08:49:18, ccameron wrote:
> I'm a bit worried that this is plastering over a real bug in damage tracking.
> Because of that fear, I went and tried to re-verify all of the damage tracking
> code, and it was a bit hard to keep track of it.
> 
> I think it would be simpler if we were to break the Overlay and Underlay
> strategies into two functions and re-use the OverlayProcessor variables for
> overlay and underlay tracking.
> 
> So, would it be possible to do that split-up before making more changes to
> damage tracking code?

I've switched this patch to just modify DirectRenderer, so I'd prefer not right
here. But that's a good idea and I'm working on doing it in another patch.
> 
> If that's a giant pain, then I can do it when I get back to my Windows box
later
> this week.
> 
> And, if you're certain that this isn't an underlying bug in the damage
tracking,
> then lgtm to unblock you.

I'm pretty certain this isn't a bug in the damage tracking. The issue is
whenever a reshape happens, we need to draw to the entire surface  or else
DirectComposition gives an error because we'd be leaving part of the surface
undefined. Normally the damage rects from the browser compositor account for
that properly. However if there's a single on top opaque overlay then the
contents underneath the overlay aren't visible and don't actually matter, so the
overlay processor will try to remove them from the damage rect.

> 
>
https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay.cc
> File cc/output/dc_layer_overlay.cc (right):
> 
>
https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay....
> cc/output/dc_layer_overlay.cc:125: DCLayerOverlayList* ca_layer_overlays) {
> This function is doing both the "overlay" strategy (which is pretty simple)
and
> the "underlay" strategy (which is pretty subtle).
> 
> Do you think we could internally split this into two functions -- bool
> ProcessForOverlays and bool ProcessForUnderlays, and have this function do
>   if (ProcessForOverlays(...))
>     ProcessForUnderlays(...);
> 
>
https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay....
> cc/output/dc_layer_overlay.cc:178: !was_reshaped) {
> Drive-by comment -- is this if missing a
>   else {
>     damage_rect->Union(quad_rectangle);
>   }
> ?
> 
No, the damage rect for the underlying surface is correct (or a bit pessimistic)
in this case because the damage rect from the surface aggregator should be
correct. The only times it isn't are when some damage is removed because it's
handled by the overlay (not in this case, so it's pessimistic) or when switching
into or out of overlays (not in this case, because the current and previous
underlay rects are the same).

> https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay.h
> File cc/output/dc_layer_overlay.h (right):
> 
>
https://codereview.chromium.org/2881483002/diff/1/cc/output/dc_layer_overlay....
> cc/output/dc_layer_overlay.h:95: 
> It may be preferable to use OverlayProcessor::previous_frame_underlay_rect_
> directly (by pointer, like |overlay_damage_rect|) here.
> 
> https://codereview.chromium.org/2881483002/diff/1/cc/output/direct_renderer.cc
> File cc/output/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/2881483002/diff/1/cc/output/direct_renderer.c...
> cc/output/direct_renderer.cc:285: was_reshaped = true;
> Would it be easier call something like
>   overlay_processor_->OutputSurfaceWasReshaped()
> here, instead of tracking the state?

[commit-bot: I haz the power, 2017-05-12 00:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 00:24:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ccameron, 2017-05-12 05:56:44]

lgtm, thanks for the clarification!

[jbauman, 2017-05-12 07:06:27]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2017-05-12 07:06:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 07:48:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494572787796300,
"parent_rev": "09e883097b5671d92c307025c9d47c2feadac0f2", "commit_rev":
"2f1f8dd5a7f7d15da2c24fc2ffa27ef48c7441a1"}

[commit-bot: I haz the power, 2017-05-12 07:48:32]

Description was changed from

==========
Always set damage rect to output rect if 3D context was reshaped.

The first draw after a reshape needs to draw the entire contents of the
window, so ensure the overlay processor doesn't accidentally shrink the
damage rect.

BUG=721131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Always set damage rect to output rect if 3D context was reshaped.

The first draw after a reshape needs to draw the entire contents of the
window, so ensure the overlay processor doesn't accidentally shrink the
damage rect.

BUG=721131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2881483002
Cr-Commit-Position: refs/heads/master@{#471244}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f1f8dd5a7f7d15da2c24fc2ffa2...
==========

[commit-bot: I haz the power, 2017-05-12 07:48:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2f1f8dd5a7f7d15da2c24fc2ffa2...