Closed Bug 1940883 Opened 1 month ago Closed 29 days ago

Media playback stalls when out of data without dispatching "waiting" (or transitioning to readyState < HAVE_FUTURE_DATA)

Categories

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

defect

Tracking

()

RESOLVED FIXED
136 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox-esr128 --- wontfix
firefox134 --- wontfix
firefox135 --- wontfix
firefox136 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(8 files, 2 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

The test for bug 1915045 times out intermittently on Linux at least for the reason described below.

The test removes data from the audio SourceBuffer after "canplay".
Logging indicates that the MediaFormatReader

  1. Requests demux of more audio,
  2. Sets mAudio.mReceivedNewData, in NotifyTrackDemuxers(), presumably in response to the SourceBuffer operation,
  3. Triggers a drain, is response to demux failure with NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA,
  4. Completes the drain of the audio decoder,
  5. Starts an InternalSeek(),
  6. Skips rejection of the RequestAudioData() promise because mReceivedNewData is set,
  7. Clears decoder.mDrainState,
  8. Fails the seek from 5,
  9. Clears mReceivedNewData,
  10. Starts another InternalSeek() because mReceivedNewData was set,
  11. Sets decoder.mWaitingForDataStartTime again in response to the seek failing with NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA,
  12. Skips rejection of the RequestAudioData() promise again because decoder.mDrainState has been reset.

This could be triggered when running out of data, and presumably appending data for the current playback position would trigger recovery through a successful seek, but the bug means that content is missing the notification that playback is waiting on more data.

Removal of the audio is not necessary to trigger 2. Other SourceBuffer operations would also do this.

Blocks: yt-playback
Severity: -- → S2

I guess Update() would have tried to demux more samples again before needInput was changed to false when HasInternalSeekPending().

That changeset also introduced the logic to go directly to the second internal seek without another demux attempt.

Keywords: regression
Regressed by: 1269408

Set release status flags based on info from the regressing bug 1269408

UpdateReceivedNewData() would have aborted Update() early on
mSeekRequest.Exists() if there were an external seek, so at this point any
seek request would be internal.

This doesn't change behavior because needInput is false when
HasInternalSeekPending(), but merely clarifies that nothing more will
happen.

No behavior change, except in logging when !decoder.HasPromise().

The mWaiting check is no longer necessary since
https://hg.mozilla.org/mozilla-central/rev/df20f438502df1a33c71aea95d3ded5b84a2d2a7#l1.34

needInput is false if an internal seek is pending and so Update() will
return before doing anything further.

If decoder.HasPromise() then this waiting for data test is not even reached
because a previous early return is taken on IsInternalSeekPending().

The behavior of continuing to demux and decode to skip frames when
mTimeThreshold->mHasSeeked is maintained.

This came from
https://hg.mozilla.org/mozilla-central/rev/271a6b2de3ad4be77b1c91305f5c5a49d4924120#l1.14

IsWaitingForData() always returns false because
mWaitingForDataStartTime.reset() is called above.

Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6820e30c2a77 consider rejecting MediaData promise when internal seek is waiting on data r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f594e6f00208 return early even when an internal seek is waiting r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1216b45b86ea simplify waiting for data early return logic a little r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/37fadc8438b4 remove IsWaitingForData() call that always returns false r=media-playback-reviewers,padenot https://hg.mozilla.org/integration/autoland/rev/0268263a1ef1 add some media resource waiting and decode error logging r=media-playback-reviewers,padenot
Regressions: 1941164
Resolution: FIXED → WONTFIX
Target Milestone: 136 Branch → ---
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → RESOLVED
Closed: 1 month ago1 month ago
Resolution: --- → WONTFIX

Still intending to fix this, once Bug 1941164 is understood.

Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Regressions: 1941256

The crash state could be reached via this sequence of events:

  1. An internal seek successfully seeks to the preceding random access point.
  2. Demux of samples is requested while decoder output is less than the seek target time.
  3. The demux fails with NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA, requests a drain and calls NotifyWaitingForData(), which sets decoder.mTimeThreshold->mWaiting.
  4. HasInternalSeekWaiting() now returns true even though HasInternalSeekPending() is false.
  5. mDrainState is cleared even though not complete.
  6. Another demux is requested and fails, which triggers another drain.

Backed out changeset 1216b45b86ea (Bug 1940883) due to risk of unproductive cpu usage : https://hg.mozilla.org/integration/autoland/rev/5ee87c6b2828c6f7d32fd939fe19f8d0e02fd68d

Flags: needinfo?(karlt)

What I missed in writing https://hg.mozilla.org/mozilla-central/rev/1216b45b86ea also was that mWaiting can be set even when the MediaTrackDemuxer::Seek() has completed the seek to a random access point, but a demux has failed while Update() is trying to advance through dependent frames before the target. HasInternalSeekPending() is false, but Update() should not (repeatedly) schedule another demux until mReceivedNewData is set.

Flags: needinfo?(karlt)
Attachment #9446747 - Attachment is obsolete: true
Attachment #9446746 - Attachment is obsolete: true
Attachment #9446745 - Attachment description: Bug 1940883 consider rejecting MediaData promise when internal seek is waiting on data r?#media-playback-reviewers → Bug 1940883 reject MediaData promise when waiting on resource data with no drain r?#media-playback-reviewers
Keywords: leave-open
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/01b46c4c797f add comments and assertions to MediaFormatReader r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/069caa4d0484 reset mPreviousDecodedKeyframeTime_us after dropping frames only for video tracks r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff56d620f395 skip an array allocation and clarify that demux happens only when mQueuedSamples is empty r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/080623fd072c set mWaitingForDataStartTime only if not already set r=media-playback-reviewers,padenot
Keywords: leave-open
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/742de48395f3 add a state diagram for MediaFormatReader::Update() r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a47c8e651937 reject MediaData promise when waiting on resource data with no drain r=media-playback-reviewers,padenot
Status: REOPENED → RESOLVED
Closed: 1 month ago29 days ago
Resolution: --- → FIXED
Target Milestone: --- → 136 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: