Bug 135855 – [MSE] YouTube will lose audio, video after seeking backwards to an unbuffered range.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[MSE] YouTube will lose audio, video after seeking backwards to an unbuffered range.

[Jer Noble]

Created attachment 236471 [details]
Patch

[Jer Noble]

Committed r172506: <http://trac.webkit.org/changeset/172506>

[Jer Noble]

<rdar://problem/19269013>

[Alicia Boya García]

Comment on attachment 236471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236471&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1389
> +        // Do not enqueue samples spanning a significant unbuffered gap:

Why not avoid pushing such samples to the decodeQueue in the first place?

[Jer Noble]

Comment on attachment 236471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236471&action=review

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1389
>> +        // Do not enqueue samples spanning a significant unbuffered gap:
> 
> Why not avoid pushing such samples to the decodeQueue in the first place?

You would also have to guarantee that removing a range of samples from the decodeQueue (as in removeSamplesFromTrackBuffer) also removed any samples that resulted in a large gap.

[Alicia Boya García]

(In reply to Jer Noble from comment #5)
> Comment on attachment 236471 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=236471&action=review
> 
> >> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1389
> >> +        // Do not enqueue samples spanning a significant unbuffered gap:
> > 
> > Why not avoid pushing such samples to the decodeQueue in the first place?
> 
> You would also have to guarantee that removing a range of samples from the
> decodeQueue (as in removeSamplesFromTrackBuffer) also removed any samples
> that resulted in a large gap.

Thanks for replying. I think I get it; but let's use an example to make sure
we're on the same page. For this example PTS=DTS and all frames are sync frames
(as is the case with audio) and every frame is 1 second long to make it
simpler. Playback has not started yet and the first 10 seconds of audio were
just appended.

decodeQueue == trackBuffer.samples == [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

Now the user calls SourceBuffer.remove(4, 7), which in turns calls
removeSamplesFromTrackBuffer([4, 5, 6]). These are the resulting frames in the
track buffer:

decodeQueue == trackBuffer.samples = [0, 1, 2, 3, 7, 8, 9]

So the condition discussed above is necessary to avoid [7, 8, 9] to be actually
enqueued until [4, 5, 6] are re-appended, right?

Let's say right after this we append a frame with DTS=10.
sourceBufferPrivateDidReceiveSample() is called and we reach this condition:

if (trackBuffer.lastEnqueuedDecodeEndTime.isInvalid() || decodeTimestamp >= trackBuffer.lastEnqueuedDecodeEndTime) {
    DecodeOrderSampleMap::KeyType decodeKey(sample.decodeTime(), sample.presentationTime());
    trackBuffer.decodeQueue.insert(DecodeOrderSampleMap::MapType::value_type(decodeKey, &sample));
}

decodeTimestamp == lastEnqueuedDecodeEndTime == 10, so the condition is true
and 10 is added to the decodeQueue.

decodeQueue == trackBuffer.samples == [0, 1, 2, 3, 7, 8, 9, 10]
lastEnqueuedDecodeEndTime == 4

At the end of the append, provideMediaData() is called. The loop enqueues the
first 4 frames successfully. In the next iteration it reaches DTS=7 and breaks
the loop in the condition we're discussing.

The problem I see is that DTS=7 is still removed from the decodeQueue because
the condition is after the `.erase()`. The next frames (8 to 10) remain in the
decodeQueue, at least until the next call. I'm not the first person to point
this problem: https://bugs.webkit.org/show_bug.cgi?id=180643