1176918 - Frames are incorrectly removed upon appendBuffer

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

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
"

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Only remove coded frames if presentation time is later than previous frame.

Comment 2

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

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)) {
    ...

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92adb24e3f8a

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92adb24e3f8a