Frames are incorrectly removed upon appendBuffer

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Frames are sometimes removed that shouldn't be.

This trigger the assertion: 
    MOZ_ASSERT(trackBuffer.mNextInsertionIndex.isNothing() ||
               trackBuffer.mNextInsertionIndex.ref() <= start);

This appears to due to incorrect handling of MSE's https://w3c.github.io/media-source/#widl-SourceBuffer-buffered
step 14:
"Remove existing coded frames in track buffer:

If highest end timestamp for track buffer is not set:
    Remove all coded frames from track buffer that have a presentation timestamp greater than or equal to presentation timestamp and less than frame end timestamp.
If highest end timestamp for track buffer is set and less than or equal to presentation timestamp:
    Remove all coded frames from track buffer that have a presentation timestamp greater than or equal to highest end timestamp and less than frame end timestamp
"
(Assignee)

Comment 1

3 years ago
Created attachment 8625520 [details] [diff] [review]
Only remove coded frames if presentation time is later than previous frame.
Attachment #8625520 - Flags: review?(gsquelart)
Comment on attachment 8625520 [details] [diff] [review]
Only remove coded frames if presentation time is later than previous frame.

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

r+ with comments addressed or not, as they probably have very little impact.

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +1389,5 @@
>    TrackBuffer& data = trackBuffer.mBuffers.LastElement();
> +  bool removeCodedFrames =
> +    trackBuffer.mHighestEndTimestamp.isNothing() ||
> +    (trackBuffer.mHighestEndTimestamp.isSome() &&
> +     trackBuffer.mHighestEndTimestamp.ref() <= presentationTimestamp);

Suggestion: Instead of something like 'A || (!A && B)' where you have 3 tests, you could do 'A ? true : B' with only 2 tests.
So the above code would become:
if (trackBuffer.mHighestEndTimestamp.isNothing() ? true : (trackBuffer.mHighestEndTimestamp.ref() <= presentationTimestamp))

@@ +1393,5 @@
> +     trackBuffer.mHighestEndTimestamp.ref() <= presentationTimestamp);
> +  TimeUnit lowerBound =
> +    trackBuffer.mHighestEndTimestamp.valueOr(presentationTimestamp);
> +  if (removeCodedFrames &&
> +      trackBuffer.mBufferedRanges.ContainsStrict(lowerBound)) {

If lowerBound is only used when 'removeCodedFrames' is true, then you don't need to calculate it when 'removeCodedFrames' is false.
So the whole 8 lines above could become:
if ( <insert removeCodedFrames test here> ) {
  TimeUnit lowerBound = ...;
  if (trackBuffer.mBufferedRanges.ContainsStrict(lowerBound)) {
    ...
Attachment #8625520 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/92adb24e3f8a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.