1053008 - MediaDecoderStateMachine::FlushDecoding() not giving ResetDecode a chance to run

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Spin off from bug 1046837

https://bugzilla.mozilla.org/show_bug.cgi?id=1046837#c38

> mDecodeTaskQueue->Flush(); at [1] is suspicious to me.
> 
> Flush() doesn't guarantee MediaDecoderReader::ResetDecode() dispatched above
> will be executed. It looks like we should call AwaitIdle() or reverse the
> order of Flush()/MediaDecoderReader::ResetDecode().
> 
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/e6614d8d85f9/content/media/
> MediaDecoderStateMachine.cpp#l2462


(In reply to Chris Pearce (:cpearce) from comment #48)
> (In reply to JW Wang [:jwwang] from comment #38)
> > mDecodeTaskQueue->Flush(); at [1] is suspicious to me.
> > 
> > Flush() doesn't guarantee MediaDecoderReader::ResetDecode() dispatched above
> > will be executed. It looks like we should call AwaitIdle() or reverse the
> > order of Flush()/MediaDecoderReader::ResetDecode().
> 
> You're right. That Flush() should be an AwaitIdle(). Good catch!
> 
> The idea here was to purge events in the task queue that would deliver
> samples. But in hindsight, I don't think that's safe; the Reader may be
> using the task queue for something else, and it will likely not expect this
> behaviour.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

The purpose of Flush() is to abort any existing decoding tasks (since SHUTDOWN doesn't care about it anymore) so that SHUTDOWN can proceed as fast as possible. It looks sub-optimal for AwaitIdle() waits for decoding tasks to finish which we don't need anymore during SHUTDOWN.

However, I don't think it will make a noticable difference as long as the machine runs fast enough.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Btw, for the async behavior of Request{Audio|Video}Data (though it is not truly async for now), it is possible for tasks to be enqueued and run later than MediaDecoderReader::ResetDecode which is the situation we want to prevent.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah. Based on the comment there, we do need the Flush(), otherwise we get problems on B2G... I recall I got backed out without the Flush there.

We could do Flush(), syncDispatch(ResetDecode task), Flush().

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Sounds promising. Given the code complexity of our media stack, I would also like to add some assertions to ensure the code flow proceed as expected and no tasks enqueued/run after the 2nd Flush().

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: WIP Patch

Here's a WIP patch. Once Try re-opens, we can test this cross platforms easier...

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://tbpl.mozilla.org/?tree=Try&rev=2e1cfc7c2f16

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Compile errors in first push, trying again:
https://tbpl.mozilla.org/?tree=Try&rev=85a60179fb27

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Bug 1033912 will allow running decoding tasks in separate MediaTaskQueues. It is possible for MediaCodecReader::DecodeAudioDataTask() to call GetCallback()->OnAudioDecoded() which will post a task back to |MediaDecoderStateMachine::mDecodeTaskQueue|, even after MediaDecoderReader::ResetDecode() is run. Is that something we want to avoid too?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

After a seek, all samples up to the next "discontinuity" are ignored, so if OnAudioDecoded() is called after a seek with data from before the seek, it will just be ignored. The Reader therefore, needs to be careful that after ResetDecode is called, only the first sample that is input and decoded after the ResetDecode() is marked as a discontinuity, not any samples that were pending before the ResetDecode.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Patch; ensure the ResetDecode() task actually runs, and we flush other tasks and reject tasks while flushing.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5211f7049dc0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5211f7049dc0