User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.1 Safari/603.1.30 Steps to reproduce: 1. acquire a media stream from an external usb camera 2. disconnect the usb camera 3. acquire a new media stream from the same external usb camera JSfiddle for reproducing the issue: https://jsfiddle.net/6pL4vx0q/3/ Simply disconnect and reconnect your camera after clicking the "audio & video" button. Oddly enough after removing and re-granting device permissions, the stream is usable. Actual results: The stream acquired after reconnecting the camera is unusable (at least a .srcObject assignment does not longer work). Expected results: The stream acquired after reconnecting the camera should be usable, meaning srcObject assignments should work as expected and the stream is viewable.

The root issue here is that when you pull the camera out the corresponding track is supposed to end, but we don't implement that bit yet. So when you request the second gUM we wrongly believe that the capture is still live and that you requested the same device as is already captured, and we return a copy of this capture. jib, munro, feel free to take this from me if you have the time to look into this.

Munro, any chance you'd have time to look at this?

There are several ways to detect the plug out event. 1. Detect the plug out event on each platform (windows / Linux / MacOS). 2. Set a timeout for the CamerasChild::RecvDeliverFrame(). Treat timeout as a plug out event. Do you prefer any one?

1 seems like the right way to do this. Do you have an idea of how long it would take to implement?

If we can leverage the flow for ondevicechange, maybe that can be done very soon. Another problem is how to shutdown & clear all the components in this case. I am wondering if it is ok to call SourceMediaStream::EndAllTrackAndFinish() in MediaEngineRemoteVideoSource, just like we did in http://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#928

The SourceMediaStream is shared between the sources right? Then unplugging one source shouldn't stop the other track, just the track belonging to the source. I.e., `aMediaStream->EndTrack(aId);`, or like [1]. The audiocapture source should be changed accordingly too, but it's prefed off by default so it's not a biggie. Whether you need to call Finish on the SourceMediaStream I'm not sure. It'd be best to do it when all sources for that stream have been stopped, but I'd like to think that GC/shutdown code takes care of it. Should show up on try if it's necessary. For clearing other components, it's probably easiest to draw inspiration from [1] too, and similarly [2] for video. [1] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#493 [2] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#205

I have implemented a prototype on osx for camera. Please let me know if there is any concern.

Great! My only high level concern is the signaling from the MediaEngine to MediaManager that the track has ended. Instead of exposing an implementation detail of MediaManager to MediaEngine I think we should use other means. Perhaps a Pledge, a MozPromise or a WatchManager would be suitable. JW can advise on the latter two if you want.

replace callback function with Pledge

My original plan was to implement the disconnection detection for each platform and pop up the event to SourceListener. For camera, I think it is doable. However, there are some problems in microphone. Currently, cubeb doesn't support detecting a microphone session disconnection. We have two options: 1) File an issue to cubeb GitHub to request supporting this feature in cubeb. 2) As achronop's advice, we can utilize the cubeb api cubeb_register_device_collection_changed() and cubeb_enumerate_devices(). For each time we receive a collection change event, compare the device enumeration and decide which device has been removed. I worry there may be timing issues. Assuming user plugs out the device and then plugs in very quickly before cubeb_enumerate_devices() is called, then we will not be able to detect the disconnection. But this way is platform independent, which need less effort to implement. Any suggestions?

This is a draft. I only verify it on Mac. Need more time to verify it on Windows/Linux.

Comment on attachment 8885595 [details] Bug 1364038 - Call SourceListener::StopTrack when the coresponding external device is disconnected. https://reviewboard.mozilla.org/r/156452/#review161904 Nice idea! But don't we need to initialize mDeviceIDs? Things might be plugged in before Firefox starts... Some minor code reorg suggestions. ::: dom/media/MediaManager.h:337 (Diff revision 1) > static StaticRefPtr<MediaManager> sSingleton; > > media::CoatCheck<PledgeSourceSet> mOutstandingPledges; > media::CoatCheck<PledgeChar> mOutstandingCharPledges; > media::CoatCheck<PledgeVoid> mOutstandingVoidPledges; > + nsTArray<nsString> mDevicesID; Maybe mDeviceIDs? ::: dom/media/MediaManager.cpp:2074 (Diff revision 1) > RefPtr<MediaManager> self(this); > NS_DispatchToMainThread(media::NewRunnableFrom([self,this]() mutable { > MOZ_ASSERT(NS_IsMainThread()); > DeviceChangeCallback::OnDeviceChange(); > + > + RefPtr<PledgeSourceSet> p = EnumerateRawDevices(0, MediaSourceEnum::Camera, MediaSourceEnum::Microphone, false); I'm making a mental note to remove the seemingly unused aWindowId arg someday... ::: dom/media/MediaManager.cpp:2075 (Diff revision 1) > NS_DispatchToMainThread(media::NewRunnableFrom([self,this]() mutable { > MOZ_ASSERT(NS_IsMainThread()); > DeviceChangeCallback::OnDeviceChange(); > + > + RefPtr<PledgeSourceSet> p = EnumerateRawDevices(0, MediaSourceEnum::Camera, MediaSourceEnum::Microphone, false); > + p->Then([self,this](SourceSet*& aDevices) mutable { Passing this in lambda capture is forbidden now thanks to static analysis. You'll have to write: self->mDevicesID ::: dom/media/MediaManager.cpp:2088 (Diff revision 1) > + for (auto& id : mDevicesID) { > + if (!devicesID.Contains(id)) { > + removedDevicesID.AppendElement(id); I'd put the stopping code in here, and remove removedDevicesID. ::: dom/media/MediaManager.cpp:2096 (Diff revision 1) > + } > + } > + > + mDevicesID = devicesID; > + > + // Shutdown the coresponding SourceListner s/Shutdown/Stop/ s/SourceListner/SourceListener/ ::: dom/media/MediaManager.cpp:4045 (Diff revision 1) > void > +GetUserMediaWindowListener::StopRawIDs(void *aData) > +{ > + MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread"); > + nsTArray<nsString>* removedDevicesID = > + static_cast<nsTArray<nsString>*>(aData); Nit: I think it'd be cleaner to define: void GetUserMediaWindowListener::StopRawIDs(const nsTArray<nsString>& removedDevicesID) { and do the casting in the call arg. Also, let's simplify and pass in one id. Let top-side code for-loop the atypical case of > 1 instead: void GetUserMediaWindowListener::StopRawIDs(const nsString& removedDeviceID) { Should simplify this function a bit.

Comment on attachment 8885595 [details] Bug 1364038 - Call SourceListener::StopTrack when the coresponding external device is disconnected. https://reviewboard.mozilla.org/r/156452/#review162404 IlI'll let jib take this one. I'm not back until August. (and on mobile you can apparently not erase misspelled words)

Comment on attachment 8885595 [details] Bug 1364038 - Call SourceListener::StopTrack when the coresponding external device is disconnected. https://reviewboard.mozilla.org/r/156452/#review163492 Looks great! Just fix to use existing `mgr` instead of passing `self` in one place. r=me with that. ::: dom/media/MediaManager.cpp:2071 (Diff revisions 1 - 2) > } > > > void MediaManager::OnDeviceChange() { > RefPtr<MediaManager> self(this); > NS_DispatchToMainThread(media::NewRunnableFrom([self,this]() mutable { Odd, I didn't notice we were passing this in runnables in the tree still. Maybe I was wrong about static analysis...? I recall it being true or discussed at some point, or maybe it never landed? ::: dom/media/MediaManager.cpp:2704 (Diff revisions 1 - 2) > - p->Then([id, aWindowId, aOriginKey](SourceSet*& aDevices) mutable { > + p->Then([self, id, aWindowId, aOriginKey](SourceSet*& aDevices) mutable { > UniquePtr<SourceSet> devices(aDevices); // secondary result > > + self->mDeviceIDs.Clear(); > + for (auto& device : *devices) { > + nsString id; > + device->GetId(id); > + self->mDeviceIDs.AppendElement(id); > + } > + > // Only run if window is still on our active list. > RefPtr<MediaManager> mgr = MediaManager_GetInstance(); > if (!mgr) { > return NS_OK; We should follow existing pattern here and use mgr rather than pass self. I forget why, I think I was worried about delaying cleanup of MediaManager on shutdown. Either way, no need for two ways to do the same thing. ::: dom/media/MediaManager.cpp:4054 (Diff revisions 1 - 2) > for (auto& source : mActiveListeners) { > if (source->GetAudioDevice()) { > nsString id; > source->GetAudioDevice()->GetRawId(id); > - for (auto& str : *removedDevicesID) { > + if (removedDeviceID.Equals(id)) { > - if (str.Equals(id)) { > - source->StopTrack(kAudioTrack); > + source->StopTrack(kAudioTrack); > - break; > + break; In the previous patch this break broke out of the inner for-loop only, but now it breaks out of the outer one. Is that what you meant? I think that makes sense, and it was actually a bug in the previous patch where it might have called StopTrack multiple times in certain cases, just checking.

Do you want me to review the other patches?

Munro, can you confirm the ended event fires when you pull a device on a live track? Is there any way we can simulate this action, so we can test it? How do we test ondevicechange?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #17) > In the previous patch this break broke out of the inner for-loop only, but > now it breaks out of the outer one. Is that what you meant? > > I think that makes sense, and it was actually a bug in the previous patch > where it might have called StopTrack multiple times in certain cases, just > checking. Ah, I didn't do this intentionally bit just move the snippet from the original place to here. Is there any chance that multiple SourceListeners would share the same videodevice/audiodevice? If true, then we may need to remove the break. (In reply to Jan-Ivar Bruaroey [:jib] from comment #19) > Munro, can you confirm the ended event fires when you pull a device on a > live track? Is there any way we can simulate this action, so we can test it? > How do we test ondevicechange? Yes. I verified it with my MBP. We test ondevicechange with fake devicechange event. http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/media/systemservices/CamerasChild.cpp#674 If we want to simulate the removal of an external camera, we may need to use fake camera (MediaEngineDefault).

Comment on attachment 8885595 [details] Bug 1364038 - Call SourceListener::StopTrack when the coresponding external device is disconnected. https://reviewboard.mozilla.org/r/156452/#review164134 ::: dom/media/MediaManager.cpp:1737 (Diff revisions 2 - 3) > } > if ((!fakeCams && hasVideo) || (!fakeMics && hasAudio)) { > RefPtr<MediaManager> manager = MediaManager_GetInstance(); > realBackend = manager->GetBackend(aWindowId); > + // We need to listen to this event even JS didn't listen to it. > + realBackend->AddDeviceChangeCallback(manager); Without this change, you will only see the track stopped when the external cam/mic is removed if JS listen to ondevicechange event.

This patch doesn't work on Win7. SourceListener::NotifyFinished() is never called when the external camera is removed.

If I trace the code correctly, MSG is driven by AudioCallbackDriver when audio track exists. In OSX, when the external microphone, e.g., my Logitech C920, is removed, I still see AudioCallbackDriver::DataCallback_s being called, which allows us to do some cleanup job like MediaStreamGraphImpl::CloseAudioInput(). However, In Windows (7), when the external microphone is removed, AudioCallbackDriver::DataCallback_s won't be called anymore, which stocks MSG. Alex, I suppose AudioCallbackDriver::DataCallback_s() is called by libcubeb (please correct me if I am wrong). Do you know why there is such a difference between OSX and Windows?

That's correct AudioCallbackDriver is driven by cubeb in audio thread. I am not sure what do you mean that you still see the callback being called. In OSX when the device in unplugged we try to connect to the new default device and the stream will continue. The wasapi backend (windows) is expected to do the same (if there is a new default). In any case the correct way to detect a change in the status of the stream is the state change callback. Would you like to provide more context in this specific issue and what you want to achieve? Is there any mic left in the machine after you unglug your device?

(In reply to Alex Chronopoulos [:achronop] from comment #25) > I am not sure what do you mean that you still see the callback being called. > In OSX when the device in unplugged we try to connect to the new default > device and the stream will continue. The wasapi backend (windows) is > expected to do the same (if there is a new default). Yes, that explains why I still see the callback because there is a built-in mic in my MBP. > Is there any mic left in the machine after you unglug your device? There is no other mic left in my PC running Windows. > In any case the correct way to detect a change in the status of the stream > is the state change callback. Would you like to provide more context in this > specific issue and what you want to achieve? I am trying to fix a bug which blocks the feature I did in this bug. (I will create a new bug later) Steps: 1) Choose a PC or laptop without any built-in mic. 2) Plug in a usb mic or usb webcam with mic 3) Connect to https://webrtc.github.io/samples/src/content/getusermedia/audio/ and run gUM 4) Remove the usb mic during gUM streaming 5) Close Firefox Then Firefox would crash.

(In reply to Munro Mengjue Chiang [:mchiang] from comment #26) > I still see the callback I still observed the callback AudioCallbackDriver::DataCallback_s being called after removing the external webcam

I will leave this problem to be addressed in bug 1384805 and land this patch first.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0cce9f382987 Call SourceListener::StopTrack when the coresponding external device is disconnected. r=jib

Backed out for failing mda's dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html: https://hg.mozilla.org/integration/autoland/rev/4274b45cbe398ee7528a3633100387151d0cfbb1 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0cce9f38298794e32ee9a29de703ce6813381154&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118508905&repo=autoland

(In reply to Munro Mengjue Chiang [:mchiang] from comment #26) > > Is there any mic left in the machine after you unglug your device? > There is no other mic left in my PC running Windows. None? Note that the system may well consider an analog headset jack a "mic"

(In reply to Randell Jesup [:jesup] from comment #32) > (In reply to Munro Mengjue Chiang [:mchiang] from comment #26) > > > > Is there any mic left in the machine after you unglug your device? > > There is no other mic left in my PC running Windows. > > None? Note that the system may well consider an analog headset jack a "mic" I will plug in a analog headset into my PC and see if there is any input device shown.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/00af61b0dd2f Call SourceListener::StopTrack when the coresponding external device is disconnected. r=jib