Closed Bug 1286738 Opened 8 years ago Closed 8 years ago

Firefox on Android consumed more power than Chrome on Android devices

Categories

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Version:
48 Branch
Platform:
ARM
Android
Type:
defect

Tracking

(fennec+, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
fennec + ---
firefox52 --- fixed

People

(Reporter: atsai, Unassigned)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(4 files)

Bug 1286738 - part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1286738 - part2-HardwareCodecCapabilityUtils: Add HasHWVP9() function to check the vp9 hw decoder.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1286738 - part3-mediasource: Enable fennec vp9 only if we have hw deocder.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1286738 - part4-fixWebMAudio: audio/webm doesn't need the IsWebMForced() check.
58 bytes, text/x-review-board-request
jya
: review+
Details 
Blake and I discussed about if Firefox on Android consumed more power than Chrome. Thus I did some test and here's the rough result.

http://www.pcworld.com/article/3087338/browsers/which-browser-is-best-on-battery-we-test-edge-vs-chrome-vs-opera-vs-firefox.html

DuT: Nexus 5, Android 5.1.1. Clean Base Image

Preparation:
Full Charged Nexus 5

Step:
1. Play a YouTube Video for 2 hours
2. Observe the battery life

Result:
*. Firefox on Android consumes around 15% more than Chrome.
Run 1 - 02:15	Firefox	37%	Chrome	46%
Run 2 - 02:00	Firefox	42%	Chrome	56%
Run 3 - 02:05	Firefox	42%	Chrome	46%
(In reply to Al Tsai [:atsai] from comment #1)
> Run 1 - 02:15	Firefox	37%	Chrome	46%
> Run 2 - 02:00	Firefox	42%	Chrome	56%
> Run 3 - 02:05	Firefox	42%	Chrome	46%

Note that the percentages are the battery life left on the devices. All tests start from full charged (100%).
Al,
Thank you very much!
Priority: -- → P3
tracking-fennec: --- → ?
tracking-fennec: ? → +
Benjamin, 
Per discussing, please help check this bug. Probably we get VP9 data from youtube and use a SW decoder for decoding.
Flags: needinfo?(bechen)
Update status.
I'm using sony z3c, open the same youtube video then compare the fennec and chrome power consumption.
I observed the cpu usage by "top" command and the result seems no difference between fennec and chrome. The cpu usages are both around 35%~40%, nothing special.
Flags: needinfo?(bechen)
So maybe we are receiving different resolution data?
On Nexus5, fennec uses vp9 sw decoder, but chrome uses hw decoder.
After some investigation, the youtube website query webm/mp4 through MediaSource::IsTypeSupported, we return true for both. Then fennec get the vp9 content for sw decoder.
I try to return false for webm, then fennec can get mp4 content for hw decoder.

Hi jya:
Do you have any idea about this?

Blake:
Seems that it is not a bug for fennec, maybe we should contact youtube.
Flags: needinfo?(jyavenard)
Flags: needinfo?(bwu)
This is not up to us to decide. 

If we do not want vp9; you'll have to fully disable it. Note that if there's a hardware decoder it will be hsed
Flags: needinfo?(jyavenard)
Hi Richard, 
Firefox on Android can support both MP4 and WebM, but it gets WebM instead of MP4 from YouTube. Since normally mobile phone has HW decoder for MP4/H264, can YouTube send Firefox MP4 data for this case?
Flags: needinfo?(bwu) → needinfo?(rleider)
Severity: normal → critical
Priority: P3 → P1
(In reply to Benjamin Chen [:bechen] from comment #7)
> On Nexus5, fennec uses vp9 sw decoder, but chrome uses hw decoder.

Shouldn't we fix this instead?
Why don't we use the hw decoder?
(In reply to Jean-Yves Avenard [:jya] from comment #11)
> (In reply to Benjamin Chen [:bechen] from comment #7)
> > On Nexus5, fennec uses vp9 sw decoder, but chrome uses hw decoder.
> 
> Shouldn't we fix this instead?
> Why don't we use the hw decoder?
I just double confirmed with Benjamin. That comment is really confusing...
What he means is Fennec got vp9 data from Youtube and uses SW decoder but Chrome got H264 data and uses HW decoder.
After discussing Anthony, we should check if there is a VP9 HW decoder or not. If yes, we just use VP9. If no, let's use h264. That's what we can do it without Youtube's help.
Flags: needinfo?(rleider)
I already opened a bug to do just that. 

Bug 1261273. 

Btw; I had warned that this problem would happen when we removed the VP9 estimizer on android because it was too big.
Blocks: 1266102
Keywords: regression
Yeah. Important things/bugs never go away and they always come back! VP9 estimizer might be too heavy on Android. AFAIK, we can easily check media_codecs.xml to know if there is VP9 HW decoder or not.
The issue here is that when the estimizer was removed, MSE/VP9 was turned on all the time. That it was going to have a serious impact on battery life was a given.
(In reply to Jean-Yves Avenard [:jya] from comment #16)
> The issue here is that when the estimizer was removed, MSE/VP9 was turned on
> all the time. That it was going to have a serious impact on battery life was
> a given.
Thanks for your explanations and more contexts! I thought before estimizer was added, MSE/VP9 was already default chosen. And with estimizer, VP9 would be not recommended and h264 would be chosen. After estimizer was removed, Fennec goes back to choose VP9.
Benjamin,
Please further check how to know if there is VP9 HW decoder via media_codecs.xml. John knows more details about it. You can ask him. :-) It would be better to fix this bug on 52. 
Thanks.
Flags: needinfo?(bechen)
Target Milestone: --- → Firefox 52
(In reply to Blake Wu [:bwu][:blakewu] from comment #18)
> Benjamin,
> Please further check how to know if there is VP9 HW decoder via
> media_codecs.xml. John knows more details about it. You can ask him. :-) It
> would be better to fix this bug on 52. 
> Thanks.

According to the media_codec.xml on nexus5, it doesn't support vp9 hw decoding.
Flags: needinfo?(bechen)
Snorp, 
May I know if you have concerns to use H264 if there is no VP9 HW decoder as comment 13 described?
Flags: needinfo?(snorp)
(In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> Snorp, 
> May I know if you have concerns to use H264 if there is no VP9 HW decoder as
> comment 13 described?

Nope, this sounds great.
Flags: needinfo?(snorp)
Comment on attachment 8802839 [details]
Bug 1286738 - part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder.

https://reviewboard.mozilla.org/r/87130/#review86150

::: dom/media/mediasource/MediaSource.cpp:112
(Diff revision 2)
>    if (mimeType.EqualsASCII("video/webm")) {
>      if (!(Preferences::GetBool("media.mediasource.webm.enabled", false) ||
>            IsWebMForced(aDiagnostics))) {
>        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>      }
> +    if (mozilla::jni::IsFennec()) {

You must put this within #ifdef so thr code is conditional on android

::: dom/media/mediasource/MediaSource.cpp:114
(Diff revision 2)
>            IsWebMForced(aDiagnostics))) {
>        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>      }
> +    if (mozilla::jni::IsFennec()) {
> +      const nsACString& codec = NS_ConvertUTF16toUTF8(contentType.GetCodecs());
> +      if (codec.EqualsASCII("vp8") && mozilla::java::HardwareCodecCapabilityUtils::

This whole block should be called from within webMForced

so that any overrides for enabling webm is in the same location
(In reply to Jean-Yves Avenard [:jya] from comment #24)
> Comment on attachment 8802839 [details]
> Bug 1286738 - For Fennec MSE vp8/vp9, use hardware decoder and don't use
> software decoder.
> 
> https://reviewboard.mozilla.org/r/87130/#review86150
> 
> ::: dom/media/mediasource/MediaSource.cpp:112
> (Diff revision 2)
> >    if (mimeType.EqualsASCII("video/webm")) {
> >      if (!(Preferences::GetBool("media.mediasource.webm.enabled", false) ||
> >            IsWebMForced(aDiagnostics))) {
> >        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
> >      }
> > +    if (mozilla::jni::IsFennec()) {
> 
> You must put this within #ifdef so thr code is conditional on android
> 
> ::: dom/media/mediasource/MediaSource.cpp:114
> (Diff revision 2)
> >            IsWebMForced(aDiagnostics))) {
> >        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
> >      }
> > +    if (mozilla::jni::IsFennec()) {
> > +      const nsACString& codec = NS_ConvertUTF16toUTF8(contentType.GetCodecs());
> > +      if (codec.EqualsASCII("vp8") && mozilla::java::HardwareCodecCapabilityUtils::
> 
> This whole block should be called from within webMForced
> 
> so that any overrides for enabling webm is in the same location

Hi jya: 
Do you know why we call IsWebMForced function for "audio/webm"?
https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/MediaSource.cpp#116

Because I want to move my code into VP9Benchmark::IsVP9DecodeFast() that check the video decoding speed.
So I don't think audio/webm need IsWebMForced function. Or always return true in IsWebMForced for audio/webm.
Flags: needinfo?(jyavenard)
(In reply to Benjamin Chen [:bechen] from comment #25)

> > ::: dom/media/mediasource/MediaSource.cpp:112
> > (Diff revision 2)
> > >    if (mimeType.EqualsASCII("video/webm")) {
> > >      if (!(Preferences::GetBool("media.mediasource.webm.enabled", false) ||
> > >            IsWebMForced(aDiagnostics))) {
> > >        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
> > >      }
> > > +    if (mozilla::jni::IsFennec()) {
> > 
> > You must put this within #ifdef so thr code is conditional on android
> > 
> > ::: dom/media/mediasource/MediaSource.cpp:114
> > (Diff revision 2)
> > >            IsWebMForced(aDiagnostics))) {
> > >        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
> > >      }
> > > +    if (mozilla::jni::IsFennec()) {
> > > +      const nsACString& codec = NS_ConvertUTF16toUTF8(contentType.GetCodecs());
> > > +      if (codec.EqualsASCII("vp8") && mozilla::java::HardwareCodecCapabilityUtils::
> > 
> > This whole block should be called from within webMForced
> > 
> > so that any overrides for enabling webm is in the same location
> 
> Hi jya: 
> Do you know why we call IsWebMForced function for "audio/webm"?
> https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/
> MediaSource.cpp#116

It's an error. I don't see the need for it. As media.mediasource.webm.audio.enabled is true by default, this will never be called. 

> 
> Because I want to move my code into VP9Benchmark::IsVP9DecodeFast() that
> check the video decoding speed.

I don't believe you should put this new code there, VP9Benchmark is about measuring actual decoding speed.

I think the ideal place would be to put this in the Android PDM SupportsMimeType and only return true for VP9 if hardware decoding is supported.

BTW we have a bug set for this in bug 1261273.

For now, just change the VP9Benchmark::IsVP9DecodeFast to always return false on android, and continue the work to enable MSE/VP9 by default on machine with an hardware decoder as part of bug 1261273

It's the easiest way to solve this bug, it would be a line change and can be easily uplifted. 



> So I don't think audio/webm need IsWebMForced function. Or always return
> true in IsWebMForced for audio/webm

Right.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #26)
> I don't believe you should put this new code there, VP9Benchmark is about
> measuring actual decoding speed.
> 
> I think the ideal place would be to put this in the Android PDM
> SupportsMimeType and only return true for VP9 if hardware decoding is
> supported.
> 
> BTW we have a bug set for this in bug 1261273.
If we check the VP9 hw in Android PDM, that will affect the normal playback case <video src=xxx.vp9>. Although it should fallback to our gecko AgnosticDecoderModule libvpx library, but I still prefer to modify the VP9Benchmark::IsVP9DecodeFast.
I disagree...
VP9Benchmark is about VP9 benchmark, that is measuring how fast decoding can be done in VP9. 
That you can place your code in there because it's convenient and quick doesn't mean that's where it should be done. You would be breaking the scope of the code and the abstraction level.
Ideally, VP9Benchmark shouldn't even have to know about Android, that someone decided it was too big to be included on Fennec is a bummer, but that's how it is, so we have to make do.
But let's not break it even more by adding specialised code in there. 

If you don't want to disable VP9 outside of MSE (and I agree with you there) we still come down to two issues:
1-To solve this particular bug, VP9Benchmark::IsFast should return false, i had already indicated this when the original change was made.

2-Add a new method, such has bool PlatformDecoderModule::IsHardwareAccelerated(mediacodectype) and use that in IsWebMForced. This work should be done in bug 1261273.
Comment on attachment 8802839 [details]
Bug 1286738 - part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder.

https://reviewboard.mozilla.org/r/87130/#review86544

::: dom/media/Benchmark.cpp:39
(Diff revision 3)
>  VP9Benchmark::IsVP9DecodeFast()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>  #ifdef MOZ_WIDGET_ANDROID
> -  return true;
> +  return mozilla::java::HardwareCodecCapabilityUtils::

Please dont do that.. i have indicated on the why

::: dom/media/mediasource/MediaSource.cpp:107
(Diff revision 3)
>        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>      }
>      return NS_OK;
>    }
>    if (mimeType.EqualsASCII("video/webm")) {
> -    if (!(Preferences::GetBool("media.mediasource.webm.enabled", false) ||
> +    if (!(Preferences::GetBool("media.mediasource.webm.enabled", false) &&

Why that change, youve inverted the logic and disable webm on machine that used to support it.

i think you may have overlooked on what the test was

if (!(A || B))
Attachment #8802839 - Flags: review?(jyavenard)
Comment on attachment 8802839 [details]
Bug 1286738 - part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder.

https://reviewboard.mozilla.org/r/87130/#review86544

> Why that change, youve inverted the logic and disable webm on machine that used to support it.
> 
> i think you may have overlooked on what the test was
> 
> if (!(A || B))

https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#567
The pref "media.mediasource.webm.enabled" is true for fennec, so no matter what the reslut of IsWebMForced, we always use vp9. 
I'm thinking about to change the pref value or don't check the pref here.
(In reply to Benjamin Chen [:bechen] from comment #31)
> Comment on attachment 8802839 [details]
> Bug 1286738 - For Fennec MSE vp9, function IsTypeSupported retrun true only
> if we have hw decoder.
> 
> https://reviewboard.mozilla.org/r/87130/#review86544
> 
> > Why that change, youve inverted the logic and disable webm on machine that used to support it.
> > 
> > i think you may have overlooked on what the test was
> > 
> > if (!(A || B))
> 
> https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#567
> The pref "media.mediasource.webm.enabled" is true for fennec, so no matter
> what the reslut of IsWebMForced, we always use vp9. 

Sure, but that line would still cause it to break all other platforms where the pref is false. You've in effect disabled MSE/webm on desktop.

> I'm thinking about to change the pref value or don't check the pref here.

I would change it back to false, and let the mechanism of webmforced handle it, adding what's needed there.

In any case, if you really want to put your changes in the Benchmark code, then do so and I'll reorganise things in bug 1261273
Comment on attachment 8802839 [details]
Bug 1286738 - part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder.

reviewing by jya.
Attachment #8802839 - Flags: review?(jolin)
Comment on attachment 8802839 [details]
Bug 1286738 - part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder.

https://reviewboard.mozilla.org/r/87130/#review86976

Please split the JNI parts in a separate patch; it should be the first patch. Then you can have the two other patches that only touch MSE separately.

::: dom/media/mediasource/MediaSource.cpp:79
(Diff revision 4)
>    bool mp4supported =
>      DecoderTraits::IsMP4TypeAndEnabled(NS_LITERAL_CSTRING("video/mp4"),
>                                         aDiagnostics);
>    bool hwsupported = gfx::gfxVars::CanUseHardwareVideoDecoding();
> +#ifdef MOZ_WIDGET_ANDROID
> +  return !mp4supported || !hwsupported || VP9Benchmark::IsVP9DecodeFast() ||

YouTube always set the codec type, so you'll know if they are requesting VP8 or VP9.

It would be nicer to not assumer this is always for VP9.

So bonus point if you allow "video/webm" and "video/webm; codecs=vp9" and "video/webm; codecs=vp8"
differently

::: dom/media/mediasource/MediaSource.cpp:81
(Diff revision 4)
>                                         aDiagnostics);
>    bool hwsupported = gfx::gfxVars::CanUseHardwareVideoDecoding();
> +#ifdef MOZ_WIDGET_ANDROID
> +  return !mp4supported || !hwsupported || VP9Benchmark::IsVP9DecodeFast() ||
> +         java::HardwareCodecCapabilityUtils::
> +           GetHWDecoderCapability(nsCString("video/x-vnd.on2.vp9"));

Use the NS_LITERAL_CSTRING macro rather than the constructor

It would also be better to have a dedicated method for this.
e.g. java::HardwareCodecCapabilityUtils::HasHWVP9()
So we don't have to deal nor remember the obsolete on2 mimetype

::: dom/media/mediasource/MediaSource.cpp:125
(Diff revision 4)
>      }
>      return NS_OK;
>    }
>    if (mimeType.EqualsASCII("audio/webm")) {
>      if (!(Preferences::GetBool("media.mediasource.webm.enabled", false) ||
> -          Preferences::GetBool("media.mediasource.webm.audio.enabled", true) ||
> +          Preferences::GetBool("media.mediasource.webm.audio.enabled", true))) {

Please split this in a separate patch, it's out of scope (even if fly by fix)
Attachment #8802839 - Flags: review?(jyavenard) → review+
Comment on attachment 8804601 [details]
Bug 1286738 - part2-HardwareCodecCapabilityUtils: Add HasHWVP9() function to check the vp9 hw decoder.

https://reviewboard.mozilla.org/r/88510/#review87570

LGTM, but you probably want someone else to review this.. I know nothing about JNI and Fennec
Attachment #8804601 - Flags: review?(jyavenard) → review+
Comment on attachment 8804602 [details]
Bug 1286738 - part3-mediasource: Enable fennec vp9 only if we have hw deocder.

https://reviewboard.mozilla.org/r/88512/#review87574
Attachment #8804602 - Flags: review?(jyavenard) → review+
Comment on attachment 8804603 [details]
Bug 1286738 - part4-fixWebMAudio: audio/webm doesn't need the IsWebMForced() check.

https://reviewboard.mozilla.org/r/88514/#review87576
Attachment #8804603 - Flags: review?(jyavenard) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e494b7861699
part1-pref: Disable pref "media.mediasource.webm.enabled" on fennec because we prefer not to use sw vp9 decoder. r=jya
https://hg.mozilla.org/integration/autoland/rev/05c8cd834019
part2-HardwareCodecCapabilityUtils: Add HasHWVP9() function to check the vp9 hw decoder. r=jya
https://hg.mozilla.org/integration/autoland/rev/aa540c9dbd11
part3-mediasource: Enable fennec vp9 only if we have hw deocder. r=jya
https://hg.mozilla.org/integration/autoland/rev/577e23f7124a
part4-fixWebMAudio: audio/webm doesn't need the IsWebMForced() check. r=jya
Keywords: checkin-needed
Depends on: 1299105
See Also: → 1292474
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: