VideoDecodeAccelerator now SupportsWeakPtr instead of being RefCountedThreadSafe.

VideoDecodeAccelerator now SupportsWeakPtr instead of being RefCountedThreadSafe.

Claiming both (which GpuVideoDecodeAcceleratorHost was doing) is confusing and
RCTS encourages sloppy lifecycle management.  Instead drop RCTS in favor of
WeakPtr<>s and explicit lifecycle management (scoped_ptr, with Pass).

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

[Ami GONE FROM CHROMIUM, 2012-07-16 21:07:53]

https://chromiumcodereview.appspot.com/10749019/diff/17001/content/common/gpu...
File content/common/gpu/media/h264_bit_reader.h (right):

https://chromiumcodereview.appspot.com/10749019/diff/17001/content/common/gpu...
content/common/gpu/media/h264_bit_reader.h:22: // See the definition of
more_rbsp_data() in spec.
Ignore changes in h264_bit_reader.{h,cc} and h264_parser.cc; I'm landing these
changes separately in https://chromiumcodereview.appspot.com/10782019/

[Ryan Sleevi, 2012-07-16 21:26:07]

Drive-by, since I filed the original bug, but I'm a little concerned, because
while it addresses the scoped_ptr<> ness, it still leaves the lifetime very
ambiguous and, as written, may have threading issues.

I've highlighted below where I think the issue is.

Note that in general, I'd advise deriving from SupportsWeakPtr<> - it exposes a
lot of the internal details about how an object is implemented, and it's
"safety" is only guaranteed if /all/ callers play nice. It is often safer to use
a wrapper object (that initializes on the correct thread and that, if called on
!correct thread, that forwards to the correct thread via WeakPtr), then to
directly expose SupportsWeakPtr<> to your callers. Just FWIW.

https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
media/filters/gpu_video_decoder.cc:71: 
So the invariant here is that:

gvd_loop_proxy->BelongsToCurrentThread()

https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
media/filters/gpu_video_decoder.cc:97: &VideoDecodeAccelerator::Reset,
vda_->AsWeakPtr()));
Now, you're posing vda_ as a WeakPtr to vda_loop_proxy_. If vda_loop_proxy_ ==
gvd_loop_proxy_, this is generally safe, because
vda_loop_proxy_->BelongsToCurrentThread() must also be true.

However, from this code, it would seem that
gvd_loop_proxy_->BelongsToCurrentThread() is false.

If vda_ has no outstanding WeakPtr<>s, or they have all been invalidated, then
AsWeakPtr<> is going to return a WeakPtr<> that is bound to the /current/ thread
(gvd_loop_proxy_), and such a WeakPtr /should not/ be de-referenced on
vda_loop_proxy_, which is exactly what will happen, since vda_ is bound as the
first argument.

If there /is/ an existing WeakPtr<>, and /if/ it was first created on
vda_loop_proxy_, then this is safe. But that's a very big if, and one that
cannot be seen just from looking at this code.

For example, when I look to where vda_ is created (line 145-146), I see that
it's on gvd_loop_proxy_ (line 120), and factories_->CreateVideoDecodeAccelerator
is a synchronous call, so I /know/ that vda_loop_proxy_ cannot have been invoked
to bind this object.

[Ami GONE FROM CHROMIUM, 2012-07-16 21:31:57]

https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
media/filters/gpu_video_decoder.cc:97: &VideoDecodeAccelerator::Reset,
vda_->AsWeakPtr()));
On 2012/07/16 21:26:07, Ryan Sleevi wrote:
> Now, you're posing vda_ as a WeakPtr to vda_loop_proxy_. If vda_loop_proxy_ ==
> gvd_loop_proxy_, this is generally safe, because
> vda_loop_proxy_->BelongsToCurrentThread() must also be true.
> 
> However, from this code, it would seem that
> gvd_loop_proxy_->BelongsToCurrentThread() is false.
> 
> If vda_ has no outstanding WeakPtr<>s, or they have all been invalidated, then
> AsWeakPtr<> is going to return a WeakPtr<> that is bound to the /current/
thread
> (gvd_loop_proxy_), and such a WeakPtr /should not/ be de-referenced on
> vda_loop_proxy_, which is exactly what will happen, since vda_ is bound as the
> first argument.
> 
> If there /is/ an existing WeakPtr<>, and /if/ it was first created on
> vda_loop_proxy_, then this is safe. But that's a very big if, and one that
> cannot be seen just from looking at this code.

Blort, you were too quick for me.
This file in rietveld reflects an out-of-date version.
In the coming code I'm making vda_ be set on vda_loop_proxy_ and tracking a
(new) weak_vda_ ptr on that thread to do the posting from GVD->VDA.

> For example, when I look to where vda_ is created (line 145-146), I see that
> it's on gvd_loop_proxy_ (line 120), and
factories_->CreateVideoDecodeAccelerator
> is a synchronous call, so I /know/ that vda_loop_proxy_ cannot have been
invoked
> to bind this object.

While it's not easy to see, CreateVDA appears synchronous but really does create
the object on the right thread.

[Ryan Sleevi, 2012-07-16 21:47:01]

On 2012/07/16 21:31:57, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
> File media/filters/gpu_video_decoder.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10749019/diff/17001/media/filters/gpu_...
> media/filters/gpu_video_decoder.cc:97: &VideoDecodeAccelerator::Reset,
> vda_->AsWeakPtr()));
> On 2012/07/16 21:26:07, Ryan Sleevi wrote:
> > Now, you're posing vda_ as a WeakPtr to vda_loop_proxy_. If vda_loop_proxy_
==
> > gvd_loop_proxy_, this is generally safe, because
> > vda_loop_proxy_->BelongsToCurrentThread() must also be true.
> > 
> > However, from this code, it would seem that
> > gvd_loop_proxy_->BelongsToCurrentThread() is false.
> > 
> > If vda_ has no outstanding WeakPtr<>s, or they have all been invalidated,
then
> > AsWeakPtr<> is going to return a WeakPtr<> that is bound to the /current/
> thread
> > (gvd_loop_proxy_), and such a WeakPtr /should not/ be de-referenced on
> > vda_loop_proxy_, which is exactly what will happen, since vda_ is bound as
the
> > first argument.
> > 
> > If there /is/ an existing WeakPtr<>, and /if/ it was first created on
> > vda_loop_proxy_, then this is safe. But that's a very big if, and one that
> > cannot be seen just from looking at this code.
> 
> Blort, you were too quick for me.
> This file in rietveld reflects an out-of-date version.
> In the coming code I'm making vda_ be set on vda_loop_proxy_ and tracking a
> (new) weak_vda_ ptr on that thread to do the posting from GVD->VDA.

Yup, that sounds about the right way to do it.

Create the object on the VDA loop (not technically necessary, but makes it
easier to reason about, since thats where it will need to be destroyed), then
post from the VDA loop to the GVD loop a WeakPtr<>. Since it was created on the
VDA loop, it will always be bound to the VDA loop, and the GVD loop can always
copy-construct the object whenever it needs to post stuff back to the VDA loop.
When the object is deleted, the object will need to be deleted on the VDA loop
(or, at a minimum, InvalidateWeakPtrs called on the VDA loop).

> 
> > For example, when I look to where vda_ is created (line 145-146), I see that
> > it's on gvd_loop_proxy_ (line 120), and
> factories_->CreateVideoDecodeAccelerator
> > is a synchronous call, so I /know/ that vda_loop_proxy_ cannot have been
> invoked
> > to bind this object.
> 
> While it's not easy to see, CreateVDA appears synchronous but really does
create
> the object on the right thread.

Very subtle, since the only documentation for CreateVideoDecodeAccelerator says
that the caller owns the returned pointer.

Perhaps update the factory documentation for the GpuVideoDecoder that the
returned pointer /must/ be deleted via Destroy(). This is because we don't
support custom deleter traits in scoped_ptr, only scoped_ptr_malloc, and
deriving from content::BrowserThread::DeleteOnThread is not appropriate.

[Ami GONE FROM CHROMIUM, 2012-07-16 21:51:02]

piman: please OWNERS review for content/common/gpu/client and
webkit/plugins/ppapi
brettw: please OWNERS-review for content/content_tests.gypi 
rsleevi: FYI, PS#2 addresses your comment

[Ami GONE FROM CHROMIUM, 2012-07-16 21:57:36]

On 2012/07/16 21:47:01, Ryan Sleevi wrote:
> Create the object on the VDA loop (not technically necessary, but makes it
> easier to reason about, since thats where it will need to be destroyed)

It actually is necessary for reasons of implementation, IIRC.

> then post from the VDA loop to the GVD loop a WeakPtr<>.

It's actually enough to do an assignment on the VDA loop since we have tasks
acting as barriers.

> Since it was created on the
> VDA loop, it will always be bound to the VDA loop, and the GVD loop can always
> copy-construct the object whenever it needs to post stuff back to the VDA
loop.

Right; this is what the updated code does.

> When the object is deleted, the object will need to be deleted on the VDA loop
> (or, at a minimum, InvalidateWeakPtrs called on the VDA loop).

Yep; using DeleteSoon for this.

> Very subtle, since the only documentation for CreateVideoDecodeAccelerator
says
> that the caller owns the returned pointer.
> 
> Perhaps update the factory documentation for the GpuVideoDecoder that the
> returned pointer /must/ be deleted via Destroy(). 

Destroy() is a bit of a misdirection here, because it's not guaranteed to
trampoline to the right thread (but it sort of works b/c Destroy() must be
called on the right thread in the first place).
I'll think about this issue some more and probably clarify the Factories
interface in a different (comment-only) CL.

[brettw, 2012-07-16 22:11:54]

LGTM with comment for content_tests.gypi

https://chromiumcodereview.appspot.com/10749019/diff/24002/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/10749019/diff/24002/content/content_te...
content/content_tests.gypi:632: 'os_posix == 1 and OS != "mac" and OS !=
"android" and '
Can you add another set of parens here to make the order more obvious between
the or on the previous line and all of the ands on this line?

[scherkus (not reviewing), 2012-07-16 22:14:31]

fischman gave me the rundown on a whiteboard -- lgtm

[piman, 2012-07-16 22:16:01]

https://chromiumcodereview.appspot.com/10749019/diff/24002/media/video/video_...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/10749019/diff/24002/media/video/video_...
media/video/video_decode_accelerator.h:134: virtual void
Destroy(scoped_ptr<VideoDecodeAccelerator> self) = 0;
I can't say I'm a fan of this pattern (commenting here because it impacts all
the implementations)...
Among others, it means the Task takes ownership of self, meaning it may delete
it without calling Destroy, or even delete it on the wrong thread if the message
loop is destroyed before executing the task.
Also, it seems like an implementation detail is leaking in the interface which
is a bit of a code smell...

Is there a way we could either ensure that no inappropriate usage will happen,
or change the interface to avoid this?

[Ami GONE FROM CHROMIUM, 2012-07-16 22:34:03]

https://chromiumcodereview.appspot.com/10749019/diff/24002/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/10749019/diff/24002/content/content_te...
content/content_tests.gypi:632: 'os_posix == 1 and OS != "mac" and OS !=
"android" and '
On 2012/07/16 22:11:54, brettw wrote:
> Can you add another set of parens here to make the order more obvious between
> the or on the previous line and all of the ands on this line?

Done.

https://chromiumcodereview.appspot.com/10749019/diff/24002/media/video/video_...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/10749019/diff/24002/media/video/video_...
media/video/video_decode_accelerator.h:134: virtual void
Destroy(scoped_ptr<VideoDecodeAccelerator> self) = 0;
On 2012/07/16 22:16:01, piman wrote:
> I can't say I'm a fan of this pattern (commenting here because it impacts all
> the implementations)...
> Among others, it means the Task takes ownership of self, meaning it may delete
> it without calling Destroy, or even delete it on the wrong thread if the
message
> loop is destroyed before executing the task.

Interesting thoughts.
In practice this is only posted to the ChildThread (render thread for host, GPU
thread for impl), so I don't think this is a problem (these are the threads on
which the WeakPtr<>'s would have been tested for validity).

> Also, it seems like an implementation detail is leaking in the interface which
> is a bit of a code smell...

Agreed it smells, but also that it's only a /bit/ of a smell.

> Is there a way we could either ensure that no inappropriate usage will happen,

I'm not sure what you mean by inappropriate usage.

> or change the interface to avoid this?

Easiest way to avoid this would be to make Destroy() async again (I made it sync
(fire-and-forget) in http://codereview.chromium.org/7361010).  I don't remember
the details of why this was necessary now [:(], but IIRC it had to do with
flapper requirements.
I think keeping Destroy() sync presents a better API, even with the need to pass
ownership here.
So, I think I'm happiest leaving it as-is.

Overall I value this CL's improvement in lifecycle clarity significantly more
than I mind this aspect of the API.  WDYT?

[Ryan Sleevi, 2012-07-16 22:45:01]

Thanks. Looking at the patchset diffs, I think this addresses my lifetime
concerns. I'm never thrilled with having a class derive from SupportsWeakPtr<>
(I think it leaks too much detail), but what you've got here is a lot better
than the scoped_refptr<> confusion, so thanks :)

https://chromiumcodereview.appspot.com/10749019/diff/24002/media/filters/gpu_...
File media/filters/gpu_video_decoder.h (right):

https://chromiumcodereview.appspot.com/10749019/diff/24002/media/filters/gpu_...
media/filters/gpu_video_decoder.h:123: // thread).
nit: Based on the media/DEPS rule, it would seem like saying "in practice, the
render thread" is a leaky abstraction, in that media/ doesn't seem to know about
content/ or chrome/ . In net/ code, we avoid saying "THE IO thread" or "THE UI
thread" for similar reasons. I'd suggest dropping the parenthetical if this is
the case, but that's just my $.02 nit.

[piman, 2012-07-16 22:46:25]

On Mon, Jul 16, 2012 at 3:34 PM, <fischman@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10749019/diff/**
>
24002/content/content_tests.**gypi<https://chromiumcodereview.appspot.com/10749019/diff/24002/content/content_tests.gypi>
> File content/content_tests.gypi (right):
>
> https://chromiumcodereview.**appspot.com/10749019/diff/**
>
24002/content/content_tests.**gypi#newcode632<https://chromiumcodereview.appspot.com/10749019/diff/24002/content/content_tests.gypi#newcode632>
> content/content_tests.gypi:**632: 'os_posix == 1 and OS != "mac" and OS !=
> "android" and '
> On 2012/07/16 22:11:54, brettw wrote:
>
>> Can you add another set of parens here to make the order more obvious
>>
> between
>
>> the or on the previous line and all of the ands on this line?
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10749019/diff/**
>
24002/media/video/video_**decode_accelerator.h<https://chromiumcodereview.appspot.com/10749019/diff/24002/media/video/video_decode_accelerator.h>
> File media/video/video_decode_**accelerator.h (right):
>
> https://chromiumcodereview.**appspot.com/10749019/diff/**
>
24002/media/video/video_**decode_accelerator.h#**newcode134<https://chromiumcodereview.appspot.com/10749019/diff/24002/media/video/video_decode_accelerator.h#newcode134>
> media/video/video_decode_**accelerator.h:134: virtual void
> Destroy(scoped_ptr<**VideoDecodeAccelerator> self) = 0;
> On 2012/07/16 22:16:01, piman wrote:
>
>> I can't say I'm a fan of this pattern (commenting here because it
>>
> impacts all
>
>> the implementations)...
>> Among others, it means the Task takes ownership of self, meaning it
>>
> may delete
>
>> it without calling Destroy, or even delete it on the wrong thread if
>>
> the message
>
>> loop is destroyed before executing the task.
>>
>
> Interesting thoughts.
> In practice this is only posted to the ChildThread (render thread for
> host, GPU thread for impl), so I don't think this is a problem (these
> are the threads on which the WeakPtr<>'s would have been tested for
> validity).


>
>  Also, it seems like an implementation detail is leaking in the
>>
> interface which
>
>> is a bit of a code smell...
>>
>
> Agreed it smells, but also that it's only a /bit/ of a smell.
>
>
>  Is there a way we could either ensure that no inappropriate usage will
>>
> happen,
>
> I'm not sure what you mean by inappropriate usage.


e.g. DCHECK that Destroy() is called before the destructor. DCHECK that
self == this or NULL.


>  or change the interface to avoid this?
>>
>
> Easiest way to avoid this would be to make Destroy() async again (I made
> it sync (fire-and-forget) in
http://codereview.chromium.**org/7361010<http://codereview.chromium.org/7361010>).
>  I
> don't remember the details of why this was necessary now [:(], but IIRC
> it had to do with flapper requirements.
> I think keeping Destroy() sync presents a better API, even with the need
> to pass ownership here.
> So, I think I'm happiest leaving it as-is.
>

You could also e.g. replace the scoped_ptr "this" by a boolean
"take_ownership", or even have 2 methods, one that cleans up and delete,
the other one that just cleans up. For the first kind you can take a
scoped_ptr<...> self(this) in the implementations if that's helpful.

>
> Overall I value this CL's improvement in lifecycle clarity significantly
> more than I mind this aspect of the API.  WDYT?
>

I'm not sure I agree that the lifecycle clarity is improved - dropping the
refcount, sure, but duplicating this as a parameter that could be NULL to
indicate whether or not to delete after Destroy is not the clearest thing
IMO.


>
>
https://chromiumcodereview.**appspot.com/10749019/<https://chromiumcodereview...
>

[Ami GONE FROM CHROMIUM, 2012-07-16 23:16:10]

On 2012/07/16 22:46:25, piman wrote:
> e.g. DCHECK that Destroy() is called before the destructor. 

Most (all?) impls should be doing this already.

> DCHECK that self == this or NULL.

Ah, good idea.

> You could also e.g. replace the scoped_ptr "this" by a boolean
> "take_ownership"

I like this suggestion best.  Doing this now.

[sail, 2012-07-17 16:15:14]

mac * LGTM!
I ran video_decoder_accelerator_unittest with this patch and everything ran
fine.

http://codereview.chromium.org/10749019/diff/29030/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10749019/diff/29030/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:254:
&MacVideoDecodeAccelerator::NotifyInputBufferRead, this,
also need base::AsWeakPtr here

[Ami GONE FROM CHROMIUM, 2012-07-17 16:54:23]

http://codereview.chromium.org/10749019/diff/29030/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10749019/diff/29030/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:254:
&MacVideoDecodeAccelerator::NotifyInputBufferRead, this,
On 2012/07/17 16:15:14, sail wrote:
> also need base::AsWeakPtr here

Done.

[piman, 2012-07-17 17:14:28]

https://chromiumcodereview.appspot.com/10749019/diff/20009/content/common/gpu...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://chromiumcodereview.appspot.com/10749019/diff/20009/content/common/gpu...
content/common/gpu/client/command_buffer_proxy_impl.cc:32: :
video_decoder_hosts_deleter_(&video_decoder_hosts_),
So, doesn't this mean that when the CommandBufferProxyImpl gets destroyed, the
left over GpuVideoDecodeAcceleratorHost get deleted, not Destroy()ed ? Isn't it
a problem?

https://chromiumcodereview.appspot.com/10749019/diff/20009/media/video/video_...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/10749019/diff/20009/media/video/video_...
media/video/video_decode_accelerator.h:133: virtual void Destroy(bool
pass_ownership) = 0;
So I looked carefully at the implementations. There's only 2 places where
Destroy(false) is called, and that's internally to the Mac and the VAAPI
decoders. It's never called externally.
From what I can tell in both those cases above, Destroy will be called twice,
once with false on error, which does cleanup but doesn't delete the object, and
a second time with true, which doesn't really do any cleanup (because it's
already been done) but then deletes the object.

It sounds to me it would be simpler, and better served everywhere to make the
public API always take ownership (and remove the bool). On Mac and VAAPI
decoders, you can have a private Cleanup function, that's called from
StopOnError and NotifyError respectively, as well as Destroy.

WDYT?

[Ami GONE FROM CHROMIUM, 2012-07-17 17:58:50]

https://chromiumcodereview.appspot.com/10749019/diff/20009/content/common/gpu...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://chromiumcodereview.appspot.com/10749019/diff/20009/content/common/gpu...
content/common/gpu/client/command_buffer_proxy_impl.cc:32: :
video_decoder_hosts_deleter_(&video_decoder_hosts_),
On 2012/07/17 17:14:28, piman wrote:
> So, doesn't this mean that when the CommandBufferProxyImpl gets destroyed, the
> left over GpuVideoDecodeAcceleratorHost get deleted, not Destroy()ed ? Isn't
it
> a problem?

Done.

https://chromiumcodereview.appspot.com/10749019/diff/20009/media/video/video_...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/10749019/diff/20009/media/video/video_...
media/video/video_decode_accelerator.h:133: virtual void Destroy(bool
pass_ownership) = 0;
On 2012/07/17 17:14:28, piman wrote:
> So I looked carefully at the implementations. There's only 2 places where
> Destroy(false) is called, and that's internally to the Mac and the VAAPI
> decoders. It's never called externally.
> From what I can tell in both those cases above, Destroy will be called twice,
> once with false on error, which does cleanup but doesn't delete the object,
and
> a second time with true, which doesn't really do any cleanup (because it's
> already been done) but then deletes the object.
> 
> It sounds to me it would be simpler, and better served everywhere to make the
> public API always take ownership (and remove the bool). On Mac and VAAPI
> decoders, you can have a private Cleanup function, that's called from
> StopOnError and NotifyError respectively, as well as Destroy.
> 
> WDYT?

SGTM.  Done.

[piman, 2012-07-17 18:04:07]

Great, I like it.
LGTM

[commit-bot: I haz the power, 2012-07-17 18:07:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/10749019/25030

[Ami GONE FROM CHROMIUM, 2012-07-17 18:07:30]

On 2012/07/17 18:04:07, piman wrote:
> Great, I like it.
> LGTM

Thanks for the attentive review!

[commit-bot: I haz the power, 2012-07-17 19:37:55]

Try job failure for 10749019-25030 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...