Remove reference counting from media::VideoDecoder and friends.

Remove reference counting from media::VideoDecoder and friends.

In addition:
  * VideoRenderer is now passed a list of decoders via constructor instead of Initialize()
  * WebMediaPlayerImpl's FilterCollection is now built in one shot instead of incrementally

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

[scherkus (not reviewing), 2013-03-22 00:04:20]

xhwang: please review everything
fischman: take a look at GpuVideoDecoder

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:193: // XXX: how else would weak_vda be null?
FYI

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:203: // Wait until
VideoDecodeAccelerator::Destroy() completes.
fischman:
http://blog.operationreality.org/wp-content/uploads/2012/03/chemistry-dog-no-...

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
media/filters/video_decoder_selector_unittest.cc:79: selected_decoder_ =
decoder.Pass();
xhwang: this replaces SaveArg<>

[xhwang, 2013-03-22 01:00:32]

lgtm

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:193: // XXX: how else would weak_vda be null?
On 2013/03/22 00:04:20, scherkus wrote:
> FYI

Defer this part to fischman :)

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
File media/filters/video_decoder_selector.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
media/filters/video_decoder_selector.cc:123: input_stream_,
nit: move this after "Initialize(" and align accordingly?

[scherkus (not reviewing), 2013-03-22 01:06:55]

well that was easy ;)

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
File media/filters/video_decoder_selector.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
media/filters/video_decoder_selector.cc:123: input_stream_,
On 2013/03/22 01:00:32, xhwang wrote:
> nit: move this after "Initialize(" and align accordingly?

looks super funky due to the BTCL(base::Bind()) combo :\

ADS has this formatting to so I'm inclined to leave as is

[Ami GONE FROM CHROMIUM, 2013-03-22 23:48:01]

I like it!

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
media/filters/decrypting_video_decoder.cc:44: weak_this_ =
weak_factory_.GetWeakPtr();
Nothing ever invalidates the underlying WeakReference.
You should call weak_factory_.InvalidateWeakPtrs() in the dtor of each class in
this CL that uses this idiom.

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
media/filters/decrypting_video_decoder.cc:235:
base::Bind(&DecryptingVideoDecoder::DecryptAndDecodeBuffer, weak_this_));
What guarantees you get called back on this thread?  (which is required for
reading weak_this_ to be legit).
IOW do you want s/Bind/BindToCurrentLoop/?

https://codereview.chromium.org/12989009/diff/4001/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder.cc:229: demuxer_stream_->Read(base::Bind(
Again, BindToCurrentLoop ?

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:193: // XXX: how else would weak_vda be null?
VDA unfortunately SupportsWeakPtr, so others can invalidate its weakptrs, and
you might not know about it.
E.g. if an error is signaled from the GPU process the VDA (GVDAH) in the
renderer may be released without telling you.
(I think?)

https://codereview.chromium.org/12989009/diff/4001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:207: event.Wait();
This looks like a deadlock: Stop() calls this function, so the render thread is
blocked waiting for this to return, so DestroyAndSignal won't get a chance to
run.

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
media/filters/video_decoder_selector_unittest.cc:117:
decoder_selector_.reset(new VideoDecoderSelector(
operator=() ?

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:52: CHECK_EQ(thread_,
base::kNullThreadHandle);
should also weak_factory_.InvalidateWeakPtrs().

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:299: MockVideoDecoder* decoder_;
It's strange to hold this raw pointer after transferring ownership of the
decoder to the renderer...
At least add a comment, if you need to keep it for nosy tests?

[xhwang, 2013-03-22 23:59:02]

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
media/filters/decrypting_video_decoder.cc:44: weak_this_ =
weak_factory_.GetWeakPtr();
On 2013/03/22 23:48:01, Ami Fischman wrote:
> Nothing ever invalidates the underlying WeakReference.
> You should call weak_factory_.InvalidateWeakPtrs() in the dtor of each class
in
> this CL that uses this idiom.

Hmm, is this required for all classes that uses WeakPtrFactory? Looks like
WeakPtrFactory owns internal::WeakReferenceOwner, whose dtor calls Invalidate
automatically.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[Ami GONE FROM CHROMIUM, 2013-03-23 00:02:01]

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
media/filters/decrypting_video_decoder.cc:44: weak_this_ =
weak_factory_.GetWeakPtr();
On 2013/03/22 23:59:03, xhwang wrote:
> On 2013/03/22 23:48:01, Ami Fischman wrote:
> > Nothing ever invalidates the underlying WeakReference.
> > You should call weak_factory_.InvalidateWeakPtrs() in the dtor of each class
> in
> > this CL that uses this idiom.
> 
> Hmm, is this required for all classes that uses WeakPtrFactory? Looks like
> WeakPtrFactory owns internal::WeakReferenceOwner, whose dtor calls Invalidate
> automatically.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> 
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

Oh good, I think you're right!
(this is how I _thought_ it worked, but then when I looked today I missed this,
visually replacing WeakReferenceOwner with WeakReference).

[scherkus (not reviewing), 2013-03-28 02:00:01]

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/decrypting_v...
media/filters/decrypting_video_decoder.cc:235:
base::Bind(&DecryptingVideoDecoder::DecryptAndDecodeBuffer, weak_this_));
On 2013/03/22 23:48:01, Ami Fischman wrote:
> What guarantees you get called back on this thread?  (which is required for
> reading weak_this_ to be legit).
> IOW do you want s/Bind/BindToCurrentLoop/?

short term: it's part of the API contract to call back async-ly and on the
calling thread

medium term: xhwang is in the midst of having video decoders not make async
calls to demuxers, so this code will go away

https://codereview.chromium.org/12989009/diff/4001/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder.cc:229: demuxer_stream_->Read(base::Bind(
On 2013/03/22 23:48:01, Ami Fischman wrote:
> Again, BindToCurrentLoop ?

ditto

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_decode...
media/filters/video_decoder_selector_unittest.cc:117:
decoder_selector_.reset(new VideoDecoderSelector(
On 2013/03/22 23:48:01, Ami Fischman wrote:
> operator=() ?

../../media/filters/video_decoder_selector_unittest.cc:117:23: error: no viable
overloaded '='
    decoder_selector_ = new VideoDecoderSelector(


perhaps you're thinking of:
scoped_ptr<T> a(new T);
scoped_ptr<T> b = a.Pass();

?

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:52: CHECK_EQ(thread_,
base::kNullThreadHandle);
On 2013/03/22 23:48:01, Ami Fischman wrote:
> should also weak_factory_.InvalidateWeakPtrs().

Was this comment based on how you thought weak_ptr_ worked? (i.e., can I ignore
it?)

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12989009/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:299: MockVideoDecoder* decoder_;
On 2013/03/22 23:48:01, Ami Fischman wrote:
> It's strange to hold this raw pointer after transferring ownership of the
> decoder to the renderer...
> At least add a comment, if you need to keep it for nosy tests?

Done.

https://codereview.chromium.org/12989009/diff/17001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/17001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:198: // Note: I don't think it's possible to
guarantee that VDA *won't* call us
FYI

It's a bit late in the day, but I'm not sure if there's a way to solve this w/o
involving refcounting somehow.

If that's the case, we could introduce a ref-counted proxy class (sigh...) that
implements VDA::Client + target message loop.

We could then remove trampolining code inside GVD and null out the pointer to
GVD during Stop().


I wonder if we can get WMPI to help out here, perhaps via
GpuVideoDecoder::Factories. It would be nice to not rely on a ref-counted proxy
and figure out a proper shutdown + deletion sequence.

Hm.....

[Ami GONE FROM CHROMIUM, 2013-03-29 23:55:19]

https://codereview.chromium.org/12989009/diff/17001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/17001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:198: // Note: I don't think it's possible to
guarantee that VDA *won't* call us
Yes, this is the key point of this CL :)
Today GVD _relies_ on being refcounted as a means for staying alive until the
VDA::Destroy() is actually called (and pending callbacks in the VDA loop's queue
being handled), b/c that's the guarantee of no-more-callbacks.

I think you can still get what you want by replacing the current sequence of:

AddRef()
PostTaskAndReply(Destroy, Release)

with:
GVD* self = VRB::decoder_.release();
PostTaskAndReply(Destroy, Bind(Delete, self));

WDYT?

[scherkus (not reviewing), 2013-04-05 01:18:30]

PTAL

apologies for not rebasing separately! I swore I hadn't rebased but apparently I
did :|

xhwang: you may want to take a look at changes to VideoFrameStream

https://codereview.chromium.org/12989009/diff/17001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/17001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:198: // Note: I don't think it's possible to
guarantee that VDA *won't* call us
On 2013/03/29 23:55:19, Ami Fischman wrote:
> Yes, this is the key point of this CL :)
> Today GVD _relies_ on being refcounted as a means for staying alive until the
> VDA::Destroy() is actually called (and pending callbacks in the VDA loop's
queue
> being handled), b/c that's the guarantee of no-more-callbacks.
> 
> I think you can still get what you want by replacing the current sequence of:
> 
> AddRef()
> PostTaskAndReply(Destroy, Release)
> 
> with:
> GVD* self = VRB::decoder_.release();
> PostTaskAndReply(Destroy, Bind(Delete, self));
> 
> WDYT?

I'd prefer to not mess around w/ transferring ownership from VRB to GVD :\

FWIW I think it'd have to be a scoped_ptr<T> instead of T* otherwise we can risk
leaking when the reply thread has been stopped:
  * VDA runs on render thread
  * WMPI is blocking render thread on Stop()
  * GVD receives Stop() on media thread
  * GVD posts VDA::Destroy() to render thread w/ reply to media thread
  * WMPI + GVD are destroyed
  **** Media thread is stopped ****
  * WMPI returns control to render thread
  * VDA::Destroy() is run
  * PostTaskAndReply() posts task to stopped media thread to destroy GVD
  * MessageLoopProxy drops task


I wrote up a client proxy. It's not too bad ... but I can't say I'm super happy.

[Ami GONE FROM CHROMIUM, 2013-04-05 21:05:48]

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:23: // a thread of the client's choice.
Crazy Question Time(tm):
What would happen if you made GVD *not* be a VDA::Client, and instead implement
VDA::Client in a single class here (GpuVDAClient), made it refcounted, stored a
ref in GVD itself, and gave GpuVDAClient a Detach() method that would turn all
future calls on it into no-ops.  GpuVDAClient would live on the VDA (render)
thread.

When GVD::Stop() was called (on the media thread) it would post to the VDA
thread the tasks:
- GpuVDAClient::Detach()
- VDA::Destroy()
- GpuVDAClient::DoNothing() (just as a way of keeping the GVC alive)
and immediately clear its ref to the GVC.

I think the result would be easier to understand since:
1) only one class would be impl'ing VDA::Client; and 
2) each of the classes will live on a single thread.

It'll also allow us to get rid of some craxy, such as the current thread-hopping
when textures are requested:
render thread calls ProvidePictureBuffers
ProvidePictureBuffers posts itself to media thread
ProvidePictureBuffers calls factories_->CreateTextures
CreateTextures calls CreateTexturesAsync on the compositor thread and waits
ProvidePictureBuffers posts AssignPictureBuffers back to the render thread.
That's 2 unnecessary thread hops!  It's not really an efficiency concern (since
texture assignment is one-off) but it's a lot of needless complexity.

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:28: VDAClientProxy(const
scoped_refptr<base::MessageLoopProxy>& client_loop,
This is always MessageLoopProxy::current() so I'd drop it.

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:71: VDAClientProxy::~VDAClientProxy() {}
DCHECK already Detach()d

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:74:
DCHECK(client_loop_->BelongsToCurrentThread());
DCHECK not already detached to catch programming errors?

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:302: const scoped_refptr<VDAClientProxy>&
client_proxy,
so this is only here to keep the proxy alive until this exits?
DCHECK(client_proxy->HasOneRef());
?

[scherkus (not reviewing), 2013-04-05 21:16:12]

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:23: // a thread of the client's choice.
On 2013/04/05 21:05:48, Ami Fischman wrote:
> Crazy Question Time(tm):
> What would happen if you made GVD *not* be a VDA::Client, and instead
implement
> VDA::Client in a single class here (GpuVDAClient), made it refcounted, stored
a
> ref in GVD itself, and gave GpuVDAClient a Detach() method that would turn all
> future calls on it into no-ops.  GpuVDAClient would live on the VDA (render)
> thread.
> 
> When GVD::Stop() was called (on the media thread) it would post to the VDA
> thread the tasks:
> - GpuVDAClient::Detach()
> - VDA::Destroy()
> - GpuVDAClient::DoNothing() (just as a way of keeping the GVC alive)
> and immediately clear its ref to the GVC.
> 
> I think the result would be easier to understand since:
> 1) only one class would be impl'ing VDA::Client; and 
> 2) each of the classes will live on a single thread.
> 
> It'll also allow us to get rid of some craxy, such as the current
thread-hopping
> when textures are requested:
> render thread calls ProvidePictureBuffers
> ProvidePictureBuffers posts itself to media thread
> ProvidePictureBuffers calls factories_->CreateTextures
> CreateTextures calls CreateTexturesAsync on the compositor thread and waits
> ProvidePictureBuffers posts AssignPictureBuffers back to the render thread.
> That's 2 unnecessary thread hops!  It's not really an efficiency concern
(since
> texture assignment is one-off) but it's a lot of needless complexity.

I don't think it's too crazy but let's make sure I better understand what you're
proposing.

- GpuVDAClient holds onto factories_ and take care of some of the work that is
current done in GVD
- GpuVDAClient exposes a subset/simpler set of APIs for GVD (including Detach(),
etc...)
- GVD holds a ref to GpuVDAClient instead of a VDA

My one question/concern is whether GpuVDAClient will ever call out to GVD. If
so, then while the task to Detach() is queued, we can still race with the media
thread destroying GVD before Detach() is run. I guess one way to get around that
is to have GpuVDAClient accept base::Bind()s so GVD can use weak_this_

[Ami GONE FROM CHROMIUM, 2013-04-05 21:22:21]

> - GpuVDAClient holds onto factories_ and take care of some of the
> work that is current done in GVD
>

Yes.


> - GpuVDAClient exposes a subset/simpler set of APIs for GVD
> (including Detach(), etc...)
>

I was thinking the only API it exposes towards GVD is Detach().  Nothing
else.
And the only other API it exposes at all is VDA::Client, which it exposes
towards the VDA, obvs.


> - GVD holds a ref to GpuVDAClient instead of a VDA
>

No, GVD still needs a VDA handle.

My one question/concern is whether GpuVDAClient will ever call out to GVD.


Yes, I was expecting it to (esp. for picture delivery).

If so, then while the task to Detach() is queued, we can still race
> with the media thread destroying GVD before Detach() is run. I guess one
> way to get around that is to have GpuVDAClient accept base::Bind()s so
> GVD can use weak_this_
>

Yeah, I should have said: GVD passes its own weak_this_ (bound on the media
thread) to the GpuVDAClient ctor, and that's GpuVDAClient's only way to
communicate back to the GVD (and all such communications must happen
through a gvd_loop_proxy_->PostTask()).

-a

[scherkus (not reviewing), 2013-04-05 21:26:31]

On 2013/04/05 21:22:21, Ami Fischman wrote:
> > - GpuVDAClient holds onto factories_ and take care of some of the
> > work that is current done in GVD
> >
> 
> Yes.
> 
> 
> > - GpuVDAClient exposes a subset/simpler set of APIs for GVD
> > (including Detach(), etc...)
> >
> 
> I was thinking the only API it exposes towards GVD is Detach().  Nothing
> else.
> And the only other API it exposes at all is VDA::Client, which it exposes
> towards the VDA, obvs.
> 
> 
> > - GVD holds a ref to GpuVDAClient instead of a VDA
> >
> 
> No, GVD still needs a VDA handle.

Hmm... I wonder what GVD needs a VDA handle for if GpuVDAClient is doing more of
the heavy lifting. Guess I can prototype it and find out.

> My one question/concern is whether GpuVDAClient will ever call out to GVD.
> 
> 
> Yes, I was expecting it to (esp. for picture delivery).
> 
> If so, then while the task to Detach() is queued, we can still race
> > with the media thread destroying GVD before Detach() is run. I guess one
> > way to get around that is to have GpuVDAClient accept base::Bind()s so
> > GVD can use weak_this_
> >
> 
> Yeah, I should have said: GVD passes its own weak_this_ (bound on the media
> thread) to the GpuVDAClient ctor, and that's GpuVDAClient's only way to
> communicate back to the GVD (and all such communications must happen
> through a gvd_loop_proxy_->PostTask()).

Yeah I'll play around.

[Ami GONE FROM CHROMIUM, 2013-04-05 21:33:15]

On Fri, Apr 5, 2013 at 2:26 PM, <scherkus@chromium.org> wrote:

> Hmm... I wonder what GVD needs a VDA handle for if GpuVDAClient is doing
> more of
> the heavy lifting. Guess I can prototype it and find out.
>

VDAClient is the callback interface: it's how the VDA impl lets you know of
events happening.
VDA is the request interface: it's how the pipeline can request decode, and
so on.

[scherkus (not reviewing), 2013-04-08 16:53:10]

I took a stab at your idea and it's a bit more invasive than I'd like.

Before I continue down that path I want to ask ... do we feel VDAClientProxy
addresses enough of the GVD cruft for now? File a bug w/ TODO containing the
details? Do we have other non-invasive changes in mind?

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

On 2013/04/08 16:53:10, scherkus wrote:
> I took a stab at your idea and it's a bit more invasive than I'd like.
> 
> Before I continue down that path I want to ask ... do we feel VDAClientProxy
> addresses enough of the GVD cruft for now? File a bug w/ TODO containing the
> details? Do we have other non-invasive changes in mind?

I don't have other unrelated non-invasive changes in mind, but xhwang@ might
(for config-change support).

If you do decide to keep the VDAClientProxy idea, then a further minor
decrufting of GVD would be to drop the gvd_loop_proxy_ member entirely, in favor
of making GVD own a base::ThreadChecker (since the only uses of g_l_p_ in the
VDAClientProxy world are to DCHECK that we're on the right thread, and to post
to the loop from a task already running on the loop).  Then GVD could be
annotated as being callable on only a single thread (the media thread) after
construction (which will happen on the compositor thread, which may also be the
render thread).  Similarly, VDAClientProxy can doco & DCHECK that all its
methods are  called from the compositor thread except Detach which must be
called from the media thread.
(IOW, VCP can have two ThreadCheckers: one used by the ctor and Detach (the
media thread), and the other used by the dtor & VDA::Client methods (the
compositor thread)).

[xhwang, 2013-04-15 21:14:13]

On 2013/04/08 17:27:08, Ami Fischman wrote:
> On 2013/04/08 16:53:10, scherkus wrote:
> > I took a stab at your idea and it's a bit more invasive than I'd like.
> > 
> > Before I continue down that path I want to ask ... do we feel VDAClientProxy
> > addresses enough of the GVD cruft for now? File a bug w/ TODO containing the
> > details? Do we have other non-invasive changes in mind?
> 
> I don't have other unrelated non-invasive changes in mind, but xhwang@ might
> (for config-change support).
> 
> If you do decide to keep the VDAClientProxy idea, then a further minor
> decrufting of GVD would be to drop the gvd_loop_proxy_ member entirely, in
favor
> of making GVD own a base::ThreadChecker (since the only uses of g_l_p_ in the
> VDAClientProxy world are to DCHECK that we're on the right thread, and to post
> to the loop from a task already running on the loop).  Then GVD could be
> annotated as being callable on only a single thread (the media thread) after
> construction (which will happen on the compositor thread, which may also be
the
> render thread).  Similarly, VDAClientProxy can doco & DCHECK that all its
> methods are  called from the compositor thread except Detach which must be
> called from the media thread.
> (IOW, VCP can have two ThreadCheckers: one used by the ctor and Detach (the
> media thread), and the other used by the dtor & VDA::Client methods (the
> compositor thread)).

The change in VideoFrameStream looks good to me.

Here's my two cents and question about the discussion on VDAClientProxy.
1) It seems this can be solved by forcing VDA to post GVD callbacks (which use
GVD's weak_ptr) instead of directly calling into GVD. This is more consistent
with the rest of media code, where we pass in callbacks instead of client
objects. But this requires changes in all VDA implementations, which is invasive
:(
2) Given (1), I think the idea of VDAClientProxy is not bad.
3) I wonder (thinking loudly), is there any way to fix the shut down hack
completely so that in GVD and DVD we can do full shutdown? Can we (thinking
widly) call Messageloop::current()->RunUntilIdle() priodically in
WebMediaPlayerImpl::Destroy() until the pipeline (video decoder) is fully
stopped?

[scherkus (not reviewing), 2013-04-17 16:54:53]

PTAL

I'm sticking with the proxy but added some details on how it could be improved

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:28: VDAClientProxy(const
scoped_refptr<base::MessageLoopProxy>& client_loop,
On 2013/04/05 21:05:48, Ami Fischman wrote:
> This is always MessageLoopProxy::current() so I'd drop it.

Done.

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:71: VDAClientProxy::~VDAClientProxy() {}
On 2013/04/05 21:05:48, Ami Fischman wrote:
> DCHECK already Detach()d

Done.

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:74:
DCHECK(client_loop_->BelongsToCurrentThread());
On 2013/04/05 21:05:48, Ami Fischman wrote:
> DCHECK not already detached to catch programming errors?

Done.

https://codereview.chromium.org/12989009/diff/27035/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:302: const scoped_refptr<VDAClientProxy>&
client_proxy,
On 2013/04/05 21:05:48, Ami Fischman wrote:
> so this is only here to keep the proxy alive until this exits?
> DCHECK(client_proxy->HasOneRef());
> ?

Done.

[xhwang, 2013-04-18 00:40:48]

I like the Proxy implementation. Just a few questions about teardown.

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:36: // Detaches the proxy. |client| will no
longer be called and can be safely
s/client/weak_client_/ ?

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:39: // Must be called on |client_loop|.
s/client_loop/client_loop_/

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:60: // Weak pointers are used to invalidate
tasks posted to |client_loop_| that
s/that/after ?

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:311: DCHECK(client_proxy->HasOneRef());
hmm, are we making assumption about vda implementation details? Can vda just
drop the reference to client_proxy if something bad happened?

Also, can there be a race condition? e.g. can this DCHECK executed before line
325 is executed?

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:312: if (weak_vda)
OOC, in which case can this happen? DestroyVDA called twice in a row? In that
case shall we check on line 323?

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:313: weak_vda->Destroy();
weak_vda.reset()?

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:69: // Returns true if Abort has been called.
s/Abort/Abort()/

[scherkus (not reviewing), 2013-04-18 18:45:28]

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:36: // Detaches the proxy. |client| will no
longer be called and can be safely
On 2013/04/18 00:40:48, xhwang wrote:
> s/client/weak_client_/ ?

Done.

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:39: // Must be called on |client_loop|.
On 2013/04/18 00:40:48, xhwang wrote:
> s/client_loop/client_loop_/

Done.

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:60: // Weak pointers are used to invalidate
tasks posted to |client_loop_| that
On 2013/04/18 00:40:48, xhwang wrote:
> s/that/after ?

Done.

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:311: DCHECK(client_proxy->HasOneRef());
On 2013/04/18 00:40:48, xhwang wrote:
> hmm, are we making assumption about vda implementation details? Can vda just
> drop the reference to client_proxy if something bad happened?
> 
> Also, can there be a race condition? e.g. can this DCHECK executed before line
> 325 is executed?

ah good catch -- fischman@ suggested this but you're right that it can race

I don't find it's a very valuable DCHECK - removed

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:312: if (weak_vda)
On 2013/04/18 00:40:48, xhwang wrote:
> OOC, in which case can this happen? DestroyVDA called twice in a row? In that
> case shall we check on line 323?

you can only check/deref a weak pointer on the thread it's bound to (hence this
task post)

IIRC fischman@ said it's possible for VDA to get destroyed by other means (e.g.,
compositor)

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:313: weak_vda->Destroy();
On 2013/04/18 00:40:48, xhwang wrote:
> weak_vda.reset()?

Destroy() is a VDA method that takes care of deleting the VDA

.reset() would release a weak reference but leak the VDA object

Did you mean something else?

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.h:69: // Returns true if Abort has been called.
On 2013/04/18 00:40:48, xhwang wrote:
> s/Abort/Abort()/

Done.

[xhwang, 2013-04-18 19:17:49]

lgtm % one comment/reply

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:313: weak_vda->Destroy();
On 2013/04/18 18:45:29, scherkus wrote:
> On 2013/04/18 00:40:48, xhwang wrote:
> > weak_vda.reset()?
> 
> Destroy() is a VDA method that takes care of deleting the VDA
> 
> .reset() would release a weak reference but leak the VDA object
> 
> Did you mean something else?

Sorry, I meant calling .reset() after ->Destroy(). I was worrying about checking
on 312 happens again after 313 is executed (this can happen if DestroyVDA() is
called twice). But since Destroy() deletes the VDA, it should already
invalidates weak_vda_. So no need to call .reset() anymore. 

BTW, does it make sense to call DCHECK(!weak_vda) after 313?

[scherkus (not reviewing), 2013-04-18 20:21:14]

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/12989009/diff/65001/media/filters/gpu_video_d...
media/filters/gpu_video_decoder.cc:313: weak_vda->Destroy();
On 2013/04/18 19:17:49, xhwang wrote:
> On 2013/04/18 18:45:29, scherkus wrote:
> > On 2013/04/18 00:40:48, xhwang wrote:
> > > weak_vda.reset()?
> > 
> > Destroy() is a VDA method that takes care of deleting the VDA
> > 
> > .reset() would release a weak reference but leak the VDA object
> > 
> > Did you mean something else?
> 
> Sorry, I meant calling .reset() after ->Destroy(). I was worrying about
checking
> on 312 happens again after 313 is executed (this can happen if DestroyVDA() is
> called twice). But since Destroy() deletes the VDA, it should already
> invalidates weak_vda_. So no need to call .reset() anymore. 
> 
> BTW, does it make sense to call DCHECK(!weak_vda) after 313?

Done.

[scherkus (not reviewing), 2013-04-18 20:23:41]

Committed patchset #7 manually as r194993 (presubmit successful).