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

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1297036: [MSE] P1. Add mochitest to verify behavior.

Seeking in the buffered range should always succeed, even if we have no data in a given track

Review commit: https://reviewboard.mozilla.org/r/74812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74812/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1297036: [MSE] P2. Make seek always succeed when attempting to seek past the end time.

Review commit: https://reviewboard.mozilla.org/r/74814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74814/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1297036: P3. Revert "Bug 1293646: [MSE] P2. Only reject a seek request with EOS if it's passed the explicit duration."

This reverts commit 5a949eb358e27

Another more complete solution will follow.

Review commit: https://reviewboard.mozilla.org/r/74816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74816/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1297036: [MSE] P4. Only report end of stream when reaching the end.

Review commit: https://reviewboard.mozilla.org/r/74818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74818/

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1297036: [MSE] P5. Make fuzz research consistent.

The aim is to only allow skipping gaps of fuzz=500ms.

Review commit: https://reviewboard.mozilla.org/r/74820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74820/

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1297036: [MSE] P6. Fix invalid mochitest.

The test accidentally worked because any demuxing failures in ended mode would be treated as EOS. There's no audio between [0-3), so playback couldn't start

Review commit: https://reviewboard.mozilla.org/r/74822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74822/

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8785735 [details]
Bug 1297036: [MSE] P1. Add mochitest to verify behavior.

https://reviewboard.mozilla.org/r/74812/#review72656

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.)

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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'

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8785740 [details]
Bug 1297036: [MSE] P6. Fix invalid mochitest.

https://reviewboard.mozilla.org/r/74822/#review72672

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785735 [details]
Bug 1297036: [MSE] P1. Add mochitest to verify behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74812/diff/1-2/

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785736 [details]
Bug 1297036: [MSE] P2. Make seek always succeed when attempting to seek past the end time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74814/diff/1-2/

Comment 16

[Jean-Yves Avenard [:jya]]

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."

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74816/diff/1-2/

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785738 [details]
Bug 1297036: [MSE] P4. Only report end of stream when reaching the end.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74818/diff/1-2/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785739 [details]
Bug 1297036: [MSE] P5. Make fuzz research consistent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74820/diff/1-2/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785740 [details]
Bug 1297036: [MSE] P6. Fix invalid mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74822/diff/1-2/

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785735 [details]
Bug 1297036: [MSE] P1. Add mochitest to verify behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74812/diff/2-3/

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785736 [details]
Bug 1297036: [MSE] P2. Make seek always succeed when attempting to seek past the end time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74814/diff/2-3/

Comment 22

[Jean-Yves Avenard [:jya]]

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."

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74816/diff/2-3/

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785738 [details]
Bug 1297036: [MSE] P4. Only report end of stream when reaching the end.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74818/diff/2-3/

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785739 [details]
Bug 1297036: [MSE] P5. Make fuzz research consistent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74820/diff/2-3/

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785740 [details]
Bug 1297036: [MSE] P6. Fix invalid mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74822/diff/2-3/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d7058e01654

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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