Open
Bug 1270747
Opened 8 years ago
Updated 2 years ago
Can't keep playing a m4a audio
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
NEW
People
(Reporter: ayang, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: compat)
Attachments
(2 files)
During writing test case for bug 1068151, I found this audio can't be played after 15s.
Updated•8 years ago
|
Priority: -- → P2
Reporter | ||
Comment 1•8 years ago
|
||
This audio has 6 consecutive errors that exceeds current max audio error count (3). Chrome is ok. Edge is ok. Safari can't play it.
Reporter | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/56880/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56880/
Attachment #8758630 -
Flags: review?(jyavenard)
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Alfredo Yang (:alfredo) from comment #2) Summary of this bug. 1. The consecutive errors is over the max number. 2. MFR stop feeding data due to NeedInput() is false.
Comment 4•8 years ago
|
||
Comment on attachment 8758630 [details] Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR. https://reviewboard.mozilla.org/r/56880/#review53556 ::: dom/media/MediaFormatReader.cpp:760 (Diff revision 1) > aDecoder.mDecodingRequested && > !aDecoder.mDemuxRequest.Exists() && > !aDecoder.HasInternalSeekPending() && > aDecoder.mOutput.Length() <= aDecoder.mDecodeAhead && > (aDecoder.mInputExhausted || !aDecoder.mQueuedSamples.IsEmpty() || > - aDecoder.mTimeThreshold.isSome() || > + aDecoder.mTimeThreshold.isSome() || aDecoder.mNumOfConsecutiveError || If we're there it's because you're handling audio error differently to video errors. I don't think you should. Just like videos, if an error is detected, then the decoder should be flushed and skip to the next keyframe (which with audio would be simply the next frame) then there's no issue about the decoder not calling NeedInput()
Attachment #8758630 -
Flags: review?(jyavenard)
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8758630 [details] Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56880/diff/1-2/
Attachment #8758630 -
Attachment description: MozReview Request: Bug 1270747: keep feeding data when decode error. r?jya → Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR.
Attachment #8758630 -
Flags: review?(jyavenard)
Updated•8 years ago
|
Attachment #8758630 -
Flags: review?(jyavenard) → review-
Comment 6•8 years ago
|
||
Comment on attachment 8758630 [details] Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR. https://reviewboard.mozilla.org/r/56880/#review54578 ::: dom/media/MediaFormatReader.cpp:1227 (Diff revision 2) > } > > if (decoder.mError && > decoder.mError.ref() == MediaDataDecoderError::DECODE_ERROR) { > decoder.mError.reset(); > + decoder.Flush(); YOU need more than flushing the decoder here, you need to call Reset(aTrack). especially following bug 1272964 as skipvideodemuxtonextkeyframe no longer flush/reset things ::: dom/media/MediaFormatReader.cpp:1236 (Diff revision 2) > } > LOG("%s decoded error count %d", TrackTypeToStr(aTrack), > decoder.mNumOfConsecutiveError); > media::TimeUnit nextKeyframe; > if (aTrack == TrackType::kVideoTrack && !decoder.HasInternalSeekPending() && > NS_SUCCEEDED(decoder.mTrackDemuxer->GetNextRandomAccessPoint(&nextKeyframe))) { this is not right. getnextrandomaccesspoimt will never error really, but it may return INT64_MAX when theres no future keyframe. youre not handling this case. plus what if there arent future keyframe, playback will now stall, shouldnt you reject the promise with an error? ::: dom/media/MediaFormatReader.cpp:1240 (Diff revision 2) > if (aTrack == TrackType::kVideoTrack && !decoder.HasInternalSeekPending() && > NS_SUCCEEDED(decoder.mTrackDemuxer->GetNextRandomAccessPoint(&nextKeyframe))) { > SkipVideoDemuxToNextKeyFrame(decoder.mLastSampleTime.refOr(TimeInterval()).Length()); > return; > + } else if (aTrack == TrackType::kAudioTrack) { > + NotifyDecodingRequested(TrackInfo::kAudioTrack); You can return early here.
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8758630 [details] Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56880/diff/2-3/
Attachment #8758630 -
Flags: review- → review?(jyavenard)
Updated•8 years ago
|
Attachment #8758630 -
Flags: review?(jyavenard) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8758630 [details] Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR. https://reviewboard.mozilla.org/r/56880/#review55034 ::: dom/media/MediaFormatReader.cpp:64 (Diff revision 3) > MediaDataDemuxer* aDemuxer, > VideoFrameContainer* aVideoFrameContainer, > layers::LayersBackend aLayersBackend) > : MediaDecoderReader(aDecoder) > , mAudio(this, MediaData::AUDIO_DATA, Preferences::GetUint("media.audio-decode-ahead", 2), > - Preferences::GetUint("media.audio-max-decode-error", 3)) > + Preferences::GetUint("media.audio-max-decode-error", 6)) I'm no fan of such value. Why 6 and not 9, or 10 or 1? What scenario should we use to determine that we've got too many errors and that there's no point continuing? ::: dom/media/MediaFormatReader.cpp:1232 (Diff revision 3) > NotifyError(aTrack); > return; > } > LOG("%s decoded error count %d", TrackTypeToStr(aTrack), > decoder.mNumOfConsecutiveError); > + if (aTrack == TrackType::kVideoTrack && !decoder.HasInternalSeekPending()) { I thought I had commented about this in the previous review. But you can't have an internal seek pending going on here. Decoding is suspended during seeking. So you can remove that extra test HasInternalSeekPending. The use of switch/case would be nicer here too. ::: dom/media/MediaFormatReader.cpp:1233 (Diff revision 3) > return; > } > LOG("%s decoded error count %d", TrackTypeToStr(aTrack), > decoder.mNumOfConsecutiveError); > + if (aTrack == TrackType::kVideoTrack && !decoder.HasInternalSeekPending()) { > - media::TimeUnit nextKeyframe; > + media::TimeUnit nextKeyframe; drop the media:: ::: dom/media/MediaFormatReader.cpp:1238 (Diff revision 3) > - media::TimeUnit nextKeyframe; > + media::TimeUnit nextKeyframe; > - if (aTrack == TrackType::kVideoTrack && !decoder.HasInternalSeekPending() && > - NS_SUCCEEDED(decoder.mTrackDemuxer->GetNextRandomAccessPoint(&nextKeyframe))) { > + if (NS_FAILED(decoder.mTrackDemuxer->GetNextRandomAccessPoint(&nextKeyframe))) { > + NotifyError(aTrack); > + } > + if (!nextKeyframe.IsValid() || nextKeyframe.IsInfinite()) { > + NotifyError(aTrack); after NotifyError, there's nothing more you can do as such error are fatal. so return early as otherwise you will unecessarily look for the next keyframe
Reporter | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/56880/#review55034 > I'm no fan of such value. Why 6 and not 9, or 10 or 1? > > What scenario should we use to determine that we've got too many errors and that there's no point continuing? That's a problem I don't have any idea currently. It looks like chrome has higher level of tolerance than us. It may be worth to check chrome's design.
Reporter | ||
Comment 10•8 years ago
|
||
Comment on attachment 8758630 [details] Bug 1270747: increase audio error count and flush decoder when DECODE_ERROR. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56880/diff/3-4/
Mass change P2 -> P3
Priority: P2 → P3
Comment 12•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: ayang → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•