Closed Bug 1374475 Opened 3 years ago Closed 3 years ago

Don't throttle media download for small files

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file)

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 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 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
https://hg.mozilla.org/mozilla-central/rev/d1adec83acac
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1364476
It looks like this can almost fix bug 1364476. 
We should consider uplifting it.
Flags: needinfo?(cpearce)
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)
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.
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?
(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 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+
Flags: needinfo?(cpearce)
(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.