1174583 - Appending data to source buffer may insert frames in the wrong order.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Jean-Yves Avenard [:jya]]

When appending data to the source that will overwrite existing data, under some circumstances we insert the new data at the wrong position.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P1. Add iterators to IntervalSet.

Make for more elegant loops.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: 0001-Bug-1174583-P2.-Fix-frame-insertion.-r-cpearce.patch

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622193 [details] [diff] [review]
0001-Bug-1174583-P2.-Fix-frame-insertion.-r-cpearce.patch

Review of attachment 8622193 [details] [diff] [review]:
-----------------------------------------------------------------

Passing review to Alfredo. Thanks for taking on this responsibility Alfredo!

Comment 5

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

Comment on attachment 8622193 [details] [diff] [review]
0001-Bug-1174583-P2.-Fix-frame-insertion.-r-cpearce.patch

Review of attachment 8622193 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with fixed asserts.
Other comments might need separate bugs if correct...

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +1323,2 @@
>          }
> +        trackBuffer.mSizeBuffer -= sizeof(*sample) + sample->mSize;

Not related to this bug:
I'm a bit scared of all the mSizeBuffer statements in this file, they all look correct, but mSizeBuffer could easily be incorrectly calculated somewhere someday...
Also they're always associated with a TrackBuffer::Append/Insert/RemoveElement.
So how about you have a few methods to add/remove frames, which also keep mSizeBuffer up to date? (Could/should be a separate low-priority maintainability bug)
It would probably be a bit less efficient though, so it might be the wrong thing to do in these time-critical algorithms.

@@ +1371,5 @@
> +  } else if (trackBuffer.mNextInsertionIndex.isSome()) {
> +    data.InsertElementAt(trackBuffer.mNextInsertionIndex.ref(), aSample);
> +    MOZ_ASSERT(trackBuffer.mNextInsertionIndex.ref() != 0 ||
> +               data[trackBuffer.mNextInsertionIndex.ref()]->mTrackInfo->GetID() == data[trackBuffer.mNextInsertionIndex.ref()-1]->mTrackInfo->GetID() ||
> +               data[trackBuffer.mNextInsertionIndex.ref()]->mKeyframe);

Is this assert correct?
If mNextInsertionIndex is 0:
- first test: |mNextInsertionIndex != 0| is false, continuing...
- now we know mNextInsertionIndex == 0, second test contains |data[mNextInsertionIndex-1]|, which is data[-1], which is probably wrong!
I'm guessing the first test should be |== 0|.

Also, if you're inserting at 0, would you want another check to ensure that it is a keyframe?
See next comment for suggestions.

@@ +1401,5 @@
> +      if (target.Intersects(sampleInterval)) {
> +        data.InsertElementAt(i, aSample);
> +        MOZ_ASSERT(i == 0 && data[i]->mKeyframe ||
> +                   data[i]->mTrackInfo->GetID() == data[i-1]->mTrackInfo->GetID() ||
> +                   data[i]->mKeyframe);

Another dangerous assert!
If i==0 but it's not a keyframe, the 1st line will be false, then the 2nd line will try to access data[-1].
How about: |i ? (d[i]->id==d[i-1]->id || d[i]->key) : d[0]->key|

@@ +1408,2 @@
>        }
>      }

As discussed offline, isn't there a risk that we're inserting a frame between a keyframe and its following dependent frame?
E.g. we have: [K 1] (K keyframe, 1 depends on K), and now we're inserting K2 in between: [K K2 1]; Would that video still work?

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1736ae4209d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/21a4cdb444f5

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1736ae4209d3
https://hg.mozilla.org/mozilla-central/rev/21a4cdb444f5