[Android WebView] Implement Ubercomp for Render Thread support

[Android WebView] Implement delegated rendering for sync compositor

Modify content SynchronousCompositor related classes to
support for content embedder to use delegated rendering. The
embedder is responsible for creating the parent compositor.
Essentially need to pass CompositorFrame and
CompositorFrameAck between embedder and
SynchronousCompositor. Also need to expose the share context
so embedder can correctly initialize a context for parent
compositor.

In android_webview implemented parent compositor for
delegated rendering while keeping the current direct
renderer working, controlled by a command line switch.

BrowserViewRenderer deals exclusively with
SynchronousCompositor (the child). And HardwareRenderer
deals exclusively with the parent compositor. The two share
data through lock protected SharedRendererState. And all are
coordinated by native AwContents.

Direct rendering path is kept alive in
HardwareRendererLegacy and code guarded by
switches::UbercompEnabled. Direct rendering path remains the
default.

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

[boliu, 2014-05-19 03:56:08]

Martin, can you give this a first pass?

This includes all of non-cc changes. Didn't really feel like breaking this up.

[mkosiba (inactive), 2014-05-19 20:01:16]

wow, quite a fair number of changes have landed in this area while I wasn't
looking... 

LGTM % comments, although you should get another pair of eyes to look at this.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:310:
DCHECK(attached_to_window_);
nit: I think the consensus is that either you DCHECK or you handle gracefully,
not both.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:315:
DCHECK(shared_renderer_state_->GetCompositor()->GetShareContext());
maybe:
  type share_context =
shared_renderer_state_->GetCompositor()->GetShareContext();
  DCHECK(share_context);
  shared_renderer_state_->SetSharedContext(share_context);

Also, is that a typo in GetShareContext() ? Should it be GetSharedContext?

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:323: ReturnResources();
is it possible for the renderer thread to try and do a draw after this but
before DemandDrawHw and is that a potential problem?

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:376:
shared_renderer_state_->SwapReturnedResources(&frame_ack.resources);
It seems like this logic could just live in the shared_renderer_state? I know it
doesn't do any logic now but you have a couple of places where you simply "get"
things out of one thing accessible from the shared_renderer_state to "set" them
on another thing accessible from the shared_renderer_state.

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

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.h:202: gfx::Vector2d
scroll_offset_;
I'd prefer we call this something like last_on_draw_scroll_offset_ or have a
separate struct or something. BVR already handles scroll offset calculations so
I'd like the separation between this and the totalscrolloffset code to be as
clear as possible.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:60: false /* share_resources */,
you say share resources in line 46, but not here. this is probably correct but
just wanted to double-check

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:87: // Copied.
from?

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:120: // Order is important.
..because one references the other I assume?

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:204:
gl_surface_->SetBackingFrameBufferObject(framebuffer_binding_ext);
could you please explain a bit about what is framebuffer_binding_ext? I'm
guessing we're not using an FBO to render offscreen.

[boliu, 2014-05-19 23:13:01]

Thanks for the review!

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:310:
DCHECK(attached_to_window_);
On 2014/05/19 20:01:16, mkosiba wrote:
> nit: I think the consensus is that either you DCHECK or you handle gracefully,
> not both.

Just checked that this can't actually happen. OnDraw doesn't call
OnDrawHardware(Legacy) unless view is already attached, removed this.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:315:
DCHECK(shared_renderer_state_->GetCompositor()->GetShareContext());
On 2014/05/19 20:01:16, mkosiba wrote:
> maybe:
>   type share_context =
> shared_renderer_state_->GetCompositor()->GetShareContext();
>   DCHECK(share_context);
>   shared_renderer_state_->SetSharedContext(share_context);
> 

Done

> Also, is that a typo in GetShareContext() ? Should it be GetSharedContext?

Not a typo.

It's actually "from GL", "share group" is a real GL name, and "share context" is
chromium command buffer's immitation of share group.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:323: ReturnResources();
On 2014/05/19 20:01:16, mkosiba wrote:
> is it possible for the renderer thread to try and do a draw after this but
> before DemandDrawHw and is that a potential problem?

ReturnResources only return the resources that the parent compositor has
released already. Parent won't release resources that it still needs, and is of
course is free to keep drawing any time it want with them.

This is why there are concerns about doubling memory usage (or halving our
memory limit). Haven't looked closely at this yet.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:376:
shared_renderer_state_->SwapReturnedResources(&frame_ack.resources);
On 2014/05/19 20:01:16, mkosiba wrote:
> It seems like this logic could just live in the shared_renderer_state? I know
it
> doesn't do any logic now but you have a couple of places where you simply
"get"
> things out of one thing accessible from the shared_renderer_state to "set"
them
> on another thing accessible from the shared_renderer_state.

Once legacy path is removed, SharedRendererState won't have a pointer to the
child SynchronousCompositor anymore.

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

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.h:202: gfx::Vector2d
scroll_offset_;
On 2014/05/19 20:01:16, mkosiba wrote:
> I'd prefer we call this something like last_on_draw_scroll_offset_ or have a
> separate struct or something. BVR already handles scroll offset calculations
so
> I'd like the separation between this and the totalscrolloffset code to be as
> clear as possible.

Done, changed global_visible_rect_ too

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:60: false /* share_resources */,
On 2014/05/19 20:01:16, mkosiba wrote:
> you say share resources in line 46, but not here. this is probably correct but
> just wanted to double-check

Yes that's correct and not a bug...that's just how it works

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:120: // Order is important.
On 2014/05/19 20:01:16, mkosiba wrote:
> ..because one references the other I assume?

Unfortunately these things are refcounted. Basically we need to make sure that
when HR is destroyed, all resources are returned to the child. And we can only
return resources when resource_collection_ client is not NULL, so need to free
everything before we set it to null.

That's why there's the DCHECK_IS_ON block.

https://codereview.chromium.org/287993004/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:204:
gl_surface_->SetBackingFrameBufferObject(framebuffer_binding_ext);
On 2014/05/19 20:01:16, mkosiba wrote:
> could you please explain a bit about what is framebuffer_binding_ext? I'm
> guessing we're not using an FBO to render offscreen.

Well, frameworks is. That's what happens when we are put into a hardware layer.
Frameworks creates an fbo and we render into that, and frameworks applies
alpha/rotation/whatever to the fbo, rather than us doing it.

[boliu, 2014-05-20 00:05:33]

Sorry for the massive patch and reviewer list..

piman/danakj please review:
synchronous_compositor.h for api changes to add cc and gpu types.
synchronous_compositor_output_surface.cc for child delegated rendering changes
hardware_renderer.h/cc, parent_output_surface.h/cc for parent compositor. This
is a brand new cc embedder.

sievers:
Please review content sync compositor changes, and how the share context is
passed to HardwareRenderer. And overall review.

aelias:
Matrix/clip/viewport related code in BrowserViewRenderer::OnDrawHardware and
HardwareRenderer::DrawGL. And overall review.

This can land in parallel to changes in cc since delegated path is not turned on
yet.

[no sievers, 2014-05-20 20:48:25]

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:66: if
(!switches::UbercompEnabled() &&
add TODO w/bug number

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:333: viewport;  // Should
really be last_on_draw_global_visible_rect_.
So why is it not? :)

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:78:
RequestProcessGLOnUIThread();
So this now needs to request processing GL on the render thread, which doesn't
match the name of the function. What does it actually do?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.h:30: // TODO(boliu): Teach
this class about RT.
Is this TODO taken care of by this patch?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:196: if (output_surface_) {
Either DCHECK() or if

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:213: // Android webview does not
lose output surface.
nit: does not *support* losing the output surface

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:34: return
weak_factory_.GetWeakPtr();
Any chance we can avoid the need for WeakPtr?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:40: if
(ui_loop_->BelongsToCurrentThread()) {
Does this request a draw on the UI or RenderThread?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:131: share_context_ = context;
This looks like it might not be thread-safe. Set/Get happen across Render and UI
thread, no?

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_impl.cc:125:
DCHECK(CalledOnValidThread());
So this is either on the UI thread (legacy) or on the RenderThread (ubercomp)?

I find it a bit confusing that this class deals with both child and parent
compositor stuff.

Also it seems like keeping the legacy path around makes the code noticeably more
confusing than it already is (or needs to be for other reasons).

[danakj, 2014-05-20 21:50:01]

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:81:
root_layer_->SetIsDrawable(false);
this is the default already

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:87: // Copied from
compositor_impl_android.cc.
Maybe you want a "Keep in sync with compositor_impl_android.cc" ? And throw a
similar comment over there?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:91:
settings.calculate_top_controls_position = false;
this is the default, should remove in both places?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:92: settings.top_controls_height =
0.f;
ditto

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:96: settings.scrollbar_animator =
cc::LayerTreeSettings::NoAnimator;
ditto, also there are scrollbars in the browser compositor? probably not?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:103:
cc::LayerTreeHost::CreateSingleThreaded(this, this, NULL, settings);
you need a non-NULL HostSharedMemoryManager here. Since this isn't actually IPC,
we could maybe have a different manager and child manager pair that don't use
shared memory just fake it.

without a manager here software mode ubercomp is going to blow up (i hope)

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:116:
layer_tree_host_->SetRootLayer(NULL);
not really needed

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:118:
root_layer_->RemoveAllChildren();
why?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:129:
DCHECK(returned_resources.empty());
nit: DCHECK_EQ(0, size()) gives you more data if it fails.. since it's supposed
to be empty this shouldn't be more costly than empty()?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:196: if (output_surface_) {
On 2014/05/20 20:48:26, sievers wrote:
> Either DCHECK() or if

Why do we need a weakptr here. If the OutputSurface is destroyed, that's because
either the compositor is destroyed (can't happen), or we got to
CreateOutputSurface() a 2nd time.

Things stop drawing at that point, can we just set a bool and early out of this
method in that case instead of using a weakptr and calling Composite() which
will never work anyways.

Actually after 5 tries to Composite() cc will LOG(FATAL).

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:197:
output_surface_->SetDrawConstraints(viewport, clip_rect);
future: maybe we could make these inputs to Composite() or something, instead of
having CC pull them off the OutputSurface.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.h:85:
scoped_refptr<cc::DelegatedRendererLayer> layer_;
better name please, which layer?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.h:88:
base::WeakPtr<ParentOutputSurface> output_surface_;
can name this output_surface_weak_ptr_? then it's obvious at use sites what they
are getting.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:24: UpdateClientBeforeDraw();
why? you call SetDrawConstraints before every Composite() already

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:29:
context_provider_->ContextGL()->ShallowFlushCHROMIUM();
why?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:41: if (client_)
How would you get here with a null client, on the first Composite()? Maybe we
can solve that without needing to do this UpdateClientBeforeDraw in two places..

If you update the OutputSurface state in DidBeginMainFrame(), that will happen
inside Composite() after it has set up the OutputSurface. (You could make a
cc_unittest to verify this and ensure we don't break that.) Then you'd need to
store the viewport/clip on the HardwareRenderer to use them there, but you
wouldn't need to deal with this in both BindToClient and here, and check for
client_ being null and stuff. Less branches!

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:80:
capabilities_.draw_and_swap_full_viewport_every_frame = true;
Is this still needed here then? The browser side is setting it.

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:81:
capabilities_.adjust_deadline_for_parent = false;
Still needed?

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:181:
cc::CompositorFrame* frame) {
It feels a bit awkward to be passing in a CompositorFrame* and storing it for
SwapBuffers to assign to.

What if SwapBuffers simply stored the scoped_ptr of the frame on this class, and
then DemandDrawHw returned a scoped_ptr<CompositorFrame> instead of a bool? Then
we don't have some slightly-awkward raw pointers going on, and can follow the
path of things more easily?

DemandDrawSw could return the same. WDYT?

[boliu, 2014-05-21 01:33:26]

Addressed most comments.

What's *not* done yet:
Recheck every LayerTreeSetting I copied to see if they are needed.
Have DemandDrawHw/Sw return scoped_ptr<CompositorFrame>

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:66: if
(!switches::UbercompEnabled() &&
On 2014/05/20 20:48:26, sievers wrote:
> add TODO w/bug number

Done.

Also add disable webgl here.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:333: viewport;  // Should
really be last_on_draw_global_visible_rect_.
On 2014/05/20 20:48:26, sievers wrote:
> So why is it not? :)

This shoudl also be a TODO with crbug.com/372073

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:78:
RequestProcessGLOnUIThread();
On 2014/05/20 20:48:26, sievers wrote:
> So this now needs to request processing GL on the render thread, which doesn't
> match the name of the function. What does it actually do?

The "OnUIThread" bit means run it on UI thread. But the thread hopping logic as
since moved into the renderer_state. Removing the "OnUIThread" bit.

It's always requesting "for the android GL thread", and this class doesn't know
if it's RT or UI.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.h:30: // TODO(boliu): Teach
this class about RT.
On 2014/05/20 20:48:26, sievers wrote:
> Is this TODO taken care of by this patch?

This is better rephrased as "avoid PostTask storm" comment I added in the cc
file. Removed this.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:81:
root_layer_->SetIsDrawable(false);
On 2014/05/20 21:50:02, danakj wrote:
> this is the default already

Done.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:96: settings.scrollbar_animator =
cc::LayerTreeSettings::NoAnimator;
On 2014/05/20 21:50:02, danakj wrote:
> ditto, also there are scrollbars in the browser compositor? probably not?

No scroll bar layers in parent compositor, removed.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:103:
cc::LayerTreeHost::CreateSingleThreaded(this, this, NULL, settings);
On 2014/05/20 21:50:02, danakj wrote:
> you need a non-NULL HostSharedMemoryManager here. Since this isn't actually
IPC,
> we could maybe have a different manager and child manager pair that don't use
> shared memory just fake it.
> 
> without a manager here software mode ubercomp is going to blow up (i hope)

Ubercomp in webview is guaranteed to be hardware only. Software doesn't change
at all with ubercomp, and still uses the resourceless software draw path in
child compositor.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:116:
layer_tree_host_->SetRootLayer(NULL);
On 2014/05/20 21:50:02, danakj wrote:
> not really needed

Removed.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:118:
root_layer_->RemoveAllChildren();
On 2014/05/20 21:50:02, danakj wrote:
> why?

Want to ensure DelegatedRendererLayer, which is a child of the root layer, to be
destroyed before SetClient(NULL) below.

Updated the comment, and just reset to everything to NULL instead. There are no
circular ref-counting in layer tree :)

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:129:
DCHECK(returned_resources.empty());
On 2014/05/20 21:50:02, danakj wrote:
> nit: DCHECK_EQ(0, size()) gives you more data if it fails.. since it's
supposed
> to be empty this shouldn't be more costly than empty()?

Done.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:196: if (output_surface_) {
On 2014/05/20 21:50:02, danakj wrote:
> On 2014/05/20 20:48:26, sievers wrote:
> > Either DCHECK() or if

DCHECK.

> 
> Why do we need a weakptr here. If the OutputSurface is destroyed, that's
because
> either the compositor is destroyed (can't happen), or we got to
> CreateOutputSurface() a 2nd time.

So already stated below in that we do not support losing OutputSurface.

Weakptr is really just for the benefit of the DCHECK. It's not possible to get
draw in the middle of init or tear down.

> 
> Things stop drawing at that point, can we just set a bool and early out of
this
> method in that case instead of using a weakptr and calling Composite() which
> will never work anyways.
> 
> Actually after 5 tries to Composite() cc will LOG(FATAL).

We DCHECK way earlier :)

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:197:
output_surface_->SetDrawConstraints(viewport, clip_rect);
On 2014/05/20 21:50:02, danakj wrote:
> future: maybe we could make these inputs to Composite() or something, instead
of
> having CC pull them off the OutputSurface.

Hmm, this is mirroring how webview sets the viewport/clip information in the
child compositor, and it will still be needed for child compositor even after
ubercomp is turned on. So adding it to Composite for single thread only is kind
of duplicating a code path.

For webview viewport/clip (both here and in child compositor) are not
necessarily same as view_width/length

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:213: // Android webview does not
lose output surface.
On 2014/05/20 20:48:26, sievers wrote:
> nit: does not *support* losing the output surface

Heh, ok.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.h:85:
scoped_refptr<cc::DelegatedRendererLayer> layer_;
On 2014/05/20 21:50:02, danakj wrote:
> better name please, which layer?

Umm...delegated_layer_?

There's the root, and the root only has this one layer as the child.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.h:88:
base::WeakPtr<ParentOutputSurface> output_surface_;
On 2014/05/20 21:50:02, danakj wrote:
> can name this output_surface_weak_ptr_? then it's obvious at use sites what
they
> are getting.

Done.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:29:
context_provider_->ContextGL()->ShallowFlushCHROMIUM();
On 2014/05/20 21:50:02, danakj wrote:
> why?

This is the "synchronous" behavior of parent compositor. This will flush all the
GL of this draw from the command buffer before returning.

And this reminds me, we don't need the flush in child compositor swap for
ubercomp.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:34: return
weak_factory_.GetWeakPtr();
On 2014/05/20 20:48:26, sievers wrote:
> Any chance we can avoid the need for WeakPtr?

It's really only needed for the DCHECK for the UpdateClientBeforeDraw dance.
Refactored using DidBeginMainFrame and got rid of it.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:41: if (client_)
On 2014/05/20 21:50:02, danakj wrote:
> How would you get here with a null client, on the first Composite()? Maybe we
> can solve that without needing to do this UpdateClientBeforeDraw in two
places..
> 
> If you update the OutputSurface state in DidBeginMainFrame(), that will happen
> inside Composite() after it has set up the OutputSurface. (You could make a
> cc_unittest to verify this and ensure we don't break that.) Then you'd need to
> store the viewport/clip on the HardwareRenderer to use them there, but you
> wouldn't need to deal with this in both BindToClient and here, and check for
> client_ being null and stuff. Less branches!

Done. Thanks for the suggestion :D

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:40: if
(ui_loop_->BelongsToCurrentThread()) {
On 2014/05/20 20:48:26, sievers wrote:
> Does this request a draw on the UI or RenderThread?

The ClientRequestDrawGLOnUIThread call has to happen on UI thread. But it
requests draw on whatever the framework GL thread is (ie RT).

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/shared_renderer_state.cc:131: share_context_ = context;
On 2014/05/20 20:48:26, sievers wrote:
> This looks like it might not be thread-safe. Set/Get happen across Render and
UI
> thread, no?

Ehh...that's because I forgot the lock :p

Fixed.

Technically, this actually should be safe since Setter is called before
requesting for DrawGL, but don't need to make this hard-to-test assumption.

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_impl.cc:125:
DCHECK(CalledOnValidThread());
On 2014/05/20 20:48:26, sievers wrote:
> So this is either on the UI thread (legacy) or on the RenderThread (ubercomp)?

Ehh, exact opposite.

This is called on UI thread in ubercomp (correct behavior), and RT thread on
legacy (which will fail this DCHECK, and we are relying on not compiling in
DCHECKs to make legacy path work)

> 
> I find it a bit confusing that this class deals with both child and parent
> compositor stuff.

There isn't any parent stuff...? do you mean parent as in updating frame
metadata? Or just the fact it could be called on RT on legacy path?

> 
> Also it seems like keeping the legacy path around makes the code noticeably
more
> confusing than it already is (or needs to be for other reasons).

There are the PostTasks to UI. But nothing else too bad..?

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:80:
capabilities_.draw_and_swap_full_viewport_every_frame = true;
On 2014/05/20 21:50:02, danakj wrote:
> Is this still needed here then? The browser side is setting it.

Still needed for software path, which child compositor still draws directly "to
screen" (bitmap from android which is immdiately put on screen).

If we ignore the software path, then we can make an argument that this won't be
needed.

[danakj, 2014-05-21 15:22:54]

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:103:
cc::LayerTreeHost::CreateSingleThreaded(this, this, NULL, settings);
On 2014/05/21 01:33:26, boliu wrote:
> On 2014/05/20 21:50:02, danakj wrote:
> > you need a non-NULL HostSharedMemoryManager here. Since this isn't actually
> IPC,
> > we could maybe have a different manager and child manager pair that don't
use
> > shared memory just fake it.
> > 
> > without a manager here software mode ubercomp is going to blow up (i hope)
> 
> Ubercomp in webview is guaranteed to be hardware only. Software doesn't change
> at all with ubercomp, and still uses the resourceless software draw path in
> child compositor.

What about non-resourceless software mode before you initialize the deferred GL?
Do we actually never draw in that state?

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:197:
output_surface_->SetDrawConstraints(viewport, clip_rect);
On 2014/05/21 01:33:26, boliu wrote:
> On 2014/05/20 21:50:02, danakj wrote:
> > future: maybe we could make these inputs to Composite() or something,
instead
> of
> > having CC pull them off the OutputSurface.
> 
> Hmm, this is mirroring how webview sets the viewport/clip information in the
> child compositor, and it will still be needed for child compositor even after
> ubercomp is turned on. So adding it to Composite for single thread only is
kind
> of duplicating a code path.
> 
> For webview viewport/clip (both here and in child compositor) are not
> necessarily same as view_width/length

Ah, it wasn't clear that the renderer would still need this stuff.

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.h:85:
scoped_refptr<cc::DelegatedRendererLayer> layer_;
On 2014/05/21 01:33:26, boliu wrote:
> On 2014/05/20 21:50:02, danakj wrote:
> > better name please, which layer?
> 
> Umm...delegated_layer_?
> 
> There's the root, and the root only has this one layer as the child.

Yes, sure, that's much better :)

[boliu, 2014-05-21 15:35:14]

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:103:
cc::LayerTreeHost::CreateSingleThreaded(this, this, NULL, settings);
On 2014/05/21 15:22:54, danakj wrote:
> On 2014/05/21 01:33:26, boliu wrote:
> > On 2014/05/20 21:50:02, danakj wrote:
> > > you need a non-NULL HostSharedMemoryManager here. Since this isn't
actually
> > IPC,
> > > we could maybe have a different manager and child manager pair that don't
> use
> > > shared memory just fake it.
> > > 
> > > without a manager here software mode ubercomp is going to blow up (i hope)
> > 
> > Ubercomp in webview is guaranteed to be hardware only. Software doesn't
change
> > at all with ubercomp, and still uses the resourceless software draw path in
> > child compositor.
> 
> What about non-resourceless software mode before you initialize the deferred
GL?
> Do we actually never draw in that state?

Right, webview either does resourceless software draws, or full hardware draws.
We never do a delegated software frame.

The Renderer in LTHI *before* deferred GL initialize is just a placeholder
that's never used to draw.

[danakj, 2014-05-21 15:36:32]

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/40001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:103:
cc::LayerTreeHost::CreateSingleThreaded(this, this, NULL, settings);
On 2014/05/21 15:35:14, boliu wrote:
> On 2014/05/21 15:22:54, danakj wrote:
> > On 2014/05/21 01:33:26, boliu wrote:
> > > On 2014/05/20 21:50:02, danakj wrote:
> > > > you need a non-NULL HostSharedMemoryManager here. Since this isn't
> actually
> > > IPC,
> > > > we could maybe have a different manager and child manager pair that
don't
> > use
> > > > shared memory just fake it.
> > > > 
> > > > without a manager here software mode ubercomp is going to blow up (i
hope)
> > > 
> > > Ubercomp in webview is guaranteed to be hardware only. Software doesn't
> change
> > > at all with ubercomp, and still uses the resourceless software draw path
in
> > > child compositor.
> > 
> > What about non-resourceless software mode before you initialize the deferred
> GL?
> > Do we actually never draw in that state?
> 
> Right, webview either does resourceless software draws, or full hardware
draws.
> We never do a delegated software frame.
> 
> The Renderer in LTHI *before* deferred GL initialize is just a placeholder
> that's never used to draw.

Ohmygosh, I never even realized that. Ok thanks for the explanation!

[danakj, 2014-05-21 15:45:39]

LG % the not yet done stuff. a couple last nits/questions about what's done so
far.

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:217:
scoped_ptr<ParentOutputSurface> output_surface_holder_(
remove _ suffix

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:30:
SetNeedsRedrawRect(gfx::Rect(surface_size));
Do you need this given the caps flag to draw the full frame always?

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/parent_output_surface.h (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/parent_output_surface.h:18: // OutputSurface.
// OutputSurface implementation.

or overrides, or something, just more than the class name please.

[boliu, 2014-05-21 15:54:15]

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/parent_output_surface.cc (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/parent_output_surface.cc:30:
SetNeedsRedrawRect(gfx::Rect(surface_size));
On 2014/05/21 15:45:39, danakj wrote:
> Do you need this given the caps flag to draw the full frame always?

Yes this is still needed. draw_and_swap_full_viewport_every_frame was one the
settings added just for webview, but *after* webview already had
SetNeedsRedrawRect before each draw. So instead of changing cc internally, we
just added some DCHECKs that there's always damage from the OutputSurface:

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

I guess it's a bit counter-intuitive, but I think it does makes sense for cc to
assume OutputSurface behaves a certain way, since this is an OutputSurface
capability after all.

[boliu, 2014-05-21 19:58:22]

DemandDrawHw now returns cc::CompositorFrame. This also allowed did_swap_buffer_
to be removed.

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:81:
capabilities_.adjust_deadline_for_parent = false;
On 2014/05/20 21:50:02, danakj wrote:
> Still needed?

Yes. This is to avoid enabling part of the deadline scheduler I believe. Webview
does not control draw scheduling, and has BeginImplFrame and
BeginImplFrameDeadline be the same call.

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:94: settings.highp_threshold_min =
2048;
refresh_rate
impl_side_painting
calculate_top_controls_position
top_controls_height

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:217:
scoped_ptr<ParentOutputSurface> output_surface_holder_(
On 2014/05/21 15:45:39, danakj wrote:
> remove _ suffix

Done.

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/parent_output_surface.h (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/parent_output_surface.h:18: // OutputSurface.
On 2014/05/21 15:45:39, danakj wrote:
> // OutputSurface implementation.
> 
> or overrides, or something, just more than the class name please.

overrides, also fixed HardwareRenderer

[boliu, 2014-05-21 20:00:25]

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/287993004/diff/60001/android_webview/browser/...
android_webview/browser/hardware_renderer.cc:94: settings.highp_threshold_min =
2048;
On 2014/05/21 19:58:22, boliu wrote:
> refresh_rate
> impl_side_painting
> calculate_top_controls_position
> top_controls_height

Argg...those are removed due to same as default.

[danakj, 2014-05-21 20:01:00]

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:81:
capabilities_.adjust_deadline_for_parent = false;
On 2014/05/21 19:58:22, boliu wrote:
> On 2014/05/20 21:50:02, danakj wrote:
> > Still needed?
> 
> Yes. This is to avoid enabling part of the deadline scheduler I believe.
Webview
> does not control draw scheduling, and has BeginImplFrame and
> BeginImplFrameDeadline be the same call.

This is true for the browser compositor, but is that still true for the
renderer? How will the renderer produce frames? Is it synchronous within the
browser compositor's draw functor?

[boliu, 2014-05-21 20:12:19]

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:81:
capabilities_.adjust_deadline_for_parent = false;
On 2014/05/21 20:01:01, danakj wrote:
> On 2014/05/21 19:58:22, boliu wrote:
> > On 2014/05/20 21:50:02, danakj wrote:
> > > Still needed?
> > 
> > Yes. This is to avoid enabling part of the deadline scheduler I believe.
> Webview
> > does not control draw scheduling, and has BeginImplFrame and
> > BeginImplFrameDeadline be the same call.
> 
> This is true for the browser compositor, but is that still true for the
> renderer? How will the renderer produce frames? Is it synchronous within the
> browser compositor's draw functor?

This renderer compositor still needs to be synchronous, regardless if it's for
ubercomp or resourceless software or the existing hardware path. It's because
the new frame must be produced to take into account the scroll offset (and
matrix). Otherwise for example in the gmail android app, the email might not
scroll in sync with email header.

For delegated rendering, renderer compositor draw maps to onDraw on UI, and
parent compositor draw maps to draw functor on RT. onDraw and functor can happen
asynchronously, but "how asynchronous" is out of our control, as we don't
control the callsite of either function.

Hope this makes sense?

[danakj, 2014-05-21 20:13:39]

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/40001/content/browser/android/...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:81:
capabilities_.adjust_deadline_for_parent = false;
On 2014/05/21 20:12:20, boliu wrote:
> On 2014/05/21 20:01:01, danakj wrote:
> > On 2014/05/21 19:58:22, boliu wrote:
> > > On 2014/05/20 21:50:02, danakj wrote:
> > > > Still needed?
> > > 
> > > Yes. This is to avoid enabling part of the deadline scheduler I believe.
> > Webview
> > > does not control draw scheduling, and has BeginImplFrame and
> > > BeginImplFrameDeadline be the same call.
> > 
> > This is true for the browser compositor, but is that still true for the
> > renderer? How will the renderer produce frames? Is it synchronous within the
> > browser compositor's draw functor?
> 
> This renderer compositor still needs to be synchronous, regardless if it's for
> ubercomp or resourceless software or the existing hardware path. It's because
> the new frame must be produced to take into account the scroll offset (and
> matrix). Otherwise for example in the gmail android app, the email might not
> scroll in sync with email header.
> 
> For delegated rendering, renderer compositor draw maps to onDraw on UI, and
> parent compositor draw maps to draw functor on RT. onDraw and functor can
happen
> asynchronously, but "how asynchronous" is out of our control, as we don't
> control the callsite of either function.
> 
> Hope this makes sense?

Oh ok. Ya, I bumped into onDraw in the removal of CompositeAndReadback :)

[no sievers, 2014-05-23 19:47:35]

lgtm if/when Dana is happy

https://codereview.chromium.org/287993004/diff/120001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/287993004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:824: if (!draw_functor_succeeded &&
hardware_initialized) {
|hardware_initialized| is always true in here.

[danakj, 2014-05-23 19:54:55]

LGTM

https://codereview.chromium.org/287993004/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:140:
cc::CompositorFrame* frame) {
We should make this a scoped_ptr in the future maybe?

[boliu, 2014-05-23 20:36:51]

Just need Antoine to approve the content/public change.

https://codereview.chromium.org/287993004/diff/120001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/287993004/diff/120001/android_webview/native/...
android_webview/native/aw_contents.cc:824: if (!draw_functor_succeeded &&
hardware_initialized) {
On 2014/05/23 19:47:35, sievers wrote:
> |hardware_initialized| is always true in here.

Done.

https://codereview.chromium.org/287993004/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/287993004/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:140:
cc::CompositorFrame* frame) {
On 2014/05/23 19:54:56, danakj wrote:
> We should make this a scoped_ptr in the future maybe?

Yeah, I agree that would make more sense.

[piman, 2014-05-23 21:20:41]

lgtm

[boliu, 2014-05-23 21:23:26]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-23 21:24:21]

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

[boliu, 2014-05-23 21:35:50]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-23 21:38:01]

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

[commit-bot: I haz the power, 2014-05-24 01:16:22]

Change committed as 272668