Closed Bug 1174584 Opened 4 years ago Closed 4 years ago

Trackbuffer coded frame removal may leave content in unplayable state.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(3 files, 4 obsolete files)

When calling sourcebuffer.remove() it may leave the trackbuffers in invalid state.
Assignee: nobody → jyavenard
Attached patch P2. Fix frames removal. (obsolete) — Splinter Review
Attached patch P1. Fix logging.Splinter Review
Attachment #8622195 - Flags: review?(cpearce)
Attachment #8622196 - Flags: review?(cpearce)
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.
Attachment #8622197 - Flags: review?(cpearce)
Attachment #8622194 - Attachment is obsolete: true
W3C bug pending.
Attachment #8622274 - Flags: review?(cpearce)
Prevent reparsing the entire stream in the next call to appendBuffer.
Attachment #8622276 - Flags: review?(cpearce)
W3C bug pending.
Attachment #8622277 - Flags: review?(cpearce)
Comment on attachment 8622274 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

wrong bug #
Attachment #8622274 - Attachment is obsolete: true
Attachment #8622274 - Flags: review?(cpearce)
Attachment #8622276 - Attachment is obsolete: true
Attachment #8622276 - Flags: review?(cpearce)
Attachment #8622277 - Attachment is obsolete: true
Attachment #8622277 - Flags: review?(cpearce)
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!
Attachment #8622196 - Flags: review?(cpearce) → review?(ayang)
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!
Attachment #8622197 - Flags: review?(cpearce) → review?(ayang)
Attachment #8622195 - Flags: review?(cpearce) → review+
Comment on attachment 8622196 [details] [diff] [review]
P2. Fix frames removal.

Thank you for help, Gerald.
Attachment #8622196 - Flags: review?(ayang) → review?(gsquelart)
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.
Attachment #8622197 - Flags: review?(ayang) → review?(gsquelart)
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.
Attachment #8622196 - Flags: review?(gsquelart) → review+
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.
Attachment #8622197 - Flags: review?(gsquelart) → review+
(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).
but but but... what if we do have INT32_MAX frames in our array ???
Then UINT32_MAX is still bigger, so it's fine.
You need to log in before you can comment on or make changes to this bug.