872430 - FM Radio shouldn`t use CPU wake lock, should have sleep state.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[leo.bugzilla.gecko]

BuildID revision="7cb46499e0b91ca20f6aed58d6067d7c451875b9"
gaia revision="5cbb19e4bb78a7ad879fbe4b9a841e1c35714f5c"
gecko revision="950b402b6188bb2f3ce3176e620ed5249719d720"

The media apps generally use CPU wake lock. But FM Radio is available to play without CPU wake lock.
Currently FM Radio dosen`t use wake lock in fm.js, but I checked to use wake lock by Bug 828283. 
===================================================================
screen_manager.js
          // If some audio channel is active we refresh the cpuSleepAllowed
          // immediately, otherwise we just use a timer in order to prevert
          // rapid stop/start.
          if (audioActive) {
            this._audioActive = true;
            this.refreshCpuSleepAllowed();
          } else {
            var self = this;
            this._audioCpuSleepTimerId = setTimeout(function cpuSleepTimer() {
              self._audioActive = false;
              self.refreshCpuSleepAllowed();
              self._audioCpuSleepTimerId = 0;
            }, 2000);
          }
        }
===================================================================
I think It is at risk for power consumption and it should need to make improvement Cpu wake lock/unlock.
Please let you test FM Radio.
1. Enable FM Radio during plying music.
2. Play Music/Video during enabling FM Radio.
3. Plug/Unplug headset during enabling FM Radio in foreground/backgrond state.
4. Incomming calls during enabling FM Radio.

Comment 1

[Marco Chen [:mchen]]

Hi Andrea,

I remember that the reason for adding audioActive is for keeping system alive for playing audio. Since bug 859824 already automatically add cpu wakelock for each media element. could I remove the code section for audioActive?

Thanks.

Comment 2

[Andrea Marchesini [:baku]]

> I remember that the reason for adding audioActive is for keeping system
> alive for playing audio. Since bug 859824 already automatically add cpu
> wakelock for each media element. could I remove the code section for
> audioActive?

Yes, we should move this timer from gaia to gecko.

The MediaElement should keep the wakelock for a few seconds before releasing it.
Jlebar, if you agree with this, I can go implementing it. I think I already have a patch for it...

I think we should land the gaia patch (where you remove this timer) and gecko (where we add the timer) at the same time.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: gecko patch

In the meantime I upload the patch + tests.

Comment 4

[leo.bugzilla.gecko]

But in this case, FM Radio shouldn`t add wakelock like telephony because it is available to play FM Radio in sleep state.

Additionally, Can you all of patch about Wakelock after the below revision? 

<x-mozbuild buildid="20130425070204" update_channel="nightly" version="18.0" gecko-fingerprint="127402238b731a080ceef2dc607fb5575bffc2cb"/>
gecko: revision="bdbd181fa434d15976a2ae965d82ec7fe85a963f"
gaia: revision="9c6e9decdd1333f3b6dbed22538a85361cf944dc"

Comment 5

[leo.bugzilla.gecko]

I`m reviewing the below patch. But It is not easy to sync the code. 
I`t like to receive the full souce (nsHTMLMediaElement.h, nsHTMLMediaElement.cpp, screen_manager.js) Can you share the full source? 

Bug 859824 - [AudioChannel/Music App] Music App Didn't Release Wakelock When paused by AudioChannel
Bug 868325 - VideoElement should require a 'screen' wakeLock when active
Bug 868943 - HTMLMediaElement::mWakeLock has to be unlocked and not just set to null

Comment 6

[Andrea Marchesini [:baku]]

Is this enough?

https://mxr.mozilla.org/mozilla-b2g18/source/content/html/content/src/nsHTMLMediaElement.cpp
https://mxr.mozilla.org/mozilla-b2g18/source/content/html/content/public/nsHTMLMediaElement.h
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/screen_manager.js

Comment 7

[Andrea Marchesini [:baku]]

http://www.pastebin.mozilla.org/2409858 here nsHTMLMediaElement.h + patch
http://www.pastebin.mozilla.org/2409859 here nsHTMLMediaElement.cpp + patch

Comment 8

[Alex Keybl [:akeybl]]

Please renominate for blocking if this patch verifiably improves power. Otherwise, we'd accept a low risk uplift.

Comment 9

[leo.bugzilla.gecko]

Thanks for your comment.
I reviewed and I think the recent patch is nothing to do with FM Radio sleep. 

I tried to added the log code and checked "mozFMRadio.enabled" flag. but It set after enabling FM for 2sec.
I think FM need another attribute not to set wakelock.
If this methord use, please check to unplug the headset for enabling FM.

-------------------------------------------------------------------------------------------
      case 'mozChromeEvent':
        if (evt.detail.type == 'audio-channel-changed') {
          var audioActive = (evt.detail.channel !== 'none' ||evt.detail.channel!=='telephony');
          console.log('screen_manager.js : audio-channel-changed audioActive: ' + audioActive);
          if (this._audioCpuSleepTimerId) {
            clearTimeout(this._audioCpuSleepTimerId);
            this._audioCpuSleepTimerId = 0;
          }

          // If some audio channel is active we refresh the cpuSleepAllowed
          // immediately, otherwise we just use a timer in order to prevert
          // rapid stop/start.
          if (audioActive) {
            this._audioActive = true;
            var mozFMRadio = navigator.mozFM || navigator.mozFMRadio;
            console.log('screen_manager.js : ' + (mozFMRadio.enabled ? 'on' : 'off'));
            this.refreshCpuSleepAllowed();
--------------------------------------------------------------------------------------------

Comment 10

[leo.bugzilla.gecko]

I have the question. 
WakeLockBoolWrapper(nsHTMLMediaElement.cpp) means media`s wakelock instead of mozChromeEvent(screen_manager.js)?
I think 'audio-channel-changed'`s wakelock don`t need. I don`t konw why this code is remained. 
If this code is remove, FM Radio can resolve.

Comment 11

[Andrea Marchesini [:baku]]

(In reply to leo.bugzilla.gecko from comment #10)
> I have the question. 
> WakeLockBoolWrapper(nsHTMLMediaElement.cpp) means media`s wakelock instead
> of mozChromeEvent(screen_manager.js)?
> I think 'audio-channel-changed'`s wakelock don`t need. I don`t konw why this
> code is remained. 
> If this code is remove, FM Radio can resolve.

Yes. This is the idea. That code must be removed. I'm writing and testing a patch.
But this existing code has a timer to avoid rapid stop/start audio and rapid lock/unlock the wakelock. This is the porpoise of my first patch.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: gaia patch

This patch removes the wakelock in the screen_manager.js. This is not needed because the MediaElement already has one and we don't need it for the FMRadio.

The Gecko patch implements the timer as it was implemented in gaia.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 750985 [details] [diff] [review]
gaia patch

r=me, looks like just to remove what you did in bug 828283?

Comment 14

[Andrea Marchesini [:baku]]

> r=me, looks like just to remove what you did in bug 828283?

Unfortunately just part of it. The CPU lock is not needed.

https://github.com/mozilla-b2g/gaia/pull/9850

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Do we need to wait for gecko part landing?

Comment 16

[Andrea Marchesini [:baku]]

yes we do.

Comment 17

[Justin Lebar (not reading bugmail)]

> The MediaElement should keep the wakelock for a few seconds before releasing it.

Okay.  Can we please update the summary of this bug to match what we're doing?

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 749847 [details] [diff] [review]
gecko patch

>diff --git a/content/html/content/src/nsHTMLMediaElement.cpp b/content/html/content/src/nsHTMLMediaElement.cpp
>--- a/content/html/content/src/nsHTMLMediaElement.cpp
>+++ b/content/html/content/src/nsHTMLMediaElement.cpp

>@@ -1898,23 +1898,44 @@ nsHTMLMediaElement::WakeLockBoolWrapper:
> {
>   if (!mOuter) {
>     return;
>   }
> 
>   bool playing = (!mValue && mCanPlay);
> 
>   if (playing) {
>+    if (mTimer) {
>+      mTimer->Cancel();
>+      mTimer = nullptr;
>+    }
>+
>     mOuter->WakeLockCreate();
>+
>   } else {
>-    mOuter->WakeLockRelease();
>+    if (!mTimer) {
>+      mTimer = do_CreateInstance("@mozilla.org/timer;1");
>+      mTimer->InitWithFuncCallback(TimerCallback, this, 2000,
>+                                   nsITimer::TYPE_ONE_SHOT);

Please make this a pref.  Then you can SpecialPowers.pushPrefEnv in your test
and set this to something small; this will make your tests go much faster.

>+    } else {
>+      mTimer = nullptr;
>+      mOuter->WakeLockRelease();
>+    }
>   }
> }

Is there a reason we can't do mOuter->WakeLockRelease() in the timer callback?
Then we don't have to use !!mTimer as state; using it that way is really
confusing.

We also need to make sure that this object doesn't get destroyed before the
timer fires.  It's often better to avoid InitWithFuncCallback when you're going
to call a method on |this|, so you can avoid this problem.  We also have to
make sure that mOuter is not a dangling pointer when the timeout fires.  Weak
refs might help.

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 749847 [details] [diff] [review]
gecko patch

whoops; I meant f+.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: gecko patch

What about this approach? WakeLockBoolWrapper is deleted when nsHTMLMediaElement is destroyed. Here the DTOR cancels the timer if it exists. It should make safe enough the existence of mOuter.

Comment 21

[Justin Lebar (not reading bugmail)]

Okay, this is fine.  I'd still like to take another look, though.

>diff --git a/content/html/content/src/nsHTMLMediaElement.cpp b/content/html/content/src/nsHTMLMediaElement.cpp
>--- a/content/html/content/src/nsHTMLMediaElement.cpp
>+++ b/content/html/content/src/nsHTMLMediaElement.cpp

>@@ -1881,40 +1881,69 @@ nsHTMLMediaElement::WakeLockBoolWrapper:
>     return *this;
>   }
> 
>   mValue = val;
>   UpdateWakeLock();
>   return *this;
> }
> 
>+nsHTMLMediaElement::WakeLockBoolWrapper::~WakeLockBoolWrapper()
>+{
>+  if (mTimer) {
>+    mOuter->WakeLockRelease();
>+    mTimer->Cancel();
>+    mTimer = nullptr;
>+  }

mTimer = nullptr isn't necessary; we're about to destruct that member.

Why do we call WakeLockRelease only if !!mTimer?

>+}

> void
> nsHTMLMediaElement::WakeLockBoolWrapper::UpdateWakeLock()
> {
>   if (!mOuter) {
>     return;
>   }
> 
>   bool playing = (!mValue && mCanPlay);
> 
>+ if (mTimer) {
>+   mTimer->Cancel();
>+   mTimer = nullptr;
>+  }

Surely you don't want to restart the timer if it's already running and !playing?

> void
>+nsHTMLMediaElement::WakeLockBoolWrapper::TimerCallback(nsITimer* aTimer,
>+                                                       void* aClosure)
>+{
>+  WakeLockBoolWrapper* wakeLock = static_cast<WakeLockBoolWrapper*>(aClosure);
>+  wakeLock->mOuter->WakeLockRelease();
>+
>+  MOZ_ASSERT(wakeLock->mTimer);

We'll crash on the next line of wakeLock->mTimer is null; this assertion
doesn't help us.

>+  wakeLock->mTimer->Cancel();

You don't need to cancel the timer; it's a one-shot.

>+  wakeLock->mTimer = nullptr;
>+}

>diff --git a/content/html/content/test/test_audio_wakelock.html b/content/html/content/test/test_audio_wakelock.html
>--- a/content/html/content/test/test_audio_wakelock.html
>+++ b/content/html/content/test/test_audio_wakelock.html

>+var timeoutWakelock = SpecialPowers.getIntPref("media.wakelock_timeout");
>+SpecialPowers.setIntPref("media.wakelock_timeout", 1500);

Please use pushPrefEnv (which will do the right thing, including ensuring that
enough time passes between setting the pref and running the test and ensuring
that the pref gets unset if it was unset to begin with) and please set it to a
much shorter timeout, so the test runs faster.

It would probably be good to use a constant for this, instead of hardcoding the
delays into the test.

>diff --git a/content/html/content/test/test_video_wakelock.html b/content/html/content/test/test_video_wakelock.html
>--- a/content/html/content/test/test_video_wakelock.html
>+++ b/content/html/content/test/test_video_wakelock.html

Please make the same changes to this test with respect to the timeout.

Comment 22

[Justin Lebar (not reading bugmail)]

Oh, it's also invalid to call into mOuter from the WakeLockBoolWrapper's destructor, because members get destructed /after/ their containing class is destructed.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: gecko patch

Comment 24

[Justin Lebar (not reading bugmail)]

(In reply to Justin Lebar [:jlebar] from comment #17)
> > The MediaElement should keep the wakelock for a few seconds before releasing it.
> 
> Okay.  Can we please update the summary of this bug to match what we're
> doing?

Comment 25

[Justin Lebar (not reading bugmail)]

Comment on attachment 754099 [details] [diff] [review]
gecko patch

>   if (playing) {
>+   if (mTimer) {

Indentation.

>+     mTimer->Cancel();
>+     mTimer = nullptr;
>+    }
>     mOuter->WakeLockCreate();
>-  } else {
>-    mOuter->WakeLockRelease();
>+  } else if (!mTimer) {

I think it would be helpful if we had a comment here saying something like

  Don't release the wake lock immediately; instead, release it after a grace
  period.

Looks good aside from these nits; r=me.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: b2g18 gecko patch

Comment 27

[Andrea Marchesini [:baku]]

Attachment: m-c gecko patch

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/72f59840a15e

Comment 29

[Ryan VanderMeulen [:RyanVM]]

John, can you assist with the Gaia landing please?

Comment 30

[Leo]

As discussed in San Diego work week marking this as leo+.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72f59840a15e

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/15621d6da171

All Gecko parts have landed. Please resolve this and set the status-b2g18 flag to fixed when the Gaia patch lands on master and v1-train.

Comment 33

[Michael Wu [:mwu]]

https://github.com/mozilla-b2g/gaia/pull/10049
https://github.com/mozilla-b2g/gaia/pull/10050

Comment 34

[Michael Wu [:mwu]]

https://github.com/mozilla-b2g/gaia/commit/b334931ad80d5985eb73b152ef27c5b62bb60c83 (master)
https://github.com/mozilla-b2g/gaia/commit/f72582c3e01645239c988261a3a471d07a0d440a (v1-train)

Comment 35

[Gene Lian (I already quit Mozilla)]

The Gaia patch here is causing a bad regression (please see bug 880931, comment #31). We cannot remove the setting of |power.cpuSleepAllowed| from screen_manager.js. Gecko needs to depend on this to acquire/release the system CPU wake lock. Without this, all the CPU wake locking mechanism will fail. Please correct me if I'm wrong. Thanks!

Comment 36

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Gene Lian [:gene] from comment #35)
> The Gaia patch here is causing a bad regression (please see bug 880931,
> comment #31). We cannot remove the setting of |power.cpuSleepAllowed| from
> screen_manager.js. Gecko needs to depend on this to acquire/release the
> system CPU wake lock. Without this, all the CPU wake locking mechanism will
> fail. Please correct me if I'm wrong. Thanks!

I think the patch mistakenly remove the cpu wake lock related code. I will recover it.

Comment 37

[Gene Lian (I already quit Mozilla)]

(In reply to Alive Kuo [:alive] from comment #36)
> I think the patch mistakenly remove the cpu wake lock related code. I will
> recover it.

Thank you for the prompt help. That would be nice if you can ask Kanru or me to have a double check when you're done.