Closed Bug 1402377 Opened 7 years ago Closed 7 years ago

Mic goes silent after unplugging (different device) camera (regression)

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Platform:
x86_64
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 + verified
firefox58 + verified

People

(Reporter: jib, Assigned: mchiang)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Bug 1402377 - Stop the corresponding track instead of the whole stream when an external device is plugged out.
59 bytes, text/x-review-board-request
jib
: review+
pehrsons
: review+
ritu
: approval-mozilla-beta+
Details 
+++ This bug was initially created as a clone of Bug #1392930 comment 40 +++

Steps to reproduce (OSX):
1. On a machine with internal mic, attach a USB camera (I used Logitech c920)
2. Open https://jsfiddle.net/jib1/pz5pynyf/
3. In the permission prompt, I pick "HD Pro Webcam C920" and "External microphone"
   (which is the builtin in OSX because I have the headset plugged in)
4. Accept
5. Observe video comes from USB cam, and audio comes from headset mic (tap it) 
6. Unplug USB camera.

Expected result:
  Mic audio continues

Actual result:
  Mic audio goes silent. A close audio message, as seen in bug 1392930 comment 41.
The audio stops because it is requested to do so on camera unplug:

 * frame #0: 0x0000000106046104 XUL`mozilla::AudioInputCubeb::StopRecording(this=0x00000001375ccba0, aStream=0x000000013a361830) + 20 at MediaEngineWebRTC.h:334
    frame #1: 0x0000000106058bce XUL`mozilla::MediaEngineWebRTCMicrophoneSource::Stop(this=0x0000000133809e00, aSource=0x000000013a361830, aID=2) + 654 at MediaEngineWebRTCAudio.cpp:570
    frame #2: 0x0000000105c45fed XUL`mozilla::SourceListener::StopTrack(this=0x00000001271346e8)::$_12::operator()() const + 93 at MediaManager.cpp:3692
    frame #3: 0x0000000105c45e69 XUL`mozilla::media::LambdaTask<mozilla::SourceListener::StopTrack(int)::$_12>::Run(this=0x00000001271346c0) + 25 at MediaTaskUtils.h:37
    frame #4: 0x0000000101eba8d6 XUL`nsThread::ProcessNextEvent(this=0x000000018731d270, aMayWait=true, aResult=0x0000700001e589ee) + 2358 at nsThread.cpp:1039
    frame #5: 0x0000000101ebe4ac XUL`NS_ProcessNextEvent(aThread=0x000000018731d270, aMayWait=true) + 140 at nsThreadUtils.cpp:521
    frame #6: 0x000000010294737f XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000187526f00, aDelegate=0x0000700001e58d58) + 1039 at MessagePump.cpp:368
    frame #7: 0x0000000102823165 XUL`MessageLoop::RunInternal(this=0x0000700001e58d58) + 117 at message_loop.cc:326
    frame #8: 0x00000001028230c5 XUL`MessageLoop::RunHandler(this=0x0000700001e58d58) + 21 at message_loop.cc:319
    frame #9: 0x000000010282306d XUL`MessageLoop::Run(this=0x0000700001e58d58) + 45 at message_loop.cc:299
    frame #10: 0x0000000102872134 XUL`base::Thread::ThreadMain(this=0x0000000187526ec0) + 1172 at thread.cc:181
    frame #11: 0x000000010282f21e XUL`ThreadFunc(closure=0x0000000187526ec0) + 30 at platform_thread_posix.cc:38
    frame #12: 0x00007fff997d399d libsystem_pthread.dylib`_pthread_body + 131
    frame #13: 0x00007fff997d391a libsystem_pthread.dylib`_pthread_start + 168
    frame #14: 0x00007fff997d1351 libsystem_pthread.dylib`thread_start + 13
This is where we get the stop from main thread:

* thread #1: tid = 0x1736c7, 0x0000000105c055fc XUL`mozilla::SourceListener::Stop(this=0x00000001323f0dd0) + 252 at MediaManager.cpp:3616, queue = 'com.apple.main-thread', stop reason = breakpoint 4.1
  * frame #0: 0x0000000105c055fc XUL`mozilla::SourceListener::Stop(this=0x00000001323f0dd0) + 252 at MediaManager.cpp:3616
    frame #1: 0x0000000105c08a54 XUL`mozilla::GetUserMediaWindowListener::StopRawID(this=0x0000000131e7f240, removedDeviceID=0x0000000134162a08) + 468 at MediaManager.cpp:4077
    frame #2: 0x0000000105c37218 XUL`mozilla::StopRawIDCallback(aThis=0x0000000187552800, aWindowID=28, aListener=0x0000000131e7f240, aData=0x0000000134162a08) + 72 at MediaManager.cpp:2060
    frame #3: 0x0000000105c01225 XUL`mozilla::MediaManager::IterateWindowListeners(this=0x0000000187552800, aWindow=0x000000013388f820, aCallback=(XUL`mozilla::StopRawIDCallback(mozilla::MediaManager*, unsigned long long, mozilla::GetUserMediaWindowListener*, void*) at MediaManager.cpp:2054), aData=0x0000000134162a08)(mozilla::MediaManager*, unsigned long long, mozilla::GetUserMediaWindowListener*, void*), void*) + 101 at MediaManager.cpp:3453
    frame #4: 0x0000000105c3714f XUL`mozilla::MediaManager::OnDeviceChange(this=0x0000000125c5e9a8, aDevices=0x0000000127892f98)::$_3::operator()()::'lambda'(nsTArray<RefPtr<mozilla::MediaDevice> >*&)::operator()(nsTArray<RefPtr<mozilla::MediaDevice> >*&) + 735 at MediaManager.cpp:2098
    frame #5: 0x0000000105c36d3c XUL`void mozilla::media::Pledge<nsTArray<RefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::OnDeviceChange(this=0x0000000125c5e9a0, result=0x0000000127892f98)::$_3::operator()()::'lambda'(nsTArray<RefPtr<mozilla::MediaDevice> >*&), mozilla::MediaManager::OnDeviceChange()::$_3::operator()()::'lambda'(mozilla::dom::MediaStreamError*&)>(mozilla::MediaManager::OnDeviceChange()::$_3::operator()()::'lambda'(nsTArray<RefPtr<mozilla::MediaDevice> >*&)&&, mozilla::MediaManager::OnDeviceChange()::$_3::operator()()::'lambda'(mozilla::dom::MediaStreamError*&)&&)::Functors::Succeed(nsTArray<RefPtr<mozilla::MediaDevice> >*&) + 44 at MediaUtils.h:88
    frame #6: 0x0000000105c34f4e XUL`mozilla::media::Pledge<nsTArray<RefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Resolve(this=0x0000000127892f80) + 206 at MediaUtils.h:133
    frame #7: 0x0000000105c348c7 XUL`mozilla::media::Pledge<nsTArray<RefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Resolve(this=0x0000000127892f80, aValue=0x00007fff5fbfc728) + 39 at MediaUtils.h:112
    frame #8: 0x0000000105c34710 XUL`mozilla::MediaManager::EnumerateRawDevices(this=0x0000000131cda668)::$_1::operator()()::'lambda'()::operator()() + 224 at MediaManager.cpp:1766
    frame #9: 0x0000000105c345d9 XUL`mozilla::media::LambdaRunnable<mozilla::MediaManager::EnumerateRawDevices(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool)::$_1::operator()()::'lambda'()>::Run(this=0x0000000131cda640) + 25 at MediaUtils.h:199
    frame #10: 0x0000000101eba8d6 XUL`nsThread::ProcessNextEvent(this=0x000000010063aed0, aMayWait=false, aResult=0x00007fff5fbfcec3) + 2358 at nsThread.cpp:1039
    frame #11: 0x0000000101eb805c XUL`NS_ProcessPendingEvents(aThread=0x000000010063aed0, aTimeout=10) + 140 at nsThreadUtils.cpp:463
    frame #12: 0x0000000106a60b0e XUL`nsBaseAppShell::NativeEventCallback(this=0x0000000125f4e620) + 190 at nsBaseAppShell.cpp:99
    frame #13: 0x0000000106afd0b7 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x0000000125f4e620) + 503 at nsAppShell.mm:436
    frame #14: 0x00007fff8e31f7e1 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #15: 0x00007fff8e2fef1c CoreFoundation`__CFRunLoopDoSources0 + 556
    frame #16: 0x00007fff8e2fe43f CoreFoundation`__CFRunLoopRun + 927
    frame #17: 0x00007fff8e2fde38 CoreFoundation`CFRunLoopRunSpecific + 296
    frame #18: 0x00007fff8dfaf935 HIToolbox`RunCurrentEventLoopInMode + 235
    frame #19: 0x00007fff8dfaf76f HIToolbox`ReceiveNextEventCommon + 432
    frame #20: 0x00007fff8dfaf5af HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #21: 0x00007fff8ae66df6 AppKit`_DPSNextEvent + 1067
    frame #22: 0x00007fff8ae66226 AppKit`-[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
    frame #23: 0x0000000106afba94 XUL`::-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x00000001006d27d0, _cmd="nextEventMatchingMask:untilDate:inMode:dequeue:", mask=18446744073709551615, expiration=4001-01-01 00:00:00 UTC, mode="kCFRunLoopDefaultMode", flag=YES) + 116 at nsAppShell.mm:158
    frame #24: 0x00007fff8ae5ad80 AppKit`-[NSApplication run] + 682
    frame #25: 0x0000000106afdcd7 XUL`nsAppShell::Run(this=0x0000000125f4e620) + 215 at nsAppShell.mm:715
    frame #26: 0x0000000109cda38b XUL`nsAppStartup::Run(this=0x0000000125c26d30) + 155 at nsAppStartup.cpp:288
    frame #27: 0x0000000109e39c50 XUL`XREMain::XRE_mainRun(this=0x00007fff5fbff208) + 6784 at nsAppRunner.cpp:4701
    frame #28: 0x0000000109e3b214 XUL`XREMain::XRE_main(this=0x00007fff5fbff208, argc=5, argv=0x00007fff5fbff890, aConfig=0x00007fff5fbff3d8) + 3076 at nsAppRunner.cpp:4865
    frame #29: 0x0000000109e3b98c XUL`XRE_main(argc=5, argv=0x00007fff5fbff890, aConfig=0x00007fff5fbff3d8) + 92 at nsAppRunner.cpp:4960
    frame #30: 0x0000000109e4f0f7 XUL`mozilla::BootstrapImpl::XRE_main(this=0x00000001006800d0, argc=5, argv=0x00007fff5fbff890, aConfig=0x00007fff5fbff3d8) + 39 at Bootstrap.cpp:45
    frame #31: 0x0000000100001086 firefox`do_main(argc=5, argv=0x00007fff5fbff890, envp=0x00007fff5fbff8c0) + 758 at nsBrowserApp.cpp:236
    frame #32: 0x0000000100000b61 firefox`main(argc=5, argv=0x00007fff5fbff890, envp=0x00007fff5fbff8c0) + 161 at nsBrowserApp.cpp:309
    frame #33: 0x0000000100000a74 firefox`start + 52
The problem is here: http://searchfox.org/mozilla-central/source/dom/media/MediaManager.cpp#2098

By doing that we stop video and audio. Audio flow should continue since the audio device is not the unplugged device
Flags: needinfo?(mchiang)
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

28:00.65 INFO: Narrowed inbound regression window from [f1c1543f, 00af61b0] (4 revisions) to [7cf63ef9, 00af61b0] (2 revisions) (~1 steps left)
28:00.66 INFO: No more inbound revisions, bisection finished.
28:00.66 INFO: Last good revision: 7cf63ef9ba7bc873fe69705ce98a7d332ba96f15
28:00.66 INFO: First bad revision: 00af61b0dd2ff664ecc4b23d668219505930ee92
28:00.66 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7cf63ef9ba7bc873fe69705ce98a7d332ba96f15&tochange=00af61b0dd2ff664ecc4b23d668219505930ee92

Looks like Bug 1364038 is stopping the wrong (audio) track.
Assignee: achronop → mchiang
Blocks: 1364038
Has Regression Range: --- → yes
Has STR: --- → yes
Rank: 10
status-firefox55: --- → unaffected
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
status-thunderbird_esr52: --- → unaffected
tracking-firefox57: --- → ?
tracking-firefox58: --- → ?
Keywords: regression
Priority: P1 → P2
Summary: Mic goes silent after unplugging (different device) camera → Mic goes silent after unplugging (different device) camera (regression)
Priority: P2 → P1
Flags: needinfo?(mchiang)
Comment on attachment 8912487 [details]
Bug 1402377 - Stop the corresponding track instead of the whole stream when an external device is plugged out.

https://reviewboard.mozilla.org/r/183806/#review189362

Lgtm. Would love to have Andreas take a look as well, in case there are any unforseen consequences of calling the EndTrack() function (like hopefully the source is still stopped if this is the last or lone track).
Attachment #8912487 - Flags: review?(jib) → review+
Attachment #8912487 - Flags: review?(apehrson)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #8)
> Lgtm. Would love to have Andreas take a look as well, in case there are any
> unforseen consequences of calling the EndTrack() function (like hopefully
> the source is still stopped if this is the last or lone track).
The following is the manual test I tried before submitting this patch.

1. Connect a Logitec C920 webcam and a Logitec H340 headset (with mic) to my MBP.
2. Run gUM; choose C920 camera as the video source and H340 mic as the audio source
3. Remove C920 and connect again; ensure audio stream is still running; run gUM again and ensure both streams are running
4. Remove H340 and connect again; ensure video stream is still running; run gUM again and ensure both streams are running
5. Remove both C920 and H340; ensure SourceListner::Stop() is called
Comment on attachment 8912487 [details]
Bug 1402377 - Stop the corresponding track instead of the whole stream when an external device is plugged out.

https://reviewboard.mozilla.org/r/183806/#review189654

::: dom/media/MediaManager.cpp:4054
(Diff revision 2)
> -        source->Stop();
> +        SourceMediaStream* stream = source->GetSourceStream();
> +        if (stream) {
> +          stream->EndTrack(kAudioTrack);
> +        }

This needs to be `source->StopTrack(kAudioTrack);` so the SourceListener can keep its internal state up to date, and notify chrome accordingly.

If that path breaks (we used to only call it ourselves I think, so when hit, the device would always be active), please fix that. Having a single path is important and something I worked hard to achieve.

::: dom/media/MediaManager.cpp:4064
(Diff revision 2)
> -        source->Stop();
> +        SourceMediaStream* stream = source->GetSourceStream();
> +        if (stream) {
> +          stream->EndTrack(kVideoTrack);
> +        }

And this `source->StopTrack(kVideoTrack);`.
Attachment #8912487 - Flags: review?(apehrson) → review-
(In reply to Munro Mengjue Chiang [:mchiang] from comment #9)
> (In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #8)
> > Lgtm. Would love to have Andreas take a look as well, in case there are any
> > unforseen consequences of calling the EndTrack() function (like hopefully
> > the source is still stopped if this is the last or lone track).
> The following is the manual test I tried before submitting this patch.
> 
> 1. Connect a Logitec C920 webcam and a Logitec H340 headset (with mic) to my
> MBP.
> 2. Run gUM; choose C920 camera as the video source and H340 mic as the audio
> source
> 3. Remove C920 and connect again; ensure audio stream is still running; run
> gUM again and ensure both streams are running
> 4. Remove H340 and connect again; ensure video stream is still running; run
> gUM again and ensure both streams are running
> 5. Remove both C920 and H340; ensure SourceListner::Stop() is called

I'd like to make some changes, here it is in full:
1. Connect a Logitec C920 webcam and a Logitec H340 headset (with mic) to my MBP.
2. Run gUM; choose C920 camera as the video source and H340 mic as the audio source
3. Remove C920; ensure chrome reflects us now being audio-only; ensure audio stream is still running
4. Connect C920 again; ensure chrome reflects us still being audio-only; ensure audio stream is still running; run gUM again and ensure both streams are running
5. Remove H340; ensure chrome reflects us now being video-only; ensure video stream is still running
6. Connect H340 again; ensure chrome reflects us still being video-only; ensure audio stream is still running; run gUM again and ensure both streams are running
7. Remove both C920 and H340; ensure SourceListener::Stop() is called; ensure chrome reflects capture having stopped
Attachment #8913543 - Attachment is obsolete: true
(In reply to Andreas Pehrson [:pehrsons] from comment #12)
> I'd like to make some changes, here it is in full:
> 1. Connect a Logitec C920 webcam and a Logitec H340 headset (with mic) to my
> MBP.
> 2. Run gUM; choose C920 camera as the video source and H340 mic as the audio
> source
> 3. Remove C920; ensure chrome reflects us now being audio-only; ensure audio
> stream is still running
> 4. Connect C920 again; ensure chrome reflects us still being audio-only;
> ensure audio stream is still running; run gUM again and ensure both streams
> are running
> 5. Remove H340; ensure chrome reflects us now being video-only; ensure video
> stream is still running
> 6. Connect H340 again; ensure chrome reflects us still being video-only;
> ensure audio stream is still running; run gUM again and ensure both streams
> are running
> 7. Remove both C920 and H340; ensure SourceListener::Stop() is called;
> ensure chrome reflects capture having stopped

I have verfied my latest patch with ths manual test items.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61487a834eda44d0711da6d83a48ef51875d9faa
Comment on attachment 8912487 [details]
Bug 1402377 - Stop the corresponding track instead of the whole stream when an external device is plugged out.

https://reviewboard.mozilla.org/r/183806/#review190040

Thanks!
Attachment #8912487 - Flags: review?(apehrson) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/140805019de3
Stop the corresponding track instead of the whole stream when an external device is plugged out. r=jib,pehrsons
https://hg.mozilla.org/mozilla-central/rev/140805019de3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi Munro, should we consider uplifting this fix to Beta57?
tracking-firefox57: ? → +
tracking-firefox58: ? → +
Flags: needinfo?(mchiang)
Comment on attachment 8912487 [details]
Bug 1402377 - Stop the corresponding track instead of the whole stream when an external device is plugged out.

Approval Request Comment
[Feature/Bug causing the regression]:1364038
[User impact if declined]:A MediaStreamTrack maybe stopped even its corresponding hardware device is not disconnected.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Good to have but not mandatory. We have performed manual test as comment 12
[List of other uplifts needed for the feature/fix]: n\a
[Is the change risky?]: no
[Why is the change risky/not risky?]: It only impact the behavior when user plug out external webcam while using gUM api.
[String changes made/needed]: no
Flags: needinfo?(mchiang)
Attachment #8912487 - Flags: approval-mozilla-beta?
I managed to reproduce this bug on beta 57.0b5 using Windows 10x64, Ubuntu 16.04x64 and Mac OS 10.11. The moment you unplug the webcam the microphone goes silent.

I retested everything using the latest Nightly (2017-10-02) on the same platforms and the bug seems to be fixed. You can still hear the sound after you unplug the webcam.
Comment on attachment 8912487 [details]
Bug 1402377 - Stop the corresponding track instead of the whole stream when an external device is plugged out.

Fix was verified on Nightly, Beta57+
Attachment #8912487 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I retested the bug using beta 57.0b12 and latest NIghtly 58 on macOS 10.13 and the bug is not reproducing anymore.
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: