Closed Bug 1261312 Opened 4 years ago Closed 4 years ago

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

Categories

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

47 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 + wontfix
firefox48 --- verified

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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.
Blocks: 1261020
Blocks: 1253184
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?
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
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.
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();
}
Flags: needinfo?(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/
Attachment #8738908 - Flags: review?(jwwang)
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
Attachment #8738908 - Flags: review?(jwwang) → review+
Flags: needinfo?(jwwang)
"since the conditions in the DispatchDecodeTasksIfNeeded() have already been checked"

plural
Assignee: nobody → tkuo
(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.
The try looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f78be5e7814e

Thanks for the review.
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/
Attachment #8738908 - Attachment description: MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r?jwwang → MozReview Request: Bug 1261312 - make sure that audio/video decode task is filed again; r=jwwang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/27920cf06283
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Need to uplift to 47.
Flags: needinfo?(tkuo)
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.
Flags: needinfo?(tkuo)
Attachment #8738908 - Flags: approval-mozilla-beta?
Isn't beta 46 now?
(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 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.
Attachment #8738908 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
bug 1252762 is in 47 but not 46, right?
(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 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.
Tzuhao, have you done any manual testing on the patch? It's not ideal to uplift patches to Aurora that are not tested.
Flags: needinfo?(tkuo)
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.
Attachment #8738908 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Is 46 also affected?
No. This bug is a regression from bug 1252762 which was landed to 47 only.
Tracking for 47.  If we could verify this fix it might be nice to uplift to 47.
Flags: qe-verify+
Hello, could you please nominate this patch for uplift to Beta47?
Flags: needinfo?(bwu)
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.
Flags: needinfo?(kaku)
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.
Flags: needinfo?(bwu)
(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.
Version: unspecified → 47 Branch
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.
Status: RESOLVED → VERIFIED
Thanks for verifying!
You need to log in before you can comment on or make changes to this bug.