[AudioChannel/Music App] Music App Didn't Release Wakelock When paused by AudioChannel

VERIFIED FIXED in Firefox 23

Status

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mchen, Assigned: baku)

Tracking

unspecified
1.1 QE2 (6jun)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: MiniWW)

Attachments

(5 attachments, 7 obsolete attachments)

973 bytes, patch
dkuo
: review+
justin.lebar+bug
: review+
Details | Diff | Splinter Review
625 bytes, patch
justin.lebar+bug
: feedback-
Details | Diff | Splinter Review
11.87 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.30 KB, patch
Details | Diff | Splinter Review
947 bytes, patch
Details | Diff | Splinter Review
Reporter

Description

6 years ago
Consider the case as below.
  1. Music app starts to play in the foreground.
  2. Music app is to background.
  3. Video app starts to play in the foreground. (audio from music app is paused by AudioChannel)
  4. Video app stops to play. (audio from music app is not resumed by AudioChannel until it backs to foreground.)
  5. Press power key to go into suspend.

According to music app will ask a wakelock on step 1 and wakelcok is not released at step 3 because music app doesn't know this situation. Finally the wakelock from Music app prevents device go into suspend at step 5.
Reporter

Comment 1

6 years ago
Maybe this can be solved with Bug 829409.
With Bug 829409, we can avoid to add some events into each components for notifying app about the pause/resume state by AudioChannel. Ex: MediaElement, FM.
Assignee

Comment 2

6 years ago
I think the new Audio API can fix Bug 829409. Take a look of this: https://wiki.mozilla.org/WebAPI/AudioChannels

The music app can check the soundingAudioChannels and see if there an active channel or not.
Reporter

Comment 3

6 years ago
(In reply to Andrea Marchesini (:baku) from comment #2)
> The music app can check the soundingAudioChannels and see if there an active
> channel or not.

Thanks for joining the discussion first.

1. soundingAudioChannels() is a query API, does music app need to actively query it time by time? I think a event or callback to notify the change will be better then this.

2. From app's view, it is strange that why I play a audio with a channel but doesn't see it in soundingAudioChannels(). 

3. According to 2, I think app uses a setOccupyChannel() will more make sense and app exactly know what is happened. (That api will plan a callback or event for notification to app.)
Assignee

Comment 4

6 years ago
> 3. According to 2, I think app uses a setOccupyChannel() will more make
> sense and app exactly know what is happened. (That api will plan a callback
> or event for notification to app.)

I disagree. I think the main problem is that the app has to set the wakelock.
What about if the wakelock is set by the AudioChannelService/Agent?
It seems a bit unclear why the wakelock is a property managed by the app, and the audio is not.

IF the wakelock is enabled when there is an active channel, I think the audio channel service (or agent) that has to set the wakelock.
Reporter

Comment 5

6 years ago
Hi Andrea,

I think the processing of wakelock by app is just a case here. We can put consideration on general case of how does app do corresponding actions when an audio channel is asked to paused/resumed by AudioChannelService. I think we can discuss this on bug 829409.

So maybe moving wakelock acquiring from app to gecko is an option, I also discussed this with alive today.
Reporter

Comment 6

6 years ago
Hi all,

It seems to be done by the bug as below in end of last year but not ask for uplifting.
After doing some test, I will try to ask for uplifting then music app can remove the code of wakelock.

bug 739542 - Disable screen timeout when playing HTML5 <video> (webm, H.264)
Reporter

Comment 7

6 years ago
Due to the issue here, there is a power consumption issue in the scanrio as below.

1. User starts to play a music.
2. Music app is falled to background.
3. User starts a video app and play it (music app is paused by AudioChannel)
4. Video is played to the end then user press the power button to suspend state.

Due to music app didn't release the wakelock, device can just go into early suspend only. So the power consumption will be high and cause to out of battery after several hours. So nominate this one as a blocker.
Assignee: nobody → dkuo
blocking-b2g: --- → tef?
The user impact here is not clear. The set of cases where it can occur is not clear. Blocking-. Please renominate if the user impact is dire enough to stop the release.
blocking-b2g: tef? → -
Assignee

Comment 9

6 years ago
I would like to see patch for bug 739542 (and improved a bit) and no wakelock in music app.
The code of the music app will be much much simpler and the MediaElement will take care of wakelock & C for any application, if not muted by the AudioChannelService.

In general we need a wakelock when a MediaElement is playing audio and it's not muted by the AudioChannelService. So there is not reason to have a wakelock in the app if this can be managed by gecko.

I propose to push that patch and remove wake lock for any existing app using it _just_ for audio.
Reporter

Comment 10

6 years ago
I measured the current and report here. The device is uangi with SIM card inside and modem enabled.

Normal sleep current is 3.8mA.
Abnormal sleep current caused by this bug is 21.5mA.

If battery capacity is 1400mA, then normal standby time will be 368 hrs but 65 hrs in abnormal case. So this bug cause significant degrade of battery life.

What user scenario will cause this bug?
  1. When user starts to listen a music from music app.
  2. Then user start to watch a video or listen FM Radio.
  3. Then user stop the video or FMRadio. (music is stopped at step 2 as well)
  4. User presses the power key to let device go into suspend then leave.
blocking-b2g: - → tef?
Assignee

Comment 11

6 years ago
Is the bug 739542 landed on b2g18? We should check how the battery life changes with that patch applied.
Whiteboard: [tef-triage]
Given the user impact of draining battery with very little indication of why (music app in background) we believe this should block.
blocking-b2g: tef? → tef+
Assignee

Comment 13

6 years ago
I'm going to test if the patch for 739542 fixes this bug. if it does, Dominic, can you remove the wakelock from the music app?
Assignee

Comment 14

6 years ago
This patch removes cpuLock. The lock is controlled by the MediaElement, strongly connected to the AudioChannelService.
Attachment #742417 - Flags: review?(dkuo)
Assignee

Comment 15

6 years ago
This patch is based on 739542.
It connects the current patch with the value of AudioChannelAgent->CanPlay().
Attachment #742420 - Flags: review?(mchen)
Assignee

Comment 16

6 years ago
Comment on attachment 742420 [details] [diff] [review]
gecko patch - Wakelock for MediaElement

This patch is not ready. We should use Playing_media only for ANDROID, and cpu for B2G. Still working on it.
Attachment #742420 - Attachment is obsolete: true
Attachment #742420 - Flags: review?(mchen)
Assignee

Updated

6 years ago
Attachment #742420 - Attachment is obsolete: false
Assignee

Comment 17

6 years ago
Comment on attachment 742417 [details] [diff] [review]
gaia patch - cpuLock removed

wait before reviewing this patch. I want to finish the gecko part first.
Attachment #742417 - Flags: review?(dkuo)
Reporter

Comment 18

6 years ago
Hi Andrea,

One question about wrapping wakelock into mPause.

Q: When a music is playing under the way of network streaming, if the network is very slow (maybe it need 5 minutes to fill buffer) then device will be kept to alive 5 minutes. Is this a right way?

Thanks.
baku, looks like you are the assignee there?
Assignee: dkuo → amarchesini
Assignee

Comment 20

6 years ago
I think so. But maybe jlebar has something to say about it.
Assignee: amarchesini → dkuo
Flags: needinfo?(justin.lebar+bug)
Comment on attachment 742420 [details] [diff] [review]
gecko patch - Wakelock for MediaElement

This patch is backported from trunk, but it's bad.  It's also not going to work as-is without changes to Gaia.

This code acquires the "Playing_media" wake lock, but if nobody in Gaia listens for that lock, acquiring it will have no effect whatsoever.  So at the very least, you'd need to either modify Gaia to listen to this lock or, more sanely, switch it to use the "cpu" wake lock.

But note that Android uses this code to hold the /screen/ alive, because our Android implementation of wake locks is completely broken, and any wake lock you acquire holds the screen, and only the screen, alive.

Note further that this code holds the screen on Android whenever an <audio> or <video> is playing, which is bogus.

Anyway.  To the question at hand, if audio is buffering for a long time, holding the CPU wake lock seems like the only thing we /can/ do, without killing the network connection altogether.

One last thing: You need to hold the CPU wake lock between gaps in playback, so I'm not sure you can do what this Gaia patch does.
Attachment #742420 - Flags: feedback-
Flags: needinfo?(justin.lebar+bug)
Whiteboard: [tef-triage]
Assignee

Comment 22

6 years ago
Thanks for answering. With this patch I keep the current behaviour for the MediaElement, but if MOZ_B2G is defined, we have an additional wakelock for the 'cpu'.

Probably we should create a follow up just to use the Playing_media only if ANDROID is defined.
Attachment #742420 - Attachment is obsolete: true
Attachment #743111 - Flags: review?(justin.lebar+bug)
> Probably we should create a follow up just to use the Playing_media only if ANDROID is defined.

It's not necessary to hold both "cpu" and "Playing_media".  Our Android code will behave the same regardless of which lock you hold.

So I think a better thing would be to hold "cpu" unconditionally and let Android clean up its mess if and when they want to.  Or alternatively to change the name to "playing-media" and listen for that lock in Gaia (which is a simple thing to do).
Assignee

Comment 24

6 years ago
Attachment #743111 - Attachment is obsolete: true
Attachment #743111 - Flags: review?(justin.lebar+bug)
Attachment #743148 - Flags: review?(justin.lebar+bug)
Comment on attachment 743148 [details] [diff] [review]
gecko patch - Wakelock for MediaElement

> It's not necessary to hold both "cpu" and "Playing_media".  Our Android code will behave the same 
> regardless of which lock you hold.
>
> So I think a better thing would be to hold "cpu" unconditionally.

Is there a reason not to do this?  These ifdef MOZ_B2G's make testing really difficult; we've seen cases in the past where we had serious leaks that weren't detected until we took them away.

Also, would you mind adding a comment indicating that mWakeLock automatically unlocks itself in its destructor?
Attachment #743148 - Flags: review?(justin.lebar+bug)
Reporter

Comment 26

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #21)
> Comment on attachment 742420 [details] [diff] [review]
> gecko patch - Wakelock for MediaElement
> Anyway.  To the question at hand, if audio is buffering for a long time,
> holding the CPU wake lock seems like the only thing we /can/ do, without
> killing the network connection altogether.

Let us consider cases as below when network bandwidth is slow

  1. A web page without audio tag:
       The device will go into sleep after timeout.

  2. A web page with audio tag as background and no control panel:
       The device will not go into sleep after timeount.

From user's point of view, there is no different between case 1 & 2 but case 2 obviously harm the power usage. If user saw screen is off on case 2 then leave device there, the power is gone because a wakelock there. 

Or we add a wakelock into a place which indicates there is an Audio Streaming on going to audio HW, so we can avoid the bad behavior on case 2.
I'd be fine with timing out the audio wake lock after thirty seconds or so of continuous buffering.  We don't have the infrastructure to support that right now, but we can add it.

What I don't think we should do is let go of the CPU wake lock as soon as audio starts buffering for a few seconds.
Reporter

Comment 28

6 years ago
Hi Andrea/Justin/Dominic,

Is it possible that we solve this tef+ issue on gaia side only because this is a small patch without the effort on side effect?

Then firing a following bug for general issue here.
Assignee

Comment 29

6 years ago
(In reply to Marco Chen [:mchen] from comment #28)
> Hi Andrea/Justin/Dominic,
> 
> Is it possible that we solve this tef+ issue on gaia side only because this
> is a small patch without the effort on side effect?
> 
> Then firing a following bug for general issue here.

No. I think this has to be fixed in gecko. Otherwise ANY 'audio' application has to use the wakelock. This patch is almost ready.
Assignee

Comment 30

6 years ago
Posted patch patch (obsolete) — Splinter Review
The timer will be a follow up if you don't mind.
Attachment #743148 - Attachment is obsolete: true
Attachment #743639 - Flags: review?(justin.lebar+bug)
Comment on attachment 743639 [details] [diff] [review]
patch

Review of attachment 743639 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please s/NULL/nullptr
Attachment #743639 - Flags: review?(justin.lebar+bug) → review+
Assignee

Comment 32

6 years ago
Timer in a separated patch. In the meantime if someone can review the gaia part, it will be great.
Attachment #743639 - Attachment is obsolete: true
Attachment #743666 - Flags: review+
Reporter

Comment 33

6 years ago
Hi Andrea & Justin,

Please keep in mind of that this is a tef+ bug and the progress now is going to second round of carrier's certification already. I know your patch maybe a right way (actually I post this thought on comment 1 already) but the most important thing we need to take care on this stage would be the minimal modification and side effect. Or please help to do more testing on your patch and especially on "b2g_v1.0.1" branch.
Marco, if you want attach a Gaia-only fix here, I'm sure we'd take a close look at that.
Assignee

Comment 35

6 years ago
Posted patch gecko patch - Timer (obsolete) — Splinter Review
Attachment #743699 - Flags: review?(justin.lebar+bug)
Assignee

Updated

6 years ago
Attachment #742417 - Flags: review?(dkuo)
Assignee

Comment 36

6 years ago
This new patch implements a timer for the wakelock->unlock(). paused is 'false' when the element is buffering, so this should be enough for our scenario. The timer is 5 secs.
Comment on attachment 743699 [details] [diff] [review]
gecko patch - Timer

>+    nsCOMPtr<nsITimer> mTimer;

We should cancel this timer in our destructor, otherwise I think it will still
run after this ref is lost, and then we have use-after-free's.

Also, please call it something more descriptive than mTimer.  Maybe
mWakeLockTimeoutTimer.

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

>@@ -1902,22 +1902,43 @@ nsHTMLMediaElement::WakeLockBoolWrapper:
>+    if (mTimer) {
>+      mTimer->Cancel();
>+      mTimer = nullptr;
>+    }
>+
>   } else if (mWakeLock) {
>-    // Wakelock 'unlocks' itself in its destructor.
>-    mWakeLock = nullptr;

Please add a comment here describing what we're doing and why.

>+    if (!mTimer) {

Using !!mTimer as state (to indicate whether we're coming here from a timer
callback) is pretty confusing to me.  At the very least, please add a comment
explaining this.

>+      mTimer = do_CreateInstance("@mozilla.org/timer;1");
>+      mTimer->InitWithFuncCallback(TimerCallback, this, 5000,
>+                                   nsITimer::TYPE_ONE_SHOT);

Please make 5000 a pref.

>+    } else {
>+      // Wakelock 'unlocks' itself in its destructor.
>+      mWakeLock = nullptr;
>+      mTimer = nullptr;
>+   }

Indent this line one more space.

>+void
>+nsHTMLMediaElement::WakeLockBoolWrapper::TimerCallback(nsITimer* aTimer,
>+                                                       void* aClosure)
>+{
>+  WakeLockBoolWrapper* wakeLock = static_cast<WakeLockBoolWrapper*>(aClosure);
>+  wakeLock->UpdateWakeLock();
>+}

It's probably better to use an nsIObserver or nsITimerCallback (so init() or
initWithCallback()) instead, because then we have no risk of wakeLock being a
dangling pointer.  If you do this, you'll need to make the mOuter ref a
WeakRef.

Once more?
Attachment #743699 - Flags: review?(justin.lebar+bug)
> paused is 'false' when the element is buffering, so this should be enough for our scenario.

Do you mean "true"?  Isn't the point here that we time out the wake lock after the element has been paused for 5s?  If paused is "false" while the element is buffering, then we will not time out the wake lock, right?
Assignee

Comment 39

6 years ago
Right. But what about if the input finishes and the next is going to play soon. In the meantime, a good idea is to a have a wakelock enabled... is it?

About true/false: paused is "false" when I do: audio.play() - doesn't matter if the media element is buffering, playing audio, doing an http request. paused is 'false'. So, for this bug, the first patch should be enough.
> Right. But what about if the input finishes and the next is going to play soon. In the meantime, a 
> good idea is to a have a wakelock enabled... is it?

Maybe, but that's completely different from the timer that mchen and I were talking about.

> So, for this bug, the first patch should be enough.

Well, neither the first patch nor the first plus the second is good enough if we want to address mchen's concern in comment 26, right?
Reporter

Comment 41

6 years ago
Hi all,

I post a patch for modification on Music app only.
And please help to give a feedback on this version for V1.0.1 branch.

The reason for this patch is that it takes very low risk then we did in Gecko.
Because it only effect music app only not others.
Thanks.
Attachment #744017 - Flags: feedback?(justin.lebar+bug)
Attachment #744017 - Flags: feedback?(dkuo)
Attachment #744017 - Flags: feedback?(amarchesini)
Assignee

Comment 42

6 years ago
Comment on attachment 744017 [details] [diff] [review]
Patch_v1 on Gaia_Fixed_Only

Review of attachment 744017 [details] [diff] [review]:
-----------------------------------------------------------------

This patch works, but has exactly the same behaviour of the first patch I proposed for gecko.
And it just works for music app...
Can you explain better why you think the patch for gecko is not enough and this is? Thanks.
> This patch works, but has exactly the same behaviour of the first patch I proposed for gecko.

Except it doesn't hold the CPU lock while <audio>'s are buffering, right?

I'm not convinced that releasing the lock under those circumstances is so important, fwiw.
Reporter

Comment 44

6 years ago
(In reply to Andrea Marchesini (:baku) from comment #42)
> This patch works, but has exactly the same behaviour of the first patch I
> proposed for gecko.
> And it just works for music app...
> Can you explain better why you think the patch for gecko is not enough and
> this is? Thanks.

Andrea's first patch added wakelock into HTMLMediaElement class so it doesn't just work on music app but any app used audio tag (ex: video app, web content in browser app.) So your patch effects all apps who used audio tag.

But mine just effect music app |only| and |focus on this bug only|.
Marco, now that I think about this, I think Andrea's approach, even without releasing the wake lock while <audio>'s are buffering, is better.

The reason is, if we ship with the Gaia-only fix that you've attached here, other music players will have to hold the CPU wake lock while they're playing music.  We can expect most apps to hold the lock while they're buffering.

But then even if we take Andrea's patches in a later version of Gecko, apps will still continue doing the wrong thing.  They'll have to, if they want to support old versions of FFOS.

So I think a better thing to do would be to take Andrea's patches as-is (they seem safe to me).  We run the risk of holding the CPU wake lock while audio is buffering (which I think would happen with most apps with your Gaia-only fix anyway), but at least we have the ability to fix this down the road.

Does that sound OK with you?
Reporter

Comment 46

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #45)
The compatibility of Web App on different FxOS version leads me to re-consider Music App should hold wakelock by itself. The reasons are

  1. Mozilla FxOS provided an api for acquiring wakelock from web app. And as I knew that Mozilla's vision is to put all Web API as standard.
    https://developer.mozilla.org/en-US/docs/DOM/window.navigator.requestWakeLock

  2. Once it is an standard then we can assume other OS based on Web Tech will have this API too.

  3. We can make sure our FxOS to implement auto wakelock on HTMLMediaElement then FxOS allow device to keep alive when playing audio. But we can't guarantee other OS based on Web Tech will do it the same with FxOS.

An example:
  Unagi's platfrom holds wakelock from audio driver when there is a audio in playing.
  S2 doesn't help to hold any wakelock when audio in playing.

  If an app doesn't hold wakelock then it can play on Unagi's platform but no on S2. So the apps in the market will hold wakelock by itself in order to keep it's compatibility on each Android Platform.

*** So in order to keep a music app can work on every OS based on Web Tech not only FxOS, the app should hold a wakelock by itself not depend on implementation in each OS. ***

Still thanks all yours discussion here.
Assignee

Comment 47

6 years ago
>   1. Mozilla FxOS provided an api for acquiring wakelock from web app. And
> as I knew that Mozilla's vision is to put all Web API as standard.
>   2. Once it is an standard then we can assume other OS based on Web Tech
> will have this API too.

This is true.

>   3. We can make sure our FxOS to implement auto wakelock on
> HTMLMediaElement then FxOS allow device to keep alive when playing audio.
> But we can't guarantee other OS based on Web Tech will do it the same with
> FxOS.

Right. But we can change this part too. :)
Personally I think we should make life easier to any developer.
If we can implement an audio-wakelock for MediaElement, we should.
I don't see any reason why we should not have that.

> Still thanks all yours discussion here.

Sicking, can you please give your feedback about this topic?
Both patches are already written. We have just to choose the better approach and land what we think to be the better one. Thanks.
Flags: needinfo?(jonas)
Reporter

Comment 48

6 years ago
(In reply to Andrea Marchesini (:baku) from comment #47)
> >   3. We can make sure our FxOS to implement auto wakelock on
> > HTMLMediaElement then FxOS allow device to keep alive when playing audio.
> > But we can't guarantee other OS based on Web Tech will do it the same with
> > FxOS.
> 
> Right. But we can change this part too. :)
> Personally I think we should make life easier to any developer.
> If we can implement an audio-wakelock for MediaElement, we should.
> I don't see any reason why we should not have that.
> 

Hi Andrea,

Yes, you can implement an audio-wakelock for MediaElement. I agree this.
But if you ask music app to remove wakelock, then I doubt a music app without acquiring wakelock by itself can work well on other Web Tech based OS.

Because we can't guarantee other Web Tech based OS will help app to keep system alive when it is on playing. If an OS doesn't do this then that device will go into sleep when our music app is on playing state.
> Because we can't guarantee other Web Tech based OS will help app to keep system alive when it is on 
> playing. If an OS doesn't do this then that device will go into sleep when our music app is on 
> playing state.

This is true.  We also can't guarantee that another web device will do anything when you call requestWakeLock("cpu"), or even that another web device will have requestWakeLock.

We don't design Firefox for the lowest-common-denominator.  We design Firefox do to what's right.  If some other implementation requires a music app to acquire the CPU wake lock, well, that's because their implementation isn't as good as ours.

If you've never read "The Rise of 'Worse Is Better'" [1], it may be worth a look.  What's important to note is that Firefox (and other web browsers) follow the MIT philosophy, /not/ the Unix philosophy.  That is, we strive to Do The Right Thing, even when it's complicated.  The paper argues that the Unix philosophy has some advantages, but that ship has already sailed for us on the Web.

I don't think we need to involve Jonas here; I'm happy to make this call.

[1] http://www.jwz.org/doc/worse-is-better.html
Flags: needinfo?(jonas)
Attachment #744017 - Flags: feedback?(justin.lebar+bug) → feedback-
Assignee

Comment 50

6 years ago
checkin-needed for  gecko patch - Wakelock for MediaElement.
Assignee: dkuo → amarchesini
Keywords: checkin-needed
Assignee

Comment 52

6 years ago
Posted patch b2g18 patchSplinter Review
Attachment #743666 - Attachment is obsolete: true
Attachment #743699 - Attachment is obsolete: true
Attachment #744649 - Flags: review+
Assignee

Comment 53

6 years ago
Posted patch m-c patch (obsolete) — Splinter Review
Attachment #744655 - Flags: review+
Reporter

Comment 55

6 years ago
> This is true.  We also can't guarantee that another web device will do 
> anything when you call requestWakeLock("cpu")

Mozilla FxOS provided an api for acquiring wakelock from web app. And as I knew that Mozilla's vision is to put all Web API as standard. Your opinion breaks one of Mozilla's vision.

And as I heard that FxOS's advantage is to write one Web app and use on anywhere. And Web API of requestWakeLock is implemented by Mozilla as well and Mozilla' vision is to put it as web standard.

Now you said if others doesn't follow what we did in Gecko here, then others are a bad design. So even our web app is not compatible on others that would be fine. 
In first, implementation on Gecko is not a standard. How do we ask others to follow.
Second, there is no design can be called the best.
Third, the opinion on comment 49 breaks FxOS's goal - implement one and run on anywhere.
4th, it is better to be judged by "B2G Tech Leader" not just a senior developer.

Even though, I already leave my all comments here.
Assignee

Comment 56

6 years ago
Posted patch m-c patchSplinter Review
Attachment #744655 - Attachment is obsolete: true
Comment on attachment 742417 [details] [diff] [review]
gaia patch - cpuLock removed

If we don't need the cpuLock in Music app to keep the audio alive, then this patch looks just fine as it is.
Attachment #742417 - Flags: review?(dkuo) → review+
I feel comfortable having made this call, but it sounds like you'd like Jonas to weigh in.  Please feel free to get in touch with him and ask for an opinion.  We can always back this out.

I'm sorry we weren't able to come to an agreement here.  Like you said, this is tef+, and we need to move quickly.
https://hg.mozilla.org/mozilla-central/rev/557f9ec08794
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I just recalled that video app has a similar logic to keep the screen on, by requesting the screen wakeLock in play() and unlock screen in pause(), I actually followed how video app did when I added the cpu wakeLock to music app.

And removing the cpu wakeLock from music app reminds me this, so I am thinking that, if we need screen wakeLock when using a video element to keep screen alive, but don't need cpu wakeLock for audio element to play infinite, will it confuse the developers?
Assignee

Comment 64

6 years ago
I agree, we should manage the 'screen' lock in the VideoElement. Bug 868325
Assignee

Comment 65

6 years ago
Posted patch patchSplinter Review
Read bug 868325 comment 9
Attachment #745808 - Flags: review?(justin.lebar+bug)
Comment on attachment 745808 [details] [diff] [review]
patch

Can we please put this in a separate bug?  If the only motivation for putting it in here is to get tef+ automatically, I can mark the new bug as tef+.  Also, I'd like to see a test.  :)
Attachment #745808 - Flags: review?(justin.lebar+bug)
Assignee

Updated

6 years ago
Blocks: 868943

Comment 67

6 years ago
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Assignee

Updated

6 years ago
Attachment #744017 - Flags: feedback?(amarchesini)
Assignee

Comment 68

6 years ago
https://github.com/mozilla-b2g/gaia/pull/9532 this pull request has not been applied yet.
Any reason? Can someone proceed with it?
Flags: needinfo?(mchen)
Reporter

Comment 69

6 years ago
Hi Andrea,

This patch is reviewed by dkuo, so I think he should help to merge your patch into gaia master. And I have no permission to do this.

Set needinfo to Dominic and will notify him tomorrow face to face.
Flags: needinfo?(mchen) → needinfo?(dkuo)
baku, actually I missed the new pull request you sent on github for this bug, I will landed this for you, feel free to ping me directly when you need to land some patch like this.

And I suggest you next time when you want to send a patch for gaia, you can attach the patch link on github then the reviewers can review it directly on github, and will merge for you if that patch got a r+. Or you can attach your patch file and comment that you need help on landing it, I believe the reviewers can both open a pull request and merge it for you.
Flags: needinfo?(dkuo)
https://github.com/mozilla-b2g/gaia/pull/9532

Landed on master: 00e8ded6c27b79570020bc1add0cf6c0c6413450
I realized this bug contains gecko and gaia patches, but the Tracking Flags are base on gecko states, the patches for gaia will not be uplifted and will be untrackable if we don't open a separate bug for gaia.
I have uplifted the gaia patch myself so a separate bug is not needed.

Uplifted commit 00e8ded6c27b79570020bc1add0cf6c0c6413450 as:
v1-train: 6bc5ebd5d69f0cbd946d7811a1bee0f6128e2918
v1.0.1: 6d2855abad3259da96c03927c5b425f65f9e635a

Updated

6 years ago
Attachment #744017 - Flags: feedback?(dkuo)

Updated

6 years ago
Target Milestone: --- → 1.1 QE2
Whiteboard: MiniWW

Comment 74

6 years ago
Currently, we do not have the proper tools to accurately measure the battery drainage during device testing of this bug, to verify the fix. However, I attempted to test this issue manually, following the steps provided in comment 0 
 
While having the music app in the background, as well as the video app after it ends and putting the device into suspend mode for 30 minutes, the battery drainage changed from 85% to 84% on Leo device, and from 97% to 96% on the Inari.

Leo Mozilla V1.1 build: 20130603070207                                    
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4785b1353fd7  
Gaia: 4de4354e3a99f151a834743c7b03a041ac8db12f                  
Version 18.0
                                  
Inari Mozilla V1.0.1 build: 20130604070207                       Gecko:http://hg.mozilla.org/releases/mozillab2g18_v1_0_1/rev/42555e1e72fa                                                               Gaia: bf10abc41a01516995a99f3c6727a612bbdfe755                    
Version 18.0
Reporter

Comment 75

6 years ago
I tested this case by power monitor and The normal suspend current is about 2mA.
I verified that the suspend current is about 2mA too after performing reproduce steps.

Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2a92e4932728
Gaia: e1c59baed29e4665d1da9392f86239272073f07a
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.