1374475 - Don't throttle media download for small files

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (GMT+13)]]

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 1

[Chris Pearce [:cpearce (GMT+13)]]

Attachment: Bug 1374475 - Don't throttle download of media files under 8MB in size.

The startup latency for an HTTP transaction can be high, this adds delay when
we're seeking into an unbuffered range. So we should not throttle the download
for small files, to speed up seeking.

This also works around the problem in bug 1373618 in the case where the
download is fast enough for the entire file to predownload before a seek.

Review commit: https://reviewboard.mozilla.org/r/150648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150648/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

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

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1adec83acac
Don't throttle download of media files under 8MB in size. r=jwwang

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1adec83acac

Comment 6

[Blake Wu [:bwu][:blakewu]]

It looks like this can almost fix bug 1364476. 
We should consider uplifting it.

Comment 7

[Chris Pearce [:cpearce (GMT+13)]]

I observed bug 1364476 in yesterday's Nightly, so I don't think the patch here is enough to fix bug 1364476.

Comment 8

[Blake Wu [:bwu][:blakewu]]

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 9

[Chris Pearce [:cpearce (GMT+13)]]

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.

Comment 10

[Chris Pearce [:cpearce (GMT+13)]]

(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

[Julien Cristau [:jcristau]]

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+

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d0c516a2b0d

Comment 13

[Carsten Book [:Tomcat]]

backout for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=113650402&repo=mozilla-beta

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4cd92600443e

Comment 15

[Andrei Vaida [:avaida]]

(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.