Enforce Stop() is always called before dtor in FFmepgVideoDecoder.

Enforce Stop() is always called before dtor in FFmepgVideoDecoder.

Enforce this because Stop() needs to do some shutdown work asynchronously.
- Updated comments in VideoDecoder interface.
- Added DCHECK in dtor.
- Updated FFmpegVideoDecoderTest to call Stop() in dtor.

BUG=93932, 152262
TEST=Updated media_unittest

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

[xhwang, 2012-09-25 20:50:54]

As discussed offline. PTAL!

[scherkus (not reviewing), 2012-09-25 20:58:16]

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:60: // If the VideoDecoder has been initialized,
Stop() must be called before the
/must be called/must complete/

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:60: // If the VideoDecoder has been initialized,
Stop() must be called before the
s/VideoDecoder/decoder/

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:61: // VideoDecoder can be destructed.
s/the VideoDecoder can be destructed/deleting the decoder/

http://codereview.chromium.org/10990039/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10990039/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:133: .WillRepeatedly(Invoke(this,
&FFmpegVideoDecoderTest::CancelDecrypt));
WillOnce doesn't work?

http://codereview.chromium.org/10990039/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:140: .WillRepeatedly(Invoke(this,
&FFmpegVideoDecoderTest::CancelDecrypt));
ditto

[scherkus (not reviewing), 2012-09-25 20:58:30]

...but LGTM w/ those nits

[Ami GONE FROM CHROMIUM, 2012-09-25 21:18:26]

drive-by

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:60: // If the VideoDecoder has been initialized,
Stop() must be called before the
Initialize() is async; "has been initialized" is unknowable by the caller. 
Replace with
If Initialize() has been called, Stop() must be called...
?

[scherkus (not reviewing), 2012-09-25 21:36:00]

+1 to fischman
On Sep 25, 2012 2:18 PM, <fischman@chromium.org> wrote:

> drive-by
>
>
> http://codereview.chromium.**org/10990039/diff/1/media/**
>
base/video_decoder.h<http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h>
> File media/base/video_decoder.h (right):
>
> http://codereview.chromium.**org/10990039/diff/1/media/**
>
base/video_decoder.h#newcode60<http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#newcode60>
> media/base/video_decoder.h:60: // If the VideoDecoder has been
> initialized, Stop() must be called before the
> Initialize() is async; "has been initialized" is unknowable by the
> caller.  Replace with
> If Initialize() has been called, Stop() must be called...
> ?
>
>
http://codereview.chromium.**org/10990039/<http://codereview.chromium.org/109...
>

[xhwang, 2012-09-25 21:42:31]

On 2012/09/25 21:36:00, scherkus wrote:
> +1 to fischman
> On Sep 25, 2012 2:18 PM, <mailto:fischman@chromium.org> wrote:
> 
> > drive-by
> >
> >
> > http://codereview.chromium.**org/10990039/diff/1/media/**
> >
>
base/video_decoder.h<http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h>
> > File media/base/video_decoder.h (right):
> >
> > http://codereview.chromium.**org/10990039/diff/1/media/**
> >
>
base/video_decoder.h#newcode60<http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#newcode60>
> > media/base/video_decoder.h:60: // If the VideoDecoder has been
> > initialized, Stop() must be called before the
> > Initialize() is async; "has been initialized" is unknowable by the
> > caller.  Replace with
> > If Initialize() has been called, Stop() must be called...
> > ?
> >
> >
>
http://codereview.chromium.**org/10990039/%3Chttp://codereview.chromium.org/1...>
> >

Do we want to call Stop() if Initialize() failed? Shouldn't the caller know if
initialization was successful or not by checking parameters in the status_cb? (I
assume the caller should not make any other VideoDecoder calls before status_cb
completes).

[Ami GONE FROM CHROMIUM, 2012-09-25 22:23:56]

> Do we want to call Stop() if Initialize() failed? Shouldn't the caller know if
> initialization was successful or not by checking parameters in the status_cb?
(I
> assume the caller should not make any other VideoDecoder calls before
status_cb
> completes).

From IM: 
you can make that [the assertion that Stop() must not be called until after
Initialize has returned OK] part of the API contract, but it isn't today.
you should audit callers to make sure it's not violated by ToT.

[xhwang, 2012-09-26 01:09:21]

On 2012/09/25 22:23:56, Ami Fischman wrote:
> > Do we want to call Stop() if Initialize() failed? Shouldn't the caller know
if
> > initialization was successful or not by checking parameters in the
status_cb?
> (I
> > assume the caller should not make any other VideoDecoder calls before
> status_cb
> > completes).
> 
> From IM: 
> you can make that [the assertion that Stop() must not be called until after
> Initialize has returned OK] part of the API contract, but it isn't today.
> you should audit callers to make sure it's not violated by ToT.

I realized that we should not wait for the Initialize() to complete to be able
to call Stop(). Some VideoDecoder implementations (e.g. DVD) may hold the
|status_cb| and call it later asynchronously. We should be able to Stop() a
decoder when it's initialization is still pending.

I chatted w/ scherkus@ offline about this. We agree that your suggestion of "If
Initialize() has been called, Stop() must be called" sounds good to us. If
Initialize() failed, it won't hurt to ask the caller to call Stop().

[xhwang, 2012-09-26 01:09:58]

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:60: // If the VideoDecoder has been initialized,
Stop() must be called before the
On 2012/09/25 20:58:16, scherkus wrote:
> /must be called/must complete/

Done.

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:60: // If the VideoDecoder has been initialized,
Stop() must be called before the
On 2012/09/25 20:58:16, scherkus wrote:
> s/VideoDecoder/decoder/

Done.

http://codereview.chromium.org/10990039/diff/1/media/base/video_decoder.h#new...
media/base/video_decoder.h:61: // VideoDecoder can be destructed.
On 2012/09/25 20:58:16, scherkus wrote:
> s/the VideoDecoder can be destructed/deleting the decoder/

Done.

http://codereview.chromium.org/10990039/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10990039/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:133: .WillRepeatedly(Invoke(this,
&FFmpegVideoDecoderTest::CancelDecrypt));
On 2012/09/25 20:58:16, scherkus wrote:
> WillOnce doesn't work?

WillOnce works for Reset. Updated.

http://codereview.chromium.org/10990039/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:140: .WillRepeatedly(Invoke(this,
&FFmpegVideoDecoderTest::CancelDecrypt));
On 2012/09/25 20:58:16, scherkus wrote:
> ditto

If the decoder is not initialized (including already stopped), CancelDecrypt()
will not be called. So it's really either not called or called once. Updated to
use AtMost(1).

[xhwang, 2012-09-26 01:25:30]

Added more checks in the dtor. PTAL!

[scherkus (not reviewing), 2012-09-26 05:20:01]

lgtm++ w/ nits

http://codereview.chromium.org/10990039/diff/4002/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10990039/diff/4002/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:144: EXPECT_CALL(*decryptor_,
CancelDecrypt())
hmm... can we set this expectation if decrypt_cb_ != null?

that would be even better than using AtMost(1) :)

[xhwang, 2012-09-26 17:53:38]

On 2012/09/26 05:20:01, scherkus wrote:
> lgtm++ w/ nits
> 
>
http://codereview.chromium.org/10990039/diff/4002/media/filters/ffmpeg_video_...
> File media/filters/ffmpeg_video_decoder_unittest.cc (right):
> 
>
http://codereview.chromium.org/10990039/diff/4002/media/filters/ffmpeg_video_...
> media/filters/ffmpeg_video_decoder_unittest.cc:144: EXPECT_CALL(*decryptor_,
> CancelDecrypt())
> hmm... can we set this expectation if decrypt_cb_ != null?
> 
> that would be even better than using AtMost(1) :)

decryptor_->CancelDecrypt() will be called even if decrypt_cb_ == null. The
reason that CancelDecrypt() may not be called is because we have a short cut
(http://codereview.chromium.org/10990039/diff/4002/media/filters/ffmpeg_video_...)
in FFVD::Stop().

[commit-bot: I haz the power, 2012-09-26 21:29:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10990039/4002

[commit-bot: I haz the power, 2012-09-26 23:42:02]

Change committed as 158931