Closed
Bug 853454
Opened 12 years ago
Closed 12 years ago
Voice call, music playback and FM audio power numbers regressed
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:tef+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
People
(Reporter: m1, Assigned: alive)
References
Details
(Keywords: verifyme, Whiteboard: [TD-9534])
Attachments
(1 file, 1 obsolete file)
3.12 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #828283 +++
Bug 828283 has caused a severe regression in power numbers for voice call and music playback. The reasons for fixing bug 828283 still seem valid in general, so lets find a way to keep the spirit of that fix and resolve the power issues it causes for these two important use cases.
Reporter | ||
Comment 1•12 years ago
|
||
Inlining from https://bugzilla.mozilla.org/show_bug.cgi?id=836214#c58,
==
Fix for bug 828283 increases music app power consumption when display is off.
We found its because of app's visibility is not set to false when screen is turned off
==
blocking-b2g: --- → tef?
Reporter | ||
Comment 2•12 years ago
|
||
A related bug: bug 842506
Comment 3•12 years ago
|
||
Hi,
bug 853101 tries to post a proposal for a series of Web APIs.
One goal there is to remove the hack from bug 828283.
Hmm.. this should not have happened.
The code in bug 828283 explicitly tried to only do the "keep visible" hack when an app was using the "normal" audio channel.
However the music app is using the "content" audio channel which should have meant that we should have set it to non-visible as expected when the screen was turned off.
If that's not happening then that sounds like a bug and not expected behavior. We shouldn't need something as complex as the API in bug 853101 to fix this.
Andrea: is the above correct? Or did that plan not work out for some reason?
Assignee | ||
Comment 5•12 years ago
|
||
I re-read the code in bug 828283 and it seems that "every playing audio" will keep visible state no matter what channel it is.
Except that active-audio-channel-changed event is not fired as I think.
Even we restrict the channel to "normal" only, it just works for music app. Other normal-channel app would be kept visible then.
But if our partner could accept this, I am fine to fix by this way.
Blocks: 828283
Yes, we don't have infrastructure to fix this for all apps, that's what bug 853101 is about.
I think we should fix this for the music/video apps only for now.
Reporter | ||
Comment 7•12 years ago
|
||
Dialer app too please!
Yup, that would work automatically too
Assignee | ||
Comment 10•12 years ago
|
||
Hmmm..I just change the cpu hack in bug 828283, only listen to normal channel and everything works fine.
But it's still confusing me that someone said music app doesn't know visibilitychange.
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Alive Kuo [:alive] (PTO during 3/27~4/7) from comment #10)
> Hmmm..I just change the cpu hack in bug 828283, only listen to normal
> channel and everything works fine.
> But it's still confusing me that someone said music app doesn't know
> visibilitychange.
https://bugzilla.mozilla.org/show_bug.cgi?id=828283#c97
Well...I answered this question by myself.
So I think I could send a patch about this fix in gecko, but I don't know if this fixes 836214.
https://bug836214.bugzilla.mozilla.org/attachment.cgi?id=715860&t=vO65s0ogMj Maybe we still need this one after we fix the visibility issue on non-normal channel app?
But, music player now doesn't
Assignee | ||
Comment 12•12 years ago
|
||
This patch does:
1. Fix the content channel name to 'content' instead of 'normal'.
Gaia has already known content is normal: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.js#L128
2. Only keep visibility of content when playing channel is 'normal'.
Andrea, it's my time to set review to you. ;)
Attachment #728881 -
Flags: review?(amarchesini)
Comment on attachment 728881 [details] [diff] [review]
Platform patch about visible audio channel change.
This looks like the right approach to me, but definitely needs Andrea's review.
Don't you need changes to the gaia system app too though?
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #13)
> Comment on attachment 728881 [details] [diff] [review]
> Platform patch about visible audio channel change.
>
> This looks like the right approach to me, but definitely needs Andrea's
> review.
>
> Don't you need changes to the gaia system app too though?
No IMO as this patch's main purpose is to fix the visibility issue that music couldn't get the change event. I leave the CPU sleep change in patch of bug 828283 as is because it's OOS of this bug and what bug 836214 would love to do(the patch of :dkuo) seems irrelative to CPU sleep.
I've tested (without change to system app):
1. Normal channel app still sounds when screen is off.(Which is bug 828283's target)
2. Music gets visibilitychange event when screen is off.
Comment 15•12 years ago
|
||
Comment on attachment 728881 [details] [diff] [review]
Platform patch about visible audio channel change.
Review of attachment 728881 [details] [diff] [review]:
-----------------------------------------------------------------
2 comments:
1. This visible-audio-channel will be removed soon (I hope).
2. Changing 'content' to 'normal' will change how the system app keeps track of the current audio channel in use. js/sound_manager.js line 35. Probably this code has to be changed as well.
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #15)
> Comment on attachment 728881 [details] [diff] [review]
> Platform patch about visible audio channel change.
>
> Review of attachment 728881 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> 2 comments:
>
> 1. This visible-audio-channel will be removed soon (I hope).
Sounds good.
>
> 2. Changing 'content' to 'normal' will change how the system app keeps track
> of the current audio channel in use. js/sound_manager.js line 35. Probably
> this code has to be changed as well.
No, the current gecko behavior is unexpected to me. I am surprised to see music tells me it's normal channel, although this doesn't harm to current sound_manager, but when I'd like to take the channel name to do other things(like this bug), content channel should just be 'content'.
Alive: please be very careful about what we're changing here. We have very little time for dealing with regressions so please audit any code that might be impacted by the change from "normal" to "content" here.
I agree that returning "normal" is better, but that doesn't mean it won't break existing code logic.
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #17)
> Alive: please be very careful about what we're changing here. We have very
> little time for dealing with regressions so please audit any code that might
> be impacted by the change from "normal" to "content" here.
>
> I agree that returning "normal" is better, but that doesn't mean it won't
> break existing code logic.
I'm convinced current gaia expects 'content' instead of 'normal'.
As long as gecko doesn't expects 'normal' is returned, I don't know why we don't change this.
Anything what I could do now to move forward?
Reporter | ||
Updated•12 years ago
|
Summary: Voice call and music playback power numbers regressed → Voice call, music playback and FM audio power numbers regressed
As long as both you and Andrea is happy with the change, given comment 17, then we're good to go.
Andrea: if there is anything in particular you think we should look at, please don't hesitate to speak up.
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #19)
> As long as both you and Andrea is happy with the change, given comment 17,
> then we're good to go.
>
> Andrea: if there is anything in particular you think we should look at,
> please don't hesitate to speak up.
Yap, Andrea, please challenge me if you have concerns in both gecko/gaia side.
I could dig more in gecko or explain my thought in gaia. Thank you both.
Updated•12 years ago
|
Attachment #728881 -
Flags: review?(amarchesini) → review+
Comment 21•12 years ago
|
||
No, it's fine. You answered to my questions. Thank you.
Assignee | ||
Comment 22•12 years ago
|
||
Checkin patch with commit message.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: checkin-needed
Assignee | ||
Comment 23•12 years ago
|
||
Add checkin-needed, but I am not sure if I need to change the bug status.
Comment 24•12 years ago
|
||
Michael, do you think we should block on this one so it can be uplifted later on?
Flags: needinfo?(mvines)
Reporter | ||
Comment 25•12 years ago
|
||
IMO, yes. Our power test guys are very worried about this.
Flags: needinfo?(mvines)
Updated•12 years ago
|
Attachment #728881 -
Attachment is obsolete: true
Comment 26•12 years ago
|
||
Keywords: checkin-needed
Whiteboard: checkin-needed
Comment 27•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Updated•12 years ago
|
blocking-b2g: tef? → tef+
Comment 28•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4628a5cc6307
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5774b4eba298
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → fixed
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Comment 29•12 years ago
|
||
There is a test case covering this issue.
https://moztrap.mozilla.org/manage/cases/?&pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-id=6855
Flags: in-moztrap-
Comment 30•12 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Comment 31•12 years ago
|
||
Michael Vines if you can Verify this bug out, it would be much appreciated.
Flags: needinfo?(mvines)
Keywords: verifyme
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Updated•12 years ago
|
Flags: needinfo?(mvines)
You need to log in
before you can comment on or make changes to this bug.
Description
•