Plumb cc::LatencyInfo through command buffer and output surface

Plumb cc::LatencyInfo through command buffer and output surface

This allows the LatencyInfo struct to pass from the compositor to the gpu process and up to the browser. It only works completely with the passthrough image transport surface at the moment, because that doesn't require passing data through some extra components.

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

[apatrick_chromium, 2013-03-15 19:09:35]

lgtm

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.cc:39: if (stub_)
nit: braces

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.cc:54: if (stub_)
nit: braces

https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:97: if (command_buffer_proxy_)
nit: braces

[nduca, 2013-03-18 18:38:31]

+danakj +piman +skaslev

https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h
File cc/output_surface_client.h (right):

https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h...
cc/output_surface_client.h:22: virtual void OnReceivedLatencyInfo(const
LatencyInfo&) = 0;
I think we should consider merging SwapBuffers/SendFrameData into one method on
the output surface and always passing latency info on that command.

And, doing the same for the swapbuffers ack && frame data ack.

[jbauman, 2013-03-18 20:36:42]

On 2013/03/18 18:38:31, nduca wrote:
> +danakj +piman +skaslev
> 
> https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h
> File cc/output_surface_client.h (right):
> 
>
https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h...
> cc/output_surface_client.h:22: virtual void OnReceivedLatencyInfo(const
> LatencyInfo&) = 0;
> I think we should consider merging SwapBuffers/SendFrameData into one method
on
> the output surface and always passing latency info on that command.

Maybe. Sending the latency info earlier in a separate IPC is nice if we want to
make trace events from this information, because then it's easier to understand
where some of the latency is coming from. However, it's not essential.
> 
> And, doing the same for the swapbuffers ack && frame data ack.

The Swapbuffers ack can happen before the frame is actually presented to the
screen, so I'd like to keep these separate.

[nduca, 2013-03-18 20:41:04]

I think that we have other use cases for a pre-swap coming, so lets save the
Swap/SendFrame together for now and possibly move it when that pre-swap stuff
lands.

Can we namesmith the swap and latency callbacks to be more clearly related to
the swap ack? Maybe the swapbuffersQueuedToOS and SwabufferscompletedOnScreen...

[jbauman, 2013-03-18 20:47:28]

On 2013/03/18 20:41:04, nduca wrote:
> I think that we have other use cases for a pre-swap coming, so lets save the
> Swap/SendFrame together for now and possibly move it when that pre-swap stuff
> lands.
> 
> Can we namesmith the swap and latency callbacks to be more clearly related to
> the swap ack? Maybe the swapbuffersQueuedToOS and
SwabufferscompletedOnScreen...

Which callbacks are you talking about? The echo callback and the latency info
callback?

[nduca, 2013-03-18 20:54:23]

WGC3D::Onswapbufferscompleted and this new OnReceivedLatencyInfo are basically
acks of the frame, but at different stages of processing of the frame. One comes
with ltency info.

[jbauman, 2013-03-18 21:24:00]

On 2013/03/18 20:54:23, nduca wrote:
> WGC3D::Onswapbufferscompleted and this new OnReceivedLatencyInfo are basically
> acks of the frame, but at different stages of processing of the frame. One
comes
> with ltency info.

Yeah, in practice OnReceivedLatencyInfo corresponds with when the swap buffers
is submitted to the OS (unless we can determine when it completes on a
platform). And the precise meaning of WGC3D::Onswapbufferscompleted depends on
the platform. So I'm not sure of a good way to summarize that.

[piman, 2013-03-19 04:30:28]

https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h#newcode96
cc/output_surface.h:96: virtual void SetLatencyInfo(const LatencyInfo&) {}
What calls this, and in particular, what ensures ordering wrt the swap (i.e.
calls Flush)? What is the proper ordering?

Agreed with Nat, this should probably be folded into
SwapBuffers/PostSubBuffer/SendFrameToParentCompositor (and the 2 former maybe
folded into the latter).

https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h
File cc/output_surface_client.h (right):

https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h...
cc/output_surface_client.h:22: virtual void OnReceivedLatencyInfo(const
LatencyInfo&) = 0;
At a high level, what is the purpose of making LatencyInfo flow back into the
compositor? Is it just so that we can display it? Or are we trying to measure
something with it, or use it to make decisions in the compositor?

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:537: }
note: it's rather uncommon to have the same message go in both directions.
Nothing fundamentally wrong with it, but it makes the code rather unreadable,
where OnSetLatencyInfo and SetLatencyInfo are completely unrelated, whereas
someone might expect it to be.

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_mes...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_mes...
content/common/gpu/gpu_messages.h:44: IPC_STRUCT_TRAITS_END()
could we move this to content/common/cc_messages.h ? We'll probably want to get
that struct through the SwapCompositorFrame flow as well.

https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailb...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailb...
content/renderer/gpu/mailbox_output_surface.cc:29: proxy,
Note: I don't think this will do what you want, since in the case of the
MailboxOutputSurface, SwapBuffers isn't called on the context, nor will a
SetLatencyInfo be expected to come back.

[jbauman, 2013-03-19 16:03:37]

On 2013/03/19 04:30:28, piman wrote:
> https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h
> File cc/output_surface.h (right):
> 
>
https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h#newcode96
> cc/output_surface.h:96: virtual void SetLatencyInfo(const LatencyInfo&) {}
> What calls this, and in particular, what ensures ordering wrt the swap (i.e.
> calls Flush)? What is the proper ordering?
> 
> Agreed with Nat, this should probably be folded into
> SwapBuffers/PostSubBuffer/SendFrameToParentCompositor (and the 2 former maybe
> folded into the latter).

Merging all those functions together won't work yet, as they can be called at
very different times and in different circumstances at the moment (e.g. when
doing readback). I'll add an argument to SwapBuffers/PostSubBuffer and add the
LatencyInfo to the CompositorMetaData.
> 
> https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h
> File cc/output_surface_client.h (right):
> 
>
https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h...
> cc/output_surface_client.h:22: virtual void OnReceivedLatencyInfo(const
> LatencyInfo&) = 0;
> At a high level, what is the purpose of making LatencyInfo flow back into the
> compositor? Is it just so that we can display it? Or are we trying to measure
> something with it, or use it to make decisions in the compositor?
> 
Ali Juma was working on measuring latency just from the renderer compositor to
the screen, so this helps that.

Also, I originally tried just sending this information straight from the
SwapBuffers (wherever it happened) to the RWHV directly, without going back
through the path it came from, and the routing gets pretty hairy, and there can
be a bunch of special cases for aura/non-aura, etc. So doing it this way seems
nicer, as there's always a logical place for the information to go to.

>
https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_com...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_com...
> content/common/gpu/gpu_command_buffer_stub.cc:537: }
> note: it's rather uncommon to have the same message go in both directions.
> Nothing fundamentally wrong with it, but it makes the code rather unreadable,
> where OnSetLatencyInfo and SetLatencyInfo are completely unrelated, whereas
> someone might expect it to be.
> 
I'll see if I can split this message up and find better names for each version.
>
https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_mes...
> File content/common/gpu/gpu_messages.h (right):
> 
>
https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_mes...
> content/common/gpu/gpu_messages.h:44: IPC_STRUCT_TRAITS_END()
> could we move this to content/common/cc_messages.h ? We'll probably want to
get
> that struct through the SwapCompositorFrame flow as well.
> 
>
https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailb...
> File content/renderer/gpu/mailbox_output_surface.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailb...
> content/renderer/gpu/mailbox_output_surface.cc:29: proxy,
> Note: I don't think this will do what you want, since in the case of the
> MailboxOutputSurface, SwapBuffers isn't called on the context, nor will a
> SetLatencyInfo be expected to come back.

[piman, 2013-03-19 16:50:41]

On Tue, Mar 19, 2013 at 9:03 AM, <jbauman@chromium.org> wrote:

> On 2013/03/19 04:30:28, piman wrote:
>
>> https://codereview.chromium.**org/12614013/diff/8001/cc/**
>>
output_surface.h<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h>
>> File cc/output_surface.h (right):
>>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/cc/**
>
output_surface.h#newcode96<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h#newcode96>
>
>> cc/output_surface.h:96: virtual void SetLatencyInfo(const LatencyInfo&) {}
>> What calls this, and in particular, what ensures ordering wrt the swap
>> (i.e.
>> calls Flush)? What is the proper ordering?
>>
>
>  Agreed with Nat, this should probably be folded into
>> SwapBuffers/PostSubBuffer/**SendFrameToParentCompositor (and the 2
>> former maybe
>> folded into the latter).
>>
>
> Merging all those functions together won't work yet, as they can be called
> at
> very different times and in different circumstances at the moment (e.g.
> when
> doing readback). I'll add an argument to SwapBuffers/PostSubBuffer and add
> the
> LatencyInfo to the CompositorMetaData.


Ok, the latter sounds great and it should help a lot with the
MailboxOutputSurface case
>
>
>
>  https://codereview.chromium.**org/12614013/diff/8001/cc/**
>>
output_surface_client.h<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h>
>> File cc/output_surface_client.h (right):
>>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/cc/**
>
output_surface_client.h#**newcode22<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h#newcode22>
>
>> cc/output_surface_client.h:22: virtual void OnReceivedLatencyInfo(const
>> LatencyInfo&) = 0;
>> At a high level, what is the purpose of making LatencyInfo flow back into
>> the
>> compositor? Is it just so that we can display it? Or are we trying to
>> measure
>> something with it, or use it to make decisions in the compositor?
>>
>
> Ali Juma was working on measuring latency just from the renderer
> compositor to
> the screen, so this helps that.
>

I understand the overall goal of the latency info, but to measure the
renderer -> screen latency, it's independent of the post-swap
browser->renderer messages. So I'm trying to figure out what these are for.


> Also, I originally tried just sending this information straight from the
> SwapBuffers (wherever it happened) to the RWHV directly, without going back
> through the path it came from, and the routing gets pretty hairy, and
> there can
> be a bunch of special cases for aura/non-aura, etc. So doing it this way
> seems
> nicer, as there's always a logical place for the information to go to.


The only case where the renderer swap doesn't hit the RWHV is on non-aura
win. Even then it would seem like it's just a matter of posting a task
(since you already have all the information to route from the
AcceleratedSurfaceBuffersSwapped message).

I'm still unclear about how you will route it back to the RWHV from the
compositor thread where it comes back. There's a lot of context missing
here.
I'm also missing what's happening with the timing information, how it gets
used (does it generate a trace?), which subsystem processes it, etc.


>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/**
>
content/common/gpu/gpu_**command_buffer_stub.cc<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_command_buffer_stub.cc>
>
>> File content/common/gpu/gpu_**command_buffer_stub.cc (right):
>>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/**
>
content/common/gpu/gpu_**command_buffer_stub.cc#**newcode537<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_command_buffer_stub.cc#newcode537>
>
>> content/common/gpu/gpu_**command_buffer_stub.cc:537: }
>> note: it's rather uncommon to have the same message go in both directions.
>> Nothing fundamentally wrong with it, but it makes the code rather
>> unreadable,
>> where OnSetLatencyInfo and SetLatencyInfo are completely unrelated,
>> whereas
>> someone might expect it to be.
>>
>
> I'll see if I can split this message up and find better names for each
> version.
>
>
> https://codereview.chromium.**org/12614013/diff/8001/**
>
content/common/gpu/gpu_**messages.h<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_messages.h>
>
>> File content/common/gpu/gpu_**messages.h (right):
>>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/**
>
content/common/gpu/gpu_**messages.h#newcode44<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_messages.h#newcode44>
>
>> content/common/gpu/gpu_**messages.h:44: IPC_STRUCT_TRAITS_END()
>> could we move this to content/common/cc_messages.h ? We'll probably want
>> to
>>
> get
>
>> that struct through the SwapCompositorFrame flow as well.
>>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/**
>
content/renderer/gpu/mailbox_**output_surface.cc<https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailbox_output_surface.cc>
>
>> File content/renderer/gpu/mailbox_**output_surface.cc (right):
>>
>
>
> https://codereview.chromium.**org/12614013/diff/8001/**
>
content/renderer/gpu/mailbox_**output_surface.cc#newcode29<https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailbox_output_surface.cc#newcode29>
>
>> content/renderer/gpu/mailbox_**output_surface.cc:29: proxy,
>> Note: I don't think this will do what you want, since in the case of the
>> MailboxOutputSurface, SwapBuffers isn't called on the context, nor will a
>> SetLatencyInfo be expected to come back.
>>
>
>
>
>
https://codereview.chromium.**org/12614013/<https://codereview.chromium.org/1...
>

[jbauman, 2013-03-21 17:36:42]

On Tue, Mar 19, 2013 at 9:50 AM, Antoine Labour <piman@chromium.org> wrote:

> On Tue, Mar 19, 2013 at 9:03 AM, <jbauman@chromium.org> wrote:
>
>> On 2013/03/19 04:30:28, piman wrote:
>>
>>> https://codereview.chromium.**org/12614013/diff/8001/cc/**
>>>
output_surface.h<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h>
>>> File cc/output_surface.h (right):
>>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/cc/**
>>
output_surface.h#newcode96<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface.h#newcode96>
>>
>>> cc/output_surface.h:96: virtual void SetLatencyInfo(const LatencyInfo&)
>>> {}
>>> What calls this, and in particular, what ensures ordering wrt the swap
>>> (i.e.
>>> calls Flush)? What is the proper ordering?
>>>
>>
>>  Agreed with Nat, this should probably be folded into
>>> SwapBuffers/PostSubBuffer/**SendFrameToParentCompositor (and the 2
>>> former maybe
>>> folded into the latter).
>>>
>>
>> Merging all those functions together won't work yet, as they can be
>> called at
>> very different times and in different circumstances at the moment (e.g.
>> when
>> doing readback). I'll add an argument to SwapBuffers/PostSubBuffer and
>> add the
>> LatencyInfo to the CompositorMetaData.
>
>
> Ok, the latter sounds great and it should help a lot with the
> MailboxOutputSurface case
>
>>
>>
>>  https://codereview.chromium.**org/12614013/diff/8001/cc/**
>>>
output_surface_client.h<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h>
>>> File cc/output_surface_client.h (right):
>>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/cc/**
>>
output_surface_client.h#**newcode22<https://codereview.chromium.org/12614013/diff/8001/cc/output_surface_client.h#newcode22>
>>
>>> cc/output_surface_client.h:22: virtual void OnReceivedLatencyInfo(const
>>> LatencyInfo&) = 0;
>>> At a high level, what is the purpose of making LatencyInfo flow back
>>> into the
>>> compositor? Is it just so that we can display it? Or are we trying to
>>> measure
>>> something with it, or use it to make decisions in the compositor?
>>>
>>
>> Ali Juma was working on measuring latency just from the renderer
>> compositor to
>> the screen, so this helps that.
>>
>
> I understand the overall goal of the latency info, but to measure the
> renderer -> screen latency, it's independent of the post-swap
> browser->renderer messages. So I'm trying to figure out what these are for.
>

They're for informing the renderer when the swap happened. Otherwise it
couldn't do the calculation, because it doesn't know that.

>
>
>> Also, I originally tried just sending this information straight from the
>> SwapBuffers (wherever it happened) to the RWHV directly, without going
>> back
>> through the path it came from, and the routing gets pretty hairy, and
>> there can
>> be a bunch of special cases for aura/non-aura, etc. So doing it this way
>> seems
>> nicer, as there's always a logical place for the information to go to.
>
>
> The only case where the renderer swap doesn't hit the RWHV is on non-aura
> win. Even then it would seem like it's just a matter of posting a task
> (since you already have all the information to route from the
> AcceleratedSurfaceBuffersSwapped message).
>

Well, aura is rather special, as the on-screen
AcceleratedSurfaceBuffersSwapped messages should identify the browser
compositor, not whatever renderers created frames involved in that. So to
inform them what happened, I guess we'd have to notify all the RWHVs that a
swap happened, and let them figure out if they were involved based on
whatever latencyinfo they originally passed into the browser.

>
> I'm still unclear about how you will route it back to the RWHV from the
> compositor thread where it comes back. There's a lot of context missing
> here.
> I'm also missing what's happening with the timing information, how it gets
> used (does it generate a trace?), which subsystem processes it, etc.
>

The overall design doc is
https://docs.google.com/a/google.com/document/d/1UEoVttE9UMNs3ouL0tZ41GAMkOk7...
.
My previous complete patch set, which Nat asked me to break up, is at
https://codereview.chromium.org/11293121/ . The information about when the
swap happens passes up the stack in reverse of how it was passed down, and
then each component figures out the time differential. For an input event,
it eventually passes back through the renderer compositor to the renderer
main thread, which (if this was a fake input event created for the
smooth-scroll benchmark or something like that) compares the swap time with
the time (passed down from the RWHV) of the faked input event, does the
subtraction and stores it for the gpu benchmarking extension.

>
>
>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/**
>>
content/common/gpu/gpu_**command_buffer_stub.cc<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_command_buffer_stub.cc>
>>
>>> File content/common/gpu/gpu_**command_buffer_stub.cc (right):
>>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/**
>>
content/common/gpu/gpu_**command_buffer_stub.cc#**newcode537<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_command_buffer_stub.cc#newcode537>
>>
>>> content/common/gpu/gpu_**command_buffer_stub.cc:537: }
>>> note: it's rather uncommon to have the same message go in both
>>> directions.
>>> Nothing fundamentally wrong with it, but it makes the code rather
>>> unreadable,
>>> where OnSetLatencyInfo and SetLatencyInfo are completely unrelated,
>>> whereas
>>> someone might expect it to be.
>>>
>>
>> I'll see if I can split this message up and find better names for each
>> version.
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/**
>>
content/common/gpu/gpu_**messages.h<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_messages.h>
>>
>>> File content/common/gpu/gpu_**messages.h (right):
>>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/**
>>
content/common/gpu/gpu_**messages.h#newcode44<https://codereview.chromium.org/12614013/diff/8001/content/common/gpu/gpu_messages.h#newcode44>
>>
>>> content/common/gpu/gpu_**messages.h:44: IPC_STRUCT_TRAITS_END()
>>> could we move this to content/common/cc_messages.h ? We'll probably want
>>> to
>>>
>> get
>>
>>> that struct through the SwapCompositorFrame flow as well.
>>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/**
>>
content/renderer/gpu/mailbox_**output_surface.cc<https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailbox_output_surface.cc>
>>
>>> File content/renderer/gpu/mailbox_**output_surface.cc (right):
>>>
>>
>>
>> https://codereview.chromium.**org/12614013/diff/8001/**
>>
content/renderer/gpu/mailbox_**output_surface.cc#newcode29<https://codereview.chromium.org/12614013/diff/8001/content/renderer/gpu/mailbox_output_surface.cc#newcode29>
>>
>>> content/renderer/gpu/mailbox_**output_surface.cc:29: proxy,
>>> Note: I don't think this will do what you want, since in the case of the
>>> MailboxOutputSurface, SwapBuffers isn't called on the context, nor will a
>>> SetLatencyInfo be expected to come back.
>>>
>>
>>
>>
>>
https://codereview.chromium.**org/12614013/<https://codereview.chromium.org/1...
>>
>
>

[jbauman, 2013-04-03 06:28:07]

Ok, removed the path back from the GPU process to the renderer, and made the
data always flow up to the browser process (with a new IPC message, if
necessary).

[piman, 2013-04-03 18:05:09]

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/client...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/client...
content/common/gpu/client/command_buffer_proxy_impl.cc:206: const
cc::LatencyInfo& latency_info) {
You need to check for last_state_.error != gpu::error::kNoError here (common
pattern in every entry-point that sends a message, see also DCHECK in ::Send).

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
content/common/gpu/image_transport_surface.cc:71: if (stub_) {
stub_ is !NULL here since decoder != NULL.

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
File content/common/gpu/image_transport_surface_mac.cc (right):

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
content/common/gpu/image_transport_surface_mac.cc:303: // TODO: implement
Is there anything to do here beyond keeping it around and sending it with the
GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params message?

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
File content/common/gpu/image_transport_surface_win.cc (right):

https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
content/common/gpu/image_transport_surface_win.cc:231: // TODO: implement
Same here, is there anything to do here beyond keeping it around and sending it
with the
GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params message?

https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.cc:111: if (command_buffer_proxy)
{
I think DCHECK is enough. If we're able to send commands through the context, we
have a CommandBufferProxyImpl.

https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.cc:112: context3d()->flush();
nit: shallowFlush

https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.cc:124: if (command_buffer_proxy)
{
just DCHECK

https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
content/renderer/gpu/compositor_output_surface.cc:125: context3d()->flush();
nit: shallowFlush

[jbauman, 2013-04-04 22:18:57]

Ok, fixed.

On 2013/04/03 18:05:09, piman wrote:
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/client...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/client...
> content/common/gpu/client/command_buffer_proxy_impl.cc:206: const
> cc::LatencyInfo& latency_info) {
> You need to check for last_state_.error != gpu::error::kNoError here (common
> pattern in every entry-point that sends a message, see also DCHECK in ::Send).
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
> File content/common/gpu/image_transport_surface.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
> content/common/gpu/image_transport_surface.cc:71: if (stub_) {
> stub_ is !NULL here since decoder != NULL.
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
> File content/common/gpu/image_transport_surface_mac.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
> content/common/gpu/image_transport_surface_mac.cc:303: // TODO: implement
> Is there anything to do here beyond keeping it around and sending it with the
> GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params message?
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
> File content/common/gpu/image_transport_surface_win.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/common/gpu/image_...
> content/common/gpu/image_transport_surface_win.cc:231: // TODO: implement
> Same here, is there anything to do here beyond keeping it around and sending
it
> with the
> GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params message?
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
> File content/renderer/gpu/compositor_output_surface.cc (right):
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
> content/renderer/gpu/compositor_output_surface.cc:111: if
(command_buffer_proxy)
> {
> I think DCHECK is enough. If we're able to send commands through the context,
we
> have a CommandBufferProxyImpl.
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
> content/renderer/gpu/compositor_output_surface.cc:112: context3d()->flush();
> nit: shallowFlush
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
> content/renderer/gpu/compositor_output_surface.cc:124: if
(command_buffer_proxy)
> {
> just DCHECK
> 
>
https://codereview.chromium.org/12614013/diff/49001/content/renderer/gpu/comp...
> content/renderer/gpu/compositor_output_surface.cc:125: context3d()->flush();
> nit: shallowFlush

[piman, 2013-04-04 23:24:32]

LGTM, thanks!

[nduca, 2013-04-08 20:26:23]

lgtm

[jbauman, 2013-04-09 00:12:46]

Justin, could you review the IPC message changes?

[jbauman, 2013-04-12 00:00:29]

On 2013/04/09 00:12:46, jbauman wrote:
> Justin, could you review the IPC message changes?

jschuh@, ping?

[jschuh, 2013-04-12 00:56:57]

Sorry, missed this. Is it just integer metadata for tracing purposes?

[jbauman, 2013-04-12 01:01:09]

On 2013/04/12 00:56:57, Justin Schuh wrote:
> Sorry, missed this. Is it just integer metadata for tracing purposes?

Yeah, pretty much.

[jschuh, 2013-04-12 01:04:32]

okay. ipc security rubberstamp lgtm since then.

[commit-bot: I haz the power, 2013-04-12 02:13:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/12614013/56001

[commit-bot: I haz the power, 2013-04-12 02:13:37]

Failed to apply patch for content/common/cc_messages.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/common/cc_messages.h
  Hunk #1 FAILED at 4.
  Hunk #2 succeeded at 210 (offset -7 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/common/cc_messages.h.rej

Patch:       content/common/cc_messages.h
Index: content/common/cc_messages.h
diff --git a/content/common/cc_messages.h b/content/common/cc_messages.h
index
3cc7b9ee385b58c2227f3914037c492fdb2daa4d..1f16046aed056f253e50b28061afda48d7a6cb58
100644
--- a/content/common/cc_messages.h
+++ b/content/common/cc_messages.h
@@ -4,6 +4,7 @@
 //
 // IPC Messages sent between compositor instances.
 
+#include "cc/debug/latency_info.h"
 #include "cc/layers/video_layer_impl.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/compositor_frame_ack.h"
@@ -217,6 +218,14 @@ IPC_STRUCT_TRAITS_BEGIN(cc::TransferableResource)
   IPC_STRUCT_TRAITS_MEMBER(mailbox)
 IPC_STRUCT_TRAITS_END()
 
+IPC_STRUCT_TRAITS_BEGIN(cc::LatencyInfo)
+  IPC_STRUCT_TRAITS_MEMBER(renderer_main_frame_number)
+  IPC_STRUCT_TRAITS_MEMBER(renderer_impl_frame_number)
+  IPC_STRUCT_TRAITS_MEMBER(browser_main_frame_number)
+  IPC_STRUCT_TRAITS_MEMBER(browser_impl_frame_number)
+  IPC_STRUCT_TRAITS_MEMBER(swap_timestamp)
+IPC_STRUCT_TRAITS_END()
+
 IPC_STRUCT_TRAITS_BEGIN(cc::CompositorFrameMetadata)
   IPC_STRUCT_TRAITS_MEMBER(device_scale_factor)
   IPC_STRUCT_TRAITS_MEMBER(root_scroll_offset)

[commit-bot: I haz the power, 2013-04-12 02:21:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/12614013/73001

[commit-bot: I haz the power, 2013-04-12 06:10:09]

Change committed as 193864