Audio output device cannot be changed.

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bwu, Assigned: chunmin)

Tracking

({compat})

unspecified
mozilla57
x86
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 wontfix, firefox56+ verified, firefox57+ verified)

Details

(Whiteboard: sb+)

Attachments

(11 attachments, 7 obsolete attachments)

5.00 KB, patch
Details | Diff | Splinter Review
8.01 KB, patch
Details | Diff | Splinter Review
13.96 KB, patch
Details | Diff | Splinter Review
18.50 KB, patch
Details | Diff | Splinter Review
3.83 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Reporter

Description

2 years ago
I use the latest nightly, 55.0a1 (2017-05-01) (32 bit) on my windows laptop, Dell XPS15. 

Steps to repro:
1. Plug in a headset. 
2. play a YouTube video
3. Set output device to be built-in speaker. 

Actual result:
Audio comes out from headset, not built-in speaker.

Expected result:
Audio should come out from what we set. Edge and Chrome work well.
Reporter

Comment 1

2 years ago
BTW, if I reload Youtube, audio will be output to the right device.
ChunMin,
Can you check it?
Flags: needinfo?(cchang)
Reporter

Comment 2

2 years ago
It only happens on my windows 10 laptop not on my MacBook Pro.
Keywords: compat
OS: Unspecified → Windows 10
Priority: -- → P1
Hardware: Unspecified → x86

Comment 3

2 years ago
I can confirm this behavior on the latest Windows 10 and latest Nightly. Does not occur on stable or beta.
Reporter

Comment 4

2 years ago
Shiretoko212,
Thanks for your confirmation.

Comment 5

2 years ago
I figured out that if you turn off "Enable multi-process Nightly" in the Options and restart Firefox, when you switch audio devices, it works as it should.

So, Firefox Nightly with multi-process enabled breaks audio device switching.

Comment 6

2 years ago
Specifically, nightly multiprocess being enabled breaks real-time audio output device switching for currently playing web sites.
Assignee

Comment 7

2 years ago
I can confirm that on my toshiba Protege R930 with Logitech h340. The new defualt device will work if we seek the video.
Flags: needinfo?(cchang)
Assignee

Updated

2 years ago
Assignee: nobody → cchang
Assignee

Updated

2 years ago
Duplicate of this bug: 1364668
Assignee

Comment 9

2 years ago
The cause is that the content process is unable to receive the reconfigure event[0]. 
That's why non-e10s firefox works well.

[0] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/media/libcubeb/src/cubeb_wasapi.cpp#888
Assignee

Comment 10

2 years ago
I realized it's not caused by regression.
If you changed the security.sandbox.content.level to 1 or 0, then the device change works as usual.
(The default value of security.sandbox.content.level is 2 on Nightly[0].)

Blake, can you help to check this? 
If the release build never set security.sandbox.content.level = 2, maybe we don't need to fix it?

[0] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/browser/app/profile/firefox.js#1009
Flags: needinfo?(bwu)
Reporter

Comment 11

2 years ago
Ethan,
Can you help us understand why changing security.sandbox.content.level would let content process unable to receive event? And would security.sandbox.content.level be 2 on release version?
Flags: needinfo?(bwu) → needinfo?(ettseng)
(In reply to Blake Wu [:bwu][:blakewu] from comment #11)
> Ethan,
> Can you help us understand why changing security.sandbox.content.level would
> let content process unable to receive event? And would
> security.sandbox.content.level be 2 on release version?

Please allow me to forward this question to the Sandbox expert in our team.
Julian, do you know the answers?  :)
Flags: needinfo?(ettseng) → needinfo?(julian.r.hector)
You need to talk to Bob Owen most likely. But let me try to provide some background here. We are progressively rolling out the sandbox [1] with a more strict sandbox currently on nightly than on other versions. Level 2 sets stricter controls (notably job level == JOB_INTERACTIVE, and access token == USER_INTERACTIVE ) that level 1. And yes we actually we plan to set level 3 in 56 I believe (which is stricter still), but the point of enabling stricter restrictions on nightly is to catch situations like this.

I'm not sure what the approach to fixing this would be though - maybe the parent has to forward the event to the child, or handle the switching in the parent, at a wild guess. Hopefully bob can provide a more helpful answer :)


[1] https://wiki.mozilla.org/Security/Sandbox#Current_Status
[2] https://wiki.mozilla.org/Security/Sandbox#Content
Flags: needinfo?(julian.r.hector) → needinfo?(bobowencode)
(In reply to Paul Theriault [:pauljt] from comment #13)
> You need to talk to Bob Owen most likely. But let me try to provide some
> background here. We are progressively rolling out the sandbox [1] with a
> more strict sandbox currently on nightly than on other versions. Level 2
> sets stricter controls (notably job level == JOB_INTERACTIVE, and access
> token == USER_INTERACTIVE ) that level 1. And yes we actually we plan to set
> level 3 in 56 I believe (which is stricter still), but the point of enabling
> stricter restrictions on nightly is to catch situations like this.
> 
> I'm not sure what the approach to fixing this would be though - maybe the
> parent has to forward the event to the child, or handle the switching in the
> parent, at a wild guess. Hopefully bob can provide a more helpful answer :)
> 
> 
> [1] https://wiki.mozilla.org/Security/Sandbox#Current_Status
> [2] https://wiki.mozilla.org/Security/Sandbox#Content

That was quite an NI chain :-), and thanks bwu for finding this.

I'll flag for sandboxing triage, so we look at it later, although it might make sense for the audio team to look at this, as I know they are already working on moving things.
Flags: needinfo?(bobowencode)
Whiteboard: sb?
(In reply to Chun-Min Chang[:chunmin] from comment #9)
> The cause is that the content process is unable to receive the reconfigure
> event[0]. 
> That's why non-e10s firefox works well.
> 
> [0]
> http://searchfox.org/mozilla-central/rev/
> 484d2b7f51b7aed035147bbb4a565061659d9278/media/libcubeb/src/cubeb_wasapi.
> cpp#888

We had to remote this notification for the plugin sandbox as flash wasn't receiving it either. I'd suggest taking the same route, or waiting for the remoting of all of cubeb to complete which is slated for completion toward the end of the quarter. Anthony Jones can provide more detail on the progress of that.

http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/media/libcubeb/src/cubeb_wasapi.cpp#826
http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/media/libcubeb/src/cubeb_wasapi.cpp#289
https://msdn.microsoft.com/en-us/library/windows/desktop/dd371417(v=vs.85).aspx
http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/dom/plugins/ipc/PluginUtilsWin.cpp#48
Assignee

Comment 16

2 years ago
(In reply to Jim Mathies [:jimm] from comment #15)
Thanks for such useful information!

Hi Matthew,
Do we need to remote the notification for cubeb? or we will fix this after remote cubeb API is finished(bug 1362220)?
Flags: needinfo?(kinetik)
The remoting would avoid this issue by handling the device change notification in an unsandboxed process but the current plan only involves the Linux (PulseAudio) backend; all other platforms are scheduled for a later date/release. So we'll need to do something along the lines of the PluginUtilsWin.cpp remoting of this event to avoid regressing this in 56.
Flags: needinfo?(kinetik)
Assignee

Comment 18

2 years ago
Hi Jim,
Is it possible to wrap cubeb(media/libcubeb) as a plugin and register to listen the DefaultAudioDeviceChanged event
on child side? If it's possible, how to do it?
Flags: needinfo?(jmathies)
(In reply to Chun-Min Chang[:chunmin] from comment #18)
> Hi Jim,
> Is it possible to wrap cubeb(media/libcubeb) as a plugin and register to
> listen the DefaultAudioDeviceChanged event
> on child side? If it's possible, how to do it?

No that's not possible.  If we can't wait on libcubeb remoting work, we'll need to add an interface to libcubeb for receiving these events from an external source. Then I'd suggest we move AudioNotification to a more general location and reuse it to send notifications to both consumers. Worst case we could probably support two instances of it if need be.

Is there an object on the chrome side associated with libcubeb instances in content that could handle making calls to a helper like RegisterForAudioDeviceChanges? Maybe through the linux work? You'll need an IPC channel for messaging. PBrowser might be your best bet if media doesn't have its own.
Flags: needinfo?(jmathies)
Hey Chun-Min, Are you planning on working on this?
Flags: needinfo?(cchang)
Whiteboard: sb? → sb+
Assignee

Comment 21

2 years ago
Yes, I plan to do it after finishing the work in hand.
Flags: needinfo?(cchang)
Assignee

Comment 22

2 years ago
Rough idea:

IPC
=====================
Create a PAudio.ipdl like:
-------------------------------------------------
async protocol PAudio
{
  manager XXX;
child:
  async DefaultDeviceChange();
...
...
}
-------------------------------------------------
so we will have PAudioParent::SendDeviceChange(...) and PAudioChild::RecvDeviceChange(...) to pass the message of the device change.


Chrome Process
=====================
- Implement a AudioNotification which inherits IMMNotificationClient
- Implement RegisterForAudioDeviceChanges
  - When it's called, we initialize a AudioNotification instance to receive the device-change events.
- Once AudioNotification::OnDefaultDeviceChanged is fired, notify the child side via PAudioParent::SendDeviceChange(...)


Content Process
=====================
- Add new APIs: AudioStream::ChangeDevice, cubeb_stream_change_device
- Implement a AudioNotificationReceiver
  - Maintain a subscribers' list
  - Every audio stream should be put into the subscribers' list
- Once PAudioChild::RecvDeviceChange receives the device-change message, 
  then we notify all the subscribers in the AudioNotificationReceiver's list to change the device.
  - Call AudioStream::ChangeDevice and cubeb_stream_change_device


Questions:
=====================
I am not sure where I should put the IPDL. It should be managed by PContent, PBackground, or others?
And where should I put the IPDL files? dom/media/ipc? dom/media/systemservices?

Hi Matthew,
Could you give me some suggestions?
Flags: needinfo?(kinetik)
Can you reuse most of the existing implementation in dom/plugins/ipc/PluginUtilsWin.cpp (and related IPDL) as Jim pointed out in comment 15?

Note that you'll also need to handle the libcubeb streams created via MediaStreamGraph, which doesn't use AudioStream.

What's the behaviour and parameters of the proposed cubeb_stream_change_device API?  If you're proposing something that simply takes the cubeb_stream* and no other params and sets the associated reconfigure_event internally, that's probably fine for a short-term thing until we get the remoting work done.  The existing device change code isn't ideal in libcubeb, so avoiding investing too much effort into it is good unless you want to clean it up completely.
Flags: needinfo?(kinetik)
Assignee

Comment 24

2 years ago
(In reply to Matthew Gregan [:kinetik] from comment #23)
> Can you reuse most of the existing implementation in
> dom/plugins/ipc/PluginUtilsWin.cpp (and related IPDL) as Jim pointed out in
> comment 15?
Yes, I'll try reusing the AudioNotification part. Other things use PluginXXX as parameters, so I cannot reuse it unless I also rewrite them.

> Note that you'll also need to handle the libcubeb streams created via
> MediaStreamGraph, which doesn't use AudioStream.
OK, thanks for the hints!

> What's the behaviour and parameters of the proposed
> cubeb_stream_change_device API?  If you're proposing something that simply
> takes the cubeb_stream* and no other params and sets the associated
> reconfigure_event internally, that's probably fine for a short-term thing
> until we get the remoting work done.  The existing device change code isn't
> ideal in libcubeb, so avoiding investing too much effort into it is good
> unless you want to clean it up completely.
Besides cubeb_stream*, I originally plan to pass the information of the new default device. But I think it's ok to omit it because we can just call "SetEvent(stream->reconfigure_event)" in the cubeb_stream_change_device to force the stream to reconfigure the stream to use thr new default device.

A bigger question here is that:
How could we make this change to meet our long-term requirements? Do we have a clear plan for now?
Assignee

Comment 28

2 years ago
(In reply to Chun-Min Chang[:chunmin] from comment #27)
> Created attachment 8879462 [details] [diff] [review]
> [WIP] part3: Pass the device-changed events via PBrowser by
> AudioNotificationSender/Receiver

TODO:
- Create a PAudio.ipdl to pass the device-changed events instead of using PBrowser.
Assignee

Comment 29

2 years ago
Beside the solution provided previously(attachment 8879460 [details] [diff] [review], attachment 8879461 [details] [diff] [review], attachment 8879462 [details] [diff] [review]), I find a hacky work-around to make dvice-changed notification works in Cubeb.

The reason why cubeb fails to re-configure the audio streams to the new default device is that we cannot register device-changed callback from IMMNotificationClient[0]. It returns a general access denied error: E_ACCESSDENIED[1].

However, if we register the callback from IMMNotificationClient in gecko at an early stage[2], then cubeb can also register the callbacks successfully. 

My question is: 
1) Where does the sandbox start working? Is nsLayoutStatics::Initialize() called before sandbox is working?
2) Is this a expected behavior in sandbox?

Hi Paul,
Do you have any idea?


[0] https://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/media/libcubeb/src/cubeb_wasapi.cpp#986
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa378137(v=vs.85).aspx
[2] in this patch, I register a dummy callback in nsLayoutStatics::Initialize()
Flags: needinfo?(ptheriault)
(In reply to Chun-Min Chang[:chunmin] from comment #29)
> Created attachment 8879492 [details] [diff] [review]
> Hacky solution
> 
> Beside the solution provided previously(attachment 8879460 [details] [diff] [review]
> [review], attachment 8879461 [details] [diff] [review], attachment 8879462 [details] [diff] [review]
> [details] [diff] [review]), I find a hacky work-around to make dvice-changed
> notification works in Cubeb.
> 
> The reason why cubeb fails to re-configure the audio streams to the new
> default device is that we cannot register device-changed callback from
> IMMNotificationClient[0]. It returns a general access denied error:
> E_ACCESSDENIED[1].
> 
> However, if we register the callback from IMMNotificationClient in gecko at
> an early stage[2], then cubeb can also register the callbacks successfully. 
> 
> My question is: 
> 1) Where does the sandbox start working? Is nsLayoutStatics::Initialize()
> called before sandbox is working?
> 2) Is this a expected behavior in sandbox?

The sandbox restrictions kick in before the main thread executes as we start child processes suspended. That said, there is a space in here where we start with fewer restrictions to do some initialization, then lower rights before content is loaded. Here's some info on that with src links - 

https://wiki.mozilla.org/Security/Sandbox#Windows_2
Flags: needinfo?(ptheriault)
Assignee

Comment 31

2 years ago
(In reply to Jim Mathies [:jimm] from comment #30)
> The sandbox restrictions kick in before the main thread executes as we start
> child processes suspended. That said, there is a space in here where we
> start with fewer restrictions to do some initialization, then lower rights
> before content is loaded. Here's some info on that with src links - 
> 
> https://wiki.mozilla.org/Security/Sandbox#Windows_2
Thanks for the information!
Assignee

Comment 34

2 years ago
Attachment #8879460 - Attachment is obsolete: true
Attachment #8879461 - Attachment is obsolete: true
Attachment #8879462 - Attachment is obsolete: true
Attachment #8882338 - Attachment is obsolete: true
Assignee

Comment 37

2 years ago
The previous patches cannot build on the non-Windows platforms.
Attachment #8882389 - Attachment is obsolete: true
Attachment #8882390 - Attachment is obsolete: true
Attachment #8882392 - Attachment is obsolete: true
Assignee

Comment 40

2 years ago
Leave an architecture figure in the comments.
Comment hidden (mozreview-request)
Assignee

Comment 45

2 years ago
mozreview-review
Comment on attachment 8883865 [details]
Bug 1361336 - part1: A new API for AudioStream that it can reset stream to the default device;

https://reviewboard.mozilla.org/r/154836/#review159834

This is a preview for the changing of Cubeb. I'll upload the change to Cubeb if you think it's ok.

::: commit-message-0893f:2
(Diff revision 1)
> +Bug 1361336 - part0: Add a new api to reconfigure stream to the default device; r?kinetik
> +

Comment 46

2 years ago
mozreview-review
Comment on attachment 8883866 [details]
Bug 1361336 - part2: Prevent AudioStream::ResetDefaultDevice() from being called before stream is started;

https://reviewboard.mozilla.org/r/154838/#review160094
Attachment #8883866 - Flags: review?(kinetik) → review+

Comment 47

2 years ago
mozreview-review
Comment on attachment 8883865 [details]
Bug 1361336 - part1: A new API for AudioStream that it can reset stream to the default device;

https://reviewboard.mozilla.org/r/154836/#review160096

::: media/libcubeb/include/cubeb.h:537
(Diff revision 1)
>      @param stream
>      @retval CUBEB_OK
>      @retval CUBEB_ERROR */
>  CUBEB_EXPORT int cubeb_stream_stop(cubeb_stream * stream);
>  
> +/** Reset playback to default device.

Remove "playback", because this can affect capture as well.
Attachment #8883865 - Flags: review?(kinetik) → review+

Comment 48

2 years ago
mozreview-review
Comment on attachment 8883865 [details]
Bug 1361336 - part1: A new API for AudioStream that it can reset stream to the default device;

https://reviewboard.mozilla.org/r/154836/#review160098

LGTM, but needs to go via a PR into upstream libcubeb (as you said).  Also need to update the cubeb_ops table for the other backends, and synchronize changes to the cubeb_ops table with :kamidphish so cubeb-pulse-rs doesn't break.
Assignee

Updated

2 years ago
Depends on: 1379048

Comment 49

2 years ago
mozreview-review
Comment on attachment 8883867 [details]
Bug 1361336 - part3: Pass audio default device-changed message via PContent;

https://reviewboard.mozilla.org/r/154840/#review160138

::: dom/media/systemservices/PAudio.ipdl:16
(Diff revision 1)
> +{
> +  manager PContent;
> +
> +child:
> +  async __delete__();
> +  async DefaultDeviceChange();

Given that your protocol only adds one new message, can you just add your message to PContent instead? Then you don't need the overhead of creating a new protocol?

Comment 50

2 years ago
mozreview-review
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review160140

Lots of good work here, but I think we can make it simpler. Also, please use `./mach clang-format` as it will save you time and make your life, and your reviewers' lives easier.

::: dom/media/AudioNotificationReceiver.cpp:31
(Diff revision 1)
> +// An IPC child to receive the device-changed notification.
> +static AudioChild* sChild = nullptr;
> +
> +
> +/*
> + * A runnable task to create an IPC channel(IPC need to be done in worker thread).

s/channel(IPC/channel (IPC/

i.e. insert space between "channel" and "(IPC"

::: dom/media/AudioNotificationReceiver.cpp:41
(Diff revision 1)
> +  explicit AudioIPCRunnable(): Runnable("AudioIPCRunnable")
> +  {}
> +
> +  NS_IMETHOD Run() override
> +  {
> +    ANR_LOG("Establishing an IPC channel to receive device-changed notification.");

You should assert on this thread which thread it's run on.

::: dom/media/AudioNotificationReceiver.cpp:102
(Diff revision 1)
> +/* static */ void
> +AudioNotificationReceiver::NotifyDefaultDeviceChanged()
> +{
> +  MOZ_ASSERT(XRE_IsContentProcess());
> +
> +  for (auto iter = sSubscribers->ConstIter(); !iter.Done(); iter.Next()) {

Don't you need to lock sMutex here? Otherwise the table can change while you're iterating over it!

::: dom/media/AudioNotificationReceiver.cpp:106
(Diff revision 1)
> +
> +  for (auto iter = sSubscribers->ConstIter(); !iter.Done(); iter.Next()) {
> +    AudioStream* subscriber = iter.Get()->GetKey();
> +    ANR_LOG("Notify the AudioStream: %p that the default device has been changed.", subscriber);
> +    subscriber->ResetDefaultDevice();
> +  }

What happens if the NotifyDeviceChanged notification comes after we create an AudioStream's cubeb stream but before we register the AudioStream? Wouldn't the AudioStream then miss the device change notification, and so not play?

If you register streams before initializing them, maybe in the AudioStream constructor, you should prevent this race I think. You'll then need to make sure that AudioStream::ResetDefaultDevice() can handle being called after the constructor has been run but before the cubeb stream has been opened.

::: dom/media/AudioNotificationSender.cpp:27
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace audio {
> +
> +/*
> + * A list containing all IPC clients subscribering the device-changed

"subscribering" -> "subscribing".

::: dom/media/AudioNotificationSender.cpp:30
(Diff revision 1)
> +
> +/*
> + * A list containing all IPC clients subscribering the device-changed
> + * notifications.
> + */
> +typedef nsTHashtable<nsPtrHashKey<AudioParent>> Subscribers;

You're using a hash table as a list. You're paying the overhead of a hash table for no benefit. Just use an nsTArray<AudioParent*> or even a std::list<AudioParent*> if you want list behaviour.

::: dom/media/AudioNotificationSender.cpp:33
(Diff revision 1)
> + * notifications.
> + */
> +typedef nsTHashtable<nsPtrHashKey<AudioParent>> Subscribers;
> +static StaticAutoPtr<Subscribers> sSubscribers;
> +static StaticMutex sMutex;
> +

You've got two line breaks on either side of this class definition. I recommend you run \`./mach clang-format\` before committing/amending your code to automatically format your code to conform to Mozilla code formatting standards.

::: dom/media/AudioNotificationSender.cpp:46
(Diff revision 1)
> +  explicit AudioDeviceChangedRunnable(): Runnable("AudioDeviceChangedRunnable")
> +  {}
> +
> +  NS_IMETHOD Run() override
> +  {
> +    for (auto iter = sSubscribers->ConstIter(); !iter.Done(); iter.Next()) {

Do you need to lock sMutex here?

::: dom/media/AudioNotificationSender.cpp:61
(Diff revision 1)
> +
> +/*
> + * An observer for receiving audio device events from Windows.
> + */
> +typedef void (* DefaultDeviceChangedCallback)();
> +class AudioNotification: public IMMNotificationClient

Spaces on either side of ':'. ./mach clang-format would have fixed this for you.

::: dom/media/AudioNotificationSender.cpp:70
(Diff revision 1)
> +    : mRefCt(1)
> +    , mIsRegistered(false)
> +    , mCallback(aCallback)
> +  {
> +    MOZ_ASSERT(mCallback);
> +    HRESULT hr = CoCreateInstance(__uuidof(MMDeviceEnumerator),

You should be able to use a mozilla::RefPtr<IMMDeviceEnumerator> for mDeviceEnumerator instead of a raw pointer here.

That will make working with the pointer much easier and safer.

For an example of using CoCreateInstance() with mozilla::RefPtr, see MFTDecoder::mDecoder:

https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/dom/media/platforms/wmf/MFTDecoder.cpp#31

::: dom/media/AudioNotificationSender.cpp:95
(Diff revision 1)
> +  }
> +
> +  ~AudioNotification()
> +  {
> +    // If mDeviceEnumerator exists, it must be registered.
> +    MOZ_ASSERT(!!mIsRegistered == !!mDeviceEnumerator);

You could also write this as:

!mDeviceEnumerator || mIsRegistered

Evaluates to true if we don't have a device enumerator, but if we do have a device enumerator this only evaluates to true if it's registered.

::: dom/media/AudioNotificationSender.cpp:114
(Diff revision 1)
> +    mDeviceEnumerator->Release();
> +    mDeviceEnumerator = nullptr;
> +
> +    mIsRegistered = false;
> +  }
> +

You have two line breaks between function definitions here but not elsewhere. I think I saw you do this elsewhere too.

I strongly recommend you just run `./mach clang-format` before committing/amending/qrefing to automatically format your code to be compliant with Mozilla formatting guidelines. It makes things a lot easier.

::: dom/media/AudioNotificationSender.cpp:117
(Diff revision 1)
> +    mIsRegistered = false;
> +  }
> +
> +
> +  // True whenever the notification server is set to report events to this object.
> +  bool IsRegistered()

This function could be const right?

::: dom/media/AudioNotificationSender.cpp:163
(Diff revision 1)
> +    return S_OK;
> +  }
> +
> +  // IUnknown Implementation
> +  ULONG STDMETHODCALLTYPE
> +  AddRef() override

Instead of reimplementing AddRef/Release, could you use:

NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioNotification, override)

?

::: dom/media/AudioNotificationSender.cpp:186
(Diff revision 1)
> +    if (__uuidof(IUnknown) == riid) {
> +      AddRef();
> +      *ppvInterface = (IUnknown*)this;
> +    } else if (__uuidof(IMMNotificationClient) == riid) {
> +      AddRef();
> +      *ppvInterface = (IMMNotificationClient*)this;

Best to use static_cast<> here.

::: dom/media/AudioNotificationSender.cpp:196
(Diff revision 1)
> +    return S_OK;
> +  }
> +
> +private:
> +  DefaultDeviceChangedCallback mCallback;
> +  IMMDeviceEnumerator* mDeviceEnumerator;

You should be able to use a mozilla::RefPtr\<IMMDeviceEnumerator\> instead of a raw pointer here. 

For an example, see MFTDecoder::mDecoder.

::: dom/media/AudioNotificationSender.cpp:205
(Diff revision 1)
> +
> +
> +/*
> + * A singleton observer for audio device changed events.
> + */
> +static AudioNotification* sAudioNotification = nullptr;

Extra line breaks on either side of this. ./mach clang-format would have caught this for you (I think).

::: dom/media/AudioNotificationSender.cpp:216
(Diff revision 1)
> +/* static */ nsresult
> +AudioNotificationSender::Register(AudioParent* aAudioParent)
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +
> +  if (!sAudioNotification) {

What thread is sAudioNotification accessed on? It's not clear by looking at this code that it's thread safe.

When writing multi-threaded code, please can you assert in every function what thread or TaskQueue that function is run on, to ensure you thread safety is guaranteed?

::: dom/media/AudioStream.cpp:392
(Diff revision 1)
>    LOG("creation time %sfirst: %u ms", aIsFirst ? "" : "not ",
>        (uint32_t) timeDelta.ToMilliseconds());
>    Telemetry::Accumulate(aIsFirst ? Telemetry::AUDIOSTREAM_FIRST_OPEN_MS :
>        Telemetry::AUDIOSTREAM_LATER_OPEN_MS, timeDelta.ToMilliseconds());
>  
> +#ifdef XP_WIN

As I said above, I think you should register before initializing the cubeb stream, to avoid a race.

::: dom/media/AudioStream.cpp:394
(Diff revision 1)
>    Telemetry::Accumulate(aIsFirst ? Telemetry::AUDIOSTREAM_FIRST_OPEN_MS :
>        Telemetry::AUDIOSTREAM_LATER_OPEN_MS, timeDelta.ToMilliseconds());
>  
> +#ifdef XP_WIN
> +  if (XRE_IsContentProcess()) {
> +    audio::AudioNotificationReceiver::Register(this);

Can the AudioStreams register directly on the ContentChild instead of having to add a new PAudio protocol?

If you want to encapsulate the registering functionality into its own class (so that you're not adding even more stuff to the ContentChild class), you can put it in the AudioNotificationReceiver class, and give the ContentChild object a AudioNotificationReceiver member which you call into here.
Attachment #8883868 - Flags: review?(cpearce) → review-

Comment 51

2 years ago
mozreview-review
Comment on attachment 8883867 [details]
Bug 1361336 - part3: Pass audio default device-changed message via PContent;

https://reviewboard.mozilla.org/r/154840/#review160152

I think you shouldn't need to create a new protocol just to send one message. You should be able to just add another message to PContent which does what you need. You'll probably need the AudioStreams to register directly with the ContentChild to achieve this, as I describe in your next patch. Please try that, and let me know if it's not going to work.
Attachment #8883867 - Flags: review?(cpearce) → review-
Assignee

Comment 52

2 years ago
mozreview-review
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review160162

::: dom/media/AudioNotificationReceiver.cpp:31
(Diff revision 1)
> +// An IPC child to receive the device-changed notification.
> +static AudioChild* sChild = nullptr;
> +
> +
> +/*
> + * A runnable task to create an IPC channel(IPC need to be done in worker thread).

Thanks for the correction.

::: dom/media/AudioNotificationReceiver.cpp:41
(Diff revision 1)
> +  explicit AudioIPCRunnable(): Runnable("AudioIPCRunnable")
> +  {}
> +
> +  NS_IMETHOD Run() override
> +  {
> +    ANR_LOG("Establishing an IPC channel to receive device-changed notification.");

This runnable should be removed since we don't need to build an IPC here.

::: dom/media/AudioNotificationReceiver.cpp:102
(Diff revision 1)
> +/* static */ void
> +AudioNotificationReceiver::NotifyDefaultDeviceChanged()
> +{
> +  MOZ_ASSERT(XRE_IsContentProcess());
> +
> +  for (auto iter = sSubscribers->ConstIter(); !iter.Done(); iter.Next()) {

It does need. Thanks for the correction.

::: dom/media/AudioNotificationReceiver.cpp:106
(Diff revision 1)
> +
> +  for (auto iter = sSubscribers->ConstIter(); !iter.Done(); iter.Next()) {
> +    AudioStream* subscriber = iter.Get()->GetKey();
> +    ANR_LOG("Notify the AudioStream: %p that the default device has been changed.", subscriber);
> +    subscriber->ResetDefaultDevice();
> +  }

Register the AudioStream before cubeb_stream_init should work.

Currently, if ```AudioStream::ResetDefaultDevice()``` is called before ```AudioStream::OpenCubeb```, then ```InvokeCubeb``` will return ```CUBEB_ERROR_INVALID_PARAMETER``` and put the ```mState``` to ```ERRORED```. Checking ```mCubebStream``` or ```mState``` could avoid that.

::: dom/media/AudioNotificationSender.cpp:27
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace audio {
> +
> +/*
> + * A list containing all IPC clients subscribering the device-changed

Thanks.

::: dom/media/AudioNotificationSender.cpp:30
(Diff revision 1)
> +
> +/*
> + * A list containing all IPC clients subscribering the device-changed
> + * notifications.
> + */
> +typedef nsTHashtable<nsPtrHashKey<AudioParent>> Subscribers;

Do you mean the hash table needs to be replaced only here or also in AudioNotificationReceiver?

The hash table can register/unregister items in O(1), while the list takes O(n) to remove items. If it's ok to ignore the time for deletion, then I'll replace it in next patches.

::: dom/media/AudioNotificationSender.cpp:33
(Diff revision 1)
> + * notifications.
> + */
> +typedef nsTHashtable<nsPtrHashKey<AudioParent>> Subscribers;
> +static StaticAutoPtr<Subscribers> sSubscribers;
> +static StaticMutex sMutex;
> +

Thanks for the hints.

It's weird that running ```/mach clang-format``` doesn't change anything on my laptop. I'll try to solve this to make it easier for reviewers.

::: dom/media/AudioNotificationSender.cpp:46
(Diff revision 1)
> +  explicit AudioDeviceChangedRunnable(): Runnable("AudioDeviceChangedRunnable")
> +  {}
> +
> +  NS_IMETHOD Run() override
> +  {
> +    for (auto iter = sSubscribers->ConstIter(); !iter.Done(); iter.Next()) {

Thanks for the correction.

::: dom/media/AudioNotificationSender.cpp:163
(Diff revision 1)
> +    return S_OK;
> +  }
> +
> +  // IUnknown Implementation
> +  ULONG STDMETHODCALLTYPE
> +  AddRef() override

The ```AddRef()```, ```Release()```, and ```QueryInterface``` are for Microsoft COM here. Can NS_INLINE_DECL_THREADSAFE_REFCOUNTING help in this case?

::: dom/media/AudioNotificationSender.cpp:186
(Diff revision 1)
> +    if (__uuidof(IUnknown) == riid) {
> +      AddRef();
> +      *ppvInterface = (IUnknown*)this;
> +    } else if (__uuidof(IMMNotificationClient) == riid) {
> +      AddRef();
> +      *ppvInterface = (IMMNotificationClient*)this;

Sure.

::: dom/media/AudioNotificationSender.cpp:216
(Diff revision 1)
> +/* static */ nsresult
> +AudioNotificationSender::Register(AudioParent* aAudioParent)
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +
> +  if (!sAudioNotification) {

Sure.

::: dom/media/AudioStream.cpp:392
(Diff revision 1)
>    LOG("creation time %sfirst: %u ms", aIsFirst ? "" : "not ",
>        (uint32_t) timeDelta.ToMilliseconds());
>    Telemetry::Accumulate(aIsFirst ? Telemetry::AUDIOSTREAM_FIRST_OPEN_MS :
>        Telemetry::AUDIOSTREAM_LATER_OPEN_MS, timeDelta.ToMilliseconds());
>  
> +#ifdef XP_WIN

OK.

::: dom/media/AudioStream.cpp:394
(Diff revision 1)
>    Telemetry::Accumulate(aIsFirst ? Telemetry::AUDIOSTREAM_FIRST_OPEN_MS :
>        Telemetry::AUDIOSTREAM_LATER_OPEN_MS, timeDelta.ToMilliseconds());
>  
> +#ifdef XP_WIN
> +  if (XRE_IsContentProcess()) {
> +    audio::AudioNotificationReceiver::Register(this);

I tried not to change the main function of PContent. That's why I create a new IPDL. 

To pass the notification via PContent by adding one message, registering the AudioNotification when the ContentParent is initialized and passing the notifications to the ContentChild when the default device is changed can work.
 
The ContentChild will use the same way to call the AudioNotificationReceiver as AudioChild now.
Assignee

Updated

2 years ago
Depends on: 1380233
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 62

2 years ago
mozreview-review-reply
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review160140

Thanks for the hints.
I don't know why ```./mach clang-format``` cannot work on my laptop right now, but I'll fix it.

> s/channel(IPC/channel (IPC/
> 
> i.e. insert space between "channel" and "(IPC"

This runnable should be removed since we don't need to build an IPC here.

> You should assert on this thread which thread it's run on.

Sure.

> Don't you need to lock sMutex here? Otherwise the table can change while you're iterating over it!

Thanks for the correction.

> Instead of reimplementing AddRef/Release, could you use:
> 
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioNotification, override)
> 
> ?

The AddRef/Release here is for Windows COM. Does NS_INLINE_DECL_THREADSAFE_REFCOUNTING help in this case?
Comment hidden (mozreview-request)

Comment 64

2 years ago
mozreview-review
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review162298

::: dom/media/AudioNotificationReceiver.cpp:26
(Diff revision 3)
> +
> +namespace mozilla {
> +namespace audio {
> +
> +/*
> + * A list containing all clients subscribering the device-changed notifications.

s/list/hash set/ as that's what you're using it for, right? It's a set.

For small numbers of elements, usually an ArrayList will beat a hash table on performance, even though the array has O(n) for removals, due to cache locality.

Typically you are either dealing with small numbers of elements, or you're not dealing with code in a hot path, and so it doesn't matter, and you should just pick whatever is easier to read and reason about. 

So I think using a hash table (as a set) here is fine.

::: dom/media/AudioNotificationReceiver.cpp:40
(Diff revision 3)
> +/* static */ void
> +AudioNotificationReceiver::Register(AudioStream* aAudioStream)
> +{
> +  MOZ_ASSERT(XRE_IsContentProcess());
> +
> +  if (!sSubscribers) {

What happens if two threads enter this function for the first time at about the same time? They could both read sSubscribers and see its null, and then both try to assign to sSubscribers. The last one to assign to sSubscribers would overwrite the first's assignment.

You need to take the lock while null checking and assigning to sSubscribers too.

::: dom/media/AudioNotificationReceiver.cpp:54
(Diff revision 3)
> +
> +/* static */ void
> +AudioNotificationReceiver::Unregister(AudioStream* aAudioStream)
> +{
> +  MOZ_ASSERT(XRE_IsContentProcess());
> +  MOZ_ASSERT(sSubscribers, "No subscriber.");

You should take the lock before asserting the state of sSubscribers, right?

::: dom/media/AudioNotificationReceiver.cpp:68
(Diff revision 3)
> +/* static */ void
> +AudioNotificationReceiver::NotifyDefaultDeviceChanged()
> +{
> +  MOZ_ASSERT(XRE_IsContentProcess());
> +
> +  if (!sSubscribers) {

While you don't explicitly need to take the lock while null checking sSubscribers here, as it probably won't hurt you if sSubscribers was assigned on another thread right after you null checked it here, I recommend you do anyway for consistency.

::: dom/media/AudioNotificationSender.cpp:31
(Diff revision 3)
> + * A list containing all IPC clients subscribing the device-changed
> + * notifications.
> + */
> +// There must be one existing ContentParent and the ContentParent will be
> +// registered in here immediately when it's created.
> +static nsTArray<RefPtr<dom::ContentParent>> sSubscribers;

You're holding onto a refcount on to the ContentParent. I don't see anywhere which is releasing the refcount. This means we'll leak every single ContentParent that we create.

I suggest you make this a nsTArray\<dom::ContentParent*\> and have the ContentParent destructor deregister itself, while holding the mutex.

::: dom/media/AudioNotificationSender.cpp:47
(Diff revision 3)
> +
> +  NS_IMETHOD Run() override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    StaticMutexAutoLock lock(sMutex);
> +    for (auto subscriber: sSubscribers) {

Code is read more often that it is written. So optimize code for readability.

So don't use auto unless it improves readability, which usually it does not.

Using auto here means readers of the code need to look up the type of sSubscribers to figure out what the type of auto should be.

Please only use auto for templated iterator types.

::: dom/media/AudioNotificationSender.cpp:64
(Diff revision 3)
> +typedef void (* DefaultDeviceChangedCallback)();
> +class AudioNotification final : public IMMNotificationClient
> +{
> +public:
> +  AudioNotification(DefaultDeviceChangedCallback aCallback)
> +    : mRefCt(1)

Constructing the object with a refcount of 1 means that it can never be destroyed as its refcount can never drops to 0. When you construct this, you put it in a static RefPtr immedaitely, and its refcount will then be 2.

If you want to create this object with a non-zero refcount, you should create a static factory method that returns an AlreadyAddrefed\<AudioNotification\> and make the constructor private.

::: dom/media/AudioNotificationSender.cpp:68
(Diff revision 3)
> +  AudioNotification(DefaultDeviceChangedCallback aCallback)
> +    : mRefCt(1)
> +    , mIsRegistered(false)
> +    , mCallback(aCallback)
> +  {
> +    MOZ_ASSERT(mCallback);

Please add:

MOZ_COUNT_CTOR(AudioNotification)

This ensures that we don't forget to free this.

::: dom/media/AudioNotificationSender.cpp:94
(Diff revision 3)
> +    mIsRegistered = true;
> +  }
> +
> +  ~AudioNotification()
> +  {
> +    // Assert mIsRegistered is true when we have mDeviceEnumerator.

Please add:

MOZ_COUNT_DTOR(AudioNotification)

This ensures we don't forget to free this.

::: dom/media/AudioNotificationSender.cpp:114
(Diff revision 3)
> +
> +    mIsRegistered = false;
> +  }
> +
> +  // True whenever the notification server is set to report events to this object.
> +  const bool IsRegistered()

If you have that as:

const bool IsRegistered()

that means you're return a bool that can't change. Whereas have:

bool IsRegistered() const

That means that this function cannot change the underlying state of the object. That's what we want here. So make this:

bool IsRegistered() const

::: dom/media/AudioNotificationSender.cpp:201
(Diff revision 3)
> +}; // class AudioNotification
> +
> +/*
> + * A singleton observer for audio device changed events.
> + */
> +static RefPtr<AudioNotification> sAudioNotification = nullptr;

Can you please use StaticRefPtr\<AudioNotification\> so that we destroy this on shutdown automatically?

I'm concerned that is someone changes how this class is used, we might accidentally leak it if we're not doing the refcounting carefully.

::: dom/media/AudioNotificationSender.cpp:210
(Diff revision 3)
> + */
> +/* static */ nsresult
> +AudioNotificationSender::Register(dom::ContentParent* aContentParent)
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +

Does every function in this class except NotifyDefaultDeviceChanged() run on the main thread? I'd guess so. Please add assertions that these functions run on the main thread. If so, then you could remove the monitor here entirely, as sSubscribers would only be accessed on the main thread.
Attachment #8883868 - Flags: review?(cpearce) → review-

Comment 65

2 years ago
mozreview-review
Comment on attachment 8883867 [details]
Bug 1361336 - part3: Pass audio default device-changed message via PContent;

https://reviewboard.mozilla.org/r/154840/#review162304
Attachment #8883867 - Flags: review?(cpearce) → review+

Comment 66

2 years ago
mozreview-review
Comment on attachment 8885590 [details]
Bug 1361336 - part5: Register the AudioStream into AudioNotificationReceiver when it's created;

https://reviewboard.mozilla.org/r/156438/#review162306
Attachment #8885590 - Flags: review?(cpearce) → review+

Comment 67

2 years ago
mozreview-review
Comment on attachment 8885591 [details]
Bug 1361336 - part6: Apply AudioNotificationSender/Receiver in ContentParent/Child;

https://reviewboard.mozilla.org/r/156440/#review162308

::: dom/ipc/ContentParent.cpp:2178
(Diff revision 3)
>    // From this point on, NS_WARNING, NS_ASSERTION, etc. should print out the
>    // PID along with the warning.
>    nsDebugImpl::SetMultiprocessMode("Parent");
>  
> -#if defined(XP_WIN) && !defined(MOZ_B2G)
> +#if defined(XP_WIN)
> +  audio::AudioNotificationSender::Register(this);

I just noticed line 2171; there's already a list of ContentParents there that you can use to send messages to all the child processes. Can you use that instead of maintaining your own list of ContentParents?

Comment 68

2 years ago
mozreview-review
Comment on attachment 8885591 [details]
Bug 1361336 - part6: Apply AudioNotificationSender/Receiver in ContentParent/Child;

https://reviewboard.mozilla.org/r/156440/#review162310

I'll r- this now as I'm going to assume there's a way you can use the ContentParent instead of defining an AudioNotificationSender. Please re-request review if this doesn't make sense. Thanks!
Attachment #8885591 - Flags: review?(cpearce) → review-

Comment 69

2 years ago
mozreview-review
Comment on attachment 8883866 [details]
Bug 1361336 - part2: Prevent AudioStream::ResetDefaultDevice() from being called before stream is started;

https://reviewboard.mozilla.org/r/154838/#review162312

::: dom/media/AudioStream.cpp:480
(Diff revision 2)
>  }
>  
>  void
>  AudioStream::ResetDefaultDevice()
>  {
> +  if (mState != STARTED && mState != STOPPED) {

I think you need to take the monitor before checking the value of mState, otherwise it may be changed by another thread and you may be acting on an incorrect value.
Attachment #8883866 - Flags: review?(cpearce) → review-
Assignee

Comment 70

2 years ago
mozreview-review-reply
Comment on attachment 8883866 [details]
Bug 1361336 - part2: Prevent AudioStream::ResetDefaultDevice() from being called before stream is started;

https://reviewboard.mozilla.org/r/154838/#review162312

> I think you need to take the monitor before checking the value of mState, otherwise it may be changed by another thread and you may be acting on an incorrect value.

Thanks for the correction!
Assignee

Comment 71

2 years ago
mozreview-review-reply
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review162298

> s/list/hash set/ as that's what you're using it for, right? It's a set.
> 
> For small numbers of elements, usually an ArrayList will beat a hash table on performance, even though the array has O(n) for removals, due to cache locality.
> 
> Typically you are either dealing with small numbers of elements, or you're not dealing with code in a hot path, and so it doesn't matter, and you should just pick whatever is easier to read and reason about. 
> 
> So I think using a hash table (as a set) here is fine.

If ArrayList is faster, then it should be used. Thanks for the hints.

> What happens if two threads enter this function for the first time at about the same time? They could both read sSubscribers and see its null, and then both try to assign to sSubscribers. The last one to assign to sSubscribers would overwrite the first's assignment.
> 
> You need to take the lock while null checking and assigning to sSubscribers too.

All the functions here are running in main thread. Adding assertions(checking it's on the main thread) in every functions and remove the mutex should make it easier to read.

> You should take the lock before asserting the state of sSubscribers, right?

I'll replace ```sMutex``` by ```MOZ_ASSERT(NS_IsMainThread())``` in every function call.

> You're holding onto a refcount on to the ContentParent. I don't see anywhere which is releasing the refcount. This means we'll leak every single ContentParent that we create.
> 
> I suggest you make this a nsTArray\<dom::ContentParent*\> and have the ContentParent destructor deregister itself, while holding the mutex.

Thanks for the correction. Using ```ContentParent::GetAll``` directly instead of maintaining the list should be the simplest solution.

http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/dom/ipc/ContentParent.cpp#1334

> Code is read more often that it is written. So optimize code for readability.
> 
> So don't use auto unless it improves readability, which usually it does not.
> 
> Using auto here means readers of the code need to look up the type of sSubscribers to figure out what the type of auto should be.
> 
> Please only use auto for templated iterator types.

OK.

> Constructing the object with a refcount of 1 means that it can never be destroyed as its refcount can never drops to 0. When you construct this, you put it in a static RefPtr immedaitely, and its refcount will then be 2.
> 
> If you want to create this object with a non-zero refcount, you should create a static factory method that returns an AlreadyAddrefed\<AudioNotification\> and make the constructor private.

Thanks for the corrections! I'll change it to 0 and use ```StaticRefPtr``` instead ```RefPtr```, so its ref count will be added to 1 and its ref count will be back to 0 on shutdown.

> Please add:
> 
> MOZ_COUNT_CTOR(AudioNotification)
> 
> This ensures that we don't forget to free this.

ok.

> Please add:
> 
> MOZ_COUNT_DTOR(AudioNotification)
> 
> This ensures we don't forget to free this.

ok.

> If you have that as:
> 
> const bool IsRegistered()
> 
> that means you're return a bool that can't change. Whereas have:
> 
> bool IsRegistered() const
> 
> That means that this function cannot change the underlying state of the object. That's what we want here. So make this:
> 
> bool IsRegistered() const

OK. I'll make it to a read-only function.

> Can you please use StaticRefPtr\<AudioNotification\> so that we destroy this on shutdown automatically?
> 
> I'm concerned that is someone changes how this class is used, we might accidentally leak it if we're not doing the refcounting carefully.

Sure. I'll change it on next patch.

> Does every function in this class except NotifyDefaultDeviceChanged() run on the main thread? I'd guess so. Please add assertions that these functions run on the main thread. If so, then you could remove the monitor here entirely, as sSubscribers would only be accessed on the main thread.

Yes. Thanks for the hints.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 86

2 years ago
mozreview-review-reply
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review162298

> All the functions here are running in main thread. Adding assertions(checking it's on the main thread) in every functions and remove the mutex should make it easier to read.

I was wrong here. It's not on main thread. Applying mutex instead of assertions now.

> I'll replace ```sMutex``` by ```MOZ_ASSERT(NS_IsMainThread())``` in every function call.

I was wrong here. It's not on main thread. Applying mutex instead of assertions now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 90

2 years ago
mozreview-review
Comment on attachment 8883866 [details]
Bug 1361336 - part2: Prevent AudioStream::ResetDefaultDevice() from being called before stream is started;

https://reviewboard.mozilla.org/r/154838/#review164968
Attachment #8883866 - Flags: review?(cpearce) → review+

Comment 91

2 years ago
mozreview-review
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review164972

Great, thanks!

::: dom/media/AudioNotificationReceiver.h:64
(Diff revision 7)
> + * d) There is only one ContentChild in a content process.
> + * e) All the Audiostreams are registered in the AudioNotificationReceiver.
> + * f) All the ContentParents are registered in the AudioNotificationSender.
> + */
> +
> +#include "AudioStream.h"

You could predeclare "class AudioStream" here instead of #including AudioStream.h here. That would make the build slightly faster, and simplifies the include graph.

::: dom/media/AudioNotificationReceiver.cpp:27
(Diff revision 7)
> +namespace audio {
> +
> +/*
> + * A list containing all clients subscribering the device-changed notifications.
> + */
> +static nsTArray<RefPtr<AudioStream>> sSubscribers;

You may find that the leak checker thinks you've leaked an nsTArray since the leak checker runs before destructors of static objects run. If so, you'll need to store this inside a StaticAutoPtr.

You can turn on the leak checker locally by setting the environment variable:

XPCOM_MEM_LEAK_LOG=2

::: dom/media/AudioNotificationSender.cpp:222
(Diff revision 7)
> +}
> +
> +/* static */ void
> +AudioNotificationSender::NotifyDefaultDeviceChanged()
> +{
> +  // This is running on the callback thread(from OnDefaultDeviceChanged).

Grammar correction:

s/thread (from/thread (from/
Attachment #8883868 - Flags: review?(cpearce) → review+

Comment 92

2 years ago
mozreview-review
Comment on attachment 8885591 [details]
Bug 1361336 - part6: Apply AudioNotificationSender/Receiver in ContentParent/Child;

https://reviewboard.mozilla.org/r/156440/#review164980
Attachment #8885591 - Flags: review?(cpearce) → review+

Updated

2 years ago
Duplicate of this bug: 1383036

Updated

2 years ago
Duplicate of this bug: 1243256
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 101

2 years ago
mozreview-review-reply
Comment on attachment 8883868 [details]
Bug 1361336 - part4: Create AudioNotificationSender/Receiver to pass the device-changed notification;

https://reviewboard.mozilla.org/r/154842/#review164972

> You could predeclare "class AudioStream" here instead of #including AudioStream.h here. That would make the build slightly faster, and simplifies the include graph.

Thank you so much for your guidance and wise advices.

> You may find that the leak checker thinks you've leaked an nsTArray since the leak checker runs before destructors of static objects run. If so, you'll need to store this inside a StaticAutoPtr.
> 
> You can turn on the leak checker locally by setting the environment variable:
> 
> XPCOM_MEM_LEAK_LOG=2

Thanks for the hints.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 120

2 years ago
(In reply to Chris Pearce (:cpearce) from comment #91)
> You may find that the leak checker thinks you've leaked an nsTArray since
> the leak checker runs before destructors of static objects run. If so,
> you'll need to store this inside a StaticAutoPtr.
I notice that most StaticAutoPtr should be cleared by using ClearOnShutdown[0] or by our own[1,2](mostly in nsLayoutStatics). ClearOnShutdown needs to be on the main thread[3], but AudioNotificationReceiver is called from Audiostream that is not on main thread. Therefore, it needs to use a runnable to dispatch the ClearOnShutdown task to main thread. On the other hand, adding a method AudioNotificationReceiver::Shutdown to clear the StaticAutoPtr and call it in nsLayoutStatics will be straighter solution but the code is fragmentary because it's exposed to both AudioStream and nsLayoutStatics.

Should we follow the rule even there is no memory leak detected now? If so, which approach is better?

[0] https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/media/MediaPrefs.cpp#22
[1] https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/media/CubebUtils.cpp#451
[2] https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/build/nsLayoutStatics.cpp#391
[3] https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/xpcom/base/ClearOnShutdown.cpp#25
Flags: needinfo?(cpearce)
Well what is the consequence if you don't do anything? A leak is not being reported currently. I'm surprised by that, but it's not serious as it's kind of a false positive as we'd be shutting down anyway at this stage.

However, if you don't deal with it now, and the leak checker changes in future such that it does report this as a leak, you may save someone else in future some time.

In AudioNotificationReceiver::Unregister() after removing the AudioStream from sSubscribers you could check whether sSubscribers is empty and delete it if so. That would sidestep the issue entirely, and save a little memory when audio isn't being used. That would means the logic to manage the memory is all in one place, so it would be clear what's going on.

Dispatching to the main thread to call ClearOnShutdown there is more complicated, and adding more hooks into nsLayoutStatics is not desirable, as it increases our coupling with other components.
Flags: needinfo?(cpearce)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 122

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4130e6107846
part1: A new API for AudioStream that it can reset stream to the default device; r=kinetik
https://hg.mozilla.org/integration/autoland/rev/08051e6e1af8
part2: Prevent AudioStream::ResetDefaultDevice() from being called before stream is started; r=cpearce,kinetik
https://hg.mozilla.org/integration/autoland/rev/9a0a88ff3dfb
part3: Pass audio default device-changed message via PContent; r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c3154d4481df
part4: Create AudioNotificationSender/Receiver to pass the device-changed notification; r=cpearce
https://hg.mozilla.org/integration/autoland/rev/29dce240f248
part5: Register the AudioStream into AudioNotificationReceiver when it's created; r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b0c316de0f67
part6: Apply AudioNotificationSender/Receiver in ContentParent/Child; r=cpearce
Keywords: checkin-needed
Assignee

Comment 124

2 years ago
I noticed that I forgot to update the last modification to mozreview. I'll update them in bug 1387058.
Assignee

Updated

2 years ago
Blocks: 1392930
We need this _badly_ for 56. I'm not comfortable having a big regression like this _two_ releases in a row.

bug 1392930 has just been a+ for 56, but depends on this patch set (1392930 is effectively a generalization of the mechanism introduced in 1361336).
Comment on attachment 8883865 [details]
Bug 1361336 - part1: A new API for AudioStream that it can reset stream to the default device;

This request if for patch 1 to 6 in this bug (I'm only writing this once, it applies to the patch set as a whole). This is windows only.

Approval Request Comment
[Feature/Bug causing the regression]: Stricter sandbox on Windows, which prevents receiving a particular system event in a content process, which prevents gecko from reacting to audio device changes. 

Audio Output Device hear mean any of the following:
- USB headset
- External USB sound card
- HDMI cable that transports audio
- Optical cable (SPDIF)
- Bluetooth audio devices (earpiece, bluetooth headphones, earpods, etc.)

[User impact if declined]: Multiple scenarii have to be considered:
- When playing a video and plugging in an audio device, the audio will still come out of the previous audio device (the one that was outputing sound before the new device was activated).
- When playing a video and un-plugging an audio device, the video will freeze
- When switching audio output devices in Windows's settings (right click on the speaker icon in the systray, playback devices, set default), the change will not be picked up by Firefox (i.e. the sound will still come from the previous devices).

[Is this code covered by automated tests?]: No, this is one of the only part in media where it's really hard to write tests (it requires physical access to the computer). We have plans to do that, but it requires writing a windows kernel driver.

[Has the fix been verified in Nightly?]: Yes, every person that was affected and had reported has commented back and said everything now works like before the regression.

[Needs manual test from QE? If yes, steps to reproduce]: Yes, the STR can be adapted from the [User impact if declined] section.

[List of other uplifts needed for the feature/fix]: None, this is the base patch set.

[Is the change risky?]: We haven't had any issue, this has landed some time ago. It's a bit bit.
[Why is the change risky/not risky?]: The actual code is quite straight forward.
[String changes made/needed]: None.
Attachment #8883865 - Flags: approval-mozilla-beta?
Comment on attachment 8883865 [details]
Bug 1361336 - part1: A new API for AudioStream that it can reset stream to the default device;

Sounds great to fix this serious regression from 55, let's uplift for beta 11.
Attachment #8883865 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 128

2 years ago
IIUC, Bug 1387058 needs to be uplifted as well. 
Paul, can you help uplift bug 1387:pa058 as well?
Flags: needinfo?(padenot)
Reporter

Comment 129

2 years ago
(In reply to Blake Wu [:bwu][:blakewu] from comment #128)
> IIUC, Bug 1387058 needs to be uplifted as well. 
> Paul, can you help uplift bug 1387:pa058 as well?
                                ^^^^^^^^^^ I meant bug 1387058.
Just request uplift approval on that bug and I'll do it now...
Thanks Blake, this has been taken care of.
Flags: needinfo?(padenot)
Reporter

Comment 133

2 years ago
Paul, Thank you!
I managed to reproduce the bug on an old Nightly from 2017-05-02 using Windows 10 x64. 
I retested everything on beta 56.0b11 and latest Nightly 57.0a1 using Windows 10 x64 and the bug seems to be fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.