Reland: Add Stop() to AudioDecoder. (https://codereview.chromium.org/126793002/)

Reland: Add Stop() to AudioDecoder. (https://codereview.chromium.org/126793002/)

BUG=329379, 335181
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246529

[rileya (GONE FROM CHROMIUM), 2014-01-17 18:50:19]

This is currently the exact patch as before in a new CL.

So there were/are two issues here:

1) A layout test was failing (encrypted-media/encrypted-media-needkey). From
what I can tell, this is because a couple DCHECKs were added.

It looks as though if the decryptor ready callback never gets called (this
layout test seems to exit before this happens), then the
DecryptingAudioDecoder's |init_cb_| is never dispatched, and it's left hanging
when things get cleaned up (and a DCHECK that checks for a null init_cb_ fails).

This also means the AudioDecoderSelector never dispatches its SelectDecoderCB,
thus the AudioRendererImpl never gets a pointer to the decoder to call Stop()
on, leaving the DecryptingAudioDecoder in the kDecryptorRequested state when
it's destroyed (which fails another DCHECK that tests for either kUninitialized
or kStopped in the destructor).

Thoughts on how to go about this?

2) We also saw some crashes in FFmpegAudioDecoder::RunDecodeLoop in the canary.
From what I can tell (the crash was on the first dereference of an FFmpeg
AVPacket in avcodec_decode_audio4), Stop() was called and then the decoder tried
to decode another packet despite FFmpeg resources being freed. I'm still digging
into exactly how this would happen.

[xhwang, 2014-01-17 20:14:08]

On 2014/01/17 18:50:19, rileya wrote:
> This is currently the exact patch as before in a new CL.
> 
> So there were/are two issues here:
> 
> 1) A layout test was failing (encrypted-media/encrypted-media-needkey). From
> what I can tell, this is because a couple DCHECKs were added.
> 
> It looks as though if the decryptor ready callback never gets called (this
> layout test seems to exit before this happens), then the
> DecryptingAudioDecoder's |init_cb_| is never dispatched, and it's left hanging
> when things get cleaned up (and a DCHECK that checks for a null init_cb_
fails).
> 
> This also means the AudioDecoderSelector never dispatches its SelectDecoderCB,
> thus the AudioRendererImpl never gets a pointer to the decoder to call Stop()
> on, leaving the DecryptingAudioDecoder in the kDecryptorRequested state when
> it's destroyed (which fails another DCHECK that tests for either
kUninitialized
> or kStopped in the destructor).
> 
> Thoughts on how to go about this?

For the video part, when this happens, VideoFrameStream (VFS) calls
VideoDecoderSelector::Abort(), which will handle this properly. Now it seems
that AudioDecoderSelector::Abort() is never called. Probably we should call this
in AudioRendererImpl during teardown.
 
> 2) We also saw some crashes in FFmpegAudioDecoder::RunDecodeLoop in the
canary.
> From what I can tell (the crash was on the first dereference of an FFmpeg
> AVPacket in avcodec_decode_audio4), Stop() was called and then the decoder
tried
> to decode another packet despite FFmpeg resources being freed. I'm still
digging
> into exactly how this would happen.

Stop() should not DoStop() if there are pending read or reset callbacks. Can you
help check if we really enforce this correctly?

[DaleCurtis, 2014-01-17 20:32:26]

It'd make this easier to review if you uploaded the original change as patch set
1 and the new patch set as 2+.  Then we can diff between patch sets.

If that's too much work, please comment on the parts that have changed at least.

[DaleCurtis, 2014-01-17 20:33:21]

Oh, I see that it sounds like you're already planning to do that.  The current
PS#1 is unmodified from original landing?

[rileya (GONE FROM CHROMIUM), 2014-01-17 23:09:05]

> For the video part, when this happens, VideoFrameStream (VFS) calls
> VideoDecoderSelector::Abort(), which will handle this properly. Now it seems
> that AudioDecoderSelector::Abort() is never called. Probably we should call
this
> in AudioRendererImpl during teardown.

Ahh, sounds good, I'll give that a try.

> Stop() should not DoStop() if there are pending read or reset callbacks. Can
you
> help check if we really enforce this correctly?

I think this is enforced correctly, but if my current hunch doesn't pan out I'll
take another look. I'm thinking it might be a race condition in
AudioRendererImpl where Read() ends up getting called on the AudioDecoder after
Stop(). 

This is probably hard to follow, but here's a quick brain-dump of what I'm
thinking: In ARI::Stop, calling Stop() on the decoder results in waiting on an
async callback to StopDecoderDone (where previously no async call existed)
before ARI changes to the 'kStopped' state (which tells it not to dispatch any
more reads); so it may be possible for the Decoder to do Stop(), and ARI to
sneak in and initiate another Read() before StopDecoderDone() gets run and
finishes off the state change). I'm working on seeing if I can get a repro unit
test running.

> Oh, I see that it sounds like you're already planning to do that.  The current
> PS#1 is unmodified from original landing?

Yes, it is unchanged at the moment, I don't have working fixes just yet.

[rileya (GONE FROM CHROMIUM), 2014-01-18 00:39:41]

See the new patchset to audio_renderer_impl_unittest.cc for a quick and hacky
repro case of what I think is the most likely cause of the crashes we saw in the
canary (race condition introduced by an async call in AudioRendererImpl). No fix
just yet.

[rileya (GONE FROM CHROMIUM), 2014-01-21 19:08:50]

Okay, fixes for both the canary crash (plus a unit test) and the layout test
failure are set, see comments for details.

https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:213: if (!init_cb_.is_null()) {
This resolves the layout test failure (if we have a pending |init_cb_| Abort the
decoder selector and dispatch an error to the cb).

https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:215:
base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
Is there a better error for this?

https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:222: decoder_->Stop(callback);
The canary crash was caused (so far as I can tell) by a race between
StopDecoderDone and AttemptRead, where we would post a Read() to |decoder_|
after Stop() had already been called on it, resulting in segfault-y fun when we
try to decode with a stopped decoder.

To solve this, I simply removed StopDecoderDone and the async call to it, as I
couldn't actually see any reason it was necessary (I'd originally included it to
match Reset/DecoderResetDone).

[DaleCurtis, 2014-01-21 21:14:39]

https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:215:
base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
On 2014/01/21 19:08:50, rileya wrote:
> Is there a better error for this?

Hmm, I don't see VideoFrameStream / VideoRendererImpl doing this, is that a bug
there or is this unnecessary?

[rileya (GONE FROM CHROMIUM), 2014-01-21 21:17:13]

On 2014/01/21 21:14:39, DaleCurtis wrote:
>
https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
> File media/filters/audio_renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/140823012/diff/140001/media/filters/audio_ren...
> media/filters/audio_renderer_impl.cc:215:
> base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
> On 2014/01/21 19:08:50, rileya wrote:
> > Is there a better error for this?
> 
> Hmm, I don't see VideoFrameStream / VideoRendererImpl doing this, is that a
bug
> there or is this unnecessary?

I was wondering about this. From what I've seen it seems that we're generally
pretty strict about always satisfying callbacks, but I wasn't sure there, I'll
have a closer look at what VRI/VFS are doing...

[rileya (GONE FROM CHROMIUM), 2014-01-21 22:12:15]

Ahh, looks like ARI::OnDecoderSelected had a check for state == kStopped that
early-out-ed which resulted in the init_cb not being called from there. Instead
I can just call Abort on the decoder selector if the ARI is still kUninitialized
at Stop(), and that will call OnDecoderSelector which I changed such that it
will now actually satisfy the init_cb. See latest patchset.

[DaleCurtis, 2014-01-21 22:20:26]

https://codereview.chromium.org/140823012/diff/330001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/330001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:198: if (state_ == kUninitialized) {
If uninitialized there shouldn't be any reason to call abort? VRI has a
kInitializing state which might be good to add here to indicate Initialize is in
progress.  Otherwise this seems wrong since you never transition to kStopped
despite a potential in flight Initialize.

[rileya (GONE FROM CHROMIUM), 2014-01-22 00:01:41]

On 2014/01/21 22:20:26, DaleCurtis wrote:
>
https://codereview.chromium.org/140823012/diff/330001/media/filters/audio_ren...
> File media/filters/audio_renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/140823012/diff/330001/media/filters/audio_ren...
> media/filters/audio_renderer_impl.cc:198: if (state_ == kUninitialized) {
> If uninitialized there shouldn't be any reason to call abort? VRI has a
> kInitializing state which might be good to add here to indicate Initialize is
in
> progress.  Otherwise this seems wrong since you never transition to kStopped
> despite a potential in flight Initialize.

Oh whoops, you're right. Okay, I added a kInitializing state in the most recent
patch.

[DaleCurtis, 2014-01-22 00:27:46]

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:200: callback.Run();
Instead of running this here, I believe you need to have it run during
OnDecoderSelected(); otherwise the callback will be executed before init_cb.

[xhwang, 2014-01-22 03:53:53]

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:200: callback.Run();
Agree with Dale on this. Also, usually we should post the callback instead of
directly running it.

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:221: callback.Run();
ditto, post callback instead of directly firing it.

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:904: ConsumeAllBufferedData();
Shouldn't we make sure that this never happens? If we do so, then we probably
don't need this test.

[rileya (GONE FROM CHROMIUM), 2014-01-22 17:35:27]

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:200: callback.Run();
On 2014/01/22 03:53:54, xhwang wrote:
> Agree with Dale on this. Also, usually we should post the callback instead of
> directly running it.

Done.

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:221: callback.Run();
On 2014/01/22 03:53:54, xhwang wrote:
> ditto, post callback instead of directly firing it.

And done.

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:904: ConsumeAllBufferedData();
On 2014/01/22 03:53:54, xhwang wrote:
> Shouldn't we make sure that this never happens? If we do so, then we probably
> don't need this test.

I guess this was an unrealistic repro case. I've adjusted it to a more feasible
set of events. It calls ConsumeAllBufferedData() before Stop()-ing, which queues
up some ARI::AttemptRead() calls, and then does a RunUntilIdle() after calling
Stop() which dispatches those queued calls.

It's still a somewhat contrived test, but I'd argue it has value in preventing
anyone from doing what I did in the future, since it was a pretty
subtle/annoying to track down race condition (that said, we'll soon move this
logic into the audio equivalent of VideoFrameStream anyway, so this test and any
reason for it existing will die then anyway...).

https://codereview.chromium.org/140823012/diff/480001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/480001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:296: Stop(base::ResetAndReturn(&stop_cb_));
We originally held the lock for the entirety of this function, but since Stop()
also grabs the lock we have to release it before we get here. This resulted in
rearranging this function a fair bit, let me know if you see a better way to lay
out this function's scoping/flow...

[DaleCurtis, 2014-01-22 18:43:25]

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:200: callback.Run();
On 2014/01/22 17:35:28, rileya wrote:
> On 2014/01/22 03:53:54, xhwang wrote:
> > Agree with Dale on this. Also, usually we should post the callback instead
of
> > directly running it.
> 
> Done.

FYI, VideoRendererImpl is not posting a task for these same calls.  Probably a
bug which can be fixed in another CL.

https://codereview.chromium.org/140823012/diff/270002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:221: callback.Run();
On 2014/01/22 17:35:28, rileya wrote:
> On 2014/01/22 03:53:54, xhwang wrote:
> > ditto, post callback instead of directly firing it.
> 
> And done.

Ditto.

https://codereview.chromium.org/140823012/diff/470002/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/470002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:203: if (state_ == kStopped)
This still needs to hit the stop_cb_ run below.

https://codereview.chromium.org/140823012/diff/470002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:301: } else {
Why add the else {, the block above returns early?

[rileya (GONE FROM CHROMIUM), 2014-01-22 18:58:51]

https://codereview.chromium.org/140823012/diff/470002/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/470002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:203: if (state_ == kStopped)
On 2014/01/22 18:43:26, DaleCurtis wrote:
> This still needs to hit the stop_cb_ run below.

Fixed (assuming there's no issue with posting the callback while we are holding
the lock?)

https://codereview.chromium.org/140823012/diff/470002/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:301: } else {
On 2014/01/22 18:43:26, DaleCurtis wrote:
> Why add the else {, the block above returns early? 

I needed to scope the whole block for the AutoLock anyway, so I thought it
looked nicer to add the else. Removed it.

[DaleCurtis, 2014-01-22 19:06:17]

lgtm % {} removal.

https://codereview.chromium.org/140823012/diff/590001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/590001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:304: {
No need to scope since you're holding it for the rest of the function.

[rileya (GONE FROM CHROMIUM), 2014-01-22 19:18:59]

https://codereview.chromium.org/140823012/diff/590001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/140823012/diff/590001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:304: {
On 2014/01/22 19:06:17, DaleCurtis wrote:
> No need to scope since you're holding it for the rest of the function.

Right, makes sense, for some reason I was thinking the compiler might be allowed
to decide to do the initialization at the start of the enclosing scope, but that
wouldn't actually make sense. Removed.

[rileya (GONE FROM CHROMIUM), 2014-01-22 22:01:01]

Anyone have any final thoughts on this?

[xhwang, 2014-01-23 00:03:42]

lgtm, thanks!

[commit-bot: I haz the power, 2014-01-23 00:10:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/140823012/500002

[commit-bot: I haz the power, 2014-01-23 04:48:56]

Change committed as 246529