Closed Bug 1257738 Opened 4 years ago Closed 4 years ago

Implement the audio competing mechanism on Fennec

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified
relnote-firefox --- 49+

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Fork from bug1240423 comment13, we need to implement the internal audio competing to avoid the sound from multiple tabs mixing up.

Expected STR.
1) User plays video in tab A
2) User switches to tab B
3) User plays video in tab B
4) tab A video + audio pauses
5) tab B video plays
Review commit: https://reviewboard.mozilla.org/r/45383/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45383/
Attachment #8739767 - Attachment description: MozReview Request: Bug 1257738 - Implement the audio competing mechanism. → MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/1-2/
Depends on: 1242874
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/2-3/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/1-2/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/3-4/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/2-3/
Attachment #8739767 - Flags: feedback?(amarchesini)
After the discussion last week, baku pointed out I didn't handle the visibility-changing in audio competing, eg. the background tab shouldn't interrupt the playback of the foreground tab.

I'll modify my patches and ask for review again.
Attachment #8739767 - Flags: feedback?(amarchesini)
Review commit: https://reviewboard.mozilla.org/r/50325/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50325/
Attachment #8739812 - Attachment description: MozReview Request: Bug 1257738 - part2 : add test. → MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/4-5/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/3-4/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/5-6/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/4-5/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/1-2/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/6-7/
Attachment #8739767 - Flags: review?(amarchesini)
Attachment #8739812 - Flags: review?(snorp)
Attachment #8748509 - Flags: review?(amarchesini)
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/5-6/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/2-3/
Attachment #8739767 - Flags: review?(amarchesini)
Attachment #8739812 - Flags: review?(snorp)
Attachment #8748509 - Flags: review?(amarchesini)
Hi, Baku, Snorp,
Sorry for canceling the review, because I found some problems in my patches.
I'll fix them and then ask review request again.
Thanks!
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/7-8/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/6-7/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/3-4/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/8-9/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/7-8/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/4-5/
Priority: -- → P2
Attachment #8739767 - Flags: review?(amarchesini)
Attachment #8739812 - Flags: review?(snorp)
Attachment #8748509 - Flags: review?(amarchesini)
Hi, Baku, Snorp,
Could you help me review these patches?
The implementation of them is to enable audio competing crossing tabs, that means only one tab can playback at a time.
Thanks!
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

https://reviewboard.mozilla.org/r/45383/#review48831
Attachment #8739812 - Flags: review?(snorp) → review+
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/9-10/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/8-9/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/5-6/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/10-11/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/9-10/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/6-7/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/11-12/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/10-11/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/7-8/
baku,
Any progress for review?
Status: NEW → ASSIGNED
Component: General → Audio/Video
Flags: needinfo?(amarchesini)
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

https://reviewboard.mozilla.org/r/45365/#review46253

::: dom/audiochannel/AudioChannelService.cpp:1141
(Diff revision 4)
> +}
> +
> +bool
> +AudioChannelService::AudioChannelWindow::IsActive() const
> +{
> +  return (mAgents.Length() > 0);

!IsEmpty()

::: dom/audiochannel/AudioChannelService.cpp:28
(Diff revision 12)
>  #include "nsHashPropertyBag.h"
>  #include "nsComponentManagerUtils.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsServiceManagerUtils.h"
>  #include "mozilla/dom/SettingChangeNotificationBinding.h"
> +#include "nsGlobalWindow.h"

move it next to nsPIDOMWindow.

::: dom/audiochannel/AudioChannelService.cpp:350
(Diff revision 12)
>    do {
>      winData = GetWindowData(window->WindowID());
>      if (winData) {
>        config.mVolume *= winData->mChannels[aAudioChannel].mVolume;
>        config.mMuted = config.mMuted || winData->mChannels[aAudioChannel].mMuted;
> +      config.mSuspend = (winData->mOwningAudioFocus) ?

without ()

::: dom/audiochannel/AudioChannelService.cpp:1013
(Diff revision 12)
>  
> +void
> +AudioChannelService::RefreshAgentsAudioFocusChanged(AudioChannelAgent* aAgent,
> +                                                    bool aActive)
> +{
> +  MOZ_ASSERT(aAgent);

Do we want to assert in case this aAgent is unknown by the window?

::: dom/audiochannel/AudioChannelService.cpp:1030
(Diff revision 12)
> +void
> +AudioChannelService::AudioChannelWindow::RequestAudioFocus(AudioChannelAgent* aAgent)
> +{
> +  MOZ_ASSERT(aAgent);
> +
> +  if (mOwningAudioFocus) {

write a comment here. Something like: // We already have the audio focus. No operation is needed.

::: dom/audiochannel/AudioChannelService.cpp:1049
(Diff revision 12)
> +
> +void
> +AudioChannelService::AudioChannelWindow::NotifyAudioCompetingChanged(AudioChannelAgent* aAgent,
> +                                                                     bool aActive)
> +{
> +  MOZ_ASSERT(aAgent);

Also here, a check if the aAgent is known by the window.

::: dom/audiochannel/AudioChannelService.cpp:1098
(Diff revision 12)
> +
> +void
> +AudioChannelService::AudioChannelWindow::AudioFocusChanged(AudioChannelAgent* aNewPlayingAgent,
> +                                                           bool aActive)
> +{
> +  MOZ_ASSERT(aNewPlayingAgent);

Write a comment saying that this agent can be unknown but the current window.
Attachment #8739767 - Flags: review?(amarchesini) → review+
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

https://reviewboard.mozilla.org/r/50325/#review53066
Attachment #8748509 - Flags: review?(amarchesini) → review+
Thanks for review :)
Flags: needinfo?(amarchesini)
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/12-13/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/11-12/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/8-9/
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/13-14/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/12-13/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/9-10/
(In reply to Andrea Marchesini (:baku) from comment #40)
> ::: dom/audiochannel/AudioChannelService.cpp:1013
> (Diff revision 12)
> >  
> > +void
> > +AudioChannelService::RefreshAgentsAudioFocusChanged(AudioChannelAgent* aAgent,
> > +                                                    bool aActive)
> > +{
> > +  MOZ_ASSERT(aAgent);
> 
> Do we want to assert in case this aAgent is unknown by the window?
> 
> ::: dom/audiochannel/AudioChannelService.cpp:1049
> (Diff revision 12)
> > +
> > +void
> > +AudioChannelService::AudioChannelWindow::NotifyAudioCompetingChanged(AudioChannelAgent* aAgent,
> > +                                                                     bool aActive)
> > +{
> > +  MOZ_ASSERT(aAgent);
> 
> Also here, a check if the aAgent is known by the window.
> 

The agent would be unknown to window when these functions are called in the process of the unregistration. Since they're called after the |RemoveAgentAndReduceAgentsNum()|, the agent already is removed from |mAgent|. However, the agent would still be alive because we have kungFuDeathGrip in |UnregisterAudioChannelAgent()|.

I would add comment to describe that.
Comment on attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45365/diff/14-15/
Comment on attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45383/diff/13-14/
Comment on attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50325/diff/10-11/
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15ba5d12b73b
part1 : implement the audio competing mechanism.
https://hg.mozilla.org/integration/mozilla-inbound/rev/476e1a1c0ef2
part2 : modify logic of requesting audio focus in Android.
https://hg.mozilla.org/integration/mozilla-inbound/rev/30d59f3fb7e8
part3 : add test.
https://hg.mozilla.org/mozilla-central/rev/15ba5d12b73b
https://hg.mozilla.org/mozilla-central/rev/476e1a1c0ef2
https://hg.mozilla.org/mozilla-central/rev/30d59f3fb7e8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Updates to media controls; Avoid playing sounds from multiple tabs at once. 
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Relnote 49+ added "Updated media controls to avoid playing sounds from multiple tabs at once"
Verified as fixed in build:
- Aurora 49.0a2 (2016-07-11);
- Nightly 50.0a1 (2016-07-11);
Device: LG G4 (Android 5.1) and Nexus 5 (Android 6.0.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.