Run VDA::Decode on GPU IO thread if VDA supports it.

Run VDA::Decode on GPU IO thread if VDA supports it.

Sometimes GPU main thread is very busy and VDA::Decode is not
run immediately on the main thread. Using MessageFilter to
run VDA::Decode directly on IO thread can reduce decode
latency by around 80ms on various CrOS devices.

BUG=170345
TEST=Try http://apprtc.appspot.com/?debug=loopback on Chromebook Daisy.
     Run local hardware-accelerated video on Daisy and Link.
     Run video_decode_accelerator_unittest on Daisy.

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

[wuchengli, 2013-08-22 14:27:27]

Please take a look. Let me know if there is any question. If the direction looks
good, next I'll deal with three other VDA and add a lock to protect
|video_decode_accelerator_| in GpuVideoDecodeAccelerator.

[Ami GONE FROM CHROMIUM, 2013-08-22 14:52:03]

This looks right to me.  

I'd like to also get piman@'s eyes on this CL, but he's OOO this week so you
probably don't need to wait for him.

https://codereview.chromium.org/23125014/diff/8001/content/common/gpu/media/e...
File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/8001/content/common/gpu/media/e...
content/common/gpu/media/exynos_video_decode_accelerator.cc:418:
DCHECK(!child_message_loop_proxy_->BelongsToCurrentThread());
Better to affirmatively check being on IO thread.

https://codereview.chromium.org/23125014/diff/8001/media/video/video_decode_a...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/8001/media/video/video_decode_a...
media/video/video_decode_accelerator.h:95: // bitstream buffer id. In GPU
process, this runs on IO thread.
This is a strange comment considering processes & threads are not mentioned
otherwise in this file.  I'd just drop it.

[wuchengli, 2013-08-23 15:43:10]

Not ready for review yet. I need some more time to understand the code. I tested
again and the decode latency did improve a lot.

[wuchengli, 2013-08-26 14:59:21]

This is ready for review. PTAL.

[Ami GONE FROM CHROMIUM, 2013-08-26 17:20:03]

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.h:89: scoped_refptr<base::MessageLoopProxy>
io_message_loop() const {
Since this is a process-wide singleton, it would make more sense to grab this
from someplace like ChildProcess::current()->io_message_loop_proxy() in the impl
(instead of plumbing it through to all ctors).

Doing that highlights that the VDAs now depend on an IO thread running, which
makes me wonder whether vdatest satisfies that dependency.  
But this CL doesn't update vdatest!  You should probably do that, and verify
that everything still works :)

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/android_video_decode_accelerator.cc:333: if
(!child_message_loop_->BelongsToCurrentThread) {
Can you add a TODO to make this safe from the IO thread to each of the VDA impls
you added this shim to?

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:62:
GvdaMessageFilter(GpuVideoDecodeAccelerator* gvda,
virtual dtor please

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:62:
GvdaMessageFilter(GpuVideoDecodeAccelerator* gvda,
s/gvda/owner/ here and in the member declaration at l.70 would make the comment
unnecessary. 

BUT, it is also misleading, because while GVDA "owns" the filter, it is actually
the filter's OnFilterRemoved method that deletes the GVDA.  The comment in
OnDestroy is good, but I think a further comment to this effect on (the newly
renamed) owner_ would also be good.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:64:
scoped_refptr<base::MessageLoopProxy> message_loop);
"message_loop" is an uninformative name.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:67: void
OnDecode(base::SharedMemoryHandle handle, int32 id, uint32 size);
private?

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.h:62: class
GvdaMessageFilter;
Since this is scoped inside the class no need for "Gvda" in the name.

[piman, 2013-08-26 20:04:42]

We don't want to run things that may take a long time or block on the IO thread.
IO thread is meant to be super responsive, some things rely on that.

When doing Decode on the main thread, you experience latency because the main
thread is busy with some things, but it may be reasonable to run it on a
dedicated thread (that's not the IO thread?)

[wuchengli, 2013-08-27 01:34:40]

On 2013/08/26 20:04:42, piman wrote:
> We don't want to run things that may take a long time or block on the IO
thread.
> IO thread is meant to be super responsive, some things rely on that.
Is it fast enough if Decode just trampolines to the decoding thread in EVDA or
VaapiVDA?
EVDA and VaapiVDA already have their dedicated decoding thread.
> 
> When doing Decode on the main thread, you experience latency because the main
> thread is busy with some things, but it may be reasonable to run it on a
> dedicated thread (that's not the IO thread?)
I'll see how to do that today.

[Ami GONE FROM CHROMIUM, 2013-08-27 02:57:47]

piman: I think the work here is just a trampoline so the io thread isn't
being burdened by anything it wouldn't be doing anyway.  I don't think we
want to add a thread to evda or vavda for this.
(Dxva I don't remember enough to talk about without looking, but wuchengli
made this a noop there)

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

[piman, 2013-08-27 04:01:20]

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/android_video_decode_accelerator.cc:333: if
(!child_message_loop_->BelongsToCurrentThread) {
You're missing the () to BelongsToCurrentThread().

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/dxva_video_decode_accelerator.cc:517: if
(!child_message_loop_->BelongsToCurrentThread) {
You're missing the () to BelongsToCurrentThread().

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.cc:426: if
(!bitstream_record->shm->Map(bitstream_buffer.size())) {
This can take a non-0 time. Can you move it to the decoder thread?

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:424:
RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(bitstream_buffer.size()),
The Map() can take a long time. Can you move it to the decoding thread?

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:427: base::AutoLock
auto_lock(lock_);
What else takes this lock? Can it be held for a long time?

[piman, 2013-08-27 04:03:53]

On 2013/08/27 02:57:47, Ami Fischman wrote:
> piman: I think the work here is just a trampoline so the io thread isn't
> being burdened by anything it wouldn't be doing anyway.  I don't think we
> want to add a thread to evda or vavda for this.

Ok, agreed, provided we can address the other comments.

> (Dxva I don't remember enough to talk about without looking, but wuchengli
> made this a noop there)

Would it make sense to make GVDA do the bump back to the UI thread unless the
individual decoder opts in (e.g. through a virtual bool CanDecodeOnIOThread() )
once they're ready? I worry the next *vda is going to start doing things on the
IO thread and block things.

[wuchengli, 2013-08-27 04:10:59]

On 2013/08/27 04:03:53, piman wrote:
> On 2013/08/27 02:57:47, Ami Fischman wrote:
> > piman: I think the work here is just a trampoline so the io thread isn't
> > being burdened by anything it wouldn't be doing anyway.  I don't think we
> > want to add a thread to evda or vavda for this.
> 
> Ok, agreed, provided we can address the other comments.
> 
> > (Dxva I don't remember enough to talk about without looking, but wuchengli
> > made this a noop there)
> 
> Would it make sense to make GVDA do the bump back to the UI thread unless the
> individual decoder opts in (e.g. through a virtual bool CanDecodeOnIOThread()
)
> once they're ready? I worry the next *vda is going to start doing things on
the
> IO thread and block things.
Yes. I also thought about this and I think it's good. If we add
CanDecodeOnIOThread, android VDA and DVDA won't need to register
message filter. It's just CanDecodeOnIOThread only makes sense
in GPU process and won't be used in renderer process. I'll update
the CL and you can see if it looks better.

[piman, 2013-08-27 04:21:07]

On Mon, Aug 26, 2013 at 9:11 PM, <wuchengli@chromium.org> wrote:

> On 2013/08/27 04:03:53, piman wrote:
>
>> On 2013/08/27 02:57:47, Ami Fischman wrote:
>> > piman: I think the work here is just a trampoline so the io thread isn't
>> > being burdened by anything it wouldn't be doing anyway.  I don't think
>> we
>> > want to add a thread to evda or vavda for this.
>>
>
>  Ok, agreed, provided we can address the other comments.
>>
>
>  > (Dxva I don't remember enough to talk about without looking, but
>> wuchengli
>> > made this a noop there)
>>
>
>  Would it make sense to make GVDA do the bump back to the UI thread unless
>> the
>> individual decoder opts in (e.g. through a virtual bool
>> CanDecodeOnIOThread()
>>
> )
>
>> once they're ready? I worry the next *vda is going to start doing things
>> on
>>
> the
>
>> IO thread and block things.
>>
> Yes. I also thought about this and I think it's good. If we add
> CanDecodeOnIOThread, android VDA and DVDA won't need to register
> message filter. It's just CanDecodeOnIOThread only makes sense
> in GPU process and won't be used in renderer process.


For that part, you can always add an interface that's only implemented by
the decoder implementations in the GPU process.


> I'll update
> the CL and you can see if it looks better.
>
>
>
https://codereview.chromium.**org/23125014/<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.

[Ami GONE FROM CHROMIUM, 2013-08-27 05:16:45]

Plan sgtm

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

[wuchengli, 2013-08-27 12:32:24]

All done. PTAL.

I ran the video_decode_accelerator_unittest without options ( without my CL). It
crashed at CHECK(x_display_) in RenderingHelperGL::Initialize. I'll ask other
people how to run the test tomorrow.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.h:89: scoped_refptr<base::MessageLoopProxy>
io_message_loop() const {
On 2013/08/26 17:20:03, Ami Fischman wrote:
> Since this is a process-wide singleton, it would make more sense to grab this
> from someplace like ChildProcess::current()->io_message_loop_proxy() in the
impl
> (instead of plumbing it through to all ctors).
GpuChannelManager manages more than one GpuChannel. GpuChannelManager is a
process-wide singleton, but GpuChannel is not. Do you still think ctor
io_message_loop of GpuChannelManager and GpuChannel should be removed?
> 
> Doing that highlights that the VDAs now depend on an IO thread running, which
> makes me wonder whether vdatest satisfies that dependency.  
> But this CL doesn't update vdatest!  You should probably do that, and verify
> that everything still works :)
I'll test vdatest.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/android_video_decode_accelerator.cc:333: if
(!child_message_loop_->BelongsToCurrentThread) {
On 2013/08/27 04:01:20, piman wrote:
> You're missing the () to BelongsToCurrentThread().
This part of change has been reverted.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/android_video_decode_accelerator.cc:333: if
(!child_message_loop_->BelongsToCurrentThread) {
On 2013/08/26 17:20:03, Ami Fischman wrote:
> Can you add a TODO to make this safe from the IO thread to each of the VDA
impls
> you added this shim to?
I added CanDecodeOnIOThread and this part of code has been reverted.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/dxva_video_decode_accelerator.cc:517: if
(!child_message_loop_->BelongsToCurrentThread) {
On 2013/08/27 04:01:20, piman wrote:
> You're missing the () to BelongsToCurrentThread().
Embarrassing... This part is reverted because of CanDecodeOnIOThread.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.cc:426: if
(!bitstream_record->shm->Map(bitstream_buffer.size())) {
On 2013/08/27 04:01:20, piman wrote:
> This can take a non-0 time. Can you move it to the decoder thread?

Done.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:62:
GvdaMessageFilter(GpuVideoDecodeAccelerator* gvda,
On 2013/08/26 17:20:03, Ami Fischman wrote:
> virtual dtor please
Done.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:62:
GvdaMessageFilter(GpuVideoDecodeAccelerator* gvda,
On 2013/08/26 17:20:03, Ami Fischman wrote:
> s/gvda/owner/ here and in the member declaration at l.70 would make the
comment
> unnecessary. 
> 
> BUT, it is also misleading, because while GVDA "owns" the filter, it is
actually
> the filter's OnFilterRemoved method that deletes the GVDA.  The comment in
> OnDestroy is good, but I think a further comment to this effect on (the newly
> renamed) owner_ would also be good.

Done.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:64:
scoped_refptr<base::MessageLoopProxy> message_loop);
On 2013/08/26 17:20:03, Ami Fischman wrote:
> "message_loop" is an uninformative name.
Changed to child_message_loop.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:67: void
OnDecode(base::SharedMemoryHandle handle, int32 id, uint32 size);
On 2013/08/26 17:20:03, Ami Fischman wrote:
> private?
Done.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.h:62: class
GvdaMessageFilter;
On 2013/08/26 17:20:03, Ami Fischman wrote:
> Since this is scoped inside the class no need for "Gvda" in the name.

Done.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:424:
RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(bitstream_buffer.size()),
On 2013/08/27 04:01:20, piman wrote:
> The Map() can take a long time. Can you move it to the decoding thread?
I reverted this and let CanDecodeOnIOThread return false for VaapiVDA.

https://codereview.chromium.org/23125014/diff/16001/content/common/gpu/media/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:427: base::AutoLock
auto_lock(lock_);
On 2013/08/27 04:01:20, piman wrote:
> What else takes this lock? Can it be held for a long time?
I reverted this and let CanDecodeOnIOThread return false for VaapiVDA.

[Ami GONE FROM CHROMIUM, 2013-08-27 17:10:32]

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.h:89: scoped_refptr<base::MessageLoopProxy>
io_message_loop() const {
On 2013/08/27 12:32:24, wuchengli wrote:
> On 2013/08/26 17:20:03, Ami Fischman wrote:
> > Since this is a process-wide singleton, it would make more sense to grab
this
> > from someplace like ChildProcess::current()->io_message_loop_proxy() in the
impl
> > (instead of plumbing it through to all ctors).
> GpuChannelManager manages more than one GpuChannel. GpuChannelManager is a
> process-wide singleton, but GpuChannel is not. Do you still think ctor
> io_message_loop of GpuChannelManager and GpuChannel should be removed?

I was not advocating for that (though I see no reason not to).
Instead, I was making the observation that the *VDA's themselves don't need to
be told what the IO loop is since they can get it for themselves.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.cc:604: const
media::BitstreamBuffer& bitstream_buffer) {
This is problematic: bitstream_buffer may go out of scope and/or be reused after
Decode() ends, so the const& may be invalid here.
Simplest just to pass by copy so the Bind in Decode will copy the handle there.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:73:
IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_Decode, OnDecode)
I wonder if 
s/OnDecode/owner_->OnDecode/ 
would work here through the magic of textual substitution. 
Not that you should do it, just wondering :)

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:243: // Runs on IO
thread.
Only if CanDecodeOnIOThread, no?

[piman, 2013-08-27 17:14:22]

Some nits, and please address Ami's feedback, otherwise LGTM.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.h:80: virtual bool
CanDecodeOnIOThread() OVERRIDE { return true; }
nit: don't inline virtual methods.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:243: // Runs on IO
thread.
nit: // If video_decode_accelerator_->CanDecodeOnIOThread() is true, otherwise
on the main thread.

[wuchengli, 2013-08-28 04:01:07]

>
https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
> File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
> content/common/gpu/media/exynos_video_decode_accelerator.cc:604: const
> media::BitstreamBuffer& bitstream_buffer) {
> This is problematic: bitstream_buffer may go out of scope and/or be reused
after
> Decode() ends, so the const& may be invalid here.
> Simplest just to pass by copy so the Bind in Decode will copy the handle
there.
I thought that will happen only when it uses base::Bind(...,
base::ConstRef(bitstream_buffer)). With base::Bind(..., bitstream_buffer), it
should be copied to Bind(). I'll look at callback.h and trace bind code to make
sure.

[Pawel Osciak, 2013-08-28 08:36:48]

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.cc:604: const
media::BitstreamBuffer& bitstream_buffer) {
On 2013/08/27 17:10:33, Ami Fischman wrote:
> This is problematic: bitstream_buffer may go out of scope and/or be reused
after
> Decode() ends, so the const& may be invalid here.
> Simplest just to pass by copy so the Bind in Decode will copy the handle
there.

Probably. Or create BBR in Decode and only Map() here. But copy is simpler.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.h:19: namespace base {
Nit: although the style guide doesn't explicitly say this, it seems like the
convention is to have one empty line after namespace's opening and before
closing braces.
Others might correct me though.

[wuchengli, 2013-08-28 13:18:20]

All done. PTAL.

video_decode_accelerator_unittest passed. I also tested local
hardware-accelerated video on Daisy and Link.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.h:89: scoped_refptr<base::MessageLoopProxy>
io_message_loop() const {
On 2013/08/27 17:10:33, Ami Fischman wrote:
> On 2013/08/27 12:32:24, wuchengli wrote:
> > On 2013/08/26 17:20:03, Ami Fischman wrote:
> > > Since this is a process-wide singleton, it would make more sense to grab
> this
> > > from someplace like ChildProcess::current()->io_message_loop_proxy() in
the
> impl
> > > (instead of plumbing it through to all ctors).
> > GpuChannelManager manages more than one GpuChannel. GpuChannelManager is a
> > process-wide singleton, but GpuChannel is not. Do you still think ctor
> > io_message_loop of GpuChannelManager and GpuChannel should be removed?
> 
> I was not advocating for that (though I see no reason not to).
> Instead, I was making the observation that the *VDA's themselves don't need to
> be told what the IO loop is since they can get it for themselves.
I see what you mean now. I have tried to use
ChildProcess::current()->io_message_loop_proxy() in
GpuVideoDecodeAccelerator::Initialize. But DEPS did not allow to include
content/child from GpuVideoDecodeAccelerator.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.cc:604: const
media::BitstreamBuffer& bitstream_buffer) {
On 2013/08/27 17:10:33, Ami Fischman wrote:
> This is problematic: bitstream_buffer may go out of scope and/or be reused
after
> Decode() ends, so the const& may be invalid here.
> Simplest just to pass by copy so the Bind in Decode will copy the handle
there.
This should be OK. callback.h says "By default Bind() will store
copies of all bound parameters, and attempt to refcount a target
object if the function being bound is a class method. These copies
are created even if the function takes parameters as const
references."

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.cc:604: const
media::BitstreamBuffer& bitstream_buffer) {
On 2013/08/28 08:36:49, posciak wrote:
> On 2013/08/27 17:10:33, Ami Fischman wrote:
> > This is problematic: bitstream_buffer may go out of scope and/or be reused
> after
> > Decode() ends, so the const& may be invalid here.
> > Simplest just to pass by copy so the Bind in Decode will copy the handle
> there.
> 
> Probably. Or create BBR in Decode and only Map() here. But copy is simpler.
I agree with Antoine that we should make IO thread as responsive as possible. So
I'll keep BitstreamBufferRef creation here.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/exynos_video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/exynos_video_decode_accelerator.h:80: virtual bool
CanDecodeOnIOThread() OVERRIDE { return true; }
On 2013/08/27 17:14:22, piman wrote:
> nit: don't inline virtual methods.

Done.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:73:
IPC_MESSAGE_HANDLER(AcceleratedVideoDecoderMsg_Decode, OnDecode)
On 2013/08/27 17:10:33, Ami Fischman wrote:
> I wonder if 
> s/OnDecode/owner_->OnDecode/ 
> would work here through the magic of textual substitution. 
> Not that you should do it, just wondering :)
Good idea. I found IPC_MESSAGE_FORWARD is exactly for this.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:243: // Runs on IO
thread.
On 2013/08/27 17:14:22, piman wrote:
> nit: // If video_decode_accelerator_->CanDecodeOnIOThread() is true, otherwise
> on the main thread.
Done.

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/23125014/diff/63001/content/common/gpu/media/...
content/common/gpu/media/gpu_video_decode_accelerator.h:19: namespace base {
On 2013/08/28 08:36:49, posciak wrote:
> Nit: although the style guide doesn't explicitly say this, it seems like the
> convention is to have one empty line after namespace's opening and before
> closing braces.
> Others might correct me though.
I did a search. It looks like that forward declaration doesn't need the blank
lines.

https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20MessageLoo...

[Pawel Osciak, 2013-08-29 04:11:44]

fwiw, lgtm

[wuchengli, 2013-08-29 05:38:41]

Updated the CL by Kuang-che's suggestion. Kuang-che. PTAL.

[kcwu, 2013-08-29 05:41:39]

lgtm

[wuchengli, 2013-08-29 07:25:30]

Ami. It looks like you're OOO until next Tuesday. I think I've addressed all
your comments. I'm submitting this now. If you find that any code needs to
change, let me know.

[commit-bot: I haz the power, 2013-08-29 07:28:59]

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

[commit-bot: I haz the power, 2013-08-29 11:20:31]

Change committed as 220259