1266102 - Don't run VP9 benchmark on Android

Status: RESOLVED FIXED

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

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

The samples add like 240KB to the APK size. It's likely good enough to just assume it works as long as we can play it.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't run VP9 benchmark on Android

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8743299 [details] [diff] [review]
Don't run VP9 benchmark on Android

Review of attachment 8743299 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/Benchmark.cpp
@@ +32,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +#ifdef MOZ_WIDGET_ANDROID
> +  return true;

Are we certain we want to always enable vp9 ?

this will cause youtube to always use MSE with vp9. Likely okay on devices with VP9 HW decoding, but will lead to significant extra power usage on older devices.

My advice for now would be to return false until bug 1261273 is done to avoid regressions.

Comment 3

[Jan Henning [:JanH]]

Might this be another possible use case for downloadable content, like the downloadable fonts?

Comment 4

[:Margaret Leibovic]

(In reply to Jan Henning [:JanH] from comment #3)
> Might this be another possible use case for downloadable content, like the
> downloadable fonts?

Yes, this could be a good candidate, although I'm not sure how easy it is to asynchronously fetch these resources in this case. This would be a good follow-up bug.

Bug 1263839 was uplifted to 47, we should uplift this.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/736434eaac6c

Comment 6

[Jean-Yves Avenard [:jya]]

+#ifdef MOZ_WIDGET_ANDROID
+  return true;
+#else

^

That was a mistake IMHO, and will likely cause regressions, particular battery life wise.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/736434eaac6c

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> +#ifdef MOZ_WIDGET_ANDROID
> +  return true;
> +#else
> 
> ^
> 
> That was a mistake IMHO, and will likely cause regressions, particular
> battery life wise.

This preserves the existing behavior, though. If we want to change that, it should be in another bug.

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> This preserves the existing behavior, though. If we want to change that, it
> should be in another bug.

no it doesn't.

On the contrary, you're now claiming that regardless of the speed of the device, it is always fast enough to decode VP9 and as a result, MSE/webm/VP9 will always be enabled.

that's not how the VP9 benchmark result works.
IsVP9DecodeFast only serves as an override. It's like a OR. It never turns a feature off (webm)

the algorithm to detech if mse/webm should be enabled is:

enable = 
  (media.mediasource.webm.enabled || !mp4 || !h264 || !h264_is_hardwareaccelerated || IsVP9DecodeFast())

As a consequence, youtube will now stop serving mse/mp4/h264 which is always hardware accelerated, and instead you get vp9 which may be HW accelerated, but that's only for the most recent device.

Returning false would cause it to ignore the vp9 benchmark result and *that* would keep the existing behaviour, which is to respect the value of the media.mediasource.webm.enabled pref, and not attempt to override it.

Now, looking at the default value of media.mediasource.webm.enabled in modules/libpref/init/all.js appears true on android, so maybe it doesn't matter.

*but*, if you want to keep the existing behavior of prior the vp9 benchmark being introduced, you must return false.

Comment 10

[Jean-Yves Avenard [:jya]]

I should add that I wonder the point of asking me to review code, if you go against the comment in the review.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> > This preserves the existing behavior, though. If we want to change that, it
> > should be in another bug.
> 
> no it doesn't.
> 
> On the contrary, you're now claiming that regardless of the speed of the
> device, it is always fast enough to decode VP9 and as a result, MSE/webm/VP9
> will always be enabled.

Yes, this was the behavior on Android before the benchmark was added.

> 
> that's not how the VP9 benchmark result works.
> IsVP9DecodeFast only serves as an override. It's like a OR. It never turns a
> feature off (webm)

I wasn't sure if it could turn it off or not, thanks for clarifying.

> 
> the algorithm to detech if mse/webm should be enabled is:
> 
> enable = 
>   (media.mediasource.webm.enabled || !mp4 || !h264 ||
> !h264_is_hardwareaccelerated || IsVP9DecodeFast())
> 
> As a consequence, youtube will now stop serving mse/mp4/h264 which is always
> hardware accelerated, and instead you get vp9 which may be HW accelerated,
> but that's only for the most recent device.
> 
> Returning false would cause it to ignore the vp9 benchmark result and *that*
> would keep the existing behaviour, which is to respect the value of the
> media.mediasource.webm.enabled pref, and not attempt to override it.

We've found that some videos *only* play if you have VP9 enabled, which is why we enabled it unconditionally. It's much worse for a video to not play at all than to play with higher CPU usage.

> 
> Now, looking at the default value of media.mediasource.webm.enabled in
> modules/libpref/init/all.js appears true on android, so maybe it doesn't
> matter.
> 
> *but*, if you want to keep the existing behavior of prior the vp9 benchmark
> being introduced, you must return false.

Haven't we decided that the value returned from the benchmark doesn't matter due to us having media.mediasource.webm.enabled turned on? I'm confused by the above statement.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> I should add that I wonder the point of asking me to review code, if you go
> against the comment in the review.

If you feel so strongly, use the r-. That's the point.

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> Haven't we decided that the value returned from the benchmark doesn't matter
> due to us having media.mediasource.webm.enabled turned on? I'm confused by
> the above statement.

sure, but now you have also disabled the use of the media.mediasource.webm.enabled pref. Be it true of false, mse/webm will always be available.

Comment 14

[:Margaret Leibovic]

Are you going to uplift this?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

this has reduced this installer size:
https://treeherder.mozilla.org/perf.html#/alerts?id=1005

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to :Margaret Leibovic from comment #14)
> Are you going to uplift this?

Too late now