Make DefaultDeleter for Video{De|En}codeAccelerator

Make DefaultDeleter for Video{De|En}codeAccelerator

This makes a specialization of base::DefaultDeleter so that scoped_ptr will
use Destroy() instead of the destructor. Without this, it seems wrong to
hold VideoDecodeAccelerator in a scoped_ptr, given that letting scoped_ptr
delete it via the destructor would be an error.

Also moves the destructors to protected, to prevent mistakes.

BUG=

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

[dmichael (off chromium), 2014-05-23 22:27:20]

What do you think of this? I found it confusing that these were owned by
scoped_ptr everywhere, even though it would do the wrong thing by default.

Note specializing DefaultDeleter is somewhat unusual, but not unprecedented:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

I thought it was nicer than requiring something like:
scoped_ptr<VideoDecodeAccelerator, VideoDecodeAccelerator::Deleter>
...everywhere there's a scoped_ptr of one of these. And typedefing the
scoped_ptr also would seem less readable.

With this CL scoped_ptr still conveys the ownership semantics correctly, but
does the right thing by default.

[Ami GONE FROM CHROMIUM, 2014-05-27 20:06:01]

I think this CL addresses a real concern; thanks for raising it.

An alternative to this is to specialize DefaultDeleter<V[DE]A> to LOG(FATAL). 
The benefit of doing that is that that forces releasers/destroyers to ack that
what they're doing is heavy-weight, as opposed to letting things go out of scope
silently.  That change would share this change's benefit of preventing silent
incorrect use.  WDYT?

https://codereview.chromium.org/292183011/diff/20001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/292183011/diff/20001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:215: vda_.reset(NULL);
s/NULL// ?

https://codereview.chromium.org/292183011/diff/20001/media/video/video_encode...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/292183011/diff/20001/media/video/video_encode...
media/video/video_encode_accelerator.h:147: // will delete it properly by
default.
s/delete/Destroy()/

https://codereview.chromium.org/292183011/diff/20001/media/video/video_encode...
media/video/video_encode_accelerator.h:164:
static_cast<media::VideoEncodeAccelerator*>(video_encode_accelerator)
can this be not inline?

[dmichael (off chromium), 2014-05-28 22:14:33]

On 2014/05/27 20:06:01, Ami Fischman wrote:
> I think this CL addresses a real concern; thanks for raising it.
> 
> An alternative to this is to specialize DefaultDeleter<V[DE]A> to LOG(FATAL). 
> The benefit of doing that is that that forces releasers/destroyers to ack that
> what they're doing is heavy-weight, as opposed to letting things go out of
scope
> silently.  That change would share this change's benefit of preventing silent
> incorrect use.  WDYT?
My concern with that idea is that:
1) It's a run-time error. If somebody makes a mistake, do we want to crash, or
is it basically OK for the scoped_ptr to call Destroy? I don't know enough about
what Destroy() does to know if it would be a concern for the scoped_ptr to be
released carelessly instead of purposely/explicitly.
2) We'd be using scoped_ptr, but still not letting scoped_ptr managing releasing
its pointee. As a reader, I get confused/surprised when I see
some_scoped_ptr.release().

You know the domain better than I, so I can go that way if it makes more sense.

(Will fix/respond to other comments separately in a couple minutes)

[Ami GONE FROM CHROMIUM, 2014-05-28 22:24:11]

It's ok for scoped_ptr to delete the resource.  My desire to make destruction
explicit stems from a feeling that my baby is a unique snowflake, unlike your
pedestrian run-of-the-mill resources.

Maybe that's misguided.

I can't come up with a justification for why this should require manual memory
management when so much experience says "scoped" is such a powerful concept for
correctness.

Go forth with my blessing.

[dmichael (off chromium), 2014-05-28 22:31:25]

Thanks for the review! I think I addressed your comments, but not sure about one
of them.

https://codereview.chromium.org/292183011/diff/20001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/292183011/diff/20001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:215: vda_.reset(NULL);
On 2014/05/27 20:06:02, Ami Fischman wrote:
> s/NULL// ?
Oh, right, thanks. Done here and elsewhere.

https://codereview.chromium.org/292183011/diff/20001/media/video/video_encode...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/292183011/diff/20001/media/video/video_encode...
media/video/video_encode_accelerator.h:147: // will delete it properly by
default.
On 2014/05/27 20:06:02, Ami Fischman wrote:
> s/delete/Destroy()/

Done.

https://codereview.chromium.org/292183011/diff/20001/media/video/video_encode...
media/video/video_encode_accelerator.h:164:
static_cast<media::VideoEncodeAccelerator*>(video_encode_accelerator)
On 2014/05/27 20:06:02, Ami Fischman wrote:
> can this be not inline?
operator()'s implementation is inline...  do you mean something else? I can't
fit all this code on one line, but I'm happy to reformat it if there's a way you
prefer.

Maybe you meant where I wrapped; I think I got that style wron.g I moved the
arrow.

Let me know if there's another problem I'm missing.

[dmichael (off chromium), 2014-05-28 22:33:31]

Oh, I feel silly. You meant you want that operator to _not_ be inlined.
I'll go fix that.


On Wed, May 28, 2014 at 4:31 PM, <dmichael@chromium.org> wrote:

> Thanks for the review! I think I addressed your comments, but not sure
> about one
> of them.
>
>
>
> https://codereview.chromium.org/292183011/diff/20001/
> media/filters/gpu_video_decoder.cc
> File media/filters/gpu_video_decoder.cc (right):
>
> https://codereview.chromium.org/292183011/diff/20001/
> media/filters/gpu_video_decoder.cc#newcode215
> media/filters/gpu_video_decoder.cc:215: vda_.reset(NULL);
> On 2014/05/27 20:06:02, Ami Fischman wrote:
>
>> s/NULL// ?
>>
> Oh, right, thanks. Done here and elsewhere.
>
>
> https://codereview.chromium.org/292183011/diff/20001/
> media/video/video_encode_accelerator.h
> File media/video/video_encode_accelerator.h (right):
>
> https://codereview.chromium.org/292183011/diff/20001/
> media/video/video_encode_accelerator.h#newcode147
> media/video/video_encode_accelerator.h:147: // will delete it properly
> by default.
> On 2014/05/27 20:06:02, Ami Fischman wrote:
>
>> s/delete/Destroy()/
>>
>
> Done.
>
>
> https://codereview.chromium.org/292183011/diff/20001/
> media/video/video_encode_accelerator.h#newcode164
> media/video/video_encode_accelerator.h:164:
> static_cast<media::VideoEncodeAccelerator*>(video_encode_accelerator)
> On 2014/05/27 20:06:02, Ami Fischman wrote:
>
>> can this be not inline?
>>
> operator()'s implementation is inline...  do you mean something else? I
> can't fit all this code on one line, but I'm happy to reformat it if
> there's a way you prefer.
>
> Maybe you meant where I wrapped; I think I got that style wron.g I moved
> the arrow.
>
> Let me know if there's another problem I'm missing.
>
> https://codereview.chromium.org/292183011/
>

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

[dmichael (off chromium), 2014-05-28 22:53:53]

Okay, I think it's ready for review now, thanks!

[dmichael (off chromium), 2014-05-28 22:53:56]

Okay, I think it's ready for review now, thanks!

[Ami GONE FROM CHROMIUM, 2014-05-28 23:00:47]

LGTM 
Thanks for the improvement!

[dmichael (off chromium), 2014-05-28 23:05:06]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-05-28 23:08:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001

[commit-bot: I haz the power, 2014-05-29 03:30:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-29 04:09:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 04:09:44]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[dmichael (off chromium), 2014-05-29 15:49:11]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-05-29 15:51:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001

[commit-bot: I haz the power, 2014-05-29 16:14:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-29 16:59:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 16:59:35]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[dmichael (off chromium), 2014-05-29 20:40:15]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:43:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001

[commit-bot: I haz the power, 2014-05-29 21:15:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-29 21:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 21:41:17]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[dmichael (off chromium), 2014-05-30 15:19:16]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:20:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001

[commit-bot: I haz the power, 2014-05-30 15:39:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-30 16:48:44]

Change committed as 273860