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

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

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

+#ifdef MOZ_WIDGET_ANDROID + return true; +#else ^ That was a mistake IMHO, and will likely cause regressions, particular battery life wise.

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

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

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