Closed
Bug 1249579
Opened 8 years ago
Closed 8 years ago
Request audio focus on Fennec
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P2)
Firefox for Android Graveyard
Audio/Video
Tracking
(firefox48 fixed, relnote-firefox 48+, fennec48+)
RESOLVED
FIXED
Firefox 48
People
(Reporter: alwu, Assigned: alwu)
References
Details
User Story
When users use Fennec to play any audible media content, the audio should be managed by Android audio service. It would be done by requesting AudioFocus in Android app. When the audio competing happens, Android audio service would notify app's audio focus changed, so the app can pause/resume its media content. For example, users are using Fennec to play music, (1) Pause : incoming type is ringtone, so pause the media content (2) Resume : following (1), incoming call ended, so resume the media content
Attachments
(2 files, 1 obsolete file)
We don't request android audio focus when the Fennec starts playing any audible sound, that means the audio doesn't be managed by Android's audio service. In Fennec, I want like to introduce the similar mechanism like b2g did. Every audio could only be heard after it got the approval from the system audio focus. STR. 1. Make sure your android phone is on the non-silent mode 2. Playing any audio using Fennec 3. Get incoming call Expected. 4. The audio should be paused, and user only heard the ringtone Actual. 4. The audio is still playing, mixing with the ringtone
Assignee | ||
Comment 1•8 years ago
|
||
The preliminary idea is like that,
1. Audio start playing, but default volume=0.0
2. Notify java side and request audio focus
> MediaElemet -> AudioChannelService -> browser-content.js -> browser.xml -> browser.js -> Tabs.java -> *request audio focus*
2.1 If audio focus=AUDIOFOCUS_REQUEST_GRANTED, notify gecko to change audio volume to 1.0
2.2 If audio focus=AUDIOFOCUS_REQUEST_FAILED, notify gecko to stop audio playing
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35803/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35803/
Assignee | ||
Updated•8 years ago
|
Attachment #8721951 -
Attachment description: MozReview Request: Bug 1249579 - Request audio focus on Fennec. → MozReview Request: Bug 1249579 - part1 : Request audio focus on Fennec.
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/1-2/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36003/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36003/
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc33dc04f842
Assignee | ||
Updated•8 years ago
|
User Story: (updated)
Assignee | ||
Updated•8 years ago
|
User Story: (updated)
Assignee | ||
Updated•8 years ago
|
Attachment #8721951 -
Attachment description: MozReview Request: Bug 1249579 - part1 : Request audio focus on Fennec. → MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec.
Attachment #8721951 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Attachment #8722386 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36003/diff/1-2/
Assignee | ||
Comment 8•8 years ago
|
||
Hi, Baku & Margaret,
Could you help me review these patches?
Fennec doesn't request audio focus when it starts playing audio, that would be mess if other app is producing sound at the same time. The main purpose of these patches is to let audio playback on Fennec can be managed by Android audio service.
Some important points for these patches,
> * Create an AudioFocusAgent to handle the audio focus in Android
> * Provide suspend attribute in nsIDOMWindowUtils
> * Suspend or resume whole media contents in specific tab depending on the audio focus result
Very appreciate!
Comment 9•8 years ago
|
||
This sounds like a good improvement, but it is a product change that should be tracked in our Fennec roadmap. NI to Barbara to confirm this is something we want to ship.
Flags: needinfo?(bbermes)
Comment 10•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. https://reviewboard.mozilla.org/r/35803/#review33479 Let's avoid adding logic for this in the tabs code if possible. ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1355 (Diff revision 3) > + AudioFocusAgent.getInstance().attachToContext(this); This would be more appropriate in `BrowserApp`, since it's tied to browser tabs UI. ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:412 (Diff revision 3) > + AudioFocusAgent.getInstance().abandonAudioFocusIfNeeded(); Theres are other ways a tab can be closed. It would be better to listen for the TabEvents.CLOSED event. ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:600 (Diff revision 3) > + } Why don't you do this pref check in JS to avoid sending this message in the first place? Also, if the pref changes between restarts, this value won't be updated. ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:25 (Diff revision 3) > + mAudibleTabs = 0; Nit: We use 4-space indentation for Java. ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:35 (Diff revision 3) > + mAppContext = context.getApplicationContext(); Why do you need an ApplicationContext here? ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:109 (Diff revision 3) > + private boolean isFirstAudibleTab() Code style nit: Put the open brace on the same line as the method delcaration. ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:111 (Diff revision 3) > + if (++mAudibleTabs == 1) { You could just return this value directly, instead of doing an if statement to return true/false values. Same applies to the method below.
Attachment #8721951 -
Flags: review?(margaret.leibovic)
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/36003/#review33483 ::: mobile/android/chrome/content/browser.js:3396 (Diff revision 2) > + this.isPausedByAudioFocus = false; You will need review from a mobile peer for changes to browser.js.
Updated•8 years ago
|
Flags: needinfo?(bbermes)
See Also: → https://mozilla.aha.io/features/FENN-450
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to :Margaret Leibovic from comment #10) > ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:600 > (Diff revision 3) > > + } > > Why don't you do this pref check in JS to avoid sending this message in the > first place? > > Also, if the pref changes between restarts, this value won't be updated. That's because I want let the "audio-focus" and "tab-audio-playing-icon" sharing with the same event. If I do the check in JS, that means I need to dispatch another event for "audio-focus" if the preference is off. It seems no need to add another event which is very similar with the exist one. > ::: mobile/android/chrome/content/browser.js:3396 > (Diff revision 2) > > + this.isPausedByAudioFocus = false; > > You will need review from a mobile peer for changes to browser.js. Thanks, I'll ask the mobile peer to review patch 2 :)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/3-4/
Attachment #8721951 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36003/diff/2-3/
Attachment #8722386 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/4-5/
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36003/diff/3-4/
https://reviewboard.mozilla.org/r/35803/#review34463 I think I would prefer to see a lower-level solution for audio focus that doesn't know anything about Tabs. You could just listen to the DOMAudioPlaybackStarted/Stopped events in AndroidBridge (or even better, some new place) and make JNI calls to something similar to AudioFocusAgent.
tracking-fennec: --- → 48+
Comment 19•8 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #18) > https://reviewboard.mozilla.org/r/35803/#review34463 > > I think I would prefer to see a lower-level solution for audio focus that > doesn't know anything about Tabs. You could just listen to the > DOMAudioPlaybackStarted/Stopped events in AndroidBridge (or even better, > some new place) and make JNI calls to something similar to AudioFocusAgent. I'm going to hold off on reviewing these patches until you explore this feedback from snorp. I agree with him that it would be much nicer to handle this on the platform level, as opposed to in the UI layer.
Assignee | ||
Comment 20•8 years ago
|
||
Hi, Snorp, From my understanding, do you means I should receive event in c++ code, and then use JNI to notify AudioFocusAgent (JAVA) there is something start playing or stop playing? Here I wrote an draft patch, could you help me feedback it? If you feel ok, I would update the new patch and ask for review :) BTW, because I'm not very familiar with Fennec, could you help me to explain what is the main benefit to use a lower-level solution? Very appreciate your help!
Attachment #8726641 -
Flags: feedback?(snorp)
Comment on attachment 8726641 [details] [diff] [review] Using JNI to call AudioFocusAgent Review of attachment 8726641 [details] [diff] [review]: ----------------------------------------------------------------- This one looks good to me. The reason I prefer this direction is because it is basically a platform integration point, and as such should not involve the frontend code. Tab.java and browser.js are frontend pieces for the most part. ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java @@ +27,2 @@ > > + @WrapForJNI(stubName = "notifyStartedPlayingWrapper") There is no reason to set the stub name to anything different here. The default of 'NotifyStoppedPlaying' is fine. @@ +36,2 @@ > > + @WrapForJNI(stubName = "notifyStoppedPlayingWrapper") Same
Attachment #8726641 -
Flags: feedback?(snorp) → feedback+
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/5-6/
Attachment #8721951 -
Flags: review?(snorp)
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36003/diff/4-5/
Attachment #8722386 -
Flags: review?(snorp)
Assignee | ||
Comment 24•8 years ago
|
||
Hi, Margaret & Snorp, Here are my revised patches, could you help me review them again? Thanks!
Assignee | ||
Updated•8 years ago
|
Comment 25•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. snorp is more qualified to review this than I am, since this doesn't touch our UI code.
Attachment #8721951 -
Flags: review?(margaret.leibovic)
Updated•8 years ago
|
Attachment #8722386 -
Flags: review?(margaret.leibovic)
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. https://reviewboard.mozilla.org/r/35803/#review35031 nice!
Attachment #8721951 -
Flags: review?(snorp) → review+
Attachment #8722386 -
Flags: review?(snorp) → review+
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. https://reviewboard.mozilla.org/r/36003/#review35033 looks good, but you'll still need a DOM peer to r+
Assignee | ||
Updated•8 years ago
|
Attachment #8726641 -
Attachment is obsolete: true
Comment 28•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. https://reviewboard.mozilla.org/r/36003/#review36885 ::: dom/audiochannel/AudioChannelService.cpp:387 (Diff revision 5) > *aVolume *= winData->mChannels[aAudioChannel].mVolume; > *aMuted = *aMuted || winData->mChannels[aAudioChannel].mMuted; > } > > - *aVolume *= window->GetAudioVolume(); > - *aMuted = *aMuted || window->GetAudioMuted(); > + *aVolume *= GetEffectiveVolumeFromWindow(window); > + *aMuted = *aMuted || window->GetMediaSuspended(); Here you are changing the behavior of this method. *aMuted should follow GetAudioMuted(). What's the difference between muted and suspended? ::: dom/interfaces/base/nsIDOMWindowUtils.idl:1752 (Diff revision 5) > > /** > + * Used to pause or resume all MediaElements in this window. Use-cases are > + * audio competing and remote media control. > + */ > + attribute boolean mediaSuspended; Can you use use audioMuted for this? ::: toolkit/content/browser-content.js:748 (Diff revision 5) > + // The AudioFocus:LossTransient means the media would be resumed after > + // the interruption ended, but AudioFocus:Loss doesn't. > + // TODO : distinguish these two types in ACS. > + case "Loss": > + case "LossTransient": > + utils.mediaSuspended = true; If you use audioMuted here instead mediaSuspended, you can revert all the changes in the rest of the patch.
Attachment #8722386 -
Flags: review?(amarchesini)
Assignee | ||
Comment 29•8 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #28) > > ::: dom/audiochannel/AudioChannelService.cpp:387 > (Diff revision 5) > > *aVolume *= winData->mChannels[aAudioChannel].mVolume; > > *aMuted = *aMuted || winData->mChannels[aAudioChannel].mMuted; > > } > > > > - *aVolume *= window->GetAudioVolume(); > > - *aMuted = *aMuted || window->GetAudioMuted(); > > + *aVolume *= GetEffectiveVolumeFromWindow(window); > > + *aMuted = *aMuted || window->GetMediaSuspended(); > > Here you are changing the behavior of this method. > *aMuted should follow GetAudioMuted(). > > What's the difference between muted and suspended? Please also see the bug1242874, the |aMuted| here should be renamed to |aSuspended|. Muted and suspend are totally different behaviors, muted is to control the volume only, but suspended is to stop the whole playback. > ::: dom/interfaces/base/nsIDOMWindowUtils.idl:1752 > (Diff revision 5) > > > > /** > > + * Used to pause or resume all MediaElements in this window. Use-cases are > > + * audio competing and remote media control. > > + */ > > + attribute boolean mediaSuspended; > > Can you use use audioMuted for this? Because it's not just related audio, but also Video. We would stop the whole media content. We shouldn't use mute for this name, because it's the suspended behavior (to stop the decode process) > ::: toolkit/content/browser-content.js:748 > (Diff revision 5) > > + // The AudioFocus:LossTransient means the media would be resumed after > > + // the interruption ended, but AudioFocus:Loss doesn't. > > + // TODO : distinguish these two types in ACS. > > + case "Loss": > > + case "LossTransient": > > + utils.mediaSuspended = true; > > If you use audioMuted here instead mediaSuspended, you can revert all the > changes in the rest of the patch. See above explanation.
Flags: needinfo?(amarchesini)
Comment 30•8 years ago
|
||
Ok! Then here another idea: 1. muted and volume are 2 different settings. 2. and we can consider 'suspend' as a different kind of 'mute'. So, what about if we do: enum MuteType { eNonMuted, eMuted, eMutedWithSuspend }; In that code: if (window->GetMediaSuspended()) { muteType = eMutedWithSuspended; } else { ... } Then in nsIAudioChannelAgentCallback: // write a comment here. const short NON_MUTED = 0; // write a comment here. const short MUTED = 1; // write a comment here. const short SUSPENDED = 2; void windowVolumeChanged(in float aVolume, in short aMutedType);
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. After discuss with baku on IRC yesterday, I would use the idea in comment30 to modify the patches in bug1242874, and ask review for this bug again.
Attachment #8722386 -
Flags: review?(amarchesini)
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Need to modify some codes.
Attachment #8722386 -
Flags: review?(amarchesini)
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/6-7/
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36003/diff/5-6/
Attachment #8722386 -
Flags: review?(amarchesini)
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Hi, Snorp, I modified some codes to fix the test case crash, could you help me review it again? Thanks!
Attachment #8721951 -
Flags: review+ → review?(snorp)
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. https://reviewboard.mozilla.org/r/35803/#review37619
Attachment #8721951 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 38•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a480116aeb9
Alastor, are you ready to get this pushed? Try run looks good!
Flags: needinfo?(alwu)
Assignee | ||
Comment 40•8 years ago
|
||
It's ready to push but just waiting for baku's review.
Flags: needinfo?(alwu) → needinfo?(amarchesini)
(In reply to Alastor Wu [:alwu] from comment #40) > It's ready to push but just waiting for baku's review. Ah, I missed that. Two weeks for a review is ridiculous. Maybe we can get smaug or bz to review it?
Assignee | ||
Comment 42•8 years ago
|
||
I ping baku in IRC yesterday and he would review this patch soon.
Comment 43•8 years ago
|
||
Any progress on getting this reviewed? It would be great to have this in Fx48, but the window is closing on that.
Comment 44•8 years ago
|
||
We have talked with baku a couple days ago, he would finish reviewing this week.
Comment 45•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. https://reviewboard.mozilla.org/r/36003/#review43599
Attachment #8722386 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8721951 [details] MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35803/diff/7-8/
Attachment #8721951 -
Attachment description: MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. → MozReview Request: Bug 1249579 - part1 : request audio focus on Fennec. r=snorp.
Attachment #8722386 -
Attachment description: MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. → MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku.
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8722386 [details] MozReview Request: Bug 1249579 - part2 : audio competing suspend/resume methods. r=snorp, baku. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36003/diff/6-7/
Assignee | ||
Comment 48•8 years ago
|
||
Rebase patches, here is the try-server result. https://treeherder.mozilla.org/#/jobs?repo=try&revision=50f96f64e03c
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 49•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/67672cbc496a https://hg.mozilla.org/integration/fx-team/rev/d615ba6b7fc7
Keywords: checkin-needed
Comment 50•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/67672cbc496a https://hg.mozilla.org/mozilla-central/rev/d615ba6b7fc7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Added to Fx48 Aurora release notes
relnote-firefox:
--- → 48+
Comment 53•8 years ago
|
||
Tested on latest Nightly (2016-10-26) with Samsung Galaxy Grand Prime (Android 5.1.1). Following the steps from description, the user only heard the ringtone and the media/audio content is paused when is receiving a call.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•