Closed Bug 1257738 Opened 8 years ago Closed 8 years ago

Implement the audio competing mechanism on Fennec

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect

Tracking

(firefox49 verified, relnote-firefox 49+)

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

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(3 files)

MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.
58 bytes, text/x-review-board-request
baku
: review+
Details
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.
58 bytes, text/x-review-board-request
snorp
: review+
Details
MozReview Request: Bug 1257738 - part3 : add test.
58 bytes, text/x-review-board-request
baku
: review+
Details 
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: 8 years ago
status-firefox49: --- → fixed
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"
relnote-firefox: ? → 49+
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
status-firefox49: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: