Closed Bug 1249579 Opened 4 years ago Closed 4 years ago

Request audio focus on Fennec

Categories

(Firefox for Android :: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
relnote-firefox --- 48+
fennec 48+ ---

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

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
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
Depends on: 1242874
Attachment #8721951 - Attachment description: MozReview Request: Bug 1249579 - Request audio focus on Fennec. → MozReview Request: Bug 1249579 - part1 : Request audio focus on Fennec.
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/
User Story: (updated)
User Story: (updated)
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)
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/
Attachment #8722386 - Flags: review?(amarchesini)
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/
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!
Blocks: 1240423
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 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)
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.
Duplicate of this bug: 1161907
(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 :)
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)
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)
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/
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+
(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.
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+
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)
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)
Hi, Margaret & Snorp,
Here are my revised patches, could you help me review them again?
Thanks!
No longer depends on: 1242874
See Also: → 1242874
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)
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+
Attachment #8726641 - Attachment is obsolete: true
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)
(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)
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)
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)
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)
Priority: -- → P2
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/
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)
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+
Alastor, are you ready to get this pushed? Try run looks good!
Flags: needinfo?(alwu)
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?
I ping baku in IRC yesterday and he would review this patch soon.
Any progress on getting this reviewed? It would be great to have this in Fx48, but the window is closing on that.
We have talked with baku a couple days ago, he would finish reviewing this week.
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+
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.
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/
Rebase patches, here is the try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50f96f64e03c
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67672cbc496a
https://hg.mozilla.org/mozilla-central/rev/d615ba6b7fc7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Duplicate of this bug: 925297
Depends on: 1287116
No longer depends on: 1289095
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.
You need to log in before you can comment on or make changes to this bug.