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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

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.

Comment 1

[Marco Chen [:mchen]]

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.

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

[Marco Chen [:mchen]]

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

Comment 4

[Andrea Marchesini [:baku]]

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

Comment 5

[Marco Chen [:mchen]]

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.

Comment 6

[Marco Chen [:mchen]]

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)

Comment 7

[Marco Chen [:mchen]]

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.

Comment 8

[Dietrich Ayala (:dietrich)]

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.

Comment 9

[Andrea Marchesini [:baku]]

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.

Comment 10

[Marco Chen [:mchen]]

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.

Comment 11

[Andrea Marchesini [:baku]]

Is the bug 739542 landed on b2g18? We should check how the battery life changes with that patch applied.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Given the user impact of draining battery with very little indication of why (music app in background) we believe this should block.

Comment 13

[Andrea Marchesini [:baku]]

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?

Comment 14

[Andrea Marchesini [:baku]]

Attachment: gaia patch - cpuLock removed

This patch removes cpuLock. The lock is controlled by the MediaElement, strongly connected to the AudioChannelService.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: gecko patch - Wakelock for MediaElement

This patch is based on 739542.
It connects the current patch with the value of AudioChannelAgent->CanPlay().

Comment 16

[Andrea Marchesini [:baku]]

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.

Comment 17

[Andrea Marchesini [:baku]]

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

wait before reviewing this patch. I want to finish the gecko part first.

Comment 18

[Marco Chen [:mchen]]

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.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

baku, looks like you are the assignee there?

Comment 20

[Andrea Marchesini [:baku]]

I think so. But maybe jlebar has something to say about it.

Comment 21

[Justin Lebar (not reading bugmail)]

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.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: gecko patch - Wakelock for MediaElement

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.

Comment 23

[Justin Lebar (not reading bugmail)]

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

Comment 24

[Andrea Marchesini [:baku]]

Attachment: gecko patch - Wakelock for MediaElement

Comment 25

[Justin Lebar (not reading bugmail)]

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?

Comment 26

[Marco Chen [:mchen]]

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

Comment 27

[Justin Lebar (not reading bugmail)]

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.

Comment 28

[Marco Chen [:mchen]]

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.

Comment 29

[Andrea Marchesini [:baku]]

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

Comment 30

[Andrea Marchesini [:baku]]

Attachment: patch

The timer will be a follow up if you don't mind.

Comment 31

[Justin Lebar (not reading bugmail)]

Comment on attachment 743639 [details] [diff] [review]
patch

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

r=me, but please s/NULL/nullptr

Comment 32

[Andrea Marchesini [:baku]]

Attachment: gecko patch - Wakelock for MediaElement

Timer in a separated patch. In the meantime if someone can review the gaia part, it will be great.

Comment 33

[Marco Chen [:mchen]]

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.

Comment 34

[Justin Lebar (not reading bugmail)]

Marco, if you want attach a Gaia-only fix here, I'm sure we'd take a close look at that.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: gecko patch - Timer

Comment 36

[Andrea Marchesini [:baku]]

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 37

[Justin Lebar (not reading bugmail)]

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?

Comment 38

[Justin Lebar (not reading bugmail)]

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

Comment 39

[Andrea Marchesini [:baku]]

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.

Comment 40

[Justin Lebar (not reading bugmail)]

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

Comment 41

[Marco Chen [:mchen]]

Attachment: Patch_v1 on Gaia_Fixed_Only

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.

Comment 42

[Andrea Marchesini [:baku]]

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.

Comment 43

[Justin Lebar (not reading bugmail)]

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

Comment 44

[Marco Chen [:mchen]]

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

Comment 45

[Justin Lebar (not reading bugmail)]

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?

Comment 46

[Marco Chen [:mchen]]

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

Comment 47

[Andrea Marchesini [:baku]]

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

Comment 48

[Marco Chen [:mchen]]

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

Comment 49

[Justin Lebar (not reading bugmail)]

> 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

Comment 50

[Andrea Marchesini [:baku]]

checkin-needed for  gecko patch - Wakelock for MediaElement.

Comment 51

[Andrea Marchesini [:baku]]

 https://hg.mozilla.org/releases/mozilla-b2g18/rev/06746f5ec97e

Comment 52

[Andrea Marchesini [:baku]]

Attachment: b2g18 patch

Comment 53

[Andrea Marchesini [:baku]]

Attachment: m-c patch

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/9fd2515981bf

Comment 55

[Marco Chen [:mchen]]

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

Comment 56

[Andrea Marchesini [:baku]]

Attachment: m-c patch

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Backed out for build bustage.
https://hg.mozilla.org/projects/birch/rev/76acf73d9f74

Re-landed with fixes for said bustage.
https://hg.mozilla.org/projects/birch/rev/557f9ec08794

Comment 58

[Dominic Kuo [:dkuo]]

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.

Comment 59

[Justin Lebar (not reading bugmail)]

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.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/557f9ec08794

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/cbfd20cafcfb

Comment 62

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/9532 gaia pull request

Comment 63

[Dominic Kuo [:dkuo]]

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?

Comment 64

[Andrea Marchesini [:baku]]

I agree, we should manage the 'screen' lock in the VideoElement. Bug 868325

Comment 65

[Andrea Marchesini [:baku]]

Attachment: patch

Read bug 868325 comment 9

Comment 66

[Justin Lebar (not reading bugmail)]

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

Comment 67

[sarsenyev]

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

Comment 68

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/9532 this pull request has not been applied yet.
Any reason? Can someone proceed with it?

Comment 69

[Marco Chen [:mchen]]

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.

Comment 70

[Dominic Kuo [: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.

Comment 71

[Dominic Kuo [:dkuo]]

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

Landed on master: 00e8ded6c27b79570020bc1add0cf6c0c6413450

Comment 72

[Dominic Kuo [:dkuo]]

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.

Comment 73

[Dominic Kuo [:dkuo]]

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

Comment 74

[mdaniloff]

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

Comment 75

[Marco Chen [:mchen]]

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