Android Browser Compositor: Add ScheduleComposite() callback.

Android Browser Compositor: Add ScheduleComposite() callback.

This tells the client that it should schedule a composite operation
(i.e. during next vsync signal).

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

[David Trainor- moved to gerrit, 2012-10-11 23:59:56]

lgtm

[no sievers, 2012-10-12 19:42:29]

On 2012/10/11 23:59:56, dtrainor wrote:
> lgtma

added piman for OWNERS

[piman, 2012-10-12 20:53:06]

http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/compositor_impl_android.h (right):

http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/compositor_impl_android.h:27:
CompositorImpl(Compositor::Client* client);
nit: explicit

http://codereview.chromium.org/11108004/diff/1/content/shell/android/shell_ma...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/11108004/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:42: CompositorClient client;
On destruction, you will destroy client - which is still referenced by
compositor - before you destroy compositor, thus violating the contract that the
client must outlive the compositor.
I'm pretty sure destroying compositor can involve doing layer modifications
which will call ScheduleComposite, which will then crash.

You can probably trivially solve this by moving client before compositor.

(note: same applies to testshell/tab_manager)

http://codereview.chromium.org/11108004/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:52: // Composite immediately
I'm pretty sure you don't want to do that.
This means that whenever you change any layer property, you will draw a new
frame.
E.g. changing the layer size and then the layer transform in the same function
will draw one frame with the new size/old transform, and then one frame with the
new size/new transform.
It'll be very slow too.

I suggest posting a task instead.

(note: same applies to testshell/tab_manager)

[no sievers, 2012-10-12 21:33:07]

http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_android.cc:324:
ImageTransportFactoryAndroid::GetInstance()->InsertSyncPoint();
@piman: One thing: Isn't it a problem that I insert the sync point using the
helper context here and not the compositor context?
What if this sync point gets parsed before commands from the compositor context
that still use the old texture (commands that were submitted but not parsed)?

I thought AURA also uses a different helper context for this though..

[piman, 2012-10-12 21:34:22]

On Fri, Oct 12, 2012 at 2:33 PM, <sievers@chromium.org> wrote:

>
> http://codereview.chromium.**org/11108004/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_android.cc<http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc>
> File content/browser/renderer_host/**render_widget_host_view_**android.cc
> (right):
>
> http://codereview.chromium.**org/11108004/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_android.cc#**newcode324<http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc#newcode324>
> content/browser/renderer_host/**render_widget_host_view_**android.cc:324:
> ImageTransportFactoryAndroid::**GetInstance()->**InsertSyncPoint();
> @piman: One thing: Isn't it a problem that I insert the sync point using
> the helper context here and not the compositor context?
> What if this sync point gets parsed before commands from the compositor
> context that still use the old texture (commands that were submitted but
> not parsed)?
>
> I thought AURA also uses a different helper context for this though..
>

All contexts in the same channel are ordered wrt each other.

Antoine


>
>
http://codereview.chromium.**org/11108004/<http://codereview.chromium.org/111...
>

[no sievers, 2012-10-12 22:33:50]

http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/compositor_impl_android.h (right):

http://codereview.chromium.org/11108004/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/compositor_impl_android.h:27:
CompositorImpl(Compositor::Client* client);
On 2012/10/12 20:53:06, piman wrote:
> nit: explicit

Done.

http://codereview.chromium.org/11108004/diff/1/content/shell/android/shell_ma...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/11108004/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:42: CompositorClient client;
On 2012/10/12 20:53:06, piman wrote:
> On destruction, you will destroy client - which is still referenced by
> compositor - before you destroy compositor, thus violating the contract that
the
> client must outlive the compositor.
> I'm pretty sure destroying compositor can involve doing layer modifications
> which will call ScheduleComposite, which will then crash.
> 
> You can probably trivially solve this by moving client before compositor.
> 
> (note: same applies to testshell/tab_manager)

Done.

http://codereview.chromium.org/11108004/diff/1/content/shell/android/shell_ma...
content/shell/android/shell_manager.cc:52: // Composite immediately
On 2012/10/12 20:53:06, piman wrote:
> I'm pretty sure you don't want to do that.
> This means that whenever you change any layer property, you will draw a new
> frame.
> E.g. changing the layer size and then the layer transform in the same function
> will draw one frame with the new size/old transform, and then one frame with
the
> new size/new transform.
> It'll be very slow too.
> 
> I suggest posting a task instead.
> 
> (note: same applies to testshell/tab_manager)

Done.

[piman, 2012-10-13 18:04:21]

LGTM+nit

http://codereview.chromium.org/11108004/diff/10001/chrome/android/testshell/t...
File chrome/android/testshell/tab_manager.cc (right):

http://codereview.chromium.org/11108004/diff/10001/chrome/android/testshell/t...
chrome/android/testshell/tab_manager.cc:43: bool g_scheduled_composite = false;
nit: should this be in the global state?

http://codereview.chromium.org/11108004/diff/10001/content/shell/android/shel...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/11108004/diff/10001/content/shell/android/shel...
content/shell/android/shell_manager.cc:52: bool g_scheduled_composite = false;
nit: should this be in the global state?

[piman, 2012-10-13 18:05:37]

Oh, and +jam for content/public changes

[jam, 2012-10-15 06:42:22]

lgtm

[commit-bot: I haz the power, 2012-10-15 17:34:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/11108004/14002