Some unused data will never get evicted

RESOLVED FIXED in Firefox 37

Status

()

P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla39
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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)
(Assignee)

Comment 1

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

Drop reference to audio/video reader once we stop using it.
Attachment #8565793 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8565796 [details] [diff] [review]
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
Attachment #8565796 - Flags: review?(matt.woodrow)

Updated

4 years ago
Priority: -- → P2
(Assignee)

Comment 3

4 years ago
Created attachment 8566194 [details] [diff] [review]
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.
Attachment #8566194 - Flags: review?(ajones)
(Assignee)

Comment 4

4 years ago
Created attachment 8566195 [details] [diff] [review]
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.
Attachment #8566195 - Flags: review?(matt.woodrow)
Attachment #8566194 - Flags: review?(ajones) → review+
(Assignee)

Comment 5

4 years ago
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.
Attachment #8566195 - Attachment is obsolete: true
Attachment #8566195 - Flags: review?(matt.woodrow)
(Assignee)

Comment 6

4 years ago
Created attachment 8566341 [details] [diff] [review]
Part4. Fix racing condition should data get evicted

Check that data is still available and go back into waiting mode if not
Attachment #8566341 - Flags: review?(matt.woodrow)
(Assignee)

Comment 7

4 years ago
Created attachment 8566922 [details] [diff] [review]
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
Attachment #8566922 - Flags: review?(cajbir.bugzilla)
https://hg.mozilla.org/mozilla-central/rev/fb6b682a464e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

4 years ago
Attachment #8566922 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8565793 - Flags: review?(matt.woodrow) → review+
Attachment #8565796 - Flags: review?(matt.woodrow) → review+
Attachment #8566341 - Flags: review?(matt.woodrow) → review+
Get part 3 on 37 too. Pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbf454c3c189
status-firefox37: --- → fixed
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.
Attachment #8566194 - Flags: approval-mozilla-aurora?
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.
Attachment #8566194 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8565793 - Flags: approval-mozilla-beta?
Attachment #8565793 - Flags: approval-mozilla-aurora?
Resetting status to reflect the new patch state. Only Part 3 landed before merge day.
status-firefox37: fixed → affected
status-firefox38: fixed → affected
status-firefox39: --- → fixed
Target Milestone: mozilla38 → mozilla39
(Assignee)

Comment 15

4 years ago
need beta uplift
Flags: needinfo?(giles)
Already nominated the remaining four parts. Waiting for approval to push.
Flags: needinfo?(giles)
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.
Attachment #8565793 - Flags: approval-mozilla-beta?
Attachment #8565793 - Flags: approval-mozilla-beta+
Attachment #8565793 - Flags: approval-mozilla-aurora?
Attachment #8565793 - Flags: approval-mozilla-aurora+
Needs rebasing (or more patch approvals) for beta uplift.
Flags: needinfo?(giles)
Flags: needinfo?(giles)

Updated

4 years ago
Depends on: 1173792
You need to log in before you can comment on or make changes to this bug.