Implement WebCompositorOutputSurface

Creates the WebCompositorOutputSurface, which is the new mechanism for rendering interactions between the compositor and RenderWidgetHost. 

As part of this, we plumb vsync information to the compositor.

The new IPC::ForwardingMessageFilter is modified to have route-specific handlers rather than a global handler. This simplifies message routing considerably.

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

[darin (slow to review), 2012-07-18 17:56:45]

It is hard to review this change without seeing what the other IPC messages are
that need to be intercepted.

The whole reason for InputEventFilter existing is not just to be a MessageFilter
that picks out messages and forwards them to a different thread.  The meat of
InputEventFilter is in the DidHandleInputEvent and DidNotHandleInputEvent
methods.  This class knows how to generate InputEvent_ACK messages, etc.

Will the other IPC messages have a similar request/response pattern?  Do they
have ACKs that need to be generated on the compositor thread?  Do they similarly
need the DidNotHandleInputEvent style logic that bounces the message back to the
main thread to be handled using the normal code paths?

If these other messages need to be intercepted in a simpler fashion:  i.e., if
they don't need to potentially be bounced back to the main thread, then it seems
like instantiating a separate MessageFilter might be easier.  We have a lot of
MessageFilter subclasses in the renderer.

What makes these other messages that you want to intercept similar to
InputEvents?

http://codereview.chromium.org/10798006/diff/1/content/renderer/gpu/composito...
File content/renderer/gpu/compositor_message_filter.h (right):

http://codereview.chromium.org/10798006/diff/1/content/renderer/gpu/composito...
content/renderer/gpu/compositor_message_filter.h:59: base::MessageLoopProxy*
target_loop,
nit: indentation

[darin (slow to review), 2012-07-18 17:59:43]

Also, notice how the InputEventFilter has nothing really to do with the
compositor.  Another system that wanted to possibly intercept input events could
also use this class to get first crack at input events.  It seems like a
CompositorMessageFilter class could exist alongside an InputEventFilter.  The
CompositorMessageFilter would just intercept compositor related messages.  I
would only generalize InputEventFilter if other messages need to follow the
exact same handler pattern (i.e., generating ACKs on the background thread or
bouncing messages back to the main thread for normal processing).

[nduca, 2012-07-18 22:59:55]

That sounds harmless. I'll give it a try, thanks!

[nduca, 2012-07-19 08:14:53]

Hey darin, is this more the direction that you were thinking?

I'm still wrapping my head around this approach myself. While I respect the
observation you have about "InputFilter is general purpose as written now," it
seems to me we dont have other use cases now or in the future that would need
that capability. That is to say, yes, we could write this as a module, but there
is no customer other than the compositor. As such, I'm struggling to understand
the win of the added-layering....

But, this having been said, this is how it looks as a separate filter. I added
the plubing for vsync to this to show how we'd use it.

[darin (slow to review), 2012-07-19 17:00:59]

Could you add compositor_message_filter.{h,cc} to this CL so I can see what they
do?

[piman, 2012-07-24 06:30:36]

LGTM, nice. I'm pretty sure we'll use that filter for ÜC.

[darin (slow to review), 2012-07-24 23:56:43]

http://codereview.chromium.org/10798006/diff/5004/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10798006/diff/5004/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1002: void
RenderWidgetHostImpl::UpdateVSyncParameters(base::TimeTicks timebase,
Does it make sense to pass TimeTicks values across processes?

http://codereview.chromium.org/10798006/diff/5004/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_thread.cc (right):

http://codereview.chromium.org/10798006/diff/5004/content/renderer/gpu/compos...
content/renderer/gpu/compositor_thread.cc:93: uint32 messages[] =
{ViewMsg_UpdateVSyncParameters::ID};
nit: const uint32 kMessages[] = { ViewMsg_...::ID };

http://codereview.chromium.org/10798006/diff/5004/content/renderer/gpu/compos...
content/renderer/gpu/compositor_thread.cc:113: 
nit: extra new line

http://codereview.chromium.org/10798006/diff/5004/content/renderer/gpu/compos...
content/renderer/gpu/compositor_thread.cc:190: InputHandlerMap::iterator it =
input_handlers_.find(message.routing_id());
on the webkit side, perhaps we should be pushing these parameters to a different
interface than the input handler?  maybe that's overloading the concept of input
handler too much?

[jbates, 2012-07-27 18:42:25]

http://codereview.chromium.org/10798006/diff/5004/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10798006/diff/5004/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.h:363: base::TimeDelta
interval);
If the interval is TimeDelta, it's limited to microsecond precision, which
equates to a dropped frame every ~5 minutes. I guess the timebase will give the
scheduler a chance to correct that drift though, but it's unclear whether all
platforms can get a timebase and how often we will update it. The DWM API gives
a numerator and denominator for the interval (for example: 60000 and 1001 on my
z600 box). How about changing the interval to uint32 numerator and uint32
denominator? That way the CC scheduler can have the best chance at maintaining
an accurate tick rate.

[piman, 2012-08-08 16:47:43]

I just have nits.

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:34: , client_(NULL)
nit: initializer style
Class::Class(...)
    : initializer1_(value1),
      initializer2_(value2),
      initializer3 {
  ...
}

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:37:
capabilities_.hasParentCompositor = false;
Could you add a DCHECK(output_surface_filter_) to make it clear that this
depends on RenderThreadImpl's initialization?

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:44: RemoveRoute(routing_id_);
nit: I think this line fits in 80 chars.

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:55: if (!
context3D_->makeContextCurrent())
nit: no space after !

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.h (right):

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.h:25: class
CompositorOutputSurface : public WebKit::WebCompositorOutputSurface {
Can we add a quick comment that this class is created on the main thread, but
called (and destroyed) from the compositor thread (if it's on)?
We could use base::NonThreadSafe, disconnecting from the thread in the
constructor, but checking in all entrypoints.

http://codereview.chromium.org/10798006/diff/23001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10798006/diff/23001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:517: compositor_thread_->message_loop() :
nit: indenting for continuations is +4.

http://codereview.chromium.org/10798006/diff/23001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10798006/diff/23001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1772: // disparities we explicitly disable
antialiasing.
qq, is that the current behavior from what webkit requests (disable AA)?

[jbates, 2012-08-08 16:55:08]

Nice work, looks like it will now be easy to add new browser->renderer messages
for the compositor thread.

http://codereview.chromium.org/10798006/diff/23001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10798006/diff/23001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:366: base::TimeDelta
interval);
TimeDelta is OK for now since we are in a rush, but can you file a bug to make
this more accurate in a followup patch? Some platform/OS/driver combos will not
have a timebase (eg: XP + AMD family 15), so it will be up to the interval to
keep in sync with the refresh rate.

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:43: output_surface_filter_->
nit: no need for line wrap

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:55: if (!
context3D_->makeContextCurrent())
nit: no space after !

http://codereview.chromium.org/10798006/diff/23001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:79:
IPC_MESSAGE_HANDLER(ViewMsg_UpdateVSyncParameters, OnUpdateVSyncParameters);
We're going to need the SwapAck here as well. Feel free to land this though and
we can do it in a followup patch.

http://codereview.chromium.org/10798006/diff/23001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10798006/diff/23001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:516: MessageLoop* output_surface_loop =
content::IsThreadedCompositingEnabled() ?
Should this be replaced with the existing enable from line 506? The logic is
slightly different above (includes !is_guest).

[nduca, 2012-08-09 06:29:58]

> qq, is that the current behavior from what webkit requests (disable AA)?
Yep, think that's funky?


http://codereview.chromium.org/10798006/diff/23001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_impl.h:366: base::TimeDelta
> interval);
> TimeDelta is OK for now since we are in a rush, but can you file a bug to make
> this more accurate in a followup patch?

crbug.com/141587

[piman, 2012-08-09 06:44:45]

On Wed, Aug 8, 2012 at 11:29 PM, <nduca@chromium.org> wrote:

>  qq, is that the current behavior from what webkit requests (disable AA)?
>>
> Yep, think that's funky?


Oh no, I think it makes perfect sense. I didn't know if there was a flag or
anything. I guess the flag is only used for webgl.

Still LGTM.

>
>
>
> http://codereview.chromium.**org/10798006/diff/23001/**
>
content/browser/renderer_host/**render_widget_host_impl.h#**newcode366<http://codereview.chromium.org/10798006/diff/23001/content/browser/renderer_host/render_widget_host_impl.h#newcode366>
>
>> content/browser/renderer_host/**render_widget_host_impl.h:366:
>> base::TimeDelta
>> interval);
>> TimeDelta is OK for now since we are in a rush, but can you file a bug to
>> make
>> this more accurate in a followup patch?
>>
>
> crbug.com/141587
>
>
https://chromiumcodereview.**appspot.com/10798006/<https://chromiumcodereview...
>

[jbates, 2012-08-09 16:17:24]

lgtm

[darin (slow to review), 2012-08-09 20:13:45]

http://codereview.chromium.org/10798006/diff/31003/ipc/ipc_forwarding_message...
File ipc/ipc_forwarding_message_filter.cc (right):

http://codereview.chromium.org/10798006/diff/31003/ipc/ipc_forwarding_message...
ipc/ipc_forwarding_message_filter.cc:40: if
(handlers_.find(message.routing_id()) == handlers_.end())
would it make sense to grab a reference to the handler here and pass it to the
ForwardToHandler method?  or actually, you could just eliminate that function
entirely.

  Handler handler;
  {
    base::AutoLock locked(handlers_lock_);
    std::map<int, Handler>::const_iterator it =
        handlers_.find(message.routing_id());
    if (it == handlers_.end())
      return false;
    handler = it->second;
  }
  target_task_runner_->PostTask(FROM_HERE, base::Bind(handler, message));

[nduca, 2012-08-10 01:43:16]

Updated. @darin, just need your LG for ipc/OWNERS

[darin (slow to review), 2012-08-10 16:32:57]

https://chromiumcodereview.appspot.com/10798006/diff/32002/ipc/ipc_forwarding...
File ipc/ipc_forwarding_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10798006/diff/32002/ipc/ipc_forwarding...
ipc/ipc_forwarding_message_filter.cc:39: std::map<int, Handler>::iterator it;
this doesn't look safe to me.  the iterator points into the map, which could be
mutated by another thread after you release the lock.  you need to extract the
Handler and take an owning reference to that _and_ then release the lock.

[piman, 2012-08-11 01:43:32]

On 2012/08/10 16:32:57, darin wrote:
>
https://chromiumcodereview.appspot.com/10798006/diff/32002/ipc/ipc_forwarding...
> File ipc/ipc_forwarding_message_filter.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10798006/diff/32002/ipc/ipc_forwarding...
> ipc/ipc_forwarding_message_filter.cc:39: std::map<int, Handler>::iterator it;
> this doesn't look safe to me.  the iterator points into the map, which could
be
> mutated by another thread after you release the lock.  you need to extract the
> Handler and take an owning reference to that _and_ then release the lock.

(If that helps, copying a closure is cheap).

[nduca, 2012-08-11 02:07:10]

I think I got it fixed in the latest patch. I just pulled the handle out of the
iteartor by assignment. It should work...

[piman, 2012-08-11 04:46:03]

lgtm

[darin (slow to review), 2012-08-11 06:14:43]

LGTM

[commit-bot: I haz the power, 2012-08-13 16:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/10798006/21010

[commit-bot: I haz the power, 2012-08-13 16:33:30]

Failed to apply patch for ui/compositor/compositor.cc:
While running patch -p1 --forward --force;
patching file ui/compositor/compositor.cc
Hunk #1 FAILED at 150.
Hunk #2 succeeded at 328 (offset -9 lines).
1 out of 2 hunks FAILED -- saving rejects to file
ui/compositor/compositor.cc.rej

[commit-bot: I haz the power, 2012-08-13 19:12:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/10798006/24016

[commit-bot: I haz the power, 2012-08-13 20:01:50]

Try job failure for 10798006-24016 (retry) on linux_rel for step
"content_unittests".
It's a second try, previously, step "content_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-13 21:10:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/10798006/24016

[commit-bot: I haz the power, 2012-08-13 22:03:54]

Try job failure for 10798006-24016 (retry) on mac_rel for step
"content_unittests".
It's a second try, previously, step "content_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...