Implement proper synchronization between HW video decode IPC and CommandBuffer.

Implement proper synchronization between HW video decode IPC and CommandBuffer.

This is done by inserting tokens into the command-buffer stream
when synchronization is needed, and adding a
last-read/last-written token pair to each IPC message.  This
allowed me to remove the bogus FinishGL() calls from the gles2
sample pepper plugin.

As part of this CL, the return value for VideoDecodeAccelerator::{Decode,Flush,Abort} changed from bool to void.  These are all async methods so errors ought to be signaled using callbacks.

BUG=none
TEST=gles2 works, no crashes; trybots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90971

[Ami GONE FROM CHROMIUM, 2011-06-27 17:50:29]

brettw: for content/{renderer,common}
piman: for webkit/plugins/ppapi
scherkus: please review it all.

[jam, 2011-06-27 18:21:35]

http://codereview.chromium.org/7260008/diff/5001/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/5001/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:22: // Singly-included section, not converted.
nit: the ", not converted" part is not true.

[Ami GONE FROM CHROMIUM, 2011-06-27 18:43:48]

http://codereview.chromium.org/7260008/diff/5001/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/5001/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:22: // Singly-included section, not converted.
On 2011/06/27 18:21:35, John Abd-El-Malek wrote:
> nit: the ", not converted" part is not true.

Done.  I don't know what that was talking about; I copied the wording from
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/worker...

[scherkus (not reviewing), 2011-06-27 22:54:47]

I haven't fully understood the token stuff, but from a high-level things are
making sense

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:69: return command_buffer_.get() ? 
command_buffer_->GetState().token : 0;
sntyck: worth constant-izing 0 token, or is that the default/reasonable value?

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:96: void AddSetTokenCallback(const
base::Callback<void(int32)>& callback);
IWYU: base/callback.h ?

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:27: typedef std::pair<int32, int32>
ReadWriteTokens;
do we have to use std::pair?

a struct w/ two fields names last_token_read / last_token_written is
self-documenting and avoids having to find this comment

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:30:
&GpuVideoDecodeAccelerator::OnSetToken, base::Unretained(this)));
follow-up from previous question...

if the lifetime of GCBS > GVDA, then do we need to worry about callbacks
executing into this class post-deletion?

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:33:
GpuVideoDecodeAccelerator::~GpuVideoDecodeAccelerator() {}
is it possible to get deleted w/ deferred_messages_ non-empty?

if so we need to delete them pointers

check out base/stl_util-inl.h ;)

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:50: // Note: this
always retries all deferred messages on every token arrival.
s/Note/TODO ?

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:185:
GpuVideoService::GetInstance()->InitializeVideoDecoder(decoder_route_id_);
this call ends up setting video_decode_accelerator_ ?!

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:193:
DCHECK(video_decode_accelerator_.get());
nit: some people are in the camp that DCHECK'ing a pointer immediately before
using it is considered pointless/adds noise to code

I like the security blanket that the DCHECK provides, but I think jam/brettw
will disagree w/ me haha

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.h:110:
GpuCommandBufferStub* stub_;
is this ref'd in anyway or is the lifetime of GCBS > this class?

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_service.cc (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_service.cc:97: #endif  //
defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)
#else NOTIMPLEMENTED() ???

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/omx...
File content/common/gpu/media/omx_video_decode_accelerator_unittest.cc (left):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator_unittest.cc:613:
CHECK(decoder_->Decode(bitstream_buffer));
lol

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:109: if
(MessageLoop::current() != message_loop_) {
I've been cracking down on this stuff.. who isn't calling Decode() [and other
methods] on the right thread?

If so I'd prefer to either:
  1) Force callees to call on right thread
  2) Always post the task

...for the reason being that if/when this code gets shuffled around onto
different threads a formerly asynchronous method call may get turned into a
synchronous method call & vice versa

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:23: class
GpuVideoDecodeAcceleratorHost : public IPC::Channel::Listener,
nit: while you're in here could make this initializer list style?

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:58: std::pair<int32,
int32> SyncTokens();
again we might want to use a struct defined in content/common/

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:103: MessageLoop*
message_loop_;
do we not know which thread inside the renderer process we're supposed to live
on?

http://codereview.chromium.org/7260008/diff/6031/content/renderer/pepper_plat...
File content/renderer/pepper_platform_video_decoder_impl.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.h:26: explicit
PlatformVideoDecoderImpl(
does this need to be explicit?

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:259: // Returns true when command
successfully accepted. Otherwise false.
update docs

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:289: // Returns true when command
successfully accepted. Otherwise false.
update docs

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:297: // Returns true when command
successfully accepted. Otherwise false.
update docs

[Ami GONE FROM CHROMIUM, 2011-06-28 00:06:35]

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:69: return command_buffer_.get() ? 
command_buffer_->GetState().token : 0;
On 2011/06/27 22:54:47, scherkus wrote:
> sntyck: worth constant-izing 0 token, or is that the default/reasonable value?

I am following command_buffer.h:CommandBuffer::State::token's comment that ends
with:
The default token value is zero.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:96: void AddSetTokenCallback(const
base::Callback<void(int32)>& callback);
On 2011/06/27 22:54:47, scherkus wrote:
> IWYU: base/callback.h ?

I'm not U'ing it here, I only need a fwd decl.  But fwd decl'ing concrete
templates like this is a mess, so opting to leave out.  Adding to .cc file
(which needs concrete decl).

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:27: typedef std::pair<int32, int32>
ReadWriteTokens;
On 2011/06/27 22:54:47, scherkus wrote:
> do we have to use std::pair?
> 
> a struct w/ two fields names last_token_read / last_token_written is
> self-documenting and avoids having to find this comment

That's true, but comes at the cost of having to define a custom pickling routine
pair.  I mildly prefer to use the pair with its pre-canned codecs.  Let me know
if you care enough for me to flip it.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:30:
&GpuVideoDecodeAccelerator::OnSetToken, base::Unretained(this)));
On 2011/06/27 22:54:47, scherkus wrote:
> follow-up from previous question...
> 
> if the lifetime of GCBS > GVDA, then do we need to worry about callbacks
> executing into this class post-deletion?

Done.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:33:
GpuVideoDecodeAccelerator::~GpuVideoDecodeAccelerator() {}
On 2011/06/27 22:54:47, scherkus wrote:
> is it possible to get deleted w/ deferred_messages_ non-empty?
> 
> if so we need to delete them pointers
> 
> check out base/stl_util-inl.h ;)

In the glorious future when the BUG is fixed, it will not be possible to end up
in this situation.  Until then I'll add explicit deletion.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:50: // Note: this
always retries all deferred messages on every token arrival.
On 2011/06/27 22:54:47, scherkus wrote:
> s/Note/TODO ?

I'm not sure it's necessary, which is why I didn't TODO it.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:185:
GpuVideoService::GetInstance()->InitializeVideoDecoder(decoder_route_id_);
On 2011/06/27 22:54:47, scherkus wrote:
> this call ends up setting video_decode_accelerator_ ?!

Yes; see the "wonky" comment below.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:193:
DCHECK(video_decode_accelerator_.get());
On 2011/06/27 22:54:47, scherkus wrote:
> nit: some people are in the camp that DCHECK'ing a pointer immediately before
> using it is considered pointless/adds noise to code
> 
> I like the security blanket that the DCHECK provides, but I think jam/brettw
> will disagree w/ me haha

I use it for at least 50% documentation/comment value.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.h:110:
GpuCommandBufferStub* stub_;
On 2011/06/27 22:54:47, scherkus wrote:
> is this ref'd in anyway or is the lifetime of GCBS > this class?

See the BUG comment in gpu_channel.cc.

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_service.cc (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_service.cc:97: #endif  //
defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)
On 2011/06/27 22:54:47, scherkus wrote:
> #else NOTIMPLEMENTED() ???

nice.

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:109: if
(MessageLoop::current() != message_loop_) {
On 2011/06/27 22:54:47, scherkus wrote:
> I've been cracking down on this stuff.. who isn't calling Decode() [and other
> methods] on the right thread?
> 
> If so I'd prefer to either:
>   1) Force callees to call on right thread
>   2) Always post the task
> 
> ...for the reason being that if/when this code gets shuffled around onto
> different threads a formerly asynchronous method call may get turned into a
> synchronous method call & vice versa

These calls are all async by definition; they have callbacks for signalling
completion and/or error.  So I'm not as concerned about that issue in this
interface.  I do believe that there is room for improvement to the threading
situation, but I suspect it will require plumbing SyncChannel through
gpu_{channel,video_service,video_decode_accelerator}_host and auditing their
clients (basically the pepper impl) and would rather not do that here/now.

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:23: class
GpuVideoDecodeAcceleratorHost : public IPC::Channel::Listener,
On 2011/06/27 22:54:47, scherkus wrote:
> nit: while you're in here could make this initializer list style?

Done.

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:58: std::pair<int32,
int32> SyncTokens();
On 2011/06/27 22:54:47, scherkus wrote:
> again we might want to use a struct defined in content/common/

See gpu_messages.h reply.

http://codereview.chromium.org/7260008/diff/6031/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:103: MessageLoop*
message_loop_;
On 2011/06/27 22:54:47, scherkus wrote:
> do we not know which thread inside the renderer process we're supposed to live
> on?

See other response.

http://codereview.chromium.org/7260008/diff/6031/content/renderer/pepper_plat...
File content/renderer/pepper_platform_video_decoder_impl.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.h:26: explicit
PlatformVideoDecoderImpl(
On 2011/06/27 22:54:47, scherkus wrote:
> does this need to be explicit?

Done.

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:259: // Returns true when command
successfully accepted. Otherwise false.
On 2011/06/27 22:54:47, scherkus wrote:
> update docs

Done.

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:289: // Returns true when command
successfully accepted. Otherwise false.
On 2011/06/27 22:54:47, scherkus wrote:
> update docs

Done.

http://codereview.chromium.org/7260008/diff/6031/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:297: // Returns true when command
successfully accepted. Otherwise false.
On 2011/06/27 22:54:47, scherkus wrote:
> update docs

Done.

[piman, 2011-06-28 01:19:22]

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.cc:501:
set_token_callbacks_[i].Run(token);
Can the callback remove itself ? See later comment as to why we may need to
remove.

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:26: // Pair contains: first: last_token_read,
second: last_token_written.
You can make an IPC_STRUCT which implements the pickler for you.

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:30:
&GpuVideoDecodeAccelerator::OnSetToken, base::Unretained(this)));
Do we have lifetime ordering of GpuVideoDecodeAccelerator vs the
GpuCommandBufferStub ? Think compromised renderer or malicious (or simply buggy)
pepper plugin.
If the GpuVideoDecodeAccelerator can be destroyed before the
GpuCommandBufferStub, you need to remove the callback from the stub, or create
the callback through a completion callback factory, or not Unretain.
If the GpuCommandBufferStub can be destroyed before the
GpuVideoDecodeAccelerator, then you need to be able to reset stub_ . You can use
a WeakPointer, but in any case you'll need to check for stub_ below (and
dequeue/cancel messages that reference it as appropriate).
If either (which I think is the case), then you need both.

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:73: case
AcceleratedVideoDecoderMsg_Abort::ID:
There's a few messages in here that I don't think really need a token check,
basically any one of them that doesn't relate to a change in the GL state. For
example:
GetConfigs
Initialize
Decode
AssignSysmemBuffers
Flush
Abort

Am I missing something ?

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:82: return false;
It is worth a mention in gpu_messages.h that the token pair is always expected
as the first parameter of the message.

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:114: }
I've seen this pattern before, and I can't say that I like it. It would be
better to have explicit "DecodeOnIOThread"/"DecodeOnRenderThread" functions,
making sure the Render thread one calls the IO thread one.
Also, posting a task with Unretained(this) seems dangerous at best. What ensures
that this will not go away before the task runs ? Most likely you need to add a
(thread-safe) refcount to GpuVideoDecodeAcceleratorHost and remove Unretained.
Is it even valid to copy bitstream_buffer to another thread ?
Lastly, it seems that the root of the problem is that ipc_sender_ is the raw
channel, instead of the SyncChannel represented by the GpuChannelHost. Wouldn't
it be much simpler to use the latter, and send everything from the Render
thread, skipping thread hopping (already handled by the SyncChannel) ?

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.h (right):

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:94:
gpu::CommandBufferHelper* cmd_buffer_helper_;
Note for later: in the out-of-process plugin case, you won't have a
CommandBufferHelper in the renderer. It lives in the plugin process. You can
punt for now, but you'll need to think about the synchronization in that case.
I'm thinking, rather than specifying explicitly the tokens in each message, you
could have a single WaitForToken message (because all messages are handled in
order_. Then you could expose it as a function in this class, and make the
pepper implementation use it explicitly. In the out-of-process case, it's just
one more API to remote.

In any case, in the in-process case, the plugin could destroy its context (hence
the CommandBufferHelper) before the GpuVideoDecodeAcceleratorHost. You'll need a
way to reset cmd_buffer_helper_, or add a refcount, or something.

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:104: MessageLoop*
message_loop_;
I'm pretty sure using a MessageLoop directly isn't thread-safe. You'll need at
least to use MessageLoopProxy. But see the comments in the .cc file.

http://codereview.chromium.org/7260008/diff/6037/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7260008/diff/6037/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:131: this,
command_buffer_route_id, context3d->gles2_impl()->helper()));
FYI, context3d->gles2_impl() will be NULL in out-of-process (see comments in
other file).

Also, unrelated to this CL, but FYI, you can have a race condition with
command_buffer_route_id. It is really relative to the GpuChannelHost. If the GPU
process crashes/gets killed between the GetCommandBufferRouteId (or even before
that) and the time you create the video decoder, you will spawn a new GPU
process and try to reference a route ID that doesn't exist (or worse, a valid
one but not the one you want). Definitely worth at least a TODO (and a bug).

[scherkus (not reviewing), 2011-06-28 01:22:15]

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:27: typedef std::pair<int32, int32>
ReadWriteTokens;
On 2011/06/28 00:06:35, Ami Fischman wrote:
> On 2011/06/27 22:54:47, scherkus wrote:
> > do we have to use std::pair?
> > 
> > a struct w/ two fields names last_token_read / last_token_written is
> > self-documenting and avoids having to find this comment
> 
> That's true, but comes at the cost of having to define a custom pickling
routine
> pair.  I mildly prefer to use the pair with its pre-canned codecs.  Let me
know
> if you care enough for me to flip it.

If you have an externally declared struct:
struct ReadWriteTokens {
  int32 last_token_read;
  int32 last_token_written;
};

...here you can have:
IPC_STRUCT_TRAITS_BEGIN(ReadWriteTokens)
  IPC_STRUCT_TRAITS_MEMBER(last_token_read)
  IPC_STRUCT_TRAITS_MEMBER(last_token_written)
IPC_STRUCT_TRAITS_END()

...which expands into that pickling stuff for you:
ipc/param_traits_macros.h

Take a look at GPUInfo for an example:
content/common/gpu/gpu_info.h
content/common/gpu/gpu_messages.h

[Ami GONE FROM CHROMIUM, 2011-06-28 21:00:53]

Now with 85% less crack!  

It turns out that instead of trampolining back and forth between I/O and render
threads explicitly, and managing thread-locality of lifecycle, and lots of code,
turning GpuVideoServiceHost from a MessageFilter to just a Listener that
GpuChannelHost::OMR optionally delegates to makes everything "just work" - all
the *Host stuff runs on the render thread (for client invocations & callbacks
firing) and the IO itself is trampolined to/from the IO thread using the
GpuChannelHost's pre-existing SyncChannel.  Yay!

This version has a LOT of defensive DCHECKs that we're on the render thread, b/c
I wanted to make sure.  I'm open to removing some/all of them if you think it's
overkill.

Thanks for the great reviews!

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.cc:501:
set_token_callbacks_[i].Run(token);
On 2011/06/28 01:19:22, piman wrote:
> Can the callback remove itself ? See later comment as to why we may need to
> remove.

See later comment for why I don't think we need this.

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:26: // Pair contains: first: last_token_read,
second: last_token_written.
On 2011/06/28 01:19:22, piman wrote:
> You can make an IPC_STRUCT which implements the pickler for you.

Done.

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:30:
&GpuVideoDecodeAccelerator::OnSetToken, base::Unretained(this)));
On 2011/06/28 01:19:22, piman wrote:
> Do we have lifetime ordering of GpuVideoDecodeAccelerator vs the
> GpuCommandBufferStub ? Think compromised renderer or malicious (or simply
buggy)
> pepper plugin.
> If the GpuVideoDecodeAccelerator can be destroyed before the
> GpuCommandBufferStub, you need to remove the callback from the stub, or create
> the callback through a completion callback factory, or not Unretain.
> If the GpuCommandBufferStub can be destroyed before the
> GpuVideoDecodeAccelerator, then you need to be able to reset stub_ . You can
use
> a WeakPointer, but in any case you'll need to check for stub_ below (and
> dequeue/cancel messages that reference it as appropriate).
> If either (which I think is the case), then you need both.

See the BUG TODO in gpu_channel.cc.  The future answer is that GCBS will own
GVDA.
Currently though GVDA can be torn down before GCBS.  Removed Unretain (which
never had a good reason; I mistakenly thought GVDA was not refcounted).

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:73: case
AcceleratedVideoDecoderMsg_Abort::ID:
On 2011/06/28 01:19:22, piman wrote:
> There's a few messages in here that I don't think really need a token check,
> basically any one of them that doesn't relate to a change in the GL state. For
> example:
> GetConfigs
> Initialize
> Decode
> AssignSysmemBuffers
> Flush
> Abort
> 
> Am I missing something ?

Yeah, it's more complicated because this isn't just a glFinish(), it's a sync w/
all things that flow through the commandbuffer.
For example Decode needs to have valid textures to decode into.
But textures aren't valid until the CB is flushed (id's are passed to the plugin
before they're actually allocated in the gpu process, as soon as the request to
create the texture is inserted into the CB).
Similarly others.

GetConfigs, Initialize, and AssignSysmemBuffers are the only ones I think are
likely to keep not needing sync, but the first two only get called once (so sync
is cheap) and the last is headed to the trashbin of history, so I opted for
uniformity.

http://codereview.chromium.org/7260008/diff/6037/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.cc:82: return false;
On 2011/06/28 01:19:22, piman wrote:
> It is worth a mention in gpu_messages.h that the token pair is always expected
> as the first parameter of the message.

Done.

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:114: }
On 2011/06/28 01:19:22, piman wrote:
> I've seen this pattern before, and I can't say that I like it. It would be
> better to have explicit "DecodeOnIOThread"/"DecodeOnRenderThread" functions,
> making sure the Render thread one calls the IO thread one.
> Also, posting a task with Unretained(this) seems dangerous at best. What
ensures
> that this will not go away before the task runs ? Most likely you need to add
a
> (thread-safe) refcount to GpuVideoDecodeAcceleratorHost and remove Unretained.
> Is it even valid to copy bitstream_buffer to another thread ?
> Lastly, it seems that the root of the problem is that ipc_sender_ is the raw
> channel, instead of the SyncChannel represented by the GpuChannelHost.
Wouldn't
> it be much simpler to use the latter, and send everything from the Render
> thread, skipping thread hopping (already handled by the SyncChannel) ?


As you tried to explain to me on friday, but I didn't actually grok until this
morning, the way the code was set up here was insane.  We were taking extra
pains during construction to situate the GVDAH on the IO thread, and
trampolining all the Send()'s to the IO thread (b/c we expect to be called from
the render thread, b/c what else could a plugin do?), *and* trampolining all the
callbacks from the IO thread to the render thread (b/c where else could the
plugin expect to receive them?).  That's basically reimplementing what
SyncChannel/ChannelProxy is for, and GpuChannelHost already has one of them.  So
instead, I ripped out all of this explicit thread-hopping and replaced with
using GpuChannelHost's SyncChannel.

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.h (right):

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:94:
gpu::CommandBufferHelper* cmd_buffer_helper_;
On 2011/06/28 01:19:22, piman wrote:
> Note for later: in the out-of-process plugin case, you won't have a
> CommandBufferHelper in the renderer. It lives in the plugin process. You can
> punt for now, but you'll need to think about the synchronization in that case.
> I'm thinking, rather than specifying explicitly the tokens in each message,
you
> could have a single WaitForToken message (because all messages are handled in
> order_. Then you could expose it as a function in this class, and make the
> pepper implementation use it explicitly. In the out-of-process case, it's just
> one more API to remote.
> 
> In any case, in the in-process case, the plugin could destroy its context
(hence
> the CommandBufferHelper) before the GpuVideoDecodeAcceleratorHost. You'll need
a
> way to reset cmd_buffer_helper_, or add a refcount, or something.

Done.

http://codereview.chromium.org/7260008/diff/6037/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:104: MessageLoop*
message_loop_;
On 2011/06/28 01:19:22, piman wrote:
> I'm pretty sure using a MessageLoop directly isn't thread-safe. You'll need at
> least to use MessageLoopProxy. But see the comments in the .cc file.

Done.

http://codereview.chromium.org/7260008/diff/6037/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7260008/diff/6037/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:131: this,
command_buffer_route_id, context3d->gles2_impl()->helper()));
On 2011/06/28 01:19:22, piman wrote:
> FYI, context3d->gles2_impl() will be NULL in out-of-process (see comments in
> other file).
> 
> Also, unrelated to this CL, but FYI, you can have a race condition with
> command_buffer_route_id. It is really relative to the GpuChannelHost. If the
GPU
> process crashes/gets killed between the GetCommandBufferRouteId (or even
before
> that) and the time you create the video decoder, you will spawn a new GPU
> process and try to reference a route ID that doesn't exist (or worse, a valid
> one but not the one you want). Definitely worth at least a TODO (and a bug).

Done.

[Ami GONE FROM CHROMIUM, 2011-06-28 21:02:31]

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6031/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:27: typedef std::pair<int32, int32>
ReadWriteTokens;
On 2011/06/28 01:22:15, scherkus wrote:
> On 2011/06/28 00:06:35, Ami Fischman wrote:
> > On 2011/06/27 22:54:47, scherkus wrote:
> > > do we have to use std::pair?
> > > 
> > > a struct w/ two fields names last_token_read / last_token_written is
> > > self-documenting and avoids having to find this comment
> > 
> > That's true, but comes at the cost of having to define a custom pickling
> routine
> > pair.  I mildly prefer to use the pair with its pre-canned codecs.  Let me
> know
> > if you care enough for me to flip it.
> 
> If you have an externally declared struct:
> struct ReadWriteTokens {
>   int32 last_token_read;
>   int32 last_token_written;
> };
> 
> ...here you can have:
> IPC_STRUCT_TRAITS_BEGIN(ReadWriteTokens)
>   IPC_STRUCT_TRAITS_MEMBER(last_token_read)
>   IPC_STRUCT_TRAITS_MEMBER(last_token_written)
> IPC_STRUCT_TRAITS_END()
> 
> ...which expands into that pickling stuff for you:
> ipc/param_traits_macros.h
> 
> Take a look at GPUInfo for an example:
> content/common/gpu/gpu_info.h
> content/common/gpu/gpu_messages.h

Done.

[piman, 2011-06-28 21:24:41]

LGTM+nit, thanks for the cleanup it looks great.

http://codereview.chromium.org/7260008/diff/6040/gpu/command_buffer/common/co...
File gpu/command_buffer/common/command_buffer.h (right):

http://codereview.chromium.org/7260008/diff/6040/gpu/command_buffer/common/co...
gpu/command_buffer/common/command_buffer.h:140: // This want to be private (and
const) but can't in order to support pickling.
No need for this comment... coding style says that structs should have public
members. This seems perfectly fair to be public.

[Ami GONE FROM CHROMIUM, 2011-06-28 21:27:38]

http://codereview.chromium.org/7260008/diff/6040/gpu/command_buffer/common/co...
File gpu/command_buffer/common/command_buffer.h (right):

http://codereview.chromium.org/7260008/diff/6040/gpu/command_buffer/common/co...
gpu/command_buffer/common/command_buffer.h:140: // This want to be private (and
const) but can't in order to support pickling.
On 2011/06/28 21:24:41, piman wrote:
> No need for this comment... coding style says that structs should have public
> members. This seems perfectly fair to be public.

My reason for the comment is that this isn't used as a struct - nobody outside
this class other than pickle code should be reading or writing these members;
instead they should be using InRange().

[scherkus (not reviewing), 2011-06-28 22:02:06]

LGTM w/ misc nits

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:69: return command_buffer_.get() ? 
command_buffer_->GetState().token : 0;
two spaces -> one

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:102:
IPC_STRUCT_TRAITS_BEGIN(gpu::ReadWriteTokens)
HOT

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.h:75: const
gpu::ReadWriteTokens& /* tokens */,
do you still need all the /* tokens */ or can we just have tokens sans-comment?

I was under the impression you had the /* tokens */ because we had
std::pair<int,int> but if that wasn't the case then ignore

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:37:
DCHECK_EQ(RenderThread::current()->message_loop(), MessageLoop::current());
ok this is a bit of a nit but:

RenderThread::current() uses TLS to store that value as does
MessageLoop::current()

Ergo, if RenderThread::current() != NULL, then we should be on the RenderThread

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_service_host.cc (right):

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_service_host.cc:12:
GpuVideoServiceHost::GpuVideoServiceHost()
nit: if the object is created/destroyed on same thread AND has all calls on same
thread, I believe you can inherit from NonThreadSafe<T> and after asserting in
the ctor that yes, this is indeed the render thread, you can use
DCHECK(CalledOnValidThread())

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_service_host.cc:15:
DCHECK_EQ(RenderThread::current()->message_loop(), MessageLoop::current());
ditto

[scherkus (not reviewing), 2011-06-28 22:02:07]

LGTM w/ misc nits

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:69: return command_buffer_.get() ? 
command_buffer_->GetState().token : 0;
two spaces -> one

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:102:
IPC_STRUCT_TRAITS_BEGIN(gpu::ReadWriteTokens)
HOT

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.h:75: const
gpu::ReadWriteTokens& /* tokens */,
do you still need all the /* tokens */ or can we just have tokens sans-comment?

I was under the impression you had the /* tokens */ because we had
std::pair<int,int> but if that wasn't the case then ignore

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:37:
DCHECK_EQ(RenderThread::current()->message_loop(), MessageLoop::current());
ok this is a bit of a nit but:

RenderThread::current() uses TLS to store that value as does
MessageLoop::current()

Ergo, if RenderThread::current() != NULL, then we should be on the RenderThread

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_service_host.cc (right):

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_service_host.cc:12:
GpuVideoServiceHost::GpuVideoServiceHost()
nit: if the object is created/destroyed on same thread AND has all calls on same
thread, I believe you can inherit from NonThreadSafe<T> and after asserting in
the ctor that yes, this is indeed the render thread, you can use
DCHECK(CalledOnValidThread())

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_service_host.cc:15:
DCHECK_EQ(RenderThread::current()->message_loop(), MessageLoop::current());
ditto

[Ami GONE FROM CHROMIUM, 2011-06-28 22:25:06]

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:69: return command_buffer_.get() ? 
command_buffer_->GetState().token : 0;
On 2011/06/28 22:02:07, scherkus wrote:
> two spaces -> one

Done.

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/media/gpu...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

http://codereview.chromium.org/7260008/diff/6040/content/common/gpu/media/gpu...
content/common/gpu/media/gpu_video_decode_accelerator.h:75: const
gpu::ReadWriteTokens& /* tokens */,
On 2011/06/28 22:02:07, scherkus wrote:
> do you still need all the /* tokens */ or can we just have tokens
sans-comment?
> 
> I was under the impression you had the /* tokens */ because we had
> std::pair<int,int> but if that wasn't the case then ignore

I put the param name as comment when the param is not used inside the method. 
Same convention as _ignored suffix, but less vulnerable to bitrot.

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.cc:37:
DCHECK_EQ(RenderThread::current()->message_loop(), MessageLoop::current());
On 2011/06/28 22:02:07, scherkus wrote:
> ok this is a bit of a nit but:
> 
> RenderThread::current() uses TLS to store that value as does
> MessageLoop::current()
> 
> Ergo, if RenderThread::current() != NULL, then we should be on the
RenderThread

Oh boy.  That's pretty exciting (and undocumented).  The TODO on render_thread.h
makes me want to both NULL check & compare (b/c the NULL behavior may change). 
Done.

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_service_host.cc (right):

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_service_host.cc:12:
GpuVideoServiceHost::GpuVideoServiceHost()
On 2011/06/28 22:02:07, scherkus wrote:
> nit: if the object is created/destroyed on same thread AND has all calls on
same
> thread, I believe you can inherit from NonThreadSafe<T> and after asserting in
> the ctor that yes, this is indeed the render thread, you can use
> DCHECK(CalledOnValidThread())

Done.

http://codereview.chromium.org/7260008/diff/6040/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_service_host.cc:15:
DCHECK_EQ(RenderThread::current()->message_loop(), MessageLoop::current());
On 2011/06/28 22:02:07, scherkus wrote:
> ditto

Done.

[brettw, 2011-06-29 17:28:55]

What part of this do you need me to look at?

[Ami GONE FROM CHROMIUM, 2011-06-29 17:30:29]

On 2011/06/29 17:28:55, brettw wrote:
> What part of this do you need me to look at?

content/{common,renderer} for OWNERS.

[brettw, 2011-06-29 17:37:28]

LGTM (mostly rubber stamp)