sync compositor: Signal async frame on IO thread

sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG=636164
Committed: https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c
Cr-Commit-Position: refs/heads/master@{#427096}

[boliu, 2016-10-17 20:20:09]

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

[commit-bot: I haz the power, 2016-10-17 20:20:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-10-17 22:00:22]

Description was changed from

==========
Added message filter receiving frames from compositor

Originally by ojars@ here: https://codereview.chromium.org/2383933002/

WIP

BUG=
==========

to

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

BUG=
==========

[boliu, 2016-10-17 22:06:17]

Description was changed from

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

BUG=
==========

to

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG=636164
==========

[boliu, 2016-10-17 22:06:45]

boliu@chromium.org changed reviewers:
+ tobiasjs@chromium.org

[boliu, 2016-10-17 22:06:45]

ptal

[commit-bot: I haz the power, 2016-10-17 23:23:19]

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

[commit-bot: I haz the power, 2016-10-17 23:23:20]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-10-19 17:14:18]

boliu@chromium.org changed reviewers:
+ dcheng@chromium.org

[boliu, 2016-10-19 17:14:19]

+dcheng to review IPC handling, so basically the changes to
synchronous_compositor_observer.cc.

so tl;dr is I'm want more flexible behavior than synchronous IPC provides for
the draw IPC: instead of waiting for the return IPC immediately on the UI
thread, I'm allowing the wait some time later, and on another thread, by using a
"FrameFuture". So in this case, the synchronous IPC is split into two async
ones, where the reply signals the FrameFuture with the setFrame call, on the IO
thread.

The handling of the reply IPC is in synchronous_compositor_observer.cc.

[dcheng, 2016-10-21 06:42:56]

Seems reasonable though it's a bit abstract.

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:238: bool async =
async_on_draw_hardware_ && first_sync_frame_;
This might be worth a comment: it's not clear to me why async should be true if
|first_sync_frame_| is true.

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:247: if (!frame.frame &&
!future) {
Maybe a comment that documents what this means would be helpful too, though
maybe it's obvious to someone more familiar with the compositor.

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:262: future);
Nit: std::move(future)

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/child_frame.h (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/child_frame.h:33: // These two fileds are not const for
to make async path easier.
Nit: fileds => fields and s/for//

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/hardware_renderer.h:12: #include
"base/memory/ptr_util.h"
Nit: let's not include this in headers (it's pretty rare for a header to
actually need this)

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/render_thread_manager.h (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/render_thread_manager.h:63: // RT thread methods.
Nit: Perhaps expand "RT" here (I assume RT = RenderThread, but that means this
comment then reads RenderThread thread methods)

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
File content/browser/android/synchronous_compositor_observer.cc (right):

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.cc:78:
std::unique_ptr<SynchronousCompositor::Frame> frame_ptr =
Note it's generally considered to just abbreviate the type as auto here.

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.cc:97:
future_map_[routing_id] = std::move(frame_future);
Note that std::move() here doesn't do anything, since it's a const ref, hence my
earlier suggestion.

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
File content/browser/android/synchronous_compositor_observer.h (right):

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.h:47: const int
routing_id,
Nit: usually const int, etc don't have a lot of meaning, so most people just
omit the const. I don't feel strongly about this though.

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.h:48: const
scoped_refptr<SynchronousCompositor::FrameFuture>& frame_future);
Nit: scoped_refptr<> frame_future

[boliu, 2016-10-21 15:15:32]

thanks for the review!

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:238: bool async =
async_on_draw_hardware_ && first_sync_frame_;
On 2016/10/21 06:42:56, dcheng wrote:
> This might be worth a comment: it's not clear to me why async should be true
if
> |first_sync_frame_| is true.

renamed this to "allow_async_draw_" and added a comment to the header where
there was already a TODO to remove it

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:247: if (!frame.frame &&
!future) {
On 2016/10/21 06:42:56, dcheng wrote:
> Maybe a comment that documents what this means would be helpful too, though
> maybe it's obvious to someone more familiar with the compositor.

yeah should generally be clear to compositor folks I think, compositor can
choose to not produce a new frame if it thinks the nothing has changed since the
previous frame

in chrome where things are async, it just doesn't send anything back to the
browser, in webview, the way we implemented it is don't return a frame

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/browser_view_renderer.cc:262: future);
On 2016/10/21 06:42:56, dcheng wrote:
> Nit: std::move(future)

Done.

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/child_frame.h (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/child_frame.h:33: // These two fileds are not const for
to make async path easier.
On 2016/10/21 06:42:56, dcheng wrote:
> Nit: fileds => fields and s/for//

Done.

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/hardware_renderer.h:12: #include
"base/memory/ptr_util.h"
On 2016/10/21 06:42:56, dcheng wrote:
> Nit: let's not include this in headers (it's pretty rare for a header to
> actually need this)

Done.

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
File android_webview/browser/render_thread_manager.h (right):

https://codereview.chromium.org/2418383002/diff/160001/android_webview/browse...
android_webview/browser/render_thread_manager.h:63: // RT thread methods.
On 2016/10/21 06:42:56, dcheng wrote:
> Nit: Perhaps expand "RT" here (I assume RT = RenderThread, but that means this
> comment then reads RenderThread thread methods)

Yep. Updated to "Render thread methods"

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
File content/browser/android/synchronous_compositor_observer.cc (right):

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.cc:78:
std::unique_ptr<SynchronousCompositor::Frame> frame_ptr =
On 2016/10/21 06:42:56, dcheng wrote:
> Note it's generally considered to just abbreviate the type as auto here.

Done.

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.cc:97:
future_map_[routing_id] = std::move(frame_future);
On 2016/10/21 06:42:56, dcheng wrote:
> Note that std::move() here doesn't do anything, since it's a const ref, hence
my
> earlier suggestion.

changed to pass by value in the function param, so then should keep this, right?

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
File content/browser/android/synchronous_compositor_observer.h (right):

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.h:47: const int
routing_id,
On 2016/10/21 06:42:56, dcheng wrote:
> Nit: usually const int, etc don't have a lot of meaning, so most people just
> omit the const. I don't feel strongly about this though.

Done.

https://codereview.chromium.org/2418383002/diff/160001/content/browser/androi...
content/browser/android/synchronous_compositor_observer.h:48: const
scoped_refptr<SynchronousCompositor::FrameFuture>& frame_future);
On 2016/10/21 06:42:56, dcheng wrote:
> Nit: scoped_refptr<> frame_future

Done.

[dcheng, 2016-10-21 22:20:12]

lgtm from an ipc standpoint

https://codereview.chromium.org/2418383002/diff/180001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2418383002/diff/180001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:204: // Must do a synchronous
draw first to ensure GL bindings are intialized.
Nit: initialized, though that will push this line over 80 chars =/

[boliu, 2016-10-24 14:57:33]

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

[commit-bot: I haz the power, 2016-10-24 14:57:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-10-24 15:57:23]

https://codereview.chromium.org/2418383002/diff/180001/android_webview/browse...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/2418383002/diff/180001/android_webview/browse...
android_webview/browser/browser_view_renderer.h:204: // Must do a synchronous
draw first to ensure GL bindings are intialized.
On 2016/10/21 22:20:12, dcheng wrote:
> Nit: initialized, though that will push this line over 80 chars =/

Nah, this line is only 75 chars :p

Done.

[boliu, 2016-10-24 16:09:45]

boliu@chromium.org changed reviewers:
+ sky@chromium.org

[boliu, 2016-10-24 16:09:46]

toby already verbally approved, but went on vacation without actually approving.
I own pretty much everything already, so I'll just keep going.

+sky to stamp content/public/test

[sky, 2016-10-24 16:40:46]

content/public/test LGTM

[Tobias Sargeant, 2016-10-24 16:45:35]

LGTM

[commit-bot: I haz the power, 2016-10-24 16:51:55]

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

[commit-bot: I haz the power, 2016-10-24 16:51:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[boliu, 2016-10-24 17:01:35]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-10-24 17:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2418383002/#ps200001
(title: "typo")

[commit-bot: I haz the power, 2016-10-24 17:02:06]

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, 2016-10-24 17:55:08]

Description was changed from

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG=636164
==========

to

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG=636164
==========

[commit-bot: I haz the power, 2016-10-24 17:55:09]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-24 18:22:38]

Description was changed from

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG=636164
==========

to

==========
sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG=636164

Committed: https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c
Cr-Commit-Position: refs/heads/master@{#427096}
==========

[commit-bot: I haz the power, 2016-10-24 18:22:39]

Patchset 11 (id:??) landed as
https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c
Cr-Commit-Position: refs/heads/master@{#427096}