Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Don't run VP9 benchmark on Android

RESOLVED FIXED in Firefox 49

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

1.9.2 Branch
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49 fixed, fennec47+)

Details

Attachments

(1 attachment)

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.
Created attachment 8743299 [details] [diff] [review]
Don't run VP9 benchmark on Android
Attachment #8743299 - Flags: review?(jyavenard)
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.
Attachment #8743299 - Flags: review?(jyavenard) → review+
Might this be another possible use case for downloadable content, like the downloadable fonts?

Comment 4

a year ago
(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.
Assignee: nobody → snorp
tracking-fennec: --- → 47+
status-firefox47: --- → affected
status-firefox48: --- → affected

Comment 5

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/736434eaac6c
+#ifdef MOZ_WIDGET_ANDROID
+  return true;
+#else

^

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

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/736434eaac6c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

a year ago
See Also: → bug 1268456
(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.
See Also: → bug 1268886
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
I should add that I wonder the point of asking me to review code, if you go against the comment in the review.
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Are you going to uplift this?
Flags: needinfo?(snorp)
this has reduced this installer size:
https://treeherder.mozilla.org/perf.html#/alerts?id=1005
(In reply to :Margaret Leibovic from comment #14)
> Are you going to uplift this?

Too late now
status-firefox47: affected → wontfix
status-firefox48: affected → wontfix
Flags: needinfo?(snorp)

Updated

10 months ago
Depends on: 1286738
You need to log in before you can comment on or make changes to this bug.