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

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

3 years ago
mozreview-review
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(
      [...]
Attachment #8779624 - Flags: review?(gsquelart) → review+

Comment 4

3 years ago
mozreview-review
Comment on attachment 8779625 [details]
Bug 1293576: [MSE] P2. Fix mochitest.

https://reviewboard.mozilla.org/r/70582/#review67956
Attachment #8779625 - Flags: review?(gsquelart) → review+

Comment 5

3 years ago
Pushed by jyavenard@mozilla.com:
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
(Assignee)

Comment 6

3 years ago
mozreview-review-reply
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

3 years ago
mozreview-review-reply
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f91b10c077b
https://hg.mozilla.org/mozilla-central/rev/197e4391064f
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.