1174588 - When evicting data, the next appendBuffer could render samples undecodable

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This is an actual bug in the W3C MSE spec, or more accurately, a problem that is not handled at all.

When appending a media segment, the coded frame processing algorithm (http://w3c.github.io/media-source/index.html#sourcebuffer-coded-frame-processing)
describes the step to detect a discontinuity, or how to append data to the current "coded frame group" http://w3c.github.io/media-source/index.html#coded-frame-group.

When appending a partial media segment, it is expected that the next appendBuffer will complete the media segment, and continue the current coded frame group.

When data is removed in the coded frame algorithm, it is possible that frames from the current coded frame group will be removed. If this happens, and the next appendBuffer data doesn't start with a keyframe, we will start inserting frames that can't be decoded.

I will open a W3C bug and in the mean time, implement what I believe should be done under this scenario.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Update insertion index on the fly.

Prevent reparsing the entire stream in the next call to appendBuffer.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Properly handle removal of data within the current coded frame group.

W3C bug pending.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P1. Update insertion index on the fly.

Prevent reparsing the entire stream in the next call to appendBuffer.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P2. Properly handle removal of data within the current coded frame group.

W3C bug pending.

Comment 5

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8622864 [details] [diff] [review]
P1. Update insertion index on the fly.

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

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

Comment 6

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

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

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

Comment 7

[Alfredo Yang (:alfredo)]

Comment on attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

Thank you for help, Gerald.

Comment 8

[Gerald Squelart [:gerald] (he/him)]

Comment on attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

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

::: dom/media/mediasource/TrackBuffersManager.h
@@ +229,5 @@
> +    void ResetAppendState()
> +    {
> +      mLastDecodeTimestamp.reset();
> +      mLastFrameDuration.reset();
> +      mHighestEndTimestamp.reset();

That's a lot of code in a header, and for a private method! Please consider moving it to the cpp.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e4889a65a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f78edbfbe3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2e4889a65a1
https://hg.mozilla.org/mozilla-central/rev/b3f78edbfbe3