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)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
VERIFIED FIXED
Milestone:
B2G C4 (2jan on)
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
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)

Checkin patch
(deleted), 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.
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?
A related bug: bug 842506
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?
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.
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.
Dialer app too please!
Yup, that would work automatically too
Taken this per #6, #7
Assignee: nobody → alive
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.
(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
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?
(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 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.
(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.
(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?
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.
(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.
Attachment #728881 - Flags: review?(amarchesini) → review+
No, it's fine. You answered to my questions. Thank you.
Attached patch Checkin patch (deleted) — — Splinter Review 
Checkin patch with commit message.
Keywords: checkin-needed
Whiteboard: checkin-needed
Add checkin-needed, but I am not sure if I need to change the bug status.
Michael, do you think we should block on this one so it can be uplifted later on?
Flags: needinfo?(mvines)
IMO, yes.  Our power test guys are very worried about this.
Flags: needinfo?(mvines)
Attachment #728881 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8fa6b42354
Keywords: checkin-needed
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d8fa6b42354
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
blocking-b2g: tef? → tef+
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-
Depends on: 856946
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Michael Vines if you can Verify this bug out, it would be much appreciated.
Flags: needinfo?(mvines)
Keywords: verifyme
Status: RESOLVED → VERIFIED
Flags: needinfo?(mvines)
Whiteboard: TD-9698
Whiteboard: TD-9698 → TD-9534
Whiteboard: TD-9534 → [TD-9534]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: