Fix large mis-paint when resizing windows aura chrome

Fix large mis-paint when resizing windows aura chrome

See bug attached for screenshots

The CL does two things:
- Disables the resizer lock, so that UI bits can go ahead of content bits
- Janks (blocks) the UI thread until the compositor has flushed / finished
the pending commands.

This makes the mis-draw way less, up to one frame behind, which at my regular
mouse speed of 1.2 pixels / ms comes out to 20 pixels of whiteout.


BUG=177115
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192657

[cpu_(ooo_6.6-7.5), 2013-04-04 02:09:06]

So this is the simpler version. Of the patch, the complex version of the patch
requires a synchronization object between the the compositor and the accelerated
surface presenter.

The code would be similar except that waiting can be shorter or longer depending
on the actual size of the windows during DirectX present()

This means another feedback loop in an already complex system. I can do it, and
for you to decide if you want it.

https://codereview.chromium.org/13604007/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/13604007/diff/1/ui/compositor/compositor.cc#n...
ui/compositor/compositor.cc:527: if (is_resizing_ && (last_ended_frame_ > 1)) {
when chrome starts we hit here with frames 0 and 1, so I check for that so that
I don't jank startup.

[jonathan.backer, 2013-04-04 17:25:09]

https://codereview.chromium.org/13604007/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/13604007/diff/1/ui/compositor/compositor.cc#n...
ui/compositor/compositor.cc:530:
base::TimeDelta::FromMilliseconds(kResizePaintLatency));
My experience has been that there are a lot of events that bounce through the UI
thread. Blocking the UI thread for 50-200 ms is going to slow them down.

I'm familiar with the ResizeLock code. I'm going to get a win_aura build running
today (it's been months since I've built on windows --- I'll probably just pave
everything).

Once I understand the problem, hopefully I can be more constructive. A quick
glance at the code suggests we probably want to synchronize the
|root_web_layer_| SetBounds with the ResizeLock.

[piman, 2013-04-04 18:54:43]

On Thu, Apr 4, 2013 at 10:25 AM, <backer@chromium.org> wrote:

>
> https://codereview.chromium.**org/13604007/diff/1/ui/**
>
compositor/compositor.cc<https://codereview.chromium.org/13604007/diff/1/ui/compositor/compositor.cc>
> File ui/compositor/compositor.cc (right):
>
> https://codereview.chromium.**org/13604007/diff/1/ui/**
>
compositor/compositor.cc#**newcode530<https://codereview.chromium.org/13604007/diff/1/ui/compositor/compositor.cc#newcode530>
> ui/compositor/compositor.cc:**530:
> base::TimeDelta::**FromMilliseconds(**kResizePaintLatency));
> My experience has been that there are a lot of events that bounce
> through the UI thread. Blocking the UI thread for 50-200 ms is going to
> slow them down.
>
> I'm familiar with the ResizeLock code. I'm going to get a win_aura build
> running today (it's been months since I've built on windows --- I'll
> probably just pave everything).
>
> Once I understand the problem, hopefully I can be more constructive. A
> quick glance at the code suggests we probably want to synchronize the
> |root_web_layer_| SetBounds with the ResizeLock.
>

@backer, for background (I spent some time with Carlos looking into a bunch
of this):

There's a bunch of various problems.
One is that on desktop Aura, the HoldMouseMoves logic is mostly skipped by
the special DesktopRootWindowHost, which does "layout" out-of-band wrt the
RootWindow, and there's more events (resize) to feed into this flow. We can
fix that, and that makes the RWHVA stick to the top-level window layer,
like in Ash (I'll call this constraint A)

However there's a more problematic issue, which is to make the
top-level-window layer stick to the native window (I'll call this
constraint B).
On windows, the only way to do that is to not return from the WM_SIZE
handler - i.e. block the UI thread - until we've drawn the frame, that is:
1- committed the new size
2- composited that
3- finished the Swap/Present.

Having the root layer stick to the native window is much more visually
jarring than the renderer being out-of-sync, and the ResizeLock logic is
actually detrimental to it (because it delays (1), and doesn't actually
behave well if it fails to prevent further resizes, apparently keeping a
compositor lock for longer than the expected timeout), hence why this CL is
willing to throw constraint A under the bus to achieve the goal of solving
constraint B.


That said, you're absolutely right that blindly blocking the UI thread is
problematic if we want to solve both constraints.

I also see the problem that this CL is a large hack to solve constraint B,
and we should be able to find better solutions (e.g. maybe just a Finish
after the swap would solve this without resorting to arbitrary timeouts).


>
https://codereview.chromium.**org/13604007/<https://codereview.chromium.org/1...
>

[cpu_(ooo_6.6-7.5), 2013-04-04 19:18:28]

for completeness I should mention that the old software path also has a UI
janker device. See

RenderWidgetHostViewWin::OnPaint(..)
  render_widget_host_->GetBackingStore(true)

GetBackingStore has a wait in there in done by
  process_->WaitForBackingStoreMsg()

This is done via an event and a IPC message filter on the IO thread.

@jonathan, you are welcome to take a stab. I wan't to land something soon but
open to better approaches, in fact, now that you are looking check the bug in
the CL description, in particular the screenshot. There might be a logical bug
on the resizer locks because they block sometimes over 3 seconds, which if I
understand correctly should not happen.

[cpu_(ooo_6.6-7.5), 2013-04-04 21:44:18]

New version up for review. Hacky wait removed, now we block until cc has
processed its gl queue. Still leaves us approximately 1 frame behind but it is
good enough for now.

[jbauman, 2013-04-04 21:47:58]

https://codereview.chromium.org/13604007/diff/15001/gpu/command_buffer/build_...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/13604007/diff/15001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:1460: 'defer_reads': True,
This didn't do anything.

[piman, 2013-04-04 21:52:22]

On 2013/04/04 21:47:58, jbauman wrote:
>
https://codereview.chromium.org/13604007/diff/15001/gpu/command_buffer/build_...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (right):
> 
>
https://codereview.chromium.org/13604007/diff/15001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:1460: 'defer_reads': True,
> This didn't do anything.

Missing file in CL? When we looked on Carlos's machine it did properly change
the gles2_cmd_decoder_autogen.h

[piman, 2013-04-04 21:56:13]

https://codereview.chromium.org/13604007/diff/21002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/13604007/diff/21002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:780: //
FinishAllRendering() if we are resizing.
Can you file a bug to reinstate the logic once we figure out the entire story?

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor....
ui/compositor/compositor.cc:414: is_resizing_ (false),
nit: would next_draw_is_resize_ be a better name?

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor....
ui/compositor/compositor.cc:537: if (is_resizing_ && (last_ended_frame_ > 1)) {
nit: >1 or >0?
We only want to not wait on the first frame, right?

[cpu_(ooo_6.6-7.5), 2013-04-05 17:05:50]

Please take a look again, now with the autogen files.

https://codereview.chromium.org/13604007/diff/21002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/13604007/diff/21002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:780: //
FinishAllRendering() if we are resizing.
On 2013/04/04 21:56:14, piman wrote:
> Can you file a bug to reinstate the logic once we figure out the entire story?

I am not closing the existing bug because it is not fully fixed but I can change
the description so that would be the bug.

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor....
ui/compositor/compositor.cc:414: is_resizing_ (false),
On 2013/04/04 21:56:14, piman wrote:
> nit: would next_draw_is_resize_ be a better name?

If you really want it, sure, the current name describes the state which and the
proposed name describes the intent of the fix, I like #1 but not too attached.

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor....
ui/compositor/compositor.cc:537: if (is_resizing_ && (last_ended_frame_ > 1)) {
On 2013/04/04 21:56:14, piman wrote:
> nit: >1 or >0?
> We only want to not wait on the first frame, right?

I see two passing by, one is pure white and the second one has bits.

[piman, 2013-04-05 17:15:32]

LGTM+nit

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/13604007/diff/21002/ui/compositor/compositor....
ui/compositor/compositor.cc:414: is_resizing_ (false),
On 2013/04/05 17:05:51, cpu wrote:
> On 2013/04/04 21:56:14, piman wrote:
> > nit: would next_draw_is_resize_ be a better name?
> 
> If you really want it, sure, the current name describes the state which and
the
> proposed name describes the intent of the fix, I like #1 but not too attached.

well, imo, "is_resizing_" suggests that it's a state that'd stay true while
you're dragging the window border, whereas in reality it's only true from the
point we call SetScaleAndSize to the point where we draw at that size, hence
encoding in the name that this state is only meaningful until the next draw.

[cpu_(ooo_6.6-7.5), 2013-04-05 17:42:06]

ok, I'll change the name after jb's comments. Thanks for all your help.

[jbauman, 2013-04-05 22:23:18]

lgtm

[cpu_(ooo_6.6-7.5), 2013-04-06 00:03:23]

Committed patchset #5 manually as r192657 (presubmit successful).