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)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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
Depends on: 1298617
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 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 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 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 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 on attachment 8785740 [details]
Bug 1297036: [MSE] P6. Fix invalid mochitest.

https://reviewboard.mozilla.org/r/74822/#review72672
Attachment #8785740 - Flags: review?(gsquelart) → review+
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
https://hg.mozilla.org/mozilla-central/rev/1d7058e01654
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: