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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

No description provided.
When appending data to the source that will overwrite existing data, under some circumstances we insert the new data at the wrong position.
Assignee: nobody → jyavenard
Make for more elegant loops.
Attachment #8622192 - Flags: review?(matt.woodrow)
Attachment #8622192 - Flags: review?(matt.woodrow) → review+
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!
Attachment #8622193 - Flags: review?(cpearce) → review?(ayang)
Attachment #8622193 - Flags: review?(ayang) → review?(gsquelart)
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?
Attachment #8622193 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/1736ae4209d3
https://hg.mozilla.org/mozilla-central/rev/21a4cdb444f5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.