1286441 - It is incorrect to use IsEmpty() to check InputExhausted()

Status: RESOLVED INVALID

(Core :: Audio/Video: Playback, defect, P2)

Description

[Alfredo Yang (:alfredo)]

https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/dom/media/platforms/wmf/WMFMediaDataDecoder.cpp#152

I believe other platform decoders has the same problem. I'll fix them later.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Bug 1286441: do not use TaskQueue->IsEmpty() to check input task is exist or not.

Review commit: https://reviewboard.mozilla.org/r/63846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63846/

Comment 2

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/63846/#review60888

What's the rationale behind this change?

Comment 3

[Alfredo Yang (:alfredo)]

https://reviewboard.mozilla.org/r/63846/#review60888

I believe TaskQueue->IsEmpty() is for FlushableTaskQueue which has decoder task in it only.
After FlushableTaskQueue is discarded, there are other tasks like WMFMediaDataDecoder::SetSeekThreshold in the queue and decoder stops asking for sample.
It is the root cause of the intermittent error in https://bugzilla.mozilla.org/show_bug.cgi?id=1237806#c30.

Comment 4

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

TaskQueue::IsEmpty() has nothing to do with if a queue is flushable or not. I think the logic is fragile which assumes all tasks in the queue are input tasks.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #4)
> TaskQueue::IsEmpty() has nothing to do with if a queue is flushable or not.
> I think the logic is fragile which assumes all tasks in the queue are input
> tasks.

ultimately they are.

Sure, a task can be fired to set the seek target, but setting the seek target is done prior decoding starting. While decoding has started as such, all tasks in a queue are input task.

I have a hard time to believe it could cause intermittent error, especially due to the decode ahead logic in MFR which means that even if InputExhausted isn't called, MediaDataDecoder::Input will still be called

Comment 6

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to JW Wang [:jwwang] from comment #4)
> > TaskQueue::IsEmpty() has nothing to do with if a queue is flushable or not.
> > I think the logic is fragile which assumes all tasks in the queue are input
> > tasks.
> 
> ultimately they are.
> 
> Sure, a task can be fired to set the seek target, but setting the seek
> target is done prior decoding starting. While decoding has started as such,
> all tasks in a queue are input task.
> 
> I have a hard time to believe it could cause intermittent error, especially
> due to the decode ahead logic in MFR which means that even if InputExhausted

IMHO, decoder ahead doesn't help here. Because WMF decoder has long sample queue, that causes aDecoder.mNumSamplesInput - aDecoder.mNumSamplesOutput > mDecodeAhead.

> isn't called, MediaDataDecoder::Input will still be called

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: test_MediaSource_mp4.txt

Attachment is a log from try.

The WMF decoder doesn't output any decoded sample after 21 input raw sample. And the last video input creates a input task in WMF taskqueue. And following a set-threshold task issued by SetVideoDecodeThreshold in WMF taskqueue. So there are 2 tasks in the WMF taskqueue.
At the end of input task, it checks the TaskQueue_>IsEmpty() is false and misjudges it is an another input task queue. And the result is decoder stops requesting sample from MFR.

19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Processing update for Video
19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:21 out:0 qs=21 pending:0 waiting:0 ahead:0 sid:27
19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Giving Video input to decoder
19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Input:1630000 (dts:1563344 kf:0)
19:14:09     INFO -  [MediaPlayback #4]: D/MediaFormatReader MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to 1363333

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang (:alfredo) from comment #7)
> Created attachment 8770790 [details]
> test_MediaSource_mp4.txt
> 
> Attachment is a log from try.
> 
> The WMF decoder doesn't output any decoded sample after 21 input raw sample.
> And the last video input creates a input task in WMF taskqueue. And
> following a set-threshold task issued by SetVideoDecodeThreshold in WMF
> taskqueue. So there are 2 tasks in the WMF taskqueue.
> At the end of input task, it checks the TaskQueue_>IsEmpty() is false and
> misjudges it is an another input task queue. And the result is decoder stops
> requesting sample from MFR.
> 
> 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader
> MediaFormatReader(1d1e2000)::Update: Processing update for Video
> 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader
> MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:21
> out:0 qs=21 pending:0 waiting:0 ahead:0 sid:27
> 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader
> MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Giving Video input to
> decoder
> 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader
> MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Input:1630000
> (dts:1563344 kf:0)
> 19:14:09     INFO -  [MediaPlayback #4]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to
> 1363333

I don't believe that's the cause of the problem you describe and to me it indicates another problem.
SetVideoDecodeThreshold should only ever be called after a Seek (which is always preceded by a call to ResetDecode that flush the decoders) or if the decoder has just been recreated.

As such, you should never have a call to Input preceding a call to SetVideoDecodeThreshold and even less so a pending decoded samples in the queue (where IsEmpty would be check)

So you removing the use of TaskQueue::IsEmpty is I believe just covering another problem.

Glancing at the MediaFormatReader code, I don't see how the case you describe could occur, but if it does set fix is ensuring SetVideoDecodeThreshold isn't called after decoding has started. It should only ever be called *before* and this is even more important as IIRC there's such assumption in the decoder which allows to not have to use a monitor when access the threshold value.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8770382 [details]
Bug 1286441: do not use TaskQueue->IsEmpty() to check input task is exist or not.

https://reviewboard.mozilla.org/r/63846/#review61164

I believe this is papering the problem without resolving on why the SetVideoDecodeThreshold is called after decoding has started.

Comment 10

[Blake Wu [:bwu][:blakewu]]

Alfredo, 
Are you still working on this bug?

Comment 11

[Alfredo Yang (:alfredo)]

No, you can close it directly. Although I still think it is incorrectly way to use IsEmpty().

Comment 12

[Blake Wu [:bwu][:blakewu]]

ok. close it.