Closed
Bug 1374475
Opened 4 years ago
Closed 4 years ago
Don't throttle media download for small files
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jwwang
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
As per bug 1373618 comment 19, it would be good to not throttle the download for small media files, as a seek requires a new HTTP transaction which has a startup latency, and if we can buffer the entire resource we will avoid that overhead. This will also provide a work around (but not a proper fix) for bug 1373618.
Comment hidden (mozreview-request) |
Comment 2•4 years ago
|
||
mozreview-review |
Comment on attachment 8879353 [details] Bug 1374475 - Don't throttle download of media files under 8MB in size. https://reviewboard.mozilla.org/r/150648/#review155400 ::: dom/media/MediaDecoder.cpp:1035 (Diff revision 1) > MOZ_ASSERT(NS_IsMainThread()); > NS_ENSURE_TRUE(mDecoderStateMachine, false); > > + int64_t length = mResource->GetLength(); > + if (length > 0 && > + length <= int64_t(MediaPrefs::MediaMemoryCacheMaxSize()) * 1024) { We have similar code at http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/media/MediaCache.cpp#687. IIUC, we already use MemoryBlockCache for small files and MemoryBlockCache ignore ThrottleReadahead() calls, right?
Comment 3•4 years ago
|
||
mozreview-review |
Comment on attachment 8879353 [details] Bug 1374475 - Don't throttle download of media files under 8MB in size. https://reviewboard.mozilla.org/r/150648/#review155430 ::: dom/media/MediaDecoder.cpp:1035 (Diff revision 1) > MOZ_ASSERT(NS_IsMainThread()); > NS_ENSURE_TRUE(mDecoderStateMachine, false); > > + int64_t length = mResource->GetLength(); > + if (length > 0 && > + length <= int64_t(MediaPrefs::MediaMemoryCacheMaxSize()) * 1024) { Oops! I got it wrong by mistaking MemoryBlockCache for inheriting MediaCache. Anyway, it seems weird to me to run Update() loop for a MediaCache backed by MemoryBlockCache since there is only one stream to use the cache.
Attachment #8879353 -
Flags: review?(jwwang) → review+
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d1adec83acac Don't throttle download of media files under 8MB in size. r=jwwang
Comment 5•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d1adec83acac
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 6•4 years ago
|
||
It looks like this can almost fix bug 1364476. We should consider uplifting it.
Flags: needinfo?(cpearce)
Assignee | ||
Comment 7•4 years ago
|
||
I observed bug 1364476 in yesterday's Nightly, so I don't think the patch here is enough to fix bug 1364476.
Flags: needinfo?(cpearce)
Comment 8•4 years ago
|
||
Although it is not enough to fix that bug, it might be useful to mitigate the problem to some extent. We could consider using it for 55 and then have a real solution for 56.
Assignee | ||
Comment 9•4 years ago
|
||
Comment on attachment 8879353 [details] Bug 1374475 - Don't throttle download of media files under 8MB in size. Approval Request Comment [Feature/Bug causing the regression]: Caused by recent changes to our media throttling logic in Firefox 55. [User impact if declined]: We suspect this helps reduce how often bug 1364476 occurs, where Pandora streaming audio ends up playing multiple copies of the audio stream it's trying to play slightly out of sync (so it sounds terrible). [Is this code covered by automated tests?]: We have lots of media mochitests. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No [Why is the change risky/not risky?]: This just makes us take the old path already-well-traveled path more often. [String changes made/needed]: None.
Attachment #8879353 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #7) > I observed bug 1364476 in yesterday's Nightly, so I don't think the patch > here is enough to fix bug 1364476. I've not been able to repro the problem again after several hours of playback, so the patch here may indeed help. (In reply to Blake Wu [:bwu][:blakewu] from comment #8) > Although it is not enough to fix that bug, it might be useful to mitigate > the problem to some extent. We could consider using it for 55 and then have > a real solution for 56. Since this patch may help reduce the frequency of occurrence of fix bug 1364476, so I've requested uplift.
Comment 11•4 years ago
|
||
Comment on attachment 8879353 [details] Bug 1374475 - Don't throttle download of media files under 8MB in size. don't throttle small files to try and avoid a regression, beta55+
Attachment #8879353 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5d0c516a2b0d
status-firefox55:
--- → fixed
Comment 13•4 years ago
|
||
backout for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=113650402&repo=mozilla-beta
Flags: needinfo?(cpearce)
Updated•4 years ago
|
Flags: needinfo?(cpearce)
Comment 14•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4cd92600443e
Comment 15•4 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #9) > [Is this code covered by automated tests?]: We have lots of media mochitests. > [Has the fix been verified in Nightly?]: Yes. > [Needs manual test from QE? If yes, steps to reproduce]: No. Setting qe-verify- based on Chris' assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•