Closed Bug 1134064 Opened 6 years ago Closed 6 years ago

Some unused data will never get evicted

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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)
Drop reference to audio/video reader once we stop using it.
Attachment #8565793 - Flags: review?(matt.woodrow)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
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)
Priority: -- → P2
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)
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+
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)
Check that data is still available and go back into waiting mode if not
Attachment #8566341 - Flags: review?(matt.woodrow)
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
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+
Blocks: 1136138
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
Depends on: 1131433
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)
Depends on: 1173792
You need to log in before you can comment on or make changes to this bug.