1261312 - MediaDecoderReader::WAITING_FOR_DATA & MediaDecoderReader::CANCELED are not handled in SEEKING mode, a regression from bug 1252762

Status: VERIFIED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

MDSM::OnNotDecoded() calls DispatchDecodeTasksIfNeeded() in MediaDecoderReader::WAITING_FOR_DATA & MediaDecoderReader::CANCELED cases. However, bug 1252762 makes NeedToDecodeAudio()/NeedToDecodeVideo() return FALSE when mState is DECODER_STATE_SEEKING so that DispatchDecodeTasksIfNeeded() do nothing for the SEEKING state.

Comment 1

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

It looks like the seek promise won't be solved by the reader until the buffer containing the target time is added, right? So we will never get WAITING_FOR_DATA during seeking?

Comment 2

[Jean-Yves Avenard [:jya]]

It can happen with MSE. 

When you seek to a target, you're right, the MediaSourceDemuxer check that the target time is in the buffered range, and if so will seek to the keyframe prior target time. From that point on, as far as the MediaFormatReader and MediaSourceDemuxer is concerned, seeking has completed.
The MDSM then call Request*Data until target is reached.

It is theoretically possible for data to be removed from the source buffer during that time. In which case we would enter WAITING_FOR_DATA. We could even enter EOS if data from the source buffer was removed and the mediasource was the ended (mediasource.endOfStream() called)

So, very unlikely in real-life scenarios... but it could happen.

Comment 3

[Jean-Yves Avenard [:jya]]

Now that I think of it, I remember debugging a stall in YouTube due to exactly that scenarios. Keyframes are every 4s with youtube (120 frames) and when seeking, if you quickly changed the resolution (which causes the sourcebuffers to be emptied), you would get an unrecoverable stall.

Comment 4

[Tzuhao Kuo [:kaku]]

One way is getting the MDSM::mDecodeToSeekTarget and its related logics back (which was removed by bug 1252762 and bug 1252766). 

Or, we can simply add the following code snippets into the MSDM::OnNotDecoded() for the MediaDecoderReader::WAITING_FOR_DATA & MediaDecoderReader::CANCELED cases:

if (mState == DECODER_STATE_SEEKING &&  IsAudioDecoding() && AudioQueue().GetSize() == 0) {
  EnsureAudioDecodeTaskQueued();
}

if (mState == DECODER_STATE_SEEKING &&  IsVideoDecoding() && VideoQueue().GetSize() == 0) {
  EnsureVideoDecodeTaskQueued();
}

Comment 5

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

The idea is that we can call Ensure{Audio/Video}DecodeTaskQueued() directly
since the conditions in the DispatchDecodeTasksIfNeeded() has already been
checked.

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

Comment 6

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

Comment on attachment 8738908 [details]
MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

https://reviewboard.mozilla.org/r/44763/#review41499

Comment 7

[Jean-Yves Avenard [:jya]]

"since the conditions in the DispatchDecodeTasksIfNeeded() have already been checked"

plural

Comment 8

[Tzuhao Kuo [:kaku]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> "since the conditions in the DispatchDecodeTasksIfNeeded() have already been
> checked"
> 
> plural

Sorry and thanks! I will modify it while landing the patch.

Comment 9

[Tzuhao Kuo [:kaku]]

The try looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f78be5e7814e

Thanks for the review.

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8738908 [details]
MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44763/diff/1-2/

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27920cf06283

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27920cf06283

Comment 13

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

Need to uplift to 47.

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8738908 [details]
MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

Approval Request Comment
[Feature/regressing bug #]:bug 1261312
[User impact if declined]:some kind of media seeking operations stall (Bug 1261312 comment 3).
[Describe test coverage new/current, TreeHerder]: no test case.
[Risks and why]:It might cause bad media experience.
[String/UUID change made/needed]:none.

Comment 15

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

Isn't beta 46 now?

Comment 16

[Tzuhao Kuo [:kaku]]

(In reply to JW Wang [:jwwang] PTO 4/7 ~ 4/15 from comment #15)
> Isn't beta 46 now?

Yes, you're right. 
https://wiki.mozilla.org/RapidRelease/Calendar

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8738908 [details]
MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

Approval Request Comment
[Feature/regressing bug #]:bug 1261312
[User impact if declined]:some kind of media seeking operations stall (Bug 1261312 comment 3).
[Describe test coverage new/current, TreeHerder]: no test case.
[Risks and why]:It might cause bad media experience.
[String/UUID change made/needed]:none.

Comment 18

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

bug 1252762 is in 47 but not 46, right?

Comment 19

[Tzuhao Kuo [:kaku]]

(In reply to JW Wang [:jwwang] PTO 4/7 ~ 4/15 from comment #18)
> bug 1252762 is in 47 but not 46, right?

Yes, it is in 47 so I request for uplifting to aurora now.

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8738908 [details]
MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

Approval Request Comment
[Feature/regressing bug #]:bug 1252762
[User impact if declined]:some kind of media seeking operations stall (Bug 1261312 comment 3).
[Describe test coverage new/current, TreeHerder]: no test case.
[Risks and why]:It might cause bad media experience.
[String/UUID change made/needed]:none.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tzuhao, have you done any manual testing on the patch? It's not ideal to uplift patches to Aurora that are not tested.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8738908 [details]
MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang

Please renominate once comment 21 is addressed.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Is 46 also affected?

Comment 24

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

No. This bug is a regression from bug 1252762 which was landed to 47 only.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 47.  If we could verify this fix it might be nice to uplift to 47.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello, could you please nominate this patch for uplift to Beta47?

Comment 27

[Tzuhao Kuo [:kaku]]

So, this patch passed the try server and has been at the central for a long time without causing problems. I think it's OK to uplift it. Actually, this issue was found via code review (not a real fail case) and is regarded to be a RARE case, so I would say that it is fine to just stay at 48.

Comment 28

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

Ritu, 
I agree with Kaku, and we have not seen any bugs reported due to this problem. Moreover, since we don't have much time to monitor it on Beta47, it would be better to not uplift it at current stage.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Blake Wu [:bwu][:blakewu] from comment #28)
> Ritu, 
> I agree with Kaku, and we have not seen any bugs reported due to this
> problem. Moreover, since we don't have much time to monitor it on Beta47, it
> would be better to not uplift it at current stage.

Perfect! Thanks Kaku and Blake. Appreciate the due diligence here. Even though this is a recent regression, it's a rare scenario and we can let this fix ride the trains and ship in fx48.

Comment 30

[Paul Silaghi, QA [:pauly]]

I've performed a quick spotcheck on youtube.com, vimeo.com, movies.yahoo.com.
No issues were found while seeking and changing resolutions.
Verified fixed FX 48 RC Win 7, Ubuntu 14.04, OS X 10.10.5.

Comment 31

[Tzuhao Kuo [:kaku]]

Thanks for verifying!