Closed Bug 872430 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: leo.bugzilla.gecko, Assigned: baku)

References

Details

(Whiteboard: MiniWW)

Attachments

(3 files, 3 obsolete files)

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.
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.
Target Milestone: --- → 1.1 QE2
> 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.
Flags: needinfo?(justin.lebar+bug)
Attached patch gecko patch (obsolete) — Splinter Review
In the meantime I upload the patch + tests.
Attachment #749847 - Flags: review?(justin.lebar+bug)
Assignee: nobody → amarchesini
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"
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
http://www.pastebin.mozilla.org/2409858 here nsHTMLMediaElement.h + patch
http://www.pastebin.mozilla.org/2409859 here nsHTMLMediaElement.cpp + patch
Please renominate for blocking if this patch verifiably improves power. Otherwise, we'd accept a low risk uplift.
blocking-b2g: --- → -
tracking-b2g18: --- → +
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();
--------------------------------------------------------------------------------------------
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.
(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.
Attached patch gaia patchSplinter Review
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.
Attachment #750985 - Flags: review?(alive)
Comment on attachment 750985 [details] [diff] [review]
gaia patch

r=me, looks like just to remove what you did in bug 828283?
Attachment #750985 - Flags: review?(alive) → review+
> 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
Do we need to wait for gecko part landing?
yes we do.
> 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?
Flags: needinfo?(justin.lebar+bug)
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.
Attachment #749847 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 749847 [details] [diff] [review]
gecko patch

whoops; I meant f+.
Attachment #749847 - Flags: review+ → feedback+
Attached patch gecko patch (obsolete) — Splinter Review
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.
Attachment #749847 - Attachment is obsolete: true
Attachment #753726 - Flags: review?(justin.lebar+bug)
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.
Attachment #753726 - Flags: review?(justin.lebar+bug) → review-
Oh, it's also invalid to call into mOuter from the WakeLockBoolWrapper's destructor, because members get destructed /after/ their containing class is destructed.
Attached patch gecko patch (obsolete) — Splinter Review
Attachment #753726 - Attachment is obsolete: true
Attachment #754099 - Flags: review?(justin.lebar+bug)
(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 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.
Attachment #754099 - Flags: review?(justin.lebar+bug) → review+
Attachment #754099 - Attachment is obsolete: true
Attachment #754864 - Flags: review+
Attached patch m-c gecko patchSplinter Review
Attachment #754871 - Flags: review+
Keywords: checkin-needed
Depends on: 868325
John, can you assist with the Gaia landing please?
Flags: needinfo?(jhford)
Keywords: checkin-needed
Whiteboard: [leave open]
Whiteboard: [leave open] → [leave open] , MiniWW
As discussed in San Diego work week marking this as leo+.
blocking-b2g: - → leo+
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.
Keywords: checkin-needed
Whiteboard: [leave open] , MiniWW → MiniWW
Keywords: checkin-needed
Depends on: 879214
Flags: in-moztrap-
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!
(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.
(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.
You need to log in before you can comment on or make changes to this bug.