[MSE] Seeking in the buffered range should always succeed.

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1294154
(Assignee)

Updated

3 years ago
Depends on: 1298617
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/1d7058e01654
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.