Closed
Bug 1297036
Opened 8 years ago
Closed 8 years ago
[MSE] Seeking in the buffered range should always succeed.
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
https://github.com/w3c/media-source/issues/147 suggestion was adopted. Seeking in buffered range should always succeed. This can be a problem when in ended mode as the buffered range may not indicate data that all trackbuffers contain. When readyState is ended, if we attempt to seek in a track past the highest end time it should succeed and only then return EOS. right now, it returns EOS immediately and so the last frame is never displayed
Updated•8 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8785735 [details] Bug 1297036: [MSE] P1. Add mochitest to verify behavior. https://reviewboard.mozilla.org/r/74812/#review72656
Attachment #8785735 -
Flags: review?(gsquelart) → review+
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8785736 [details] Bug 1297036: [MSE] P2. Make seek always succeed when attempting to seek past the end time. https://reviewboard.mozilla.org/r/74814/#review72658
Attachment #8785736 -
Flags: review?(gsquelart) → review+
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8785737 [details] Bug 1297036: P3. Revert "Bug 1293646: [MSE] P2. Only reject a seek request with EOS if it's passed the explicit duration." https://reviewboard.mozilla.org/r/74816/#review72660
Attachment #8785737 -
Flags: review?(gsquelart) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8785738 [details] Bug 1297036: [MSE] P4. Only report end of stream when reaching the end. https://reviewboard.mozilla.org/r/74818/#review72664 r+ with suggestions: ::: dom/media/mediasource/MediaSourceDemuxer.cpp:382 (Diff revision 1) > } > > RefPtr<MediaSourceTrackDemuxer::SeekPromise> > MediaSourceTrackDemuxer::DoSeek(media::TimeUnit aTime) > { > + typedef TrackBuffersManager::GetSampleResult Result; It seems a bit over-the-top to create a typedef when there's only a couple of nearvy uses of that type. (But fine if you think it's better that way) ::: dom/media/mediasource/MediaSourceDemuxer.cpp:432 (Diff revision 1) > } > > RefPtr<MediaSourceTrackDemuxer::SamplesPromise> > MediaSourceTrackDemuxer::DoGetSamples(int32_t aNumSamples) > { > + typedef TrackBuffersManager::GetSampleResult Result; (same) ::: dom/media/mediasource/MediaSourceDemuxer.cpp:450 (Diff revision 1) > - mManager->IsEnded() ? DemuxerFailureReason::END_OF_STREAM : > + __func__); > - DemuxerFailureReason::WAITING_FOR_DATA, __func__); > } > mReset = false; > } > - bool error = false; > + Result result = Result::NO_ERROR; This 'result' declaration could be moved down, just before its first use; in which case you wouldn't need to initialize it. ::: dom/media/mediasource/TrackBuffersManager.h:165 (Diff revision 1) > already_AddRefed<MediaRawData> GetSample(TrackInfo::TrackType aTrack, > const media::TimeUnit& aFuzz, > - bool& aError); > + GetSampleResult& aResult); Suggesting: aResult being a compulsory out-param that requires the usual awkward uninitialized declaration before use, and the implementation pretty much decides on it at return time, so wouldn't it be worth considering combining it with the returned pointer? E.g.: in a pair or tuple; or even an mfbt Variant, as really you want the result to either be a non-null already_AddRefed on success, or a GetSampleResult on failure. (Just a suggestion, feel free to ignore or open a separate bug for later.)
Attachment #8785738 -
Flags: review?(gsquelart) → review+
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8785739 [details] Bug 1297036: [MSE] P5. Make fuzz research consistent. https://reviewboard.mozilla.org/r/74820/#review72670 r+ with clarification and typo: ::: dom/media/mediasource/TrackBuffersManager.cpp:1989 (Diff revision 1) > uint32_t i = 0; > > if (aTime != TimeUnit()) { > // Determine the interval of samples we're attempting to seek to. > TimeIntervals buffered = trackBuffer.mBufferedRanges; > - buffered.SetFuzz(aFuzz); > + // Fuzz factor is +/- aFuzz, as we want to only eliminate gaps It feels like there's a missing 'but' (Fuzz is..., but as we want...) (If not, please clarify what you meant.) ::: dom/media/mediasource/TrackBuffersManager.cpp:1990 (Diff revision 1) > > if (aTime != TimeUnit()) { > // Determine the interval of samples we're attempting to seek to. > TimeIntervals buffered = trackBuffer.mBufferedRanges; > - buffered.SetFuzz(aFuzz); > + // Fuzz factor is +/- aFuzz, as we want to only eliminate gaps > + // lesser than aFuzz, we set a fuzz factor aFuzz/2. 'lesser' -> 'less'
Attachment #8785739 -
Flags: review?(gsquelart) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8785740 [details] Bug 1297036: [MSE] P6. Fix invalid mochitest. https://reviewboard.mozilla.org/r/74822/#review72672
Attachment #8785740 -
Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•8 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6fc901d75361 [MSE] P1. Add mochitest to verify behavior. r=gerald https://hg.mozilla.org/integration/autoland/rev/b70a6a464388 [MSE] P2. Make seek always succeed when attempting to seek past the end time. r=gerald https://hg.mozilla.org/integration/autoland/rev/1d7058e01654 P3. Revert "Bug 1293646: [MSE] P2. Only reject a seek request with EOS if it's passed the explicit duration." r=gerald https://hg.mozilla.org/integration/autoland/rev/2e03f016cece [MSE] P4. Only report end of stream when reaching the end. r=gerald https://hg.mozilla.org/integration/autoland/rev/1c3a3adf0256 [MSE] P5. Make fuzz research consistent. r=gerald https://hg.mozilla.org/integration/autoland/rev/4c9dcf590dd3 [MSE] P6. Fix invalid mochitest. r=gerald
Comment 27•8 years ago
|
||
backout bugherder |
https://hg.mozilla.org/mozilla-central/rev/1d7058e01654
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6fc901d75361 https://hg.mozilla.org/mozilla-central/rev/b70a6a464388 https://hg.mozilla.org/mozilla-central/rev/2e03f016cece https://hg.mozilla.org/mozilla-central/rev/1c3a3adf0256 https://hg.mozilla.org/mozilla-central/rev/4c9dcf590dd3
You need to log in
before you can comment on or make changes to this bug.
Description
•