1293576 - Frames should be treated in pts order when sourceBuffer.mode == "sequence"

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

From spec:
https://w3c.github.io/media-source/index.html#sourcebuffer-coded-frame-processing

step 1: For each coded frame in the media segment run the following steps

and in step 3: If mode equals "sequence" and group start timestamp is set, then run the following steps: 

So when we're in sequence mode, the first sample added will determine the future value of the timestampOffset.

It is a bit ambiguous if there's a requirement to handle each coded frame in pts order or not. Currently we first process all video frames followed by all audio frames. But the end result would be different if we handled the audio frames first.

So for consistency sake, let's process the earliest frame first when in sequence mode.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1293576: [MSE] P1. Always process the earliest frames first when in sequence mode.

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

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1293576: [MSE] P2. Fix mochitest.

The mochitest relied that the video track was processed first. Additionally, change for the file with only a single video track as the previous video didn't have aligned segments, making the use of sequence mode useless.
We swap the segment around, which allow to more easily visually inspect the result (counter goes forward and then back)

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

Comment 3

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

Comment on attachment 8779624 [details]
Bug 1293576: [MSE] P1. Always process the earliest frames first when in sequence mode.

https://reviewboard.mozilla.org/r/70580/#review67954

r+ as it is. But here are a couple of suggestions:

::: dom/media/mediasource/TrackBuffersManager.cpp:1228
(Diff revision 1)
> +    TimeInterval videoInterval =
> +      PresentationInterval(mVideoTracks.mQueuedSamples);
> +    TimeInterval audioInterval =
> +      PresentationInterval(mAudioTracks.mQueuedSamples);
> +    if (audioInterval.mStart < videoInterval.mStart) {

You calculate full spans for both tracks, but in the end you only really need the start time of each (i.e., the minimum 'mTime' of all samples), right?
So instead of 'presentationInterval', you could have something like 'presentationStart' for just that.

::: dom/media/mediasource/TrackBuffersManager.cpp:1344
(Diff revision 1)
> +  TimeInterval presentationInterval =
> +    TimeInterval(TimeUnit::FromMicroseconds(aSamples[0]->mTime),
> +                 TimeUnit::FromMicroseconds(aSamples[0]->GetEndTime()));
> +
> +  for (uint32_t i = 1; i < aSamples.Length(); i++) {
> +    auto& sample = aSamples[i];

Possible simplification:
You could start from 0 if you folded the [0] case into the loop; then you wouldn't really need 'i' inside the loop. And so you could use a range-for loop. (This probably costs one extra assignment, I'd say it's worth it for the clarity.)
Also I think you could make 'sample' a const ref.
E.g.:
  TimeInterval presentationInterval;
  for (const auto& sample : aSamples) {
    presentationInterval = presentationInterval.Span(
      [...]

Comment 4

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

Comment on attachment 8779625 [details]
Bug 1293576: [MSE] P2. Fix mochitest.

https://reviewboard.mozilla.org/r/70582/#review67956

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f91b10c077b
[MSE] P1. Always process the earliest frames first when in sequence mode. r=gerald
https://hg.mozilla.org/integration/autoland/rev/197e4391064f
[MSE] P2. Fix mochitest. r=gerald

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8779624 [details]
Bug 1293576: [MSE] P1. Always process the earliest frames first when in sequence mode.

https://reviewboard.mozilla.org/r/70580/#review67954

> You calculate full spans for both tracks, but in the end you only really need the start time of each (i.e., the minimum 'mTime' of all samples), right?
> So instead of 'presentationInterval', you could have something like 'presentationStart' for just that.

oops missed the comment and pushed it anyway. But I'll address it anyway.

I have a future change that rely on knowing the span of a segment. so I'll use this function anyway.

> Possible simplification:
> You could start from 0 if you folded the [0] case into the loop; then you wouldn't really need 'i' inside the loop. And so you could use a range-for loop. (This probably costs one extra assignment, I'd say it's worth it for the clarity.)
> Also I think you could make 'sample' a const ref.
> E.g.:
>   TimeInterval presentationInterval;
>   for (const auto& sample : aSamples) {
>     presentationInterval = presentationInterval.Span(
>       [...]

how do you fould the 0 case in the loop if you do not have a counter ? The problem with span of an empty interval is that it will always start at 0 which I do not want (the constructor of TimeInterval set start==end==TimeUnit(), which is 0).

Comment 7

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

Comment on attachment 8779624 [details]
Bug 1293576: [MSE] P1. Always process the earliest frames first when in sequence mode.

https://reviewboard.mozilla.org/r/70580/#review67954

> oops missed the comment and pushed it anyway. But I'll address it anyway.
> 
> I have a future change that rely on knowing the span of a segment. so I'll use this function anyway.

If you're going to use the full span, then of course it's fine to keep the function.

> how do you fould the 0 case in the loop if you do not have a counter ? The problem with span of an empty interval is that it will always start at 0 which I do not want (the constructor of TimeInterval set start==end==TimeUnit(), which is 0).

The first statement in span() is 'if (IsEmpty()) { return aOther; }', so I think the first sample would overwrite the empty presentationInterval.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/6f91b10c077b
https://hg.mozilla.org/mozilla-central/rev/197e4391064f