gpu: Record GL state to trace

gpu: Record GL state to trace

This patch adds a GL state tracer for recording information about the
context state as trace event objects. This initial version only records
a screenshot of the framebuffer contents at every SwapBuffers.

Note that because the saving this information can be expensive, the new
trace events are added under the "disabled-by-default-gpu.debug"
category.

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

[Sami, 2013-09-09 12:56:40]

PTAL. N.B. I opted not to do async readpixels to keep the memory use bounded.

[dsinclair, 2013-09-09 14:25:08]

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_state_tracer.cc (right):

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:77: return false;
Is this something that happens commonly? Should we DCHECK here?

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:79: base::StringPiece
base64_input(reinterpret_cast<const char*>(&png_data[0]),
Should the conversion of the png_data happen here or in AppendAsTraceFormat? As
currently written you'll incur the cost of converting to base64 immediately. If
you moved it into the AppendAsTraceFormat you'd incur the cost when tracing is
finished.

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:100:
TRACE_DISABLED_BY_DEFAULT("gpu.debug"), "gpu::State", this);
Why 'this' and not 'state'?

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:109: bool is_tracing;
Would it make sense to move the CATEGORY_GROUP_ENABLED check out to the
callsite? Then, we would not incur the cost of calling into the TakeSnapshot
method.

[Sami, 2013-09-09 16:32:15]

Thanks Dan. Another look, please?

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_state_tracer.cc (right):

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:77: return false;
On 2013/09/09 14:25:08, dsinclair wrote:
> Is this something that happens commonly? Should we DCHECK here?

It shouldn't really happen unless we're really out of memory, so a DCHECK is
probably sufficient.

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:79: base::StringPiece
base64_input(reinterpret_cast<const char*>(&png_data[0]),
On 2013/09/09 14:25:08, dsinclair wrote:
> Should the conversion of the png_data happen here or in AppendAsTraceFormat?
As
> currently written you'll incur the cost of converting to base64 immediately.
If
> you moved it into the AppendAsTraceFormat you'd incur the cost when tracing is
> finished.

I first did it this way to avoid keeping too many snapshots in memory. However I
just did a couple of tests with deferred encoding and it seems to work fine --
and the framerate is much better. So thanks for the idea :)

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:100:
TRACE_DISABLED_BY_DEFAULT("gpu.debug"), "gpu::State", this);
On 2013/09/09 14:25:08, dsinclair wrote:
> Why 'this' and not 'state'?

Done. Had to change base/debug/trace_event.h to accept a const pointer here
though.

https://codereview.chromium.org/23926013/diff/4001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:109: bool is_tracing;
On 2013/09/09 14:25:08, dsinclair wrote:
> Would it make sense to move the CATEGORY_GROUP_ENABLED check out to the
> callsite? Then, we would not incur the cost of calling into the TakeSnapshot
> method.

Yeah, I can see calls to TakeSnapshot() getting added in various places that
can't tolerate the extra cost.

[dsinclair, 2013-09-09 16:55:39]

lgtm with nit.

https://codereview.chromium.org/23926013/diff/7001/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_state_tracer.cc (right):

https://codereview.chromium.org/23926013/diff/7001/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_state_tracer.cc:117:
"GPUStateTrace::TakeSnapshot");
nit: GPUStateTracer

[piman, 2013-09-09 21:25:54]

https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8585:
gpu_state_tracer_->TakeSnapshot();
A non-default framebuffer could be bound when SwapBuffers is called, meaning
glReadPixels in TakeSnapshot will not grab what you expect.

https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8585:
gpu_state_tracer_->TakeSnapshot();
Could we also do this in HandlePostSubBufferCHROMIUM?

Also, another suggestion is to add a command that would grab the snapshot of the
currently bound framebuffer. In the composite-to-mailbox case, we won't call
SwapBuffers/PostSubBuffers, but it might still be useful to grab the renderer
frame. WDYT?

https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_state_tracer.cc (right):

https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_state_tracer.cc:51:
gfx::Size(state_->viewport_width, state_->viewport_height);
You're dependent on the current GL state, which may not match at all the default
(or current) framebuffer size. Should we keep track of the default framebuffer
size (via HandleResizeCHROMIUM)?

https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_state_tracer.cc:77:
(screenshot_viewport_.height() - y - 1) * bytes_per_row + x]);
Is this why you need screenshot_pixels_ to be mutable? Can't we do this in
SaveScreenshot instead?
Even though we probably don't do that, if we called AppendAsTraceFormat twice,
we'd get different results.

[Sami, 2013-09-10 12:55:49]

On 2013/09/09 21:25:54, piman wrote:
>
https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:8585:
> gpu_state_tracer_->TakeSnapshot();
> A non-default framebuffer could be bound when SwapBuffers is called, meaning
> glReadPixels in TakeSnapshot will not grab what you expect.

Great point. I'm now taking a snapshot of the default fbo.
 
>
https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:8585:
> gpu_state_tracer_->TakeSnapshot();
> Could we also do this in HandlePostSubBufferCHROMIUM?

Done.
 
> Also, another suggestion is to add a command that would grab the snapshot of
the
> currently bound framebuffer. In the composite-to-mailbox case, we won't call
> SwapBuffers/PostSubBuffers, but it might still be useful to grab the renderer
> frame. WDYT?

Yeah, that would be very useful. I've now made the state tracker take a snapshot
of the current fbo. I can add the new command for that (strawman:
glTakeStateSnapshotCHROMIUM()?) in another patch.

>
https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gpu_state_tracer.cc (right):
> 
>
https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gpu_state_tracer.cc:51:
> gfx::Size(state_->viewport_width, state_->viewport_height);
> You're dependent on the current GL state, which may not match at all the
default
> (or current) framebuffer size. Should we keep track of the default framebuffer
> size (via HandleResizeCHROMIUM)?

Is it enough to look at surface_->GetSize() or offscreen_size_?
 
>
https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gpu_state_tracer.cc:77:
> (screenshot_viewport_.height() - y - 1) * bytes_per_row + x]);
> Is this why you need screenshot_pixels_ to be mutable? Can't we do this in
> SaveScreenshot instead?
> Even though we probably don't do that, if we called AppendAsTraceFormat twice,
> we'd get different results.

Right, this is cheap enough to do while we're taking the screenshot so I've
moved it there.

[piman, 2013-09-10 17:40:08]

One last thing.

https://codereview.chromium.org/23926013/diff/19001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/23926013/diff/19001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6994: ScopedFrameBufferBinder
binder(this, 0);
you want GetBackbufferServiceId() rather than 0, to handle both the offscreen
case, and the TextureImageTransportSurface/mac case (where the surface provides
a FBO to use instead of the default one).

[piman, 2013-09-10 17:41:19]

On Tue, Sep 10, 2013 at 5:55 AM, <skyostil@chromium.org> wrote:

> On 2013/09/09 21:25:54, piman wrote:
>
> https://codereview.chromium.**org/23926013/diff/15001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc<https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/service/gles2_cmd_decoder.cc>
>
>> File gpu/command_buffer/service/**gles2_cmd_decoder.cc (right):
>>
>
>
> https://codereview.chromium.**org/23926013/diff/15001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode8585<https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode8585>
>
>> gpu/command_buffer/service/**gles2_cmd_decoder.cc:8585:
>> gpu_state_tracer_->**TakeSnapshot();
>> A non-default framebuffer could be bound when SwapBuffers is called,
>> meaning
>> glReadPixels in TakeSnapshot will not grab what you expect.
>>
>
> Great point. I'm now taking a snapshot of the default fbo.
>
>
>
> https://codereview.chromium.**org/23926013/diff/15001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode8585<https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode8585>
>
>> gpu/command_buffer/service/**gles2_cmd_decoder.cc:8585:
>> gpu_state_tracer_->**TakeSnapshot();
>> Could we also do this in HandlePostSubBufferCHROMIUM?
>>
>
> Done.
>
>
>  Also, another suggestion is to add a command that would grab the snapshot
>> of
>>
> the
>
>> currently bound framebuffer. In the composite-to-mailbox case, we won't
>> call
>> SwapBuffers/PostSubBuffers, but it might still be useful to grab the
>> renderer
>> frame. WDYT?
>>
>
> Yeah, that would be very useful. I've now made the state tracker take a
> snapshot
> of the current fbo. I can add the new command for that (strawman:
> glTakeStateSnapshotCHROMIUM()?**) in another patch.


SGTM, thanks!

>
>
>
> https://codereview.chromium.**org/23926013/diff/15001/gpu/**
>
command_buffer/service/gpu_**state_tracer.cc<https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/service/gpu_state_tracer.cc>
>
>> File gpu/command_buffer/service/**gpu_state_tracer.cc (right):
>>
>
>
> https://codereview.chromium.**org/23926013/diff/15001/gpu/**
>
command_buffer/service/gpu_**state_tracer.cc#newcode51<https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/service/gpu_state_tracer.cc#newcode51>
>
>> gpu/command_buffer/service/**gpu_state_tracer.cc:51:
>> gfx::Size(state_->viewport_**width, state_->viewport_height);
>> You're dependent on the current GL state, which may not match at all the
>>
> default
>
>> (or current) framebuffer size. Should we keep track of the default
>> framebuffer
>> size (via HandleResizeCHROMIUM)?
>>
>
> Is it enough to look at surface_->GetSize() or offscreen_size_?


Yes, I think that's right.


>  https://codereview.chromium.**org/23926013/diff/15001/gpu/**
>
command_buffer/service/gpu_**state_tracer.cc#newcode77<https://codereview.chromium.org/23926013/diff/15001/gpu/command_buffer/service/gpu_state_tracer.cc#newcode77>
>
>> gpu/command_buffer/service/**gpu_state_tracer.cc:77:
>> (screenshot_viewport_.height() - y - 1) * bytes_per_row + x]);
>> Is this why you need screenshot_pixels_ to be mutable? Can't we do this in
>> SaveScreenshot instead?
>> Even though we probably don't do that, if we called AppendAsTraceFormat
>> twice,
>> we'd get different results.
>>
>
> Right, this is cheap enough to do while we're taking the screenshot so I've
> moved it there.
>
>
https://codereview.chromium.**org/23926013/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sami, 2013-09-10 17:45:28]

https://codereview.chromium.org/23926013/diff/19001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/23926013/diff/19001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6994: ScopedFrameBufferBinder
binder(this, 0);
On 2013/09/10 17:40:08, piman wrote:
> you want GetBackbufferServiceId() rather than 0, to handle both the offscreen
> case, and the TextureImageTransportSurface/mac case (where the surface
provides
> a FBO to use instead of the default one).

Great point, done.

[Sami, 2013-09-11 17:53:38]

piman@, did you have some more comments here?

[piman, 2013-09-11 22:04:58]

Oh, sorry, LGTM.

[commit-bot: I haz the power, 2013-09-11 22:17:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/23926013/27001

[commit-bot: I haz the power, 2013-09-11 22:44:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-12 10:11:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/23926013/52001

[commit-bot: I haz the power, 2013-09-12 18:05:15]

Change committed as 222812