Mac ÜC: Make resize smooth

Mac ÜC: Make resize smooth

Add helpers to RenderWidgetHelper to associate a NSView with
a given RenderWidgetHelper (process and widget routing id), so
that browser compositor swaps can be received while pausing
waiting for correctly sized frames.

BUG=388005
TBR=cevans

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281866

[ccameron, 2014-07-03 03:30:40]

It looks like this is the simplest way to solve the problem. On a whole, I'm
very happy with the solution.

[piman, 2014-07-07 18:27:43]

https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:863: native_widget, &render_process_id,
&render_widget_id)) {
Why do we need this? What's different with the
render_process_id/render_widget_id given by
GpuSurfaceTracker::Get()->GetRenderWidgetIDForSurface?

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:419:
browser_compositor_has_pending_paint_= true;
nit: space before =

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:420:
base::MessageLoop::current()->PostTask(FROM_HERE,
It's better to avoid posting the task if:
- browser_compositor_has_pending_paint_ was already true (we already posted a
task)
- browser_compositor_has_pending_paint_ is false / browser_compositor_view_ is
NULL - DoBrowserCompositorPendingPaint will do nothing.

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:427:
browser_compositor_view_->GetCompositor()->Draw();
You'll need to check with enne wrt the interaction with the scheduler after
their patch lands - we shouldn't call Draw() any more. Maybe we need a mechanism
to do this?

[ccameron, 2014-07-07 18:45:23]

Thanks!

https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:863: native_widget, &render_process_id,
&render_widget_id)) {
On 2014/07/07 18:27:43, piman (slow to review) wrote:
> Why do we need this? What's different with the
> render_process_id/render_widget_id given by
> GpuSurfaceTracker::Get()->GetRenderWidgetIDForSurface?

The surface was created for the browser, so it doesn't have a render widget
associated with it (and arguably shouldn't, at least at the GpuSurfaceTracker
level -- those properties are static for the lifetime of a surface, but the RWH
for a browser compositor can change over time).

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:419:
browser_compositor_has_pending_paint_= true;
On 2014/07/07 18:27:43, piman (slow to review) wrote:
> nit: space before =

Done.

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:420:
base::MessageLoop::current()->PostTask(FROM_HERE,
On 2014/07/07 18:27:43, piman (slow to review) wrote:
> It's better to avoid posting the task if:
> - browser_compositor_has_pending_paint_ was already true (we already posted a
> task)
> - browser_compositor_has_pending_paint_ is false / browser_compositor_view_ is
> NULL - DoBrowserCompositorPendingPaint will do nothing.

Ah -- good point -- removed the redundant post tasks.

https://codereview.chromium.org/370513002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:427:
browser_compositor_view_->GetCompositor()->Draw();
On 2014/07/07 18:27:43, piman (slow to review) wrote:
> You'll need to check with enne wrt the interaction with the scheduler after
> their patch lands - we shouldn't call Draw() any more. Maybe we need a
mechanism
> to do this?

Yeah, we had an email thread about this ... adding enne as an FYI. The ability
to do a composite at a given point (by one mechanism or another) will be a
requirement for Mac -- I'm happy to change the mechanism through which this is
done (perhaps we'll need a way to pump cc/?).

[piman, 2014-07-07 19:40:56]

https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host.cc:863: native_widget, &render_process_id,
&render_widget_id)) {
On 2014/07/07 18:45:23, ccameron1 wrote:
> On 2014/07/07 18:27:43, piman (slow to review) wrote:
> > Why do we need this? What's different with the
> > render_process_id/render_widget_id given by
> > GpuSurfaceTracker::Get()->GetRenderWidgetIDForSurface?
> 
> The surface was created for the browser, so it doesn't have a render widget
> associated with it (and arguably shouldn't, at least at the GpuSurfaceTracker
> level -- those properties are static for the lifetime of a surface, but the
RWH
> for a browser compositor can change over time).

I'm worried about races (lookup is done on the IO thread but association is done
on the main thread). Can you detail to me how it fits together? In particular,
as you reuse a browser compositor when switching tabs, what ensures the RWHelper
you look up here will always be correct? A case to consider is: a frame from the
previous tab is queued, then the tab is switched, and the native_widget <->
render_process_id association changes, and then the GPU process draws the frame
and sends the GpuHostMsg_AcceleratedSurfaceBuffersSwapped. What ensures this
won't go to the new RWHelper instead of the old one?
Vice-versa, can there be a case where the
GpuHostMsg_AcceleratedSurfaceBuffersSwapped for the new tab comes "too early"
and goes to the old RWHelper rather than the new one?

[ccameron, 2014-07-07 20:43:59]

On 2014/07/07 19:40:56, piman (slow to review) wrote:
>
https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
> File content/browser/gpu/gpu_process_host.cc (right):
> 
>
https://codereview.chromium.org/370513002/diff/1/content/browser/gpu/gpu_proc...
> content/browser/gpu/gpu_process_host.cc:863: native_widget,
&render_process_id,
> &render_widget_id)) {
> On 2014/07/07 18:45:23, ccameron1 wrote:
> > On 2014/07/07 18:27:43, piman (slow to review) wrote:
> > > Why do we need this? What's different with the
> > > render_process_id/render_widget_id given by
> > > GpuSurfaceTracker::Get()->GetRenderWidgetIDForSurface?
> > 
> > The surface was created for the browser, so it doesn't have a render widget
> > associated with it (and arguably shouldn't, at least at the
GpuSurfaceTracker
> > level -- those properties are static for the lifetime of a surface, but the
> RWH
> > for a browser compositor can change over time).
> 
> I'm worried about races (lookup is done on the IO thread but association is
done
> on the main thread). Can you detail to me how it fits together? In particular,
> as you reuse a browser compositor when switching tabs, what ensures the
RWHelper
> you look up here will always be correct? A case to consider is: a frame from
the
> previous tab is queued, then the tab is switched, and the native_widget <->
> render_process_id association changes, and then the GPU process draws the
frame
> and sends the GpuHostMsg_AcceleratedSurfaceBuffersSwapped. What ensures this
> won't go to the new RWHelper instead of the old one?
> Vice-versa, can there be a case where the
> GpuHostMsg_AcceleratedSurfaceBuffersSwapped for the new tab comes "too early"
> and goes to the old RWHelper rather than the new one?

Indeed this could go to a different RWHelper that the one that corresponds to
the RWH that issued the composite. The
GpuHostMsg_AcceleratedSurfaceBuffersSwapped ends up being agnostic as to which
RWHelper it ends up using -- it just calls into
OnNativeSurfaceBuffersSwappedOnUIThread, which calls a method on the NSView.

Ideally, I'd like to change this entire system to accept swaps from any RWH be
accepted during resize, rather than having a filter for each RWP (but that's a
very substantial change).

Of note, though, is that there does not yet exist a mechanism to prevent
in-flight swaps from the previous from appearing in the new RWH's NSView --
there's a TODO in BrowserCompositorViewMac::BrowserCompositorViewMac about that.

[piman, 2014-07-07 21:05:26]

On Mon, Jul 7, 2014 at 1:43 PM, <ccameron@chromium.org> wrote:

> On 2014/07/07 19:40:56, piman (slow to review) wrote:
>
> https://codereview.chromium.org/370513002/diff/1/content/
> browser/gpu/gpu_process_host.cc
>
>> File content/browser/gpu/gpu_process_host.cc (right):
>>
>
>
> https://codereview.chromium.org/370513002/diff/1/content/
> browser/gpu/gpu_process_host.cc#newcode863
>
>> content/browser/gpu/gpu_process_host.cc:863: native_widget,
>>
> &render_process_id,
>
>> &render_widget_id)) {
>> On 2014/07/07 18:45:23, ccameron1 wrote:
>> > On 2014/07/07 18:27:43, piman (slow to review) wrote:
>> > > Why do we need this? What's different with the
>> > > render_process_id/render_widget_id given by
>> > > GpuSurfaceTracker::Get()->GetRenderWidgetIDForSurface?
>> >
>> > The surface was created for the browser, so it doesn't have a render
>> widget
>> > associated with it (and arguably shouldn't, at least at the
>>
> GpuSurfaceTracker
>
>> > level -- those properties are static for the lifetime of a surface, but
>> the
>> RWH
>> > for a browser compositor can change over time).
>>
>
>  I'm worried about races (lookup is done on the IO thread but association
>> is
>>
> done
>
>> on the main thread). Can you detail to me how it fits together? In
>> particular,
>> as you reuse a browser compositor when switching tabs, what ensures the
>>
> RWHelper
>
>> you look up here will always be correct? A case to consider is: a frame
>> from
>>
> the
>
>> previous tab is queued, then the tab is switched, and the native_widget
>> <->
>> render_process_id association changes, and then the GPU process draws the
>>
> frame
>
>> and sends the GpuHostMsg_AcceleratedSurfaceBuffersSwapped. What ensures
>> this
>> won't go to the new RWHelper instead of the old one?
>> Vice-versa, can there be a case where the
>> GpuHostMsg_AcceleratedSurfaceBuffersSwapped for the new tab comes "too
>> early"
>> and goes to the old RWHelper rather than the new one?
>>
>
> Indeed this could go to a different RWHelper that the one that corresponds
> to
> the RWH that issued the composite. The
> GpuHostMsg_AcceleratedSurfaceBuffersSwapped ends up being agnostic as to
> which
> RWHelper it ends up using -- it just calls into
> OnNativeSurfaceBuffersSwappedOnUIThread, which calls a method on the
> NSView.
>

But which RWHelper is used is important in the logic to unblock the main
thread, isn't it?


>
> Ideally, I'd like to change this entire system to accept swaps from any
> RWH be
> accepted during resize, rather than having a filter for each RWP (but
> that's a
> very substantial change).
>
> Of note, though, is that there does not yet exist a mechanism to prevent
> in-flight swaps from the previous from appearing in the new RWH's NSView --
> there's a TODO in BrowserCompositorViewMac::BrowserCompositorViewMac
> about that.
>
> https://codereview.chromium.org/370513002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ccameron, 2014-07-07 22:21:37]

On 2014/07/07 21:05:26, piman (slow to review) wrote:
> On Mon, Jul 7, 2014 at 1:43 PM, <mailto:ccameron@chromium.org> wrote:
> 
> > On 2014/07/07 19:40:56, piman (slow to review) wrote:
> >
> > https://codereview.chromium.org/370513002/diff/1/content/
> > browser/gpu/gpu_process_host.cc
> >
> >> File content/browser/gpu/gpu_process_host.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/370513002/diff/1/content/
> > browser/gpu/gpu_process_host.cc#newcode863
> >
> >> content/browser/gpu/gpu_process_host.cc:863: native_widget,
> >>
> > &render_process_id,
> >
> >> &render_widget_id)) {
> >> On 2014/07/07 18:45:23, ccameron1 wrote:
> >> > On 2014/07/07 18:27:43, piman (slow to review) wrote:
> >> > > Why do we need this? What's different with the
> >> > > render_process_id/render_widget_id given by
> >> > > GpuSurfaceTracker::Get()->GetRenderWidgetIDForSurface?
> >> >
> >> > The surface was created for the browser, so it doesn't have a render
> >> widget
> >> > associated with it (and arguably shouldn't, at least at the
> >>
> > GpuSurfaceTracker
> >
> >> > level -- those properties are static for the lifetime of a surface, but
> >> the
> >> RWH
> >> > for a browser compositor can change over time).
> >>
> >
> >  I'm worried about races (lookup is done on the IO thread but association
> >> is
> >>
> > done
> >
> >> on the main thread). Can you detail to me how it fits together? In
> >> particular,
> >> as you reuse a browser compositor when switching tabs, what ensures the
> >>
> > RWHelper
> >
> >> you look up here will always be correct? A case to consider is: a frame
> >> from
> >>
> > the
> >
> >> previous tab is queued, then the tab is switched, and the native_widget
> >> <->
> >> render_process_id association changes, and then the GPU process draws the
> >>
> > frame
> >
> >> and sends the GpuHostMsg_AcceleratedSurfaceBuffersSwapped. What ensures
> >> this
> >> won't go to the new RWHelper instead of the old one?
> >> Vice-versa, can there be a case where the
> >> GpuHostMsg_AcceleratedSurfaceBuffersSwapped for the new tab comes "too
> >> early"
> >> and goes to the old RWHelper rather than the new one?
> >>
> >
> > Indeed this could go to a different RWHelper that the one that corresponds
> > to
> > the RWH that issued the composite. The
> > GpuHostMsg_AcceleratedSurfaceBuffersSwapped ends up being agnostic as to
> > which
> > RWHelper it ends up using -- it just calls into
> > OnNativeSurfaceBuffersSwappedOnUIThread, which calls a method on the
> > NSView.
> >
> 
> But which RWHelper is used is important in the logic to unblock the main
> thread, isn't it?

True, but not critical. 

1. We wake up a waiting RWHelper that wasn't the one we wanted to wake up. This
is safe (and probably preferable). 

2. We end up not waking up a RWHelper that we wanted to wake up. This isn't as
benign -- we stall for the full 50 msec.

That said, the bad situation, 2, is highly unlikely (if not impossible ...
pretty sure it's actually impossible). If we're waiting for a frame, it's
because we've requested a frame to be drawn, and when we requested the frame to
be drawn on the main thread, we set up the correct RWP/RWH IDs for the NSView
beforehand. If we've re-assigned the NSView to another RWP/RWH on the main
thread, then that's because we hid the old view, which means we won't be waiting
on it anymore.

The OK/better situation, 1, is likely to happen -- we'll get frames coming from
previous tabs because of the way recycling is done (until that is fixed).

Ideally, I'd prefer that the main-thread-blocking logic not filter messages -- I
suspect the filtering was originally added because we were pumping inside
-[NSView drawRect], which is a dangerous place compared with its new home
[NSView setFrameSize].

[piman, 2014-07-07 23:01:45]

Ok, can you add TODOs/file bugs so that we clean this up? I'm fairly
uncomfortable with known races becoming worse after some (seemingly harmless)
refactoring).
LGTM after that.

[ccameron, 2014-07-08 02:35:11]

On 2014/07/07 23:01:45, piman (slow to review) wrote:
> Ok, can you add TODOs/file bugs so that we clean this up? I'm fairly
> uncomfortable with known races becoming worse after some (seemingly harmless)
> refactoring).
> LGTM after that.

Sounds like a plan -- I put in a big cautionary note, and filed crbug.com/392031
against this.

Thanks!

[ccameron, 2014-07-08 02:35:11]

On 2014/07/07 23:01:45, piman (slow to review) wrote:
> Ok, can you add TODOs/file bugs so that we clean this up? I'm fairly
> uncomfortable with known races becoming worse after some (seemingly harmless)
> refactoring).
> LGTM after that.

Sounds like a plan -- I put in a big cautionary note, and filed crbug.com/392031
against this.

Thanks!

[ccameron, 2014-07-08 02:37:12]

Adding cevans for *_message*.h rubber stamp.

[ccameron, 2014-07-08 18:54:36]

Adding inferno for _message*.h review

[ccameron, 2014-07-08 19:10:09]

On 2014/07/08 18:54:36, ccameron1 wrote:
> Adding inferno for _message*.h review

TBRing the _message*.h changes.

[ccameron, 2014-07-08 19:10:14]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-07-08 19:12:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/370513002/60001

[commit-bot: I haz the power, 2014-07-08 23:10:38]

Change committed as 281866