1174584 - Trackbuffer coded frame removal may leave content in unplayable state.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

When calling sourcebuffer.remove() it may leave the trackbuffers in invalid state.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P2. Fix frames removal.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P1. Fix logging.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P2. Fix frames removal.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P3. Do not error in prepare append if we've reached our memory threshold.

In the new MSE architecture. eviction is done asynchronously, also being super accurate, eviction will
always succeed.
Limit our eviction rate to be a minimum of 512kB.

Comment 5

[Jean-Yves Avenard [:jya]]

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

W3C bug pending.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: P1. Update insertion index on the fly.

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

Comment 7

[Jean-Yves Avenard [:jya]]

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

W3C bug pending.

Comment 8

[Jean-Yves Avenard [:jya]]

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

wrong bug #

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622196 [details] [diff] [review]
P2. Fix frames removal.

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

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

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622197 [details] [diff] [review]
P3. Do not error in prepare append if we've reached our memory threshold.

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

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

Comment 11

[Alfredo Yang (:alfredo)]

Comment on attachment 8622196 [details] [diff] [review]
P2. Fix frames removal.

Thank you for help, Gerald.

Comment 12

[Alfredo Yang (:alfredo)]

Comment on attachment 8622197 [details] [diff] [review]
P3. Do not error in prepare append if we've reached our memory threshold.

Thank you for help, Gerald.

Comment 13

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

Comment on attachment 8622196 [details] [diff] [review]
P2. Fix frames removal.

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

r+ with potential signed/unsigned issue fixed (or proved non-existent).

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +470,1 @@
>            firstRemovedIndex = i;

Implicit conversion from uint32 to int32, probably ok here; but doesn't it produce a warning? You should do a static_cast.
Alternatively, make firstRemovedIndex a uint32 and use a special value (e.g., the traditional max 32-bit value) as 'not-assigned-yet' marker.

@@ +481,5 @@
>      }
>      // 4. Remove decoding dependencies of the coded frames removed in the previous step:
>      // Remove all coded frames between the coded frames removed in the previous step and the next random access point after those removed frames.
>      if (firstRemovedIndex >= 0) {
> +      uint32_t start = firstRemovedIndex;

Implicit conversion from int32 to uint32, probably ok here as firstRemovedIndex >= 0; but doesn't it produce a warning? You should do a static_cast.
Alternatively, if you've made firstRemovedIndex a uint32, you don't even need 'start' anymore.

Comment 14

[Jean-Yves Avenard [:jya]]

the maximum size possible in a nsTArray is INT32_MAX. 
I use a negative value to indicate that no frames were removed from the array. could change that with a Maybe I guess.

Comment 15

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

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> the maximum size possible in a nsTArray is INT32_MAX. 
> I use a negative value to indicate that no frames were removed from the
> array. could change that with a Maybe I guess.

If the maximum size is INT32_MAX, then you could use any higher value as 'no-frames-removed', e.g. UINT32_MAX. ;-)
Or use Maybe.
Or as first suggested, add static_cast's to avoid warnings (if any).

Comment 16

[Jean-Yves Avenard [:jya]]

but but but... what if we do have INT32_MAX frames in our array ???

Comment 17

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

Then UINT32_MAX is still bigger, so it's fine.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e62a1881bf6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/128206149d80
https://hg.mozilla.org/integration/mozilla-inbound/rev/06868346ce62

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e62a1881bf6b
https://hg.mozilla.org/mozilla-central/rev/128206149d80
https://hg.mozilla.org/mozilla-central/rev/06868346ce62