Android WebView Merged-Thread Hardware Draw

Android WebView Merged-Thread Hardware Draw

InProcessViewRenderer no longer inherits from the current
implementation class, with most methods left unimplemented for now.
This means the browser compositor is removed when merged thread
mode is turned on.

Synchronous input filter logic is moved to AwContentRenderer[Host]Ext,
since ViewRenderer[Host] will go away after merged thread draw path
is ready. UpdatePageScaleFactor moved similarly.

Need to temporarily add a global AllowIO to temporarily work around
compositor joining raster threads on UI thread on shutdown.

Caveats:
* This code is still not used until some flags are turned on.
* Will break merged thread mode with existing browser compositor
draw path
* cc initialization changes required for webview are not there.
* Lots more work required related to GL state restore and respecting
transforms etc
* Patch ignores software draw path, including case of not attached
to window.

BUG=230202, 230195
NOTRY=true

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

[boliu, 2013-05-02 23:18:21]

I think these pieces can land now, but they won't work until some additional
hacks are patched in here: https://codereview.chromium.org/14600002/

They look much more manageable now.

Over to joth for first pass.

[joth, 2013-05-03 01:26:26]

quick thoughts... need to time landing this carefully  (unless we can flesh it
out to be nearer to working first)

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/brows...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/brows...
android_webview/browser/browser_view_renderer.h:80: static BrowserViewRenderer*
FromId(int render_process_id, int render_view_id);
rather than add these two in here, why not change in_process/ renderer_client to
#include the in_process view renderer directly? the two are closely related

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
File android_webview/browser/in_process_renderer/in_process_view_renderer.cc
(right):

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:100:
compositor_->SetClient(this);
todo: check if also attached to window, and if so ask java_helper to request gl
draw (invoke) in order to bring-up the GL mode.

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:125:
return;  // TODO(boliu): Draw background or checkerboard or something?
this shouldn't be possible, we should only request GL draw after being attached
to HW window *and* having a compositor bound (see above)

the one exception maybe in tear-down phases -- even there it's probably a bug.
I'd go for
if (!compositor_) {
  NOTREACHED();
  return;
}

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:127: //
TODO(boliu): Don't ignore other transforms and whatnot.
:)

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:167:
void InProcessViewRenderer::OnAttachedToWindow(int width, int height) {
likewise TODO , if we have a compositor, and iff the window is HW accelerated
(need some interface to check this!) we can bring up GL mode now

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:198: if
(continuous_invalidate_ && !about_to_hardware_draw_)
I don't think 'hardware' is important for this. I'd just call it
'inside_compositor_draw_'

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:199:
Invalidate();
TODO - handle not attached to window scenario?

https://codereview.chromium.org/14888002/diff/1/content/public/renderer/andro...
File content/public/renderer/android/synchronous_compositor.h (right):

https://codereview.chromium.org/14888002/diff/1/content/public/renderer/andro...
content/public/renderer/android/synchronous_compositor.h:32: virtual void
DemandDrawHw(gfx::Rect damage_rect) = 0;
return bool, for consistency

https://codereview.chromium.org/14888002/diff/1/content/public/renderer/andro...
File content/public/renderer/android/synchronous_compositor_client.h (right):

https://codereview.chromium.org/14888002/diff/1/content/public/renderer/andro...
content/public/renderer/android/synchronous_compositor_client.h:20: //
TODO(joth): Add scroll getters and setters, and invalidations.
nix 'invalidations'

https://codereview.chromium.org/14888002/diff/1/content/renderer/android/sync...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/1/content/renderer/android/sync...
content/renderer/android/synchronous_compositor_output_surface.cc:22: //
TODO(boliu): RenderWidgetImpl should create in process contexts as well.
should it?

https://codereview.chromium.org/14888002/diff/1/content/renderer/android/sync...
content/renderer/android/synchronous_compositor_output_surface.cc:109:
compositor_client_->SetContinuousInvalidate(vsync_enabled_);
makes me sad that the 'vsync_enabled_ state is duplicated in this class and in
the client (view renderer), but nothing cleaner springs to mind. (except maybe
just having SyncCompClient::Invalidate()  -- no params -- and keep all the logic
in here to drive it)

[boliu, 2013-05-03 05:25:50]

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/brows...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/brows...
android_webview/browser/browser_view_renderer.h:80: static BrowserViewRenderer*
FromId(int render_process_id, int render_view_id);
On 2013/05/03 01:26:26, joth wrote:
> rather than add these two in here, why not change in_process/ renderer_client
to
> #include the in_process view renderer directly? the two are closely related

Done.

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
File android_webview/browser/in_process_renderer/in_process_view_renderer.cc
(right):

https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_pr...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:125:
return;  // TODO(boliu): Draw background or checkerboard or something?
On 2013/05/03 01:26:26, joth wrote:
> this shouldn't be possible, we should only request GL draw after being
attached
> to HW window *and* having a compositor bound (see above)
> 
> the one exception maybe in tear-down phases -- even there it's probably a bug.
> I'd go for
> if (!compositor_) {
>   NOTREACHED();
>   return;
> }
> 

Do we ever have to worry about the compositor changing on us, and momentarily
have a NULL here?

https://codereview.chromium.org/14888002/diff/1/content/renderer/android/sync...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/1/content/renderer/android/sync...
content/renderer/android/synchronous_compositor_output_surface.cc:22: //
TODO(boliu): RenderWidgetImpl should create in process contexts as well.
On 2013/05/03 01:26:26, joth wrote:
> should it?

Oops, should have said RenderThreadImpl, which creates the offscreen context.
Should discuss with James if this function should even be here. Maybe both
should be handled directly in render_widget/thread.

[joth, 2013-05-03 19:07:07]

On 2 May 2013 22:25, <boliu@chromium.org> wrote:

>
> https://codereview.chromium.**org/14888002/diff/1/android_**
>
webview/browser/browser_view_**renderer.h<https://codereview.chromium.org/14888002/diff/1/android_webview/browser/browser_view_renderer.h>
> File android_webview/browser/**browser_view_renderer.h (right):
>
> https://codereview.chromium.**org/14888002/diff/1/android_**
>
webview/browser/browser_view_**renderer.h#newcode80<https://codereview.chromium.org/14888002/diff/1/android_webview/browser/browser_view_renderer.h#newcode80>
> android_webview/browser/**browser_view_renderer.h:80: static
> BrowserViewRenderer* FromId(int render_process_id, int render_view_id);
> On 2013/05/03 01:26:26, joth wrote:
>
>> rather than add these two in here, why not change in_process/
>>
> renderer_client to
>
>> #include the in_process view renderer directly? the two are closely
>>
> related
>
> Done.
>
>
> https://codereview.chromium.**org/14888002/diff/1/android_**
>
webview/browser/in_process_**renderer/in_process_view_**renderer.cc<https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_process_renderer/in_process_view_renderer.cc>
> File
> android_webview/browser/in_**process_renderer/in_process_**
> view_renderer.cc
> (right):
>
> https://codereview.chromium.**org/14888002/diff/1/android_**
> webview/browser/in_process_**renderer/in_process_view_**
>
renderer.cc#newcode125<https://codereview.chromium.org/14888002/diff/1/android_webview/browser/in_process_renderer/in_process_view_renderer.cc#newcode125>
> android_webview/browser/in_**process_renderer/in_process_**
> view_renderer.cc:125:
> return;  // TODO(boliu): Draw background or checkerboard or something?
> On 2013/05/03 01:26:26, joth wrote:
>
>> this shouldn't be possible, we should only request GL draw after being
>>
> attached
>
>> to HW window *and* having a compositor bound (see above)
>>
>
>  the one exception maybe in tear-down phases -- even there it's
>>
> probably a bug.
>
>> I'd go for
>> if (!compositor_) {
>>    NOTREACHED();
>>    return;
>> }
>>
>
>
> Do we ever have to worry about the compositor changing on us, and
> momentarily have a NULL here?
>
>
Not since mikhail changed it so we don't swap RenderViews on navigation in
single-process mode. Even if we did swap, I think they'd overlap lifetime
rather than underlap. (Hence the funky comment in DidDestroyCompositor)



>
> https://codereview.chromium.**org/14888002/diff/1/content/**
>
renderer/android/synchronous_**compositor_output_surface.cc<https://codereview.chromium.org/14888002/diff/1/content/renderer/android/synchronous_compositor_output_surface.cc>
> File content/renderer/android/**synchronous_compositor_output_**surface.cc
> (right):
>
> https://codereview.chromium.**org/14888002/diff/1/content/**
>
renderer/android/synchronous_**compositor_output_surface.cc#**newcode22<https://codereview.chromium.org/14888002/diff/1/content/renderer/android/synchronous_compositor_output_surface.cc#newcode22>
> content/renderer/android/**synchronous_compositor_output_**surface.cc:22:
> //
> TODO(boliu): RenderWidgetImpl should create in process contexts as well.
> On 2013/05/03 01:26:26, joth wrote:
>
>> should it?
>>
>
> Oops, should have said RenderThreadImpl, which creates the offscreen
> context. Should discuss with James if this function should even be here.
> Maybe both should be handled directly in render_widget/thread.
>
>
My opinion is this output surface is designed with one very specific
use-case in mind, so it doesn't achieve much to give the impression of more
flexibility (i.e. use with different WC3Ds) than it's intended for. Better
to pass that in in future, if/when such flexibility needed ( - so no need
for TODO as we don't have that intent today).

[boliu, 2013-05-03 19:10:27]

James: Could you take a look at the content changes.

On question in particular about where to put the code to create WGC3DIPCBI. I
decided to put it in SynchronousCompositorOutputSurface because I have in the
back of my mind to move creating the offscreen context into OutputSurface for
the "synchronous init gl" work.

[jamesr, 2013-05-03 19:46:51]

What's the lifecycle of this graphics context?  If we lose the underlying GL
context do we have to recreate this?  What's the signalling path for that?

https://codereview.chromium.org/14888002/diff/8001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/8001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.cc:22: //
TODO(boliu): RenderWidgetImpl should create in process contexts as well.
What's RenderWidgetImpl?

[boliu, 2013-05-03 20:52:35]

On 2013/05/03 19:46:51, jamesr wrote:
> What's the lifecycle of this graphics context?  If we lose the underlying GL
> context do we have to recreate this?  What's the signalling path for that?
> 

Proper initialization and recreate logic for webview is not there yet, so in
this patch, if context is lost, then the whole output surface recreated like
other platforms.

Eventually, we would need to teach compositor that in "synchronous" mode, don't
try to recreate output surface. Instead, if the context is lost, fall back to
software mode until the next GL init happens.

>
https://codereview.chromium.org/14888002/diff/8001/content/renderer/android/s...
> File content/renderer/android/synchronous_compositor_output_surface.cc
(right):
> 
>
https://codereview.chromium.org/14888002/diff/8001/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_output_surface.cc:22: //
> TODO(boliu): RenderWidgetImpl should create in process contexts as well.
> What's RenderWidgetImpl?

I meant to say RenderThreadImpl, which creates the offscreen context.

[boliu, 2013-05-07 14:13:03]

James: Could you take a look again?

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:23:
scoped_ptr<WebKit::WebGraphicsContext3D> CreateWebGraphicsContext3D() {
Here is my plan to support cc hardware init: Move
LayerTreeHostClient::OffscreenContextProviderForMainThread and
OffscreenContextProviderForCompositorThread to OutputSurface interface instead.
Simply call it in LTHI::InitializeRenderer and pass it to
ResourceProvider::set_offscreen_context_provider.

Then remove all the other places for checking/setting the offscreen context. And
the output surface is responsible for reporting the offscreen is context is lost
and report back to cc like the output surface is lost.

This way, all the objects required for hardware init will already be avaiable on
the compositor thread after software is initialized.

James: Do you see any problems with this approach?

Also can I leave this patch as is, since RenderThreadImpl is tightly coupled
with PlatformContext3DImpl which expects a non-in-process command buffer
context.

[jamesr, 2013-05-07 18:06:40]

+cc dana for the provider questions

https://codereview.chromium.org/14888002/diff/23001/content/public/renderer/a...
File content/public/renderer/android/synchronous_compositor.h (right):

https://codereview.chromium.org/14888002/diff/23001/content/public/renderer/a...
content/public/renderer/android/synchronous_compositor.h:32: virtual bool
DemandDrawHw(gfx::Rect damage_rect) = 0;
are we supposed to get a transform/clip?

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:101:
client_->DidVSync(base::TimeTicks::Now());
can we get an actual frame timestamp from the system?

[danakj, 2013-05-07 18:55:06]

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:23:
scoped_ptr<WebKit::WebGraphicsContext3D> CreateWebGraphicsContext3D() {
On 2013/05/07 14:13:03, boliu wrote:
> Here is my plan to support cc hardware init: Move
> LayerTreeHostClient::OffscreenContextProviderForMainThread and
> OffscreenContextProviderForCompositorThread to OutputSurface interface
instead.
> Simply call it in LTHI::InitializeRenderer and pass it to
> ResourceProvider::set_offscreen_context_provider.
> 
> Then remove all the other places for checking/setting the offscreen context.
And
> the output surface is responsible for reporting the offscreen is context is
lost
> and report back to cc like the output surface is lost.
> 
> This way, all the objects required for hardware init will already be avaiable
on
> the compositor thread after software is initialized.
> 
> James: Do you see any problems with this approach?
> 
> Also can I leave this patch as is, since RenderThreadImpl is tightly coupled
> with PlatformContext3DImpl which expects a non-in-process command buffer
> context.

The problem I see here is we only create the offscreen context if we need to.
They are somewhat heavy, so making a useless offscreen context for every
compositor is not desired. This is why it is a part of the BeginFrame() flow,
where we create it the first time we see we need it on the main thread, and then
just recreate it from then on when we lose contexts.

[danakj, 2013-05-07 18:57:11]

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:23:
scoped_ptr<WebKit::WebGraphicsContext3D> CreateWebGraphicsContext3D() {
On 2013/05/07 14:13:03, boliu wrote:
> Here is my plan to support cc hardware init: Move
> LayerTreeHostClient::OffscreenContextProviderForMainThread and
> OffscreenContextProviderForCompositorThread to OutputSurface interface
instead.
> Simply call it in LTHI::InitializeRenderer and pass it to
> ResourceProvider::set_offscreen_context_provider.
> 
> Then remove all the other places for checking/setting the offscreen context.
And
> the output surface is responsible for reporting the offscreen is context is
lost
> and report back to cc like the output surface is lost.
> 
> This way, all the objects required for hardware init will already be avaiable
on
> the compositor thread after software is initialized.
> 
> James: Do you see any problems with this approach?
> 
> Also can I leave this patch as is, since RenderThreadImpl is tightly coupled
> with PlatformContext3DImpl which expects a non-in-process command buffer
> context.

Also keep in mind that context creation needs to be done on the main thread.

[boliu, 2013-05-07 19:18:44]

https://codereview.chromium.org/14888002/diff/23001/content/public/renderer/a...
File content/public/renderer/android/synchronous_compositor.h (right):

https://codereview.chromium.org/14888002/diff/23001/content/public/renderer/a...
content/public/renderer/android/synchronous_compositor.h:32: virtual bool
DemandDrawHw(gfx::Rect damage_rect) = 0;
On 2013/05/07 18:06:41, jamesr wrote:
> are we supposed to get a transform/clip?

Yep: clip/damage_rect, transform, view size. Passing these down, but only using
damage_rect right now, with the assumption that damage_rect is the whole view
and that the transform is identity.

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:23:
scoped_ptr<WebKit::WebGraphicsContext3D> CreateWebGraphicsContext3D() {
On 2013/05/07 18:57:11, danakj wrote:
> Also keep in mind that context creation needs to be done on the main thread.

This is the exact behavior I'm trying to workaround for webview, that given a
output_surface that has already been initialized for software draw,
synchronously initialize the hardware bits on the impl thread such that upon
returning, cc is ready to make hardware draws.

Why does context creation need to happen on main thread? Does that restriction
only apply to the cross-process command buffer context? I can't think of
anything in the in-process one that won't work.

Also the offscreen context is created unconditionally in BeginFrame (when using
a GLRenderer). Of course OutputSurface should only create and then reuse one
global offscreen context,  as before, so I think this bit is the same. One
difference is that InitializeRenderer is called while both threads are blocked,
and BeginFrame does not block the compositor thread. I think compositor thread
won't be doing useful work before the first BeginFrame succeeds, right?

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:101:
client_->DidVSync(base::TimeTicks::Now());
On 2013/05/07 18:06:41, jamesr wrote:
> can we get an actual frame timestamp from the system?

I'm not sure. Also a bit confused about what this value represents. I see the
timestamp is saved as last_tick_time_, and is used along with the interval to
calculate when the next tick should be. So also need to get the real vsync
interval. I'll ask around to see if it is possible.

Interpreting as "last tick time", setting it to now seems completely reasonable.

[danakj, 2013-05-07 19:35:33]

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/23001/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:23:
scoped_ptr<WebKit::WebGraphicsContext3D> CreateWebGraphicsContext3D() {
On 2013/05/07 19:18:44, boliu wrote:
> On 2013/05/07 18:57:11, danakj wrote:
> > Also keep in mind that context creation needs to be done on the main thread.
> 
> This is the exact behavior I'm trying to workaround for webview, that given a
> output_surface that has already been initialized for software draw,
> synchronously initialize the hardware bits on the impl thread such that upon
> returning, cc is ready to make hardware draws.
> 
> Why does context creation need to happen on main thread? Does that restriction
> only apply to the cross-process command buffer context? I can't think of
> anything in the in-process one that won't work.

This is true, it would only be required for the gpu process version I believe,
as it's needed to set up the IPC channel. But then, cc/ doesn't have a clue
about the underlying implementation of WGC3D..

> Also the offscreen context is created unconditionally in BeginFrame (when
using
> a GLRenderer). Of course OutputSurface should only create and then reuse one
> global offscreen context,  as before, so I think this bit is the same. One
> difference is that InitializeRenderer is called while both threads are
blocked,
> and BeginFrame does not block the compositor thread. I think compositor thread
> won't be doing useful work before the first BeginFrame succeeds, right?

It's not unconditional, it depends on
layer_tree_host_->needs_offscreen_context().

There is one global one, per renderer process. But that is still too many if
we're not using them they are waste.

[boliu, 2013-05-07 19:57:02]

On 2013/05/07 19:35:33, danakj wrote:
> > Why does context creation need to happen on main thread? Does that
restriction
> > only apply to the cross-process command buffer context? I can't think of
> > anything in the in-process one that won't work.
> 
> This is true, it would only be required for the gpu process version I believe,
> as it's needed to set up the IPC channel. But then, cc/ doesn't have a clue
> about the underlying implementation of WGC3D..
> 

Hmm...it's pretty much required for webview to create the in-process context on
compositor thread. The only thing I can think of to avoid diverging from other
platforms is to create a context on main thread even for software mode, but only
initialize it or make it current on the compositor thread when ready. I think
the cross-process initialization can't move to compositor thread though, right?
So I guess webview need to diverge a bit from other platforms then.

> It's not unconditional, it depends on
> layer_tree_host_->needs_offscreen_context().
> 
> There is one global one, per renderer process. But that is still too many if
> we're not using them they are waste.

Ahh, I did not see that call. Looks like the value is set by layers. Since the
layers should be copied to the impl tree, that information should be on the impl
thread as well, since cc is already initialized in software mode. Only potential
issue is this hardware init is called after first BeginFrame that normally
creates the offscreen context, but before the needs_offscreen_context value is
committed to the impl tree. Am I understanding this flow correctly? Can this
happen in practice?

[danakj, 2013-05-07 20:03:23]

On Tue, May 7, 2013 at 3:57 PM, <boliu@chromium.org> wrote:

> On 2013/05/07 19:35:33, danakj wrote:
>
>> > Why does context creation need to happen on main thread? Does that
>>
> restriction
>
>> > only apply to the cross-process command buffer context? I can't think of
>> > anything in the in-process one that won't work.
>>
>
>  This is true, it would only be required for the gpu process version I
>> believe,
>> as it's needed to set up the IPC channel. But then, cc/ doesn't have a
>> clue
>> about the underlying implementation of WGC3D..
>>
>
>
> Hmm...it's pretty much required for webview to create the in-process
> context on
> compositor thread. The only thing I can think of to avoid diverging from
> other
> platforms is to create a context on main thread even for software mode,
> but only
> initialize it or make it current on the compositor thread when ready. I
> think
> the cross-process initialization can't move to compositor thread though,
> right?
> So I guess webview need to diverge a bit from other platforms then.


BindToCurrentThread() does the make current, and that has to happen on the
same thread where it will be used - ie, the compositor thread. This is why
we create the provider on the main thread so it can set up the channel, but
throw it over to impl before we do the BindToCurrentThread() call.


>
>
>  It's not unconditional, it depends on
>> layer_tree_host_->needs_**offscreen_context().
>>
>
>  There is one global one, per renderer process. But that is still too many
>> if
>> we're not using them they are waste.
>>
>
> Ahh, I did not see that call. Looks like the value is set by layers. Since
> the
> layers should be copied to the impl tree, that information should be on
> the impl
> thread as well, since cc is already initialized in software mode. Only
> potential
> issue is this hardware init is called after first BeginFrame that normally
> creates the offscreen context, but before the needs_offscreen_context
> value is
> committed to the impl tree. Am I understanding this flow correctly? Can
> this
> happen in practice?
>

I think that needs_offscreen_context() would be set correctly even in
software mode.
But renderer_capabilities_main_thread_copy_.using_offscreen_context3d
should be false when in software mode. This would prevent the offscreen
context from attempting to be created when we aren't going to use it.

If you might jump into hardware mode at any time, and its possible to make
hardware contexts, you could have the renderer advertise
using_offscreen_context3d = true, and then the context will just be made on
the main thread at the right time as per usual, and you can just not use it
until you need hardware mode.

This isn't super awesome clearly cuz you're wasting memory, but I'll let
you make the call on that tradeoff for webview. If you can delay this until
you want hardware mode (does the software/hardware mode come from the main
thread?) then that would be better, if most of the time you never see
hardware mode.


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

[boliu, 2013-05-07 20:28:45]

On 2013/05/07 20:03:23, danakj wrote:
> I think that needs_offscreen_context() would be set correctly even in
> software mode.
> But renderer_capabilities_main_thread_copy_.using_offscreen_context3d
> should be false when in software mode. This would prevent the offscreen
> context from attempting to be created when we aren't going to use it.
> 
> If you might jump into hardware mode at any time, and its possible to make
> hardware contexts, you could have the renderer advertise
> using_offscreen_context3d = true, and then the context will just be made on
> the main thread at the right time as per usual, and you can just not use it
> until you need hardware mode.

Do you foresee a problem in this situation: cc is in software mode and
needs_offscreen_context() is true. Then immediately after switching to hardware
mode but before the next BeginFrame, there is no offscreen context, but the
resource provider expects an offscreen context.

If it is a problem, maybe init could also set the scheduler state machine so
that the next action is a begin frame.

All this discussion might be a bit premature given I haven't started the CL yet.


> 
> This isn't super awesome clearly cuz you're wasting memory, but I'll let
> you make the call on that tradeoff for webview. If you can delay this until
> you want hardware mode (does the software/hardware mode come from the main
> thread?) then that would be better, if most of the time you never see
> hardware mode.
> 

I think it's not so bad to create an in-process context and not use it, because
all the initialization happens lazily when it's made current.

This switch software->hardware switch happens on the impl thread.

[danakj, 2013-05-07 20:34:48]

On Tue, May 7, 2013 at 4:28 PM, <boliu@chromium.org> wrote:

> On 2013/05/07 20:03:23, danakj wrote:
>
>> I think that needs_offscreen_context() would be set correctly even in
>> software mode.
>> But renderer_capabilities_main_**thread_copy_.using_offscreen_**context3d
>> should be false when in software mode. This would prevent the offscreen
>> context from attempting to be created when we aren't going to use it.
>>
>
>  If you might jump into hardware mode at any time, and its possible to make
>> hardware contexts, you could have the renderer advertise
>> using_offscreen_context3d = true, and then the context will just be made
>> on
>> the main thread at the right time as per usual, and you can just not use
>> it
>> until you need hardware mode.
>>
>
> Do you foresee a problem in this situation: cc is in software mode and
> needs_offscreen_context() is true. Then immediately after switching to
> hardware
> mode but before the next BeginFrame, there is no offscreen context, but the
> resource provider expects an offscreen context.
>
> If it is a problem, maybe init could also set the scheduler state machine
> so
> that the next action is a begin frame.
>

If you draw before the begin frame happens, you'll get a hardware frame
with no filters - so they'd essentially disappear from view at that point
until BeginFrame completed.

You'd need to either block drawing or have init jump to the main thread and
create the context, I think.


>
> All this discussion might be a bit premature given I haven't started the
> CL yet.
>
>
>
>
>  This isn't super awesome clearly cuz you're wasting memory, but I'll let
>> you make the call on that tradeoff for webview. If you can delay this
>> until
>> you want hardware mode (does the software/hardware mode come from the main
>> thread?) then that would be better, if most of the time you never see
>> hardware mode.
>>
>
>
> I think it's not so bad to create an in-process context and not use it,
> because
> all the initialization happens lazily when it's made current.
>
> This switch software->hardware switch happens on the impl thread.
>

I see, then maybe it's fine to just have this value be true for the
renderer caps, and create it eagerly in webview. I'd defer to others such
as yourself as to the costs of keeping around these contexts if we never go
to hardware mode.


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

[jamesr, 2013-05-07 20:36:24]

GL context creation is typically in two parts - first getting the context, which
just provides an interface, and then initializing which is done on the first
makeContextCurrent() and actually initializes the service side.  Can't you just
do this for webview?

[boliu, 2013-05-07 20:42:01]

On 2013/05/07 20:36:24, jamesr wrote:
> GL context creation is typically in two parts - first getting the context,
which
> just provides an interface, and then initializing which is done on the first
> makeContextCurrent() and actually initializes the service side.  Can't you
just
> do this for webview?

Yep, that's the plan. Thanks for the discussion, Dana.

James: do you still have any comments for this patch? Or do you prefer this to
land after init is implemented?

[jamesr, 2013-05-07 20:42:50]

On 2013/05/07 20:42:01, boliu wrote:
> On 2013/05/07 20:36:24, jamesr wrote:
> > GL context creation is typically in two parts - first getting the context,
> which
> > just provides an interface, and then initializing which is done on the first
> > makeContextCurrent() and actually initializes the service side.  Can't you
> just
> > do this for webview?
> 
> Yep, that's the plan. Thanks for the discussion, Dana.
> 
> James: do you still have any comments for this patch? Or do you prefer this to
> land after init is implemented?

I don't.  If you and Dana agree on a path forward for the context provider this
lgtm

[joth, 2013-05-09 10:48:52]

lgtm

https://codereview.chromium.org/14888002/diff/41001/android_webview/browser/i...
File android_webview/browser/in_process_renderer/DEPS (right):

https://codereview.chromium.org/14888002/diff/41001/android_webview/browser/i...
android_webview/browser/in_process_renderer/DEPS:10:
"+android_webview/renderer/aw_render_view_ext.h",
nit: use ! instead of +

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
File android_webview/browser/in_process_renderer/in_process_renderer_client.cc
(right):

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_renderer_client.cc:35:
AwRenderViewExt::RenderViewCreated(render_view);
I think this will be much easier to clean up if we don't override here, but
instead make ViewRenderHost::RenderViewCreated() a no-op unless the no-merge
flag is set. That way when the flag is deleted it'll be far more obvious that
whole class can be deleted.

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
File android_webview/browser/in_process_renderer/in_process_view_renderer.cc
(right):

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:133: //
TODO(boliu): Don't ignore other transforms and whatnot.
think you can remove this comment now.

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:156:
Invalidate();
TODO: this should be PostTask to UI thread, to avoid calling into java from
inside the native GL execution path.
Will need a weakptr factory too then.

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:179: //
TODO(boliu): Need invalidate here if visible?
fairly sure the framework will do this for you

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
File android_webview/browser/in_process_renderer/in_process_view_renderer.h
(right):

https://codereview.chromium.org/14888002/diff/48001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_view_renderer.h:70: bool
continuous_invalidate_;
comments on these three would be helpful

[joth, 2013-05-09 13:14:39]

https://codereview.chromium.org/14888002/diff/57002/android_webview/browser/i...
File android_webview/browser/in_process_renderer/DEPS (right):

https://codereview.chromium.org/14888002/diff/57002/android_webview/browser/i...
android_webview/browser/in_process_renderer/DEPS:10:
"+android_webview/renderer/aw_render_view_ext.h",
remove me (revert file)

https://codereview.chromium.org/14888002/diff/57002/android_webview/browser/r...
File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right):

https://codereview.chromium.org/14888002/diff/57002/android_webview/browser/r...
android_webview/browser/renderer_host/aw_render_view_host_ext.h:28: class Client
{
common preference is to not use nested classes. So, this should be
AwRenderViewHostExtClient. (OK to put in same file I think)

https://codereview.chromium.org/14888002/diff/57002/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/14888002/diff/57002/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:34: g_allow_io_in_ui_thread =
LAZY_INSTANCE_INITIALIZER;
could you add a comment to remove both these?

[commit-bot: I haz the power, 2013-05-09 15:34:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14888002/65002

[commit-bot: I haz the power, 2013-05-09 15:35:20]

Change committed as 199223

[no sievers, 2013-05-15 23:02:22]

https://codereview.chromium.org/14888002/diff/65002/content/renderer/android/...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/14888002/diff/65002/content/renderer/android/...
content/renderer/android/synchronous_compositor_output_surface.cc:79:
context3d()->finish();
Bo, you might want to try if it works replacing this with a flush(), which might
be better for performance (no glFinish()). Maybe even shallowFlushCHROMIUM (no
glFlush()).

We mostly care about forcing commands to be parsed here.