Closed Bug 1392930 Opened 7 years ago Closed 7 years ago

Audio output device cannot be changed when using WebRTC

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

x86_64
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- verified
firefox58 --- verified

People

(Reporter: chunmin, Assigned: chunmin)

References

Details

Attachments

(4 files)

Firefox version: 56.0b3 (Firefox Developer Edition)

Steps to reproduce:
1. Open https://webrtc.github.io/samples/src/content/getusermedia/audio/
2. Check the audio works in current default microphone and speaker
3. Switch the default audio output device

Expected result:
Audio should come out from the new default output device

Actual result:
Audio comes out from the original default output device
I believe the root cause is same as 1361336. We should reuse the AudioNotificationReceiver/Sender in GraphDriver or MediaStreamGraph to pass the default-device-changed event.
Depends on: 1361336
I thought the switch will happen automatically inside cubeb. cubeb on OSX does that. Is the behavior different on Windows?
Rank: 15
Flags: needinfo?(padenot)
Priority: -- → P1
Yes, sandboxing broke the automatic switch. Our sandbox level is higher on Windows and the event that tells us that the default device changed is never received.
Flags: needinfo?(padenot)
Can you try to repro with sandbox disabled?
Flags: needinfo?(cchang)
(In reply to Alex Chronopoulos [:achronop] from comment #4)
> Can you try to repro with sandbox disabled?

If I change the security.sandbox.content.level to 0 or disable e10s, then the device changing works well as usual. It's same in bug 1361336 comment 5, bug 1361336 comment 9, bug 1361336 comment 10

You can try opening a stream on https://janus.conf.meetecho.com/streamingtest.html and switch the output device to test it.
Flags: needinfo?(cchang)
Comment on attachment 8902172 [details]
Bug 1392930 - part 3: Make AudioCallbackDriver inherit from DeviceChangeListener;

https://reviewboard.mozilla.org/r/173632/#review178950

Thanks!
Attachment #8902172 - Flags: review?(padenot) → review+
Comment on attachment 8902170 [details]
Bug 1392930 - part 1: Replace AudioStream by DeviceChangeListener in AudioNotificationReceiver;

https://reviewboard.mozilla.org/r/173628/#review178956

::: dom/media/AudioNotificationReceiver.h:70
(Diff revision 1)
>  namespace audio {
>  
> +// The base class that provides a ResetDefaultDevice interface that
> +// will be called in AudioNotificationReceiver::NotifyDefaultDeviceChanged
> +// when it receives device-changed notification from the chrome process.
> +class StreamListener

StreamListener is a too general name to convey what it actually does. Please call it 'DeviceChangeListener'.
Attachment #8902170 - Flags: review?(jwwang) → review+
Comment on attachment 8902171 [details]
Bug 1392930 - part 2: Make AudioStream inherit from DeviceChangeListener;

https://reviewboard.mozilla.org/r/173630/#review178958

::: dom/media/AudioStream.h:20
(Diff revision 1)
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/UniquePtr.h"
>  #include "CubebUtils.h"
>  #include "soundtouch/SoundTouchFactory.h"
>  
> +#if defined(XP_WIN)

Why do we need this ifdef?
Comment on attachment 8902171 [details]
Bug 1392930 - part 2: Make AudioStream inherit from DeviceChangeListener;

https://reviewboard.mozilla.org/r/173630/#review178958

> Why do we need this ifdef?

AudioNotificationReceiver is only used on Windows platform since the sandbox only blocks the device-changed notification in Windows on content side for now. The device-changing will be handled in Cubeb on other platforms.
https://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/dom/media/moz.build#269
Comment on attachment 8902170 [details]
Bug 1392930 - part 1: Replace AudioStream by DeviceChangeListener in AudioNotificationReceiver;

https://reviewboard.mozilla.org/r/173628/#review178956

> StreamListener is a too general name to convey what it actually does. Please call it 'DeviceChangeListener'.

Sure.
Comment on attachment 8902171 [details]
Bug 1392930 - part 2: Make AudioStream inherit from DeviceChangeListener;

https://reviewboard.mozilla.org/r/173630/#review178986
Attachment #8902171 - Flags: review?(jwwang) → review+
Blocks: 1384805
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/131c054cb9f8
part 1: Replace AudioStream by DeviceChangeListener in AudioNotificationReceiver; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ef53c534fcc2
part 2: Make AudioStream inherit from DeviceChangeListener; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/46e178724afe
part 3: Make AudioCallbackDriver inherit from DeviceChangeListener; r=padenot
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/674670bfea13
part 1: Replace AudioStream by DeviceChangeListener in AudioNotificationReceiver; r=jwwang
https://hg.mozilla.org/mozilla-central/rev/02bf9496d1c8
part 2: Make AudioStream inherit from DeviceChangeListener; r=jwwang
https://hg.mozilla.org/mozilla-central/rev/89d020630629
part 3: Make AudioCallbackDriver inherit from DeviceChangeListener; r=padenot
will the modification merged into firefox 56?
Chun-Min, can you request approval for beta 56 ?
Flags: needinfo?(cchang)
Comment on attachment 8902172 [details]
Bug 1392930 - part 3: Make AudioCallbackDriver inherit from DeviceChangeListener;

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: When running WebRTC on Firefox Windows, the audio devices can not be changed.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes. I tested it manually and some users have confirmed it's fixed on FF57 (bug 1346676 comment 13)
[Needs manual test from QE? If yes, steps to reproduce]: Yes. STR is in comment 0.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The patch is simple and users have confirmed it's fixed.
[String changes made/needed]: None.
Flags: needinfo?(cchang)
Attachment #8902172 - Flags: approval-mozilla-beta?
(In reply to Chun-Min Chang[:chunmin] from comment #26)
> [User impact if declined]: When running WebRTC on Firefox Windows, the audio
> devices can not be changed.

I'd like to elaborate on this: this will break Web Audio API or WebRTC (media will freeze) in the event of an audio device change (physically plug/unplug, device error, etc.). This regression is caused by making the sandbox stricter on Windows, and we ca't really ship with a regression like that.

A number of people have verified the fix, there has been a number of dupes already.
Comment on attachment 8902172 [details]
Bug 1392930 - part 3: Make AudioCallbackDriver inherit from DeviceChangeListener;

Fix a WebRTC issue and was verified. Beta56+.
Attachment #8902172 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
the issue reproduced in the firefox nightly 57.0a1(2017-09-11).    

it works fine in nightly version 57.0a1(2017-09-07)
you can reproduce the issue using the demo: https://webrtc.github.io/samples/src/content/devices/input-output/
select the micphone and camera , and then plugout the micphone in use, the video frame will be paused.
firefox nightly 57.0a1(2017-09-07)  :  Pass
firefox nightly 57.0a1(2017-09-11) : fail
firefox nightly 57.0a1(2017-09-12)  :  Pass
firefox nightly 57.0a1(2017-09-13) : fail
xpeng,
Thanks for your comments. 
Can you help use mozregression[1] to the regression?

[1]http://mozilla.github.io/mozregression/
Flags: needinfo?(xpeng)
(In reply to Blake Wu [:bwu][:blakewu] from comment #32)
> xpeng,
> Thanks for your comments. 
> Can you help use mozregression[1] to the regression?
to find the regression :-)
(In reply to Blake Wu [:bwu][:blakewu] from comment #33)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #32)
> > xpeng,
> > Thanks for your comments. 
> > Can you help use mozregression[1] to the regression?
> to find the regression :-)

1. I hava download the mozregression-gui and what should i do? 
2. the issue was reproduced in the firefox 56.0b12 beta .
Flags: needinfo?(xpeng)
(In reply to xpeng from comment #34)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #33)
> > (In reply to Blake Wu [:bwu][:blakewu] from comment #32)
> > > xpeng,
> > > Thanks for your comments. 
> > > Can you help use mozregression[1] to the regression?
> > to find the regression :-)
> 
> 1. I hava download the mozregression-gui and what should i do? 
Just follow the instructions in http://mozilla.github.io/mozregression/.
Cornel,
Per comment 31 and comment 34, may we have your help to double check if this is really fixed or not?
Flags: needinfo?(cornel.ionce)
I managed to reproduce the bug on Windows 10 x32 and Windows 10 x64 using an old version of Nightly (2017-08-23).
I retested everything on the same platforms using Beta 56.0b12 and latest Nightly, but I couldn't reproduce the bug anymore. As soon as I switched the default audio output device, the sound was played from the new default device.
the issue what i reproduced was just like the bug : https://bugzilla.mozilla.org/show_bug.cgi?id=1346676  which was marked as a duplicate issue. 
    
Step 1: use the demo to select a usb device (micphone) and a camera device , the camera display normally 
https://webrtc.github.io/samples/src/content/devices/input-output/
Step 2: unplug the usb device(micphone)

Result : the camera is paused, the image do not change anymore.
I retested everything on Latest Nightly using Windows 10 x32, Windows 10 x64, Mac OS x10.11 and Ubuntu 16.04 x 64 and the bug is partially not reproducing. When you pull out the microphone the video freezes for a short moment then it continues to play. The exception being Ubuntu, where the transaction is really smooth.

On the other hand, if you unplug the webcam, the sound is not working anymore. (I checked so that the microphone used wasn't the one from the webcam.) This issue appears on all platforms and is not reproducing on Chrome. Is that something that can be related to this bug or is it a different issue?
(In reply to Oana Botisan from comment #39)
> I retested everything on Latest Nightly using Windows 10 x32, Windows 10
> x64, Mac OS x10.11 and Ubuntu 16.04 x 64 and the bug is partially not
> reproducing. When you pull out the microphone the video freezes for a short
> moment then it continues to play. The exception being Ubuntu, where the
> transaction is really smooth.

Thanks. The short freeze is expected for now, unfortunately.

> On the other hand, if you unplug the webcam, the sound is not working
> anymore. (I checked so that the microphone used wasn't the one from the
> webcam.) This issue appears on all platforms and is not reproducing on
> Chrome. Is that something that can be related to this bug or is it a
> different issue?

This is a different one. jib, can you investigate?
Flags: needinfo?(jib)
About the second problem I see in the debugger that we get a close audio message from main thread in the middle of the iteration:

  * frame #0: 0x0000000105aa89d4 XUL`mozilla::GraphDriver::SwitchAtNextIteration(this=0x00000001334d9980, aNextDriver=0x00000001274512f0) + 20 at GraphDriver.cpp:68
    frame #1: 0x0000000105c5402c XUL`mozilla::MediaStreamGraphImpl::CloseAudioInputImpl(this=0x000000012783b800, aListener=0x000000013cd05b80) + 604 at MediaStreamGraph.cpp:1071
    frame #2: 0x0000000105c76d9d XUL`mozilla::MediaStreamGraphImpl::CloseAudioInput(this=0x00000001245244e0)::Message::Run() + 45 at MediaStreamGraph.cpp:1100
    frame #3: 0x0000000105c5578a XUL`mozilla::MediaStreamGraphImpl::RunMessagesInQueue(this=0x000000012783b800) + 170 at MediaStreamGraph.cpp:1281
    frame #4: 0x0000000105c564f0 XUL`mozilla::MediaStreamGraphImpl::OneIteration(this=0x000000012783b800, aStateEnd=381056) + 48 at MediaStreamGraph.cpp:1451
    frame #5: 0x0000000105aad60b XUL`mozilla::AudioCallbackDriver::DataCallback(this=0x00000001334d9980, aInputBuffer=0x000000013112a000, aOutputBuffer=0x000000012bda7000, aFrames=512) + 1099 at GraphDriver.cpp:1028
    frame #6: 0x0000000105aac7b4 XUL`mozilla::AudioCallbackDriver::DataCallback_s(aStream=0x0000000132061400, aUser=0x00000001334d9980, aInputBuffer=0x000000013112a000, aOutputBuffer=0x000000012bda7000, aFrames=512) + 68 at GraphDriver.cpp:897
    frame #7: 0x0000000107ca968e XUL`passthrough_resampler<float>::fill(this=0x0000000125f5f640, input_buffer=0x0000000131b67000, input_frames_count=0x0000700003fa97e8, output_buffer=0x000000012bda7000, output_frames=512) + 462 at cubeb_resampler.cpp:69
    frame #8: 0x0000000107ca5174 XUL`::cubeb_resampler_fill(resampler=0x0000000125f5f640, input_buffer=0x0000000131b67000, input_frames_count=0x0000700003fa97e8, output_buffer=0x000000012bda7000, output_frames_needed=512) + 84 at cubeb_resampler.cpp:300
    frame #9: 0x0000000107c9c891 XUL`audiounit_output_callback(user_ptr=0x0000000132061400, (null)=0x0000700003fa9c78, tstamp=0x0000700003fa9c80, bus=0, output_frames=512, outBufferList=0x0000000127127e60) + 2385 at cubeb_audiounit.cpp:492
Clearing my ni? since Oana already covered the verification.
Flags: needinfo?(cornel.ionce)
I filed bug 1402377 about 40.
Flags: needinfo?(jib)
I retested everything just to be sure on the latest Nightly using Windows 10x64, Ubuntu 16.04x64 and Mac OS 10.11 but the bug is not reproducing anymore. 

Based on this result and the ones from comment 39 and comment 43, I think that this bug is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.