Closed
Bug 1134064
Opened 8 years ago
Closed 8 years ago
Some unused data will never get evicted
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
11.24 KB,
patch
|
mattwoodrow
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
ajones
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
4.94 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Drop reference to audio/video reader once we stop using it.
Attachment #8565793 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 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•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 3•8 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•8 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)
Updated•8 years ago
|
Attachment #8566194 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 5•8 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•8 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•8 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)
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb6b682a464e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•8 years ago
|
Attachment #8566922 -
Flags: review?(cajbir.bugzilla) → review+
Updated•8 years ago
|
Attachment #8565793 -
Flags: review?(matt.woodrow) → review+
Updated•8 years ago
|
Attachment #8565796 -
Flags: review?(matt.woodrow) → review+
Updated•8 years ago
|
Attachment #8566341 -
Flags: review?(matt.woodrow) → review+
Comment 9•8 years ago
|
||
Get part 3 on 37 too. Pre-approval from lmandel. https://hg.mozilla.org/releases/mozilla-aurora/rev/cbf454c3c189
status-firefox37:
--- → fixed
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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•8 years ago
|
||
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 13•8 years ago
|
||
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?
Comment 14•8 years ago
|
||
Resetting status to reflect the new patch state. Only Part 3 landed before merge day.
status-firefox39:
--- → fixed
Target Milestone: mozilla38 → mozilla39
Comment 16•8 years ago
|
||
Already nominated the remaining four parts. Waiting for approval to push.
Flags: needinfo?(giles)
Comment 17•8 years ago
|
||
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+
Comment 18•8 years ago
|
||
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•8 years ago
|
||
Needs rebasing (or more patch approvals) for beta uplift.
Flags: needinfo?(giles)
Updated•8 years ago
|
Flags: needinfo?(giles)
Comment 20•8 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•