1132796 - Eviction code only ever remove all data.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Jean-Yves Avenard [:jya]]

As per title.

TracBuffer::Evict will always evict all data from a decoder until threshold is reach.

We have logic that will evict an entire decoder if it's past the current decoder ; however TrackBuffer::mCurrentDecoder is the decoder we're currently adding data to, it is always the last decoder created and as such always the last element of TrackBuffer::mDecoders.

So we always happen to call EvictAll() for all decoders.

We should only evict all data from past decoders and be selective on future decoders we're going to evict.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Evict data we likely previously read

mCurrentDecoder is the decoder we're currently adding data to; not the decoder currently playing. It is always the last decoder in the list of decoders. As such, we were always evicting all data from all decoders but the currently playing one.
I split the loop in two as I felt it was clearer to understand.
We proceed in three steps, each step can be aborted if we've already evicted more data than required.
First step is to evict all past data from current playback position.
Step 2: Evict all data from decoders anterior to the currently playing one, assuming we've already played from those (and in practice unless you seek a lot, it's always true)
Step 3: We evict future data, starting from the furthest away, only keeping the currently playing reader and the next one we will be switching to once the current is done.

Comment 2

[cajbir (:cajbir)]

Link to try run please.

Comment 3

[cajbir (:cajbir)]

Comment on attachment 8563902 [details] [diff] [review]
Evict data we likely previously read

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

r+ if try run is green-ish.

::: dom/media/mediasource/MediaSourceDecoder.h
@@ +85,5 @@
> +  // Return a decoder from the set available in aTrackDecoders that has data
> +  // available in the range requested by aTarget.
> +  already_AddRefed<SourceBufferDecoder> SelectDecoder(int64_t aTarget,
> +                                                      int64_t aTolerance,
> +                                                      const nsTArray<nsRefPtr<SourceBufferDecoder>>& aTrackDecoders);

Comment showing what units aTarget and aTolerance are in.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Return an error when attempting to append too much data

Implement step 6 of the Prepare Append Algorithm, if our buffer is full, throw a quota exceeded error

Comment 5

[Jean-Yves Avenard [:jya]]

None of the mochitest or webref test stress the sourcebuffer size threshold.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9b7c6dfb66f

Comment 6

[cajbir (:cajbir)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> None of the mochitest or webref test stress the sourcebuffer size threshold.

It's to ensure that changes don't break something unexpected elsewhere. The number of times code changes "shouldn't" break thinks vs does is not insignificant. It's also frustrating as a reviewer to review code that doesn't build or is broken that could easily be found on try. Then have to wait for those to be fixed to review again. So a try push before review is appreciated.

You don't have to do all platforms for the try push. I usually do one platform variant of linux, windows and android.

Comment 7

[cajbir (:cajbir)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Created attachment 8565231 [details] [diff] [review]
> Part2. Return an error when attempting to append too much data
> 
> Implement step 6 of the Prepare Append Algorithm, if our buffer is full,
> throw a quota exceeded error

Should this be a separate bug? Is it related to "Eviction code only ever remove all data"?

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8565231 [details] [diff] [review]
Part2. Return an error when attempting to append too much data

Will fork this in a new bug..

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Return an error when attempting to append too much data

Implement step 6 of the Prepare Append Algorithm, if our buffer is full, throw a quota exceeded error

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8565250 [details] [diff] [review]
Return an error when attempting to append too much data

https://hg.mozilla.org/integration/mozilla-inbound/rev/dad2b3245457

Comment 11

[Jean-Yves Avenard [:jya]]

I applied an old patch that didn't include bug 1132825. It overwrote that change :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/7def63a22bed

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8563902 [details] [diff] [review]
Evict data we likely previously read

Requesting 37 uplift for this change and the followup fix for reverting bug 1132825. Ideally for 37b1 since this is a P1 bug for MSE.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Playback issues with YouTube. Less consistent testing and harder to port later fixes.
[Describe test coverage new/current, TreeHerder]: Green on inbound.
[Risks and why]: This is a large patch, but changes are specific to the MSE feature, so the risk of regression is low.
[String/UUID change made/needed]: None.

Comment 13

[Ralph Giles (:rillian)]

This has landed on central.

https://hg.mozilla.org/mozilla-central/rev/dad2b3245457
https://hg.mozilla.org/mozilla-central/rev/7def63a22bed

Comment 14

[Ralph Giles (:rillian)]

Pushed to aurora with pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/65a768a861a5

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8563902 [details] [diff] [review]
Evict data we likely previously read

As stated, this bug was pre-approved to land with a set of changes for MSE. Marking the approval after the fact.

Comment 16

[Ralph Giles (:rillian)]

Oops, I forgot to push the fixup patch from comment #11 to 37. Carrying forward a=lmandel for beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/82ef773ccb71

Comment 17

[Jean-Yves Avenard [:jya]]

need beta uplift

Comment 18

[Ralph Giles (:rillian)]

This made landed on 37 aurora, so we're good on all three MSE branches now.