Some unused data will never get evicted

RESOLVED FIXED in Firefox 37

Status

()

defect
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
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
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
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
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
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
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
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+
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.
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.