Implement the audio competing mechanism on Fennec

VERIFIED FIXED in Firefox 49

Status

()

P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 verified, relnote-firefox 49+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8739767 [details]
MozReview Request: Bug 1257738 - part1 : implement the audio competing mechanism.

Review commit: https://reviewboard.mozilla.org/r/45365/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45365/
(Assignee)

Comment 2

2 years ago
Created attachment 8739812 [details]
MozReview Request: Bug 1257738 - part2 : modify logic of requesting audio focus in Android.

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.
(Assignee)

Comment 3

2 years ago
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/
(Assignee)

Updated

2 years ago
Depends on: 1242874
(Assignee)

Comment 4

2 years ago
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/
(Assignee)

Comment 5

2 years ago
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/
(Assignee)

Comment 6

2 years ago
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/
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8739767 - Flags: feedback?(amarchesini)
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8739767 - Flags: feedback?(amarchesini)
(Assignee)

Comment 9

2 years ago
Created attachment 8748509 [details]
MozReview Request: Bug 1257738 - part3 : add test.

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.
(Assignee)

Comment 10

2 years ago
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/
(Assignee)

Comment 11

2 years ago
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/
(Assignee)

Comment 12

2 years ago
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/
(Assignee)

Comment 13

2 years ago
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/
(Assignee)

Comment 14

2 years ago
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/
(Assignee)

Comment 16

2 years ago
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)
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8739767 - Flags: review?(amarchesini)
Attachment #8739812 - Flags: review?(snorp)
Attachment #8748509 - Flags: review?(amarchesini)
(Assignee)

Comment 19

2 years ago
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!
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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/
(Assignee)

Comment 22

2 years ago
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/
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8739767 - Flags: review?(amarchesini)
Attachment #8739812 - Flags: review?(snorp)
Attachment #8748509 - Flags: review?(amarchesini)
(Assignee)

Comment 28

2 years ago
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+
(Assignee)

Comment 30

2 years ago
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/
(Assignee)

Comment 31

2 years ago
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/
(Assignee)

Comment 32

2 years ago
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/
(Assignee)

Comment 33

2 years ago
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/
(Assignee)

Comment 34

2 years ago
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/
(Assignee)

Comment 35

2 years ago
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/
(Assignee)

Comment 36

2 years ago
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/
(Assignee)

Comment 37

2 years ago
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/
(Assignee)

Comment 38

2 years ago
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+
(Assignee)

Comment 42

2 years ago
Thanks for review :)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 43

2 years ago
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/
(Assignee)

Comment 44

2 years ago
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/
(Assignee)

Comment 45

2 years ago
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/
(Assignee)

Comment 47

2 years ago
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/
(Assignee)

Comment 48

2 years ago
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/
(Assignee)

Comment 49

2 years ago
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/
(Assignee)

Comment 51

2 years ago
(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.
(Assignee)

Comment 52

2 years ago
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/
(Assignee)

Comment 53

2 years ago
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/
(Assignee)

Comment 54

2 years ago
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/

Comment 56

2 years ago
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.

Comment 57

2 years ago
bugherder
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
Last Resolved: 2 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
You need to log in before you can comment on or make changes to this bug.