1134064 - Some unused data will never get evicted

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Problem is of a greater importance due to bug 1133625.

When we run out of data to play in the MediaSourceReader's current decoder, we start waiting for more data but hang on the current decoder, even though it won't get used.

This prevent the trackbuffer from ever evicting data from that decoder. Should we be low on memory, it could prevent new data to be appended.

Once we've reached the end of the current decoder, the MediaSourceReader should release it so eviction can occurs. Then new data can be appended and playback can resume (preventing a stall)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Don't hold on reader when we stop needing it

Drop reference to audio/video reader once we stop using it.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Drop current reader when seeking outside range

Drop reference to current audio/video decoder if seeking outside buffered range. One more patch coming. Under some circumstances, we never evict data if data is being added to the current reader by the TrackBuffer and the MediaSourceReader is also using it. I see it climb to 20MB in size, even though threshold is set to 1MB

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Don't evict partial data and make resource unplayable

Fix resource being truncated and leaving it in unplayable state. Returning a negative value cause the position calculation to fallback to the default approximation. That default calculation should be fully dropped IMHO, it just can't work.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Fix racing condition should data get evicted

Fix seek/eviction race that could have occurred should data be dropped between the time we check if seek data is available and the time we actually start seeking. mWaitingForSeekData will now only be set to true once we start the actual seek.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8566195 [details] [diff] [review]
Part4. Fix racing condition should data get evicted

can cause a problem should data gets evicted between the time video seek completed and audio seek start.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Fix racing condition should data get evicted

Check that data is still available and go back into waiting mode if not

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Evict from TrackBuffer's current decoder

entirely evict decoders whose buffered end time as located prior current play time.
The MP4 demuxer only ever return a pointer to the last moof, so it will never drop everything

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb6b682a464e

Comment 9

[Ralph Giles (:rillian)]

Get part 3 on 37 too. Pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbf454c3c189

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8566194 [details] [diff] [review]
Part3. Don't evict partial data and make resource unplayable

Retroactive 37 uplift request for this patch only. Rest haven't landed.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Stalls playing youtube video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Small, isolated patch. Risk should be low.
[String/UUID change made/needed]: None.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59213406ccda
https://hg.mozilla.org/mozilla-central/rev/8f0d26e51598
https://hg.mozilla.org/mozilla-central/rev/690f343877a8
https://hg.mozilla.org/mozilla-central/rev/8cd5efa83e44

Comment 12

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

Comment on attachment 8566194 [details] [diff] [review]
Part3. Don't evict partial data and make resource unplayable

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

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8565793 [details] [diff] [review]
Part1. Don't hold on reader when we stop needing it

Requesting 37 and 38 uplift for the other four patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Playback stalls and rebuffering playing MSE video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Changes are specific to the MSE code so risk of regression side-effects is low.
[String/UUID change made/needed]: None.

Comment 14

[Ralph Giles (:rillian)]

Resetting status to reflect the new patch state. Only Part 3 landed before merge day.

Comment 15

[Jean-Yves Avenard [:jya]]

need beta uplift

Comment 16

[Ralph Giles (:rillian)]

Already nominated the remaining four parts. Waiting for approval to push.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8565793 [details] [diff] [review]
Part1. Don't hold on reader when we stop needing it

Approving for uplift as it's early still and we need these additional patches.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01ad6f6b479d
https://hg.mozilla.org/releases/mozilla-aurora/rev/b148e9363920
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f297ec7ecef
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce8525a6b2d9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing (or more patch approvals) for beta uplift.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e539009f86e
https://hg.mozilla.org/releases/mozilla-beta/rev/5f400332977d
https://hg.mozilla.org/releases/mozilla-beta/rev/c92f6beaa505
https://hg.mozilla.org/releases/mozilla-beta/rev/d461222b1a07