Closed Bug 796333 Opened 12 years ago Closed 12 years ago

Audio should be stopped if the device goes to suspend

Categories

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

Product:
Firefox OS Graveyard
Component:
General
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
VERIFIED WORKSFORME
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +

People

(Reporter: ghtobz, Assigned: kinetik)

References

Details

(Whiteboard: [label:needsGeckoSupport][label:system][LOE:M])

Attachments

(1 file)

simple approach v0
3.01 KB, patch
mchen
: feedback+
Details | Diff | Splinter Review 
[GitHub issue by tonychung on 2012-03-22T06:39:56Z, https://github.com/mozilla-b2g/gaia/issues/907]
Audio from Apps will continue to play, even though the phone screen has timed out and gone black.  

Repro:
1) install m2.5 candidate build 0, nexus S
2) launch Cut the Rope app
3) When App loads, verify that there is music playing.
4) Wait about a minute, and the phone screen will time out and fade to black
5) Verify audio continues to play, despite screen went to sleep

Expected:
- no Audio if screen goes to sleep

Actual:
- Audio continues to play
[GitHub comment by cgjones on 2012-03-22T19:48:18Z]
@davidflanagan please make sure that the resurrected hackAutoKill is set up to nuke Cut The Rope.
[GitHub comment by davidflanagan on 2012-03-23T18:24:40Z]
@cgjones: hackKillMe is already set for Cut the Rope, which means that users can't pause and resume games, sadly.

Note, however that this bug is about the screen going to sleep, not killing the app.
I have no idea how to handle that.  I don't think it is a Gaia issue, so I'm going to unassign myself.
[GitHub comment by cgjones on 2012-03-24T09:20:28Z]
Yeah, you're right, we need Gecko support for this.
[GitHub comment by cgjones on 2012-03-26T21:40:39Z]
@davidflanagan can we add a new manifest flag to request a wake-lock automatically for games?  Then the screen never goes off while the music is playing :).
[GitHub comment by davidflanagan on 2012-07-12T21:33:51Z]
automatic wake lock seems like a dangerous battery-draining manifest option...

Given that I didn't even see the suggestion untill 4 months after it was made, I'm unassigning myself from this bug so someone else can work on it.
[GitHub comment by vingtetun on 2012-08-14T00:26:30Z]
@davidflanagan @cgjones When the screen turned off we should update the visibility off the windows and the music application should stop playing music. Does that works?
[GitHub comment by cpearce on 2012-08-14T00:32:12Z]
Any changes made here obviously need to *not* stop audio from a music player playing when the screen is off...
[GitHub comment by vingtetun on 2012-08-14T01:00:28Z]
@cpearce sigh.. I should have stayed in vacation.
[GitHub comment by vingtetun on 2012-08-14T01:01:41Z]
Well actually a music player will likely use a background service to read music so it can still apply. But this won't work for application like Cut The Rope.
[GitHub comment by vingtetun on 2012-08-16T20:49:34Z]
@autonome @cleemoz I have heard that android also have this issue, is it a blocker?
[GitHub comment by cleemoz on 2012-08-30T04:52:20Z]
If we can fix the root problem which is offer a background music API that allows web apps to determine if their app should continue to play even after exit or screen turned of, would that help?
[GitHub comment by autonome on 2012-09-14T22:58:02Z]
@vingtetun I've never had this problem on Android.
[GitHub comment by vingtetun on 2012-09-17T10:48:41Z]
@autonome Which version? Also background service will be removed so this is the only way to have a music application running while you browse the web now.
This issue should depend on bug 795237 because we want the music app to continue playing music but we would like Angry Birds to stop the music at the moment the screen times out.
Depends on: 795237
In my opinion, if the game didn't acquire any wakelock then it's music will be stopped automatically during screen off.
And let me define the "setScreenEnable = false" means the system on suspend state.
Since system is on suspend state, all processes on userspace should be freezed.
So I don't know why music is still on playing.
I think this bug is related to audio stream type but system suspend state.
Josh, we should figure out how we want to handle the various audio scenarios with apps.
Priority: -- → P1
Casey, can you comment here?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #14)
> This issue should depend on bug 795237 because we want the music app to
> continue playing music but we would like Angry Birds to stop the music at
> the moment the screen times out.

I believe you're right.
(In reply to Marco Chen [:mchen] from comment #15)
> In my opinion, if the game didn't acquire any wakelock then it's music will
> be stopped automatically during screen off.
> And let me define the "setScreenEnable = false" means the system on suspend
> state.
> Since system is on suspend state, all processes on userspace should be
> freezed.
> So I don't know why music is still on playing.
> I think this bug is related to audio stream type but system suspend state.

Music app does not get wake locks currently. Dominic didn't know his app *should* acquire the wake lock. Marco, do you know who should take this issue? Thanks.
Component: Gaia → General
Flags: needinfo?(mchen)
(In reply to Vivien Nicolas (:vingtetun) from comment #18)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #14)
> > This issue should depend on bug 795237 because we want the music app to
> > continue playing music but we would like Angry Birds to stop the music at
> > the moment the screen times out.
> 
> I believe you're right.

Marco was right, not me.
No longer depends on: 795237
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #19)
> Music app does not get wake locks currently. Dominic didn't know his app
> *should* acquire the wake lock. Marco, do you know who should take this
> issue? Thanks.

The music application can try to call function from Gaia as below for wakelock.
  cpuLock = navigator.requestWakeLock('cpu');

And when music app stops music or exist, don't forget to release wakelock
  cpuLock.unlock();
Flags: needinfo?(mchen)
(In reply to Marco Chen [:mchen] from comment #21)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #19)
> > Music app does not get wake locks currently. Dominic didn't know his app
> > *should* acquire the wake lock. Marco, do you know who should take this
> > issue? Thanks.
> 
> The music application can try to call function from Gaia as below for
> wakelock.
>   cpuLock = navigator.requestWakeLock('cpu');
> 
> And when music app stops music or exist, don't forget to release wakelock
>   cpuLock.unlock();

What you described is the fix needed in bug 804468. 

My question was, do you know who we should look for to correct the platform behavior, so that audio-playing apps without wakelock can be stopped when device suspends? Thanks.

Kanru, can you help us here?
Summary: Audio continues to play in background after screen times out → Audio should be stopped if the device goes to suspend
Apparently the audio driver grabs a kernel wake lock when playing audio. We could either (1) don't use wake lock in the kernel driver and makes it handle suspend correctly or (2) turn off audio before request suspend in gecko/gaia.

[676: b2g]request_suspend_state: sleep (0->3) at 14349443908904 (2012-10-24 06:39:56.585017129 UTC)
[676: b2g][early_suspend] wq status=1 
[9341: kworker/u:3]early_suspend: call handlers
[9341: kworker/u:3]early_suspend: handlers level=0,calling akm8962_early_suspend
[9341: kworker/u:3][FYA@MSENSOR]akm8962_early_suspend[FYA@MSENSOR]suspended with flag = 1
[9341: kworker/u:3]early_suspend: handlers level=0,calling lis302dl_early_suspend
[116: akmd8962]AKM8962 AKECS_GetCloseStatus returned (0)
[9341: kworker/u:3][hp@gsensor]:lis302dl suspend
[9341: kworker/u:3]early_suspend: handlers level=30,calling hibernate_state_preparing0
[9341: kworker/u:3]early_suspend: handlers level=50,calling ath6kl_early_suspend
[9341: kworker/u:3]early_suspend: handlers level=51,calling atmel_ts_early_suspend
[9341: kworker/u:3]atmel_ts_suspend: enter
[9341: kworker/u:3]early_suspend: handlers level=91,calling msmsdcc_early_suspend
[9341: kworker/u:3]early_suspend: handlers level=92,calling msmsdcc_early_suspend
[9341: kworker/u:3]early_suspend: handlers level=100,calling stop_drawing_early_suspend
[9341: kworker/u:3]early_suspend: handlers level=100,calling kgsl_early_suspend_driver
[9341: kworker/u:3]early_suspend: handlers level=148,calling msmfb_early_suspend
[9341: kworker/u:3][ZYF] lcdc_set_bl level=0, 1
[745: b2g]vibrator_enable,0 ms,vibrator:off now.
[9341: kworker/u:3]early_suspend: handlers level=149,calling mdp_early_suspend
[9341: kworker/u:3]early_suspend: handlers level=151,calling hibernate_early_suspend1
[9341: kworker/u:3]early_suspend: sync end
------------------8<-------------------------
[9341: kworker/u:3]active wake lock adsp
[9341: kworker/u:3]active wake lock audio_pcm
------------------8<-------------------------
[9341: kworker/u:3]wake lock ath6kl_suspend_wl, expired
[9341: kworker/u:3]wake lock chg_event, expired
[9341: kworker/u:3]PM: Syncing filesystems...
[9341: kworker/u:3]sync done.
I found 1. use android chrome to playback html5 audio, the music would stop after press power key, But the android version firefox still continue play the music.
BTW, I try go grep aurora kernel source and found pcm_out driver use two wakelocks during music packback. I think partners may not change this due to it works well on many android projects.
Flags: needinfo?(kyee)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #22)
> My question was, do you know who we should look for to correct the platform
> behavior, so that audio-playing apps without wakelock can be stopped when
> device suspends? Thanks.

What do we need to do? I presume we need to listen for a notification of suspend (how do we do that?) and stop the audio stream in such a way that the driver releases its wakelock? How do we do that?
Either libcubeb or nsAudioStream might be the right place to do this.
There are two types of WakeLock. It is confusing.
 - [1] application's intent to keep app wake up
 - [2] and hardware's state to keep cpu/hw wake up.

comment #23 is about [2]. Android PowerManager's wake lock is also basically [2]
http://developer.android.com/reference/android/os/PowerManager.html

This bug is about [1], I think. Is it correct?
When an app shows an intent to keep wake up the app to some level,
gecko(platform) respect the intent. Otherwise, gecko could stop or release the app's resource.
> Otherwise, gecko could stop or release
> the app's resource.

correction:
Otherwise, gecko could stop or release the app's resource, when an application is sent to the background.

It seems that it is related some to bug 806029
Depends on: 805333
I briefly checked about when an audio tag stops playing on some mobile browsers for android.

- Chrome browser : stop audio playback when screen becomes off state or the browser go to background
- Opera browser : stop audio playback when phone call start.
                   continue to playing audio when the browser go to background
- Firefox browser : do not stop audio playback even during phone call
- android browser : do not stop audio playback even during phone call

NB:android browser is replaced by Chrome browser in recent android products(JB and recent ICS).
Like audio, video playback needs to be stopped. Further, video codecs needs to be freed when app go to background because memory and hw codecs are very scarce resource in mobile. It includes web pages in web browser app.
> https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU

The document thinks about to stop only app's audios, do not stop web contents audios. IMHO, web contents' audios also needs to be stopped. There are pros and cons. Though it could provide better user experience, I think. I briefly checked about it in android case in bug 796333 comment #29

If it is controlled by gaia's system app, OEM vendors/operators can choose, when to forcibly stop/pause audio. From my experince, operators in Japan want to control when audio payback to stop from user's availability point of view.

Anyway, when web browser go to background, video tags in web contents need to be stopped and video codecs need to be freed. In mobile memory and hw codecs are very scarce resource.
Sorry, comment #31 is for Bug 805333 ...
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
My plan is to listen for "application-background" notifications on media elements and stop the elements when this happens.  "stop" will be implemented like pause (as far as UI is concerned), but will also tear down resources (just the audio stream for now, but that can be extended to decoders as necessary).  Because resources are discard, resuming a stopped element will not be perfectly seamless.  Once bug 805333 is implemented, the stream type can be used to determine whether particular elements are important and shouldn't be stopped.

The "application-background" notification doesn't seem to be implemented on B2G, so I'll probably need some help working out how to wire it up or a pointer to the notification I should be listening for.

If this sounds wrong, or someone has a better suggestion, please let me know.
I think not all of case media elements should stop the music when it detected "application-background". Because some applications like music player will be defined that it can play the music in the background.

In bug 805333, it may be distinguished by name of wakelock or permission which owned by app.
But currently it seems no clue to identify which app can allow to play in the background.
What's the update here?  This bug has reached the end of our milestone but no activity since a week ago.  Can someone give an update here?
I'm working on it.
Can you please post a detailed status update?
Matthew, do you have cycle for this? If not, I can reassign it to Marco Chen or Randy Lin.
Dear all,

This bug is depend on bug 805333 and the policy will be
  1. The media element with AUDIO_CHANNEL_NORMAL will be muted/paused while it's window is invisible.
  2. The suspend sequence will let Top App's window be invisible too.
     (Ex: Game)
  
  3. The channels other then AUDIO_CHANNEL_NORMAL will be kept to play during invisible.
     (Ex: Music Player)
Also note that

  1. bug 805333 tried to use visibility for detecting Apps fall into background.
     And we found that the suspend procedures will cause all windows fall into background too.

  2. The wakelock required by audio driver will be released by Audio Flinger.
     The release timing is Audio Flinger found there is no active (playing) streams during 3 seconds timeout. So even 805333 is just paused not stopped stream, the device still can go to suspend.
(In reply to Andreas Gal :gal from comment #38)
> Can you please post a detailed status update?

Yeah, sorry.  This was a little stop and go due to miscommunications whether this was covered entirely by the patches in bug 805333 or not.  Plus being laid up with the flu for a couple of days.

I have a fairly straightforward patch that adds Stop() to the media playback engine, which provides roughly the same functionality as Pause() but doesn't guarantee that a subsequent Play() is seamless.  Right now (and on Gonk only) Stop() will discard the OS audio stream (and can easily be extended to drop hardware decoder resources, decoder queues, network streams, etc.).

This was conducted on the assumption that calling Pause() on the OS audio stream wasn't sufficient to drop the audio driver's wakelock and allow the device to suspend.  Having investigated this further with Marco today, we believe that calling Pause() on the OS audio stream is sufficient to allow the driver to drop the wakelock and the device to suspend, and therefore bug 805333 will directly address this bug.

It's possible that my patch is still useful longer term,  All that's left to do is wire it in to the audio channel type prioritization in bug 805333, if we want to go ahead with it.
Forgot to say: so my plan is to finish review of the patches in bug 805333 so they can land ASAP.
Actually, 805333 only mutes elements, so additional changes (in this bug) will be needed to pause elements based on their audio channel type.  Sorry for the confusion.
(In reply to Matthew Gregan [:kinetik] from comment #44)
> Actually, 805333 only mutes elements, so additional changes (in this bug)
> will be needed to pause elements based on their audio channel type.  Sorry
> for the confusion.

As I knew that the conclusion of meeting for audio competing policy (including Andrea, Robert and Jonas) is to implement "pause" not "mute".
And please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=805333#c38, Andrea noted that the first version is on mute but should be done as pause in later works.
Target Milestone: B2G C1 (to 19nov) → B2G C2 (20nov-10dec)
Very simplistic approach.  Use the existing pause and network suspend logic, extended to receive notifications for document visibility changes.

The mAudioChannelType logic will need to be changed to call into the service being added in bug 805333.  I think we also need an audio channel type for "background content", which is lower priority than most things but is not paused when the document is not visible.
Attachment #686811 - Flags: feedback?(mchen)
Flags: needinfo?(kyee)
Comment on attachment 686811 [details] [diff] [review]
simple approach v0

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

This is really a brief and great version to manipulate whether a media element can be played in backgound based on audio channel type.
Ant it keeps the nature behavior on desktop version. Also it didn't related to audio channel service.

By the way, 805333 had designed the way for this in audio channel service too. Please look into line 87 in the link as below.
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=805333&attachment=686560

I am just wondering that if only media elment will use normal channel then from my point of view we can choose this.
Attachment #686811 - Flags: feedback?(mchen) → feedback+
Dear Matthew,

May I know that the logic here for pausing the media element can be aligned to the case of asking pause by audio channel service?
I was discussing this bug with Andrea and he thinks it should be fixed now that the audio manager has landed.  Can we verify that?
Flags: needinfo?(kinetik)
The audio for case of this bug will be muted by bug 805333 not stopped.
So on unagi & otoro, device still can't go into suspend because audio driver acquired a wakelock while any active streams.
As Marco said, we do need to pause rather than mute to solve this case.

I'm also not seeing visibility events indicating the window is hidden when locking the screen (tested in the emulator), I'm trying to find the appropriate event to listen for or place to wire in a new notification for this now.
Flags: needinfo?(kinetik)
What visibility events are you listening for?  We definitely send apps to the background when the screen goes off.
From the parent document, pageshow/pagehide and as of bug 805333 changes to the document's visibility state.

For the record, the way I'm testing is by playing media in the browser app, then hitting the "power" button in the emulator to lock the screen.   The screen is locked and disabled as expected, but nsHTMLMediaElement::NotifyOwnerDocumentActivityChanged is never called on the playing element.
Which parent document are you looking at?  You should definitely see visibility-change events in the browser app and its tabs when the screen goes off.

I don't know if we change the visibility of the top-most document, the "system" iframe, or the top-top-most XUL document, when locking the screen though.  But the things within them should change visibility.
Comment on attachment 686811 [details] [diff] [review]
simple approach v0

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3377,5 @@
> +    bool hidden = false;
> +    domDoc->GetHidden(&hidden);
> +  }
> +  pauseForInactiveDocument = pauseForInactiveDocument ||
> +                             (mAudioChannelType <= AUDIO_CHANNEL_NORMAL && hidden);

Setting pauseForInactiveDocument to true will cause events for the media element to be queued up until the state changes back to false. Do we really want to do that for documents that aren't in the bfcache?
I think to solve this and bug 815569, it might be better to define a new flag, say mPausedForAudioChannel, which doesn't cause event queuing but pauses in the same way mPausedForInactiveDocument does.
Depends on: 819836
The patch I'm about to attach to bug 815569 will also address this bug, once bug 819836 is also fixed.
Depends on: 815569
Given comment 57, marking this as fixed. Naoki - can you verify?
Keywords: verifyme
QA Contact: nhirata.bugzilla
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
Matthew, is this fixed now that bug 815569 has landed?
Flags: needinfo?(kinetik)
Playback is pausing as expected when the screen is locked, but I'm still seeing the adsp and audio_pcm wake locks active during suspend.  If I pause the media manually before suspending, the wake locks are no longer active.

I wonder if there's not enough time to settle (e.g. for AudioFlinger to close the audio hardware and cause the wake locks to be dropped) between pausing and device suspend.
Flags: needinfo?(kinetik)
(In reply to Matthew Gregan [:kinetik] from comment #60)
> I wonder if there's not enough time to settle (e.g. for AudioFlinger to
> close the audio hardware and cause the wake locks to be dropped) between
> pausing and device suspend.

Looks like AudioFlinger suspends after 3 seconds (kStandbyTimeInNsecs in AudioFlinger.h).
(In reply to Matthew Gregan [:kinetik] from comment #60)
> Playback is pausing as expected when the screen is locked, but I'm still
> seeing the adsp and audio_pcm wake locks active during suspend.  If I pause
> the media manually before suspending, the wake locks are no longer active.

So everything seems OK to the user, but we have a power usage problem?

(In reply to Matthew Gregan [:kinetik] from comment #61)
> Looks like AudioFlinger suspends after 3 seconds (kStandbyTimeInNsecs in
> AudioFlinger.h).

So we don't reach the three second timeout because we've suspended first?
After adding & checking log into AudioFlinger and sydney library, I found that
  1. AudioFlinger will close dev node of audio output after no active tracks there and 3 seconds timeout.
  2. AudioFlinger will remove track from activeTracks when it is paused (and others).
  3. In scenario of this bug, AudioFlinger didn't receive pause command from sydney library.

So device can't go into suspend but just early suspend.
Comment 63 is a wrong comment because I just changed music app as a normal channel for verifying this bug. And I missed that music app will held a wakelock so device can't go into suspend.

I tried to find any root cause on b2g18 (b27b0eadf48c) but found device can go into suspend already. Now I used the html page of "html5 audio test" for verification. And music is stopped and device go into suspend too.
Hi Naoki,

Could you help to verify this bug?

I listed my test steps as below.

1. Open http://www.terrillthompson.com/tests/html5-audio.html from browser.
2. Play the audio element.
3. Press the power key to go into suspend mode.

Result: Audio is paused.

About verification of power.

1. Checking kernel messages.
  a. after steps as above please wait about 5 seconds (audio flinger needs 3 seconds to release dev node of audio driver).
  b. Plugging usb and call "adb shell cat /proc/kmsg". You will get kernel messages.
  c. Verify that there are wakelocks for preventing device from going to suspend.
  d. After 3 seconds, you will see audio driver released wakelock then device goes into suspend.

2. Checking from power meter.

Thanks.
Flags: needinfo?(nhirata.bugzilla)
mchen: this isn't pushed in yet to 18 is it?
Flags: needinfo?(nhirata.bugzilla)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Verified using steps in comment 65.
Status: RESOLVED → VERIFIED
Removing Verifyme tag as per comment 67.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: