Closed Bug 1152401 Opened 11 years ago Closed 7 years ago

enumerateDevices() should enumerate audio output devices (feature behind pref)

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jib, Assigned: achronop)

References

Details

(Keywords: dev-doc-complete)

Attachments

(10 files)

Bug 1152401 - Create MediaSinkEnum for speakers.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Implement enumeration of speaker devices.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Use mKind member of MediaDevice in MediaDeviceInfo to avoid string comparisons.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Create pref to enable enumeration of output devices.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Rename SourceSet to MediaDeviceSet to help readability.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1152401 - Augment AudioDeviceInfo with a cubeb device id.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1152401 - Expect the new MediaDevice types in GeckoView.
59 bytes, text/x-review-board-request
jchen
: review+
Details
Currently, mediaDevices.enumerateDevices enumerate our input devices only.
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Blocks: 1213399
Still bug existing, any update or plan to fix this?
Indeed, it would be great to be able to control the output device from the app (ex: different ring-device for softphone apps).
That control surface would be bug 1264375.
See Also: → 1264375
This is a critical omission for our application to be deployed in Firefox. We need to be able to control which audio output attaches to which WebRTC stream (ringing on one, handset on another, 2nd handset on another, etc).
This is something we're also waiting for. To add some extra motivation, we're offering a bounty of US$500 each for this and issue 934425 (setSinkId). To claim please contact thomas (at) inqualitymedia.com.
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
OS: Mac OS X → All
Hardware: x86 → All
I have implemented this bit. For testing I am using https://jsfiddle.net/jib1/LbtxeLvw/ (Enumerate Devices button). The output I am getting is: 9 devices. videoinput: id=ivah4X39NSDVbT1jao7AOmKw7xKbm3aim7BDeC+1+Ig= group= videoinput: id=ivah4X39NSDVbT1jao7AOmKw7xKbm3aim7BDeC+1+Ig= group= audioinput: id=hkPFcteSf0DyaX7AHkhbft4v2QCs2I4ROfrl7sJqH0I= group= audioinput: id=6ktR25G6vS2eo/VYtL/slKypENMAfDEfWalNCCSp0aM= group= audioinput: id=sAWB/qr2rJa66YooEJu09/uoMKVDzacv5UyZ1/wFvzE= group= audioinput: id=TSuViUSIP9LAlWUPxZZ8GqW2YnXyjbL9aJ6tjQY+RZU= group= audioinput: id=FNZwpeU0ZeLF7x1ZaNGwEQCLLeCx4gu5VoGlP+L5bi0= group= audiooutput: id=LbSL+WdechR2aZ8HMcBsRg2L3kn/15hkZpyB8V6GK5k= group= audiooutput: id=TSuViUSIP9LAlWUPxZZ8GqW2YnXyjbL9aJ6tjQY+RZU= group= One thing to be fixed is the deviceId between input and output devices with the same name. Right now construction of the deviceId is base on the Friendly Name of the device. In the list above entries 6 and 9 have the same id.
Assignee: nobody → achronop
Rank: 35 → 21
Priority: P4 → P3
Rank: 21 → 19
Priority: P3 → P2
Comment on attachment 8984167 [details] Bug 1152401 - Create MediaSinkEnum for speakers. https://reviewboard.mozilla.org/r/249962/#review256346 Speaker enumeration was very much an afterthought in the getUserMedia spec [1], but I think we should resist treating speakers as a sibling of input devices in our implementation. More on this in the other patches. [1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevicekind ::: dom/webidl/MediaStreamTrack.webidl:24 (Diff revision 1) > enum MediaSourceEnum { > "camera", > "screen", > "application", > "window", > "browser", > "microphone", > + "speaker", I think adding "speaker" as a MediaSourceEnum confuses things. A speaker is not a "source".
Comment on attachment 8984167 [details] Bug 1152401 - Create MediaSinkEnum for speakers. https://reviewboard.mozilla.org/r/249962/#review256362
Attachment #8984167 - Flags: review?(jib) → review-
Comment on attachment 8984169 [details] Bug 1152401 - Implement enumeration of speaker devices. https://reviewboard.mozilla.org/r/249966/#review256366 ::: dom/media/webrtc/MediaEngineWebRTC.cpp:308 (Diff revision 1) > aSources->AppendElement(aSource); > } > } > } > > +class MediaEngineWebRTCSpeakerSource : public MediaEngineSource { I don't think speakers should be a MediaEngineSource (it's not a source). Specifically, I don't see why speakers should have to implement the MediaEngineSourceInterface interface [1]. I see very little overlap. Maybe define a new MediaEngineDestinationInterface instead? Or something completely different (I suspect the setSinkId implementation will dictate what this interface needs to support)? [1] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/dom/media/webrtc/MediaEngineSource.h#69
Attachment #8984169 - Flags: review?(jib) → review-
Comment on attachment 8984168 [details] Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. https://reviewboard.mozilla.org/r/249964/#review256344 Some nits, r- over audioLoopDev.get(). ::: dom/media/MediaManager.h:247 (Diff revision 1) > EnumerateRawDevices(uint64_t aWindowId, > dom::MediaSourceEnum aVideoType, > dom::MediaSourceEnum aAudioType, > DeviceEnumerationType aVideoEnumType = DeviceEnumerationType::Normal, > - DeviceEnumerationType aAudioEnumType = DeviceEnumerationType::Normal); > + DeviceEnumerationType aAudioEnumType = DeviceEnumerationType::Normal, > + dom::MediaSourceEnum aAudioOutputType = dom::MediaSourceEnum::Other); This seems like a glorified boolean, though I suppose there's some symmetry appeal here, and not having to wonder what `true` means at the call-site. I'd either make it a boolean or make it the third argument to emphasize the symmetry. Also, to clean up a bit, maybe remove all the defaults here? They seem unused. Being explicit at the call site seems superior. ::: dom/media/MediaManager.h:254 (Diff revision 1) > EnumerateDevicesImpl(uint64_t aWindowId, > dom::MediaSourceEnum aVideoType, > dom::MediaSourceEnum aAudioType, > DeviceEnumerationType aVideoEnumType = DeviceEnumerationType::Normal, > - DeviceEnumerationType aAudioEnumType = DeviceEnumerationType::Normal); > + DeviceEnumerationType aAudioEnumType = DeviceEnumerationType::Normal, > + dom::MediaSourceEnum aAudioOutputType = dom::MediaSourceEnum::Other); same here ::: dom/media/MediaManager.cpp:1903 (Diff revision 1) > MOZ_ASSERT(aAudioEnumType != DeviceEnumerationType::Loopback || > aAudioType == MediaSourceEnum::Microphone, > "If loopback audio is requested audio type should be microphone!"); Should we MOZ_ASSERT that includeAudioOutput is only true for aAudioType == MediaSourceEnum::Microphone || aAudioType == MediaSourceEnum::Other ? ::: dom/media/MediaManager.cpp:1966 (Diff revision 1) > } > } > if (hasAudio) { > SourceSet audios; > LOG(("EnumerateRawDevices Task: Getting audio sources with %s backend", > - aVideoEnumType == DeviceEnumerationType::Fake ? "fake" : "real")); > + aAudioEnumType == DeviceEnumerationType::Fake ? "fake" : "real")); Nice catch! ::: dom/media/MediaManager.cpp:1975 (Diff revision 1) > + GetSources(realBackend, > + aWindowId, > + dom::MediaSourceEnum::Speaker, > + audios, > + audioLoopDev.get()); If `audioLoopDev` is specified, this filters out all speaker devices doesn't it? Seems wrong. We probably want "".
Attachment #8984168 - Flags: review?(jib) → review-
Comment on attachment 8984170 [details] Bug 1152401 - Use mKind member of MediaDevice in MediaDeviceInfo to avoid string comparisons. https://reviewboard.mozilla.org/r/249968/#review256370 Would need to revisit.
Attachment #8984170 - Flags: review?(jib) → review-
Comment on attachment 8984171 [details] Bug 1152401 - Create pref to enable enumeration of output devices. https://reviewboard.mozilla.org/r/249970/#review256374 ::: modules/libpref/init/all.js:604 (Diff revision 1) > > pref("media.webaudio.audiocontextoptions-samplerate.enabled", true); > > +// setSinkId expected to be unconditionally enabled in 63. Till then the > +// implementation will remain hidden behind this pref (Bug 1152401, Bug 934425). > +pref("media.setSinkId.enabled", false); (media) prefs tend to be all lowercase.
Attachment #8984171 - Flags: review?(jib) → review-
Comment on attachment 8984169 [details] Bug 1152401 - Implement enumeration of speaker devices. https://reviewboard.mozilla.org/r/249966/#review256366 > I don't think speakers should be a MediaEngineSource (it's not a source). > > Specifically, I don't see why speakers should have to implement the MediaEngineSourceInterface interface [1]. I see very little overlap. > > Maybe define a new MediaEngineDestinationInterface instead? Or something completely different (I suspect the setSinkId implementation will dictate what this interface needs to support)? > > [1] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/dom/media/webrtc/MediaEngineSource.h#69 Enumeration now works with MediaDevices.
Comment on attachment 8984167 [details] Bug 1152401 - Create MediaSinkEnum for speakers. https://reviewboard.mozilla.org/r/249962/#review261976 Do we gain anything from defining this in WebIDL? If not, we should just define it in a private .h file, to avoid needing DOM review. Also, do we need it at all? Not clear from the code, since we only have speakers as a planned type for the forseeable future. In contrast, MediaSourceEnum is in webidl to support the non-standard mediaSource constraint [1]. [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/webidl/MediaTrackSettings.webidl#23-24,26 ::: dom/webidl/MediaStreamTrack.webidl:36 (Diff revision 2) > "audioCapture", > "other" > // If values are added, adjust n_values in Histograms.json (2 places) > }; > > +enum MediaDestinationEnum { I think MediaSinkEnum is a more suitable antonym to Source in this context.
Attachment #8984167 - Flags: review?(jib) → review-
Comment on attachment 8989923 [details] Bug 1152401 - Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources. https://reviewboard.mozilla.org/r/254930/#review262024 Lgtm. ::: dom/media/MediaManager.cpp:1518 (Diff revision 1) > - for (auto& source : sources) { > - nsString deviceName = source->GetName(); > + for (auto& device : devices) { > + if (device->mName.EqualsASCII(aMediaDeviceName)) { If we're going to remove the distinction between `mName` and `mSource->GetName()` (a good idea IMHO) then I think we should also remove the name argument from the `MediaDevice` constructor [1] [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaManager.cpp#822 ::: dom/media/MediaManager.cpp:1943 (Diff revision 1) > MediaManager* manager = MediaManager::GetIfExists(); > MOZ_RELEASE_ASSERT(manager); // Must exist while media thread is alive > realBackend = manager->GetBackend(aWindowId); > } > > auto result = MakeUnique<SourceSet>(); If we're cleaning up names, we probably want s/SourceSet/MediaDeviceSet/ as well, so it reads better. [1] [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaManager.h#220 ::: dom/media/webrtc/MediaEngineWebRTC.cpp:301 (Diff revision 1) > nsRefPtrHashtable<nsStringHashKey, MediaEngineSource>* > devicesForThisWindow = mAudioSources.LookupOrAdd(aWindowId); > > - if (devicesForThisWindow->Get(uuid, getter_AddRefs(aSource)) && > - aSource->RequiresSharing()) { > - // We've already seen this device, just append. > + bool alreadySeenThisDeviceBefore = devicesForThisWindow->Get(uuid, getter_AddRefs(micSource)) && > + micSource->RequiresSharing(); > + if(!alreadySeenThisDeviceBefore) { if (
Attachment #8989923 - Flags: review?(jib) → review+
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review261974 Looks good. r- just because I'd like to see it again. ::: dom/media/MediaManager.h:72 (Diff revision 1) > + // One to one mapping with MediaDeviceKind. > + enum DeviceKind { > + Audioinput, > + Audiooutput, > + Videoinput > + }; Why not use MediaDeviceKind directly? ::: dom/media/MediaManager.h:131 (Diff revision 1) > + static nsString KindToType(DeviceKind aKind); > + DeviceKind GetDeviceKind(); If we use MediaDeviceKind directly, then we can reuse string conversion from our webidl binding code. E.g. like we do for MediaSourceEnum [1]. [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaManager.cpp#959-960 ::: dom/media/MediaManager.cpp:828 (Diff revision 1) > const nsString& aID, > const nsString& aRawID) > : mSource(aSource) > - , mIsVideo(MediaEngineSource::IsVideo(mSource->GetMediaSource())) > + , mKind(GetDeviceKind()) > , mScary(mSource->GetScary()) > - , mType(mIsVideo ? NS_LITERAL_STRING("video") : NS_LITERAL_STRING("audio")) > + , mType(KindToType(mKind)) Can we get rid of mType now? AFAICT the string isn't used directly anywhere, and the use of "video" instead of "videoinput" and "audio" instead of "audioinput" is just ... wrong. Exhibit A: we end up converting the strings back again here [1] [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaDevices.cpp#116-122 ::: dom/media/MediaManager.cpp:849 (Diff revision 1) > + // It could be used for Audioinput and Videoinput > + // when we do not instantiate a MediaEngineSource > + // during EnumerateDevices. When this is implemented > + // that assert can be removed. I dislike forward-looking statements in code. I also wonder if it's more likely we'll end up with a constructor that takes some MediaSinc object... For now this looks good, but I'd remove the predictions in this comment. ::: dom/media/MediaManager.cpp:871 (Diff revision 1) > +MediaDevice::KindToType(DeviceKind aKind) > +{ > + switch (aKind) { > + case Audioinput: > + return NS_LITERAL_STRING("audio"); > + case Audiooutput: > + return NS_LITERAL_STRING("audiooutput"); > + case Videoinput: > + return NS_LITERAL_STRING("video"); > + default: > + MOZ_ASSERT_UNREACHABLE("Unkown type"); > + } > +} Redundant, if we use webidl binding code's strings. ::: dom/media/MediaManager.cpp:888 (Diff revision 1) > +} > + > +MediaDevice::DeviceKind > +MediaDevice::GetDeviceKind() > +{ > + MOZ_ASSERT(mSource.get()); Nit: .get() is redundant. Applies throughout. ::: dom/media/MediaManager.cpp:1641 (Diff revision 1) > > for (auto& source : sources) { > - if (source->mIsVideo) { > + if (source->mKind == MediaDevice::Videoinput) { > videos.AppendElement(source); > } else { > + MOZ_ASSERT(source->mKind == MediaDevice::Audioinput); MOZ_RELEASE_ASSERT I think, just in case?
Attachment #8989924 - Flags: review?(jib) → review-
Comment on attachment 8984168 [details] Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. https://reviewboard.mozilla.org/r/249964/#review262048 This patch looks good! But fixes in earlier patches would probably make it look different, so it probably makes sense to r- so I'll see it again, with mozReview's workflow. ::: dom/media/MediaManager.h:262 (Diff revision 2) > - dom::MediaSourceEnum aVideoType, > - dom::MediaSourceEnum aAudioType, > - DeviceEnumerationType aVideoEnumType = DeviceEnumerationType::Normal, > + dom::MediaSourceEnum aVideoType, > + dom::MediaSourceEnum aAudioType, > + dom::MediaDestinationEnum aAudioOutputType, Should we maybe s/aVideoType/aVideoInputType/ and s/aAudioType/aAudioInputType/ ? ::: dom/media/MediaManager.cpp:1947 (Diff revision 2) > MediaSourceEnum aVideoType, > MediaSourceEnum aAudioType, > + dom::MediaDestinationEnum aAudioOutputType, Shouldn't need dom:: At least we should be consistent in our use of dom:: ::: dom/media/MediaManager.cpp:1989 (Diff revision 2) > // True if at least one of video or audio is a real device > bool realDeviceRequested = (aVideoEnumType != DeviceEnumerationType::Fake && hasVideo) || > - (aAudioEnumType != DeviceEnumerationType::Fake && hasAudio); > + (aAudioEnumType != DeviceEnumerationType::Fake && hasAudio) || > + hasAudioOutput; update comment ::: dom/media/MediaManager.cpp:2031 (Diff revision 2) > result->AppendElements(videos); > } > if (hasAudio) { > SourceSet audios; > LOG(("EnumerateRawDevices Task: Getting audio sources with %s backend", > - aVideoEnumType == DeviceEnumerationType::Fake ? "fake" : "real")); > + aAudioEnumType == DeviceEnumerationType::Fake ? "fake" : "real")); nice catch! ::: dom/media/MediaManager.cpp:2385 (Diff revision 2) > if (sHasShutdown) { > return NS_OK; > } > self->DeviceChangeCallback::OnDeviceChange(); > > // On some Windows machine, if we call EnumertaeRawDevices immediately after receiving Adjacent typo. Can you take care of it? ::: dom/media/webrtc/MediaEngineSource.cpp (Diff revision 2) > return true; > case MediaSourceEnum::Microphone: > case MediaSourceEnum::AudioCapture: > return false; > default: > - MOZ_ASSERT_UNREACHABLE("Unknown type"); We should not remove this.
Attachment #8984168 - Flags: review?(jib) → review-
Comment on attachment 8984171 [details] Bug 1152401 - Create pref to enable enumeration of output devices. https://reviewboard.mozilla.org/r/249970/#review262196 Lgtm.
Attachment #8984171 - Flags: review?(jib) → review+
Comment on attachment 8984167 [details] Bug 1152401 - Create MediaSinkEnum for speakers. https://reviewboard.mozilla.org/r/249962/#review261976 I've moved that in a private header. Also you are right we do not really need it. However, my opinion is that having an enum for a glorify boolean helps readability. I could chnage it though if you do not like it.
Comment on attachment 8989923 [details] Bug 1152401 - Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources. https://reviewboard.mozilla.org/r/254930/#review262024 > If we're going to remove the distinction between `mName` and `mSource->GetName()` (a good idea IMHO) then I think we should also remove the name argument from the `MediaDevice` constructor [1] > > [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaManager.cpp#822 This change has been made in order to use the `MediaDevice` instead of `MediaEngineSource`. I am not trying to remove the distinction between `mName` and `mSource->GetName()`. In addition `mName` cannot be removed from ctor because it's a `const` member. > If we're cleaning up names, we probably want s/SourceSet/MediaDeviceSet/ as well, so it reads better. [1] > > [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaManager.h#220 SourceSet still exists and is used in a number of places in a valid way. https://searchfox.org/mozilla-central/search?q=SourceSet I do not think it would be cleaner to remove it (by replacing it). I could add a new `MediaDeviceSet` though, to serve a similar meaning. I haven't done that because I am not fan of these shortcuts, readability wise. I prefer to read the full type even if it is a little bit longer.
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review261974 > If we use MediaDeviceKind directly, then we can reuse string conversion from our webidl binding code. E.g. like we do for MediaSourceEnum [1]. > > [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaManager.cpp#959-960 `mType` is used from inernal .jsm files. The expected is `"audio"` or `"video"` (for example [2]). I cannot just change it to sting version of `MediaDeviceKind`. [1] https://searchfox.org/mozilla-central/search?q=symbol%3A%23nsIMediaDevice&path=*.jsm [2] https://searchfox.org/mozilla-central/source/browser/modules/ContentWebRTC.jsm#174 > Can we get rid of mType now? > > AFAICT the string isn't used directly anywhere, and the use of "video" instead of "videoinput" and "audio" instead of "audioinput" is just ... wrong. > > Exhibit A: we end up converting the strings back again here [1] > > [1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/dom/media/MediaDevices.cpp#116-122 Similar to previous comment we cannot get rid off the `mType` without changing other components. I attempted to use the `MediaDeviceKind` in the `idl` declaration of MediaDevice but this type is declared as `webidl` (not idl) and it ends up with a parsing error. Do you know any way of doing it? > I dislike forward-looking statements in code. I also wonder if it's more likely we'll end up with a constructor that takes some MediaSinc object... > > For now this looks good, but I'd remove the predictions in this comment. We do not need MediaSink obects apparently. The way that it works through setSinkId is different than the source case. > Redundant, if we use webidl binding code's strings. Similar to other `mType` responses above.
Comment on attachment 8989923 [details] Bug 1152401 - Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources. https://reviewboard.mozilla.org/r/254930/#review262024 > SourceSet still exists and is used in a number of places in a valid way. > https://searchfox.org/mozilla-central/search?q=SourceSet > I do not think it would be cleaner to remove it (by replacing it). I could add a new `MediaDeviceSet` though, to serve a similar meaning. I haven't done that because I am not fan of these shortcuts, readability wise. I prefer to read the full type even if it is a little bit longer. Ooops wrong answer. I can do the requested change. I cofused by the name, I thought this was a list on sources.
Comment on attachment 8984168 [details] Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. https://reviewboard.mozilla.org/r/249964/#review256344 > This seems like a glorified boolean, though I suppose there's some symmetry appeal here, and not having to wonder what `true` means at the call-site. > > I'd either make it a boolean or make it the third argument to emphasize the symmetry. > > Also, to clean up a bit, maybe remove all the defaults here? They seem unused. Being explicit at the call site seems superior. Default `DeviceEnumerationType` of `EnumerateRawDevices()` is used in `MediaManager::OnDeviceChange()` [1]. [1] https://searchfox.org/mozilla-central/source/dom/media/MediaManager.cpp#2312 > Should we MOZ_ASSERT that includeAudioOutput is only true for > > aAudioType == MediaSourceEnum::Microphone || > aAudioType == MediaSourceEnum::Other > > ? That's true in the current implementation but there is no restriction if someone wants to use it differently in the future.
Comment on attachment 8984167 [details] Bug 1152401 - Create MediaSinkEnum for speakers. https://reviewboard.mozilla.org/r/249962/#review261976 I'll leave it up to you.
Comment on attachment 8989923 [details] Bug 1152401 - Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources. https://reviewboard.mozilla.org/r/254930/#review262024 > This change has been made in order to use the `MediaDevice` instead of `MediaEngineSource`. I am not trying to remove the distinction between `mName` and `mSource->GetName()`. In addition `mName` cannot be removed from ctor because it's a `const` member. For sources it can be initialized to mSource->GetName().
Comment on attachment 8984168 [details] Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. https://reviewboard.mozilla.org/r/249964/#review256344 > Default `DeviceEnumerationType` of `EnumerateRawDevices()` is used in `MediaManager::OnDeviceChange()` [1]. > > [1] https://searchfox.org/mozilla-central/source/dom/media/MediaManager.cpp#2312 I missed that one, but I still think we should just fix that one rather than rely on defaults. > That's true in the current implementation but there is no restriction if someone wants to use it differently in the future. You're right. I misread the code here. In that case, maybe we should loosen the second assert to: MOZ_ASSERT(aVideoType != MediaSourceEnum::Other || aAudioType != MediaSourceEnum::Other || aAudioOutputType != MediaSinkEnum::Other); ?
Comment on attachment 8984169 [details] Bug 1152401 - Implement enumeration of speaker devices. https://reviewboard.mozilla.org/r/249966/#review262740 Looks good. Just nits. ::: dom/media/webrtc/MediaEngineWebRTC.cpp:319 (Diff revision 2) > > void > MediaEngineWebRTC::EnumerateSpeakerDevices(uint64_t aWindowId, > nsTArray<RefPtr<MediaDevice> >* aDevices) > { > + nsTArray<RefPtr<AudioDeviceInfo>> devices; Nit: move closer to point of use. ::: dom/media/webrtc/MediaEngineWebRTC.cpp:320 (Diff revision 2) > + cubeb* context = CubebUtils::GetCubebContext(); > + if (!context) { Nit: unused stack variable makes me wonder if it's used below, which it isn't. How about: if (!CubebUtils::GetCubebContext()) { ::: dom/media/webrtc/MediaEngineWebRTC.cpp:326 (Diff revision 2) > + if (devices.IsEmpty()) { > + return; > + } Redundant. ::: dom/media/webrtc/MediaEngineWebRTC.cpp:330 (Diff revision 2) > + CubebUtils::GetDeviceCollection(devices, CubebUtils::Output); > + if (devices.IsEmpty()) { > + return; > + } > + > + for (uint32_t i = 0; i < devices.Length(); ++i) { Can we use for (auto device : devices) ? ::: dom/media/webrtc/MediaEngineWebRTC.cpp:339 (Diff revision 2) > + aDevices->AppendElement(MakeRefPtr<MediaDevice>( > + devices[i]->FriendlyName(), > + MediaDevice::Audiooutput, > + uuid)); Nit: prefer indent by 2 or 0, not 1.
Attachment #8984169 - Flags: review?(jib) → review+
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review261974 > `mType` is used from inernal .jsm files. The expected is `"audio"` or `"video"` (for example [2]). I cannot just change it to sting version of `MediaDeviceKind`. > > [1] https://searchfox.org/mozilla-central/search?q=symbol%3A%23nsIMediaDevice&path=*.jsm > [2] https://searchfox.org/mozilla-central/source/browser/modules/ContentWebRTC.jsm#174 We can change the .jsm files, can't we? "videoinput" and "audioinput" seems appropriate. > Similar to previous comment we cannot get rid off the `mType` without changing other components. > > I attempted to use the `MediaDeviceKind` in the `idl` declaration of MediaDevice but this type is declared as `webidl` (not idl) and it ends up with a parsing error. Do you know any way of doing it? Do you mean nsIMediaDevice? That's a DOMString and I see no need to change that. > Similar to other `mType` responses above. What I mean is KindToType(MediaDeviceKind aKind) can be implemented like this: return NS_ConvertUTF8toUTF16(MediaDeviceKindValues::strings[uint32_t(aKind)].value)); by relying on binding code [1] [1] https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/MediaDeviceInfoBinding.cpp#21-28,35
Comment on attachment 8984170 [details] Bug 1152401 - Use mKind member of MediaDevice in MediaDeviceInfo to avoid string comparisons. https://reviewboard.mozilla.org/r/249968/#review262754 ::: dom/media/MediaDevices.cpp:75 (Diff revision 2) > +MediaDeviceKind > +MediaDeviceTypeToKind(const nsString &type) > +{ > + if (type.EqualsLiteral("video")) { > + return MediaDeviceKind::Videoinput; > + } else if (type.EqualsLiteral("audio")) { > + return MediaDeviceKind::Audioinput; > + } else if (type.EqualsLiteral("audiooutput")) { > + return MediaDeviceKind::Audiooutput; > + } else { > + MOZ_ASSERT_UNREACHABLE("type does not exist"); > + return MediaDeviceKind::EndGuard_; > + } > +} FWIW we could probably implement MediaDeviceTypeToKind(const nsString &type) like this, I think: nsCString ctype = NS_ConvertUTF16toUTF8(type); uint32_t kind = 0; for (auto& enumEntry : MediaDeviceKindValues::strings) { if (enumEntry.value && ctype.Compare(enumEntry.value) == 0) { return static_cast<MediaDeviceKind>(kind); } kind++; } MOZ_ASSERT_UNREACHABLE("type does not exist"); return MediaDeviceKind::EndGuard_; Not pretty, but would avoid risk of string typos. YMMV.
Attachment #8984170 - Flags: review?(jib) → review+
Comment on attachment 8984168 [details] Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. https://reviewboard.mozilla.org/r/249964/#review262048 > We should not remove this. This is not any more unreachable. It can be called from [1] with `MediaSourceEnum::Other` when output devices are being enumerated. [1] https://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineWebRTC.cpp#315
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review261974 > We can change the .jsm files, can't we? "videoinput" and "audioinput" seems appropriate. Done, let me know what you think. > Do you mean nsIMediaDevice? That's a DOMString and I see no need to change that. Hmm sorry I'm confused. I am saying that we need to keep the `mType` member. I am also saying that it is not possible the declare a MediaDeviceKind member directly in `idl` declaration of nsIMediaDevice here [1]. [1] https://searchfox.org/mozilla-central/source/dom/media/nsIDOMNavigatorUserMedia.idl#9 > What I mean is KindToType(MediaDeviceKind aKind) can be implemented like this: > > return NS_ConvertUTF8toUTF16(MediaDeviceKindValues::strings[uint32_t(aKind)].value)); > > by relying on binding code [1] > > [1] https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/MediaDeviceInfoBinding.cpp#21-28,35 Done, thanks for the suggestion.
Comment on attachment 8984169 [details] Bug 1152401 - Implement enumeration of speaker devices. https://reviewboard.mozilla.org/r/249966/#review262740 > Nit: unused stack variable makes me wonder if it's used below, which it isn't. How about: > > if (!CubebUtils::GetCubebContext()) { Right, actually we can remove completely that check. It happens inside `CubebUtils::GetDeviceCollection()`.
Comment on attachment 8984170 [details] Bug 1152401 - Use mKind member of MediaDevice in MediaDeviceInfo to avoid string comparisons. https://reviewboard.mozilla.org/r/249968/#review262754 > FWIW we could probably implement MediaDeviceTypeToKind(const nsString &type) like this, I think: > > nsCString ctype = NS_ConvertUTF16toUTF8(type); > uint32_t kind = 0; > for (auto& enumEntry : MediaDeviceKindValues::strings) { > if (enumEntry.value && ctype.Compare(enumEntry.value) == 0) { > return static_cast<MediaDeviceKind>(kind); > } > kind++; > } > MOZ_ASSERT_UNREACHABLE("type does not exist"); > return MediaDeviceKind::EndGuard_; > > Not pretty, but would avoid risk of string typos. YMMV. This method has been removed. I am using instead, the MediaDeviceKind member of MediaDevice.
Comment on attachment 8984167 [details] Bug 1152401 - Create MediaSinkEnum for speakers. https://reviewboard.mozilla.org/r/249962/#review263208
Attachment #8984167 - Flags: review?(jib) → review+
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review263210 I like it, except r- for the scary downcast. ::: dom/media/MediaManager.cpp:849 (Diff revisions 1 - 2) > - , mType(KindToType(mKind)) > + , mType(NS_ConvertUTF8toUTF16(dom::MediaDeviceKindValues::strings[uint32_t(mKind)].value)) > , mName(aName) > , mID(aID) > , mRawID(aRawID) > { > // For now this ctor is used only for Audiooutup. typo ::: dom/media/MediaManager.cpp:871 (Diff revisions 1 - 2) > MediaDevice::GetDeviceKind() > { > - MOZ_ASSERT(mSource.get()); > + MOZ_ASSERT(mSource); > if (MediaEngineSource::IsVideo(mSource->GetMediaSource())) { > - return Videoinput; > + return dom::MediaDeviceKind::Videoinput; > } > - return Audioinput; > + return dom::MediaDeviceKind::Audioinput; > } Maybe inline this function at its sole call site? , mKind((mSource && MediaEngineSource::IsVideo(mSource->GetMediaSource()) ? dom::MediaDeviceKind::Videoinput : dom::MediaDeviceKind::Audioinput) { MOZ_ASSERT(mSource); } ::: dom/media/MediaManager.cpp:1625 (Diff revisions 1 - 2) > } else { > - MOZ_ASSERT(source->mKind == MediaDevice::Audioinput); > + MOZ_RELEASE_ASSERT(source->mKind == dom::MediaDeviceKind::Audioinput); I wouldn't use a RELEASE assert here. This ASSERT is really an optimization, and adding it to release seems to negate that purpose. Instead, let's just skip the optimization then, and do: } else if (source->mKind == dom::MediaDeviceKind::Audioinput) { That seems the most future proof and benign here. ::: dom/media/MediaManager.cpp:3702 (Diff revisions 1 - 2) > - nsString type; > - device->GetType(type); > + MediaDevice* dev = static_cast<MediaDevice*>(device.get()); > + if (dev->mKind == dom::MediaDeviceKind::Videoinput) { This downcast scares me. It assumes no-one is creating nsIMediaDevice instances and passing them to us but us. In the case of "getUserMedia:privileged:allow" it's fine, since device is from devicesCopy here [1]. Tests disable permission, so this is the case that'll get tested. But in the case of "getUserMedia:response:allow" it comes from 3 different JS files. I checked all three, and they all appear to be ok today, returning only the original instances we've created as new MediaDevice. However, nothing in this contract prevents future JS from instantiating copies, using: createInstance(Ci.nsIMediaDevice) ...and passing us the copies to this api. If that ever happens, we'll trash memory here, which will be hard to catch. I think we need to convert from string here, and not assume the object is deeper than nsIMediaDevice. [1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/media/MediaManager.cpp#2926 ::: dom/media/MediaDevices.cpp:115 (Diff revision 2) > for (auto& device : devices) { > - nsString type; > + MediaDevice* dev = static_cast<MediaDevice*>(device.get()); This static_cast I think is fine, because we know MediaManager::EnumerateDevices produces. In fact, this whole EnumDevResolver thing with conversion to nsIVariant and back is crazy redundant, and should be replaced by MediaManager::EnumerateDevices returning a Pledge instead (can't use MozPromise, as MediaManager::EnumerateDevices includes IPC). That would let us delete much of this code. We can do that in a separate bug though, if you prefer.
Attachment #8989924 - Flags: review?(jib) → review-
Comment on attachment 8984168 [details] Bug 1152401 - Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. https://reviewboard.mozilla.org/r/249964/#review263264 ::: dom/media/MediaManager.cpp:2051 (Diff revision 3) > - (aVideoType != MediaSourceEnum::Camera) ? u"audio" : > - (aAudioType != MediaSourceEnum::Microphone) ? u"video" : > + (aVideoInputType != MediaSourceEnum::Camera) ? u"audio" : > + (aAudioInputType != MediaSourceEnum::Microphone) ? u"video" : > u"all"; Nit: fix indent
Attachment #8984168 - Flags: review?(jib) → review+
Comment on attachment 8991226 [details] Bug 1152401 - Rename SourceSet to MediaDeviceSet to help readability. https://reviewboard.mozilla.org/r/256188/#review263268 Thanks for doing a dedicated rename patch! Always helpful.
Attachment #8991226 - Flags: review?(jib) → review+
See Also: → 1475209
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review263210 > I wouldn't use a RELEASE assert here. This ASSERT is really an optimization, and adding it to release seems to negate that purpose. Instead, let's just skip the optimization then, and do: > > } else if (source->mKind == dom::MediaDeviceKind::Audioinput) { > > That seems the most future proof and benign here. Release assert is not necessary indeed. However, I like an assert that expresses the fact that we are expecting either Audioinput or Videoinput at that point. I took your suggestion but as opposed to removing completely the assert I've added a more expressive one at the beginning of the for loop. > This static_cast I think is fine, because we know MediaManager::EnumerateDevices produces. > > In fact, this whole EnumDevResolver thing with conversion to nsIVariant and back is crazy redundant, and should be replaced by MediaManager::EnumerateDevices returning a Pledge instead (can't use MozPromise, as MediaManager::EnumerateDevices includes IPC). That would let us delete much of this code. > > We can do that in a separate bug though, if you prefer. I have opened Bug 1475209 about it. Depending on the priorities I could move it up in my task queue.
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review263210 > This downcast scares me. It assumes no-one is creating nsIMediaDevice instances and passing them to us but us. > > In the case of "getUserMedia:privileged:allow" it's fine, since device is from devicesCopy here [1]. Tests disable permission, so this is the case that'll get tested. > > But in the case of "getUserMedia:response:allow" it comes from 3 different JS files. I checked all three, and they all appear to be ok today, returning only the original instances we've created as new MediaDevice. > > However, nothing in this contract prevents future JS from instantiating copies, using: > > createInstance(Ci.nsIMediaDevice) > > ...and passing us the copies to this api. If that ever happens, we'll trash memory here, which will be hard to catch. I think we need to convert from string here, and not assume the object is deeper than nsIMediaDevice. > > [1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/media/MediaManager.cpp#2926 Hmmm I am not sure how it is possible to create an instance of nsIMediaDevice. My understanding is that we need a contract id for that and nsIMediaDevice does not have. Are you sure we can call createInstance for this class? If we were able to create an instance from JS we would also need to set an appropriate value in `MediaDevice::mKind` here. But `mKind` is a `const` field, set only in the ctor. So more problems with that. IMO the best direction would be to declare the `kind` member in idl definition. However, MediaDeviceKind is declared on a webidl file. When I tried including that webidl file in `nsIDOMNavigatorUserMedia.idl` I got a compile error (from the idl parser). Do you know if/how this can been done correctly?
Comment on attachment 8991227 [details] Bug 1152401 - Augment AudioDeviceInfo with a cubeb device id. https://reviewboard.mozilla.org/r/256190/#review263692 Nit: not that I care too much, but it's bad form to change the name of the author of a patch. This has been already r+ed by pehrsons, r+ for landing as part of this bug, we'll see who land first.
Attachment #8991227 - Flags: review?(padenot) → review+
Comment on attachment 8991227 [details] Bug 1152401 - Augment AudioDeviceInfo with a cubeb device id. https://reviewboard.mozilla.org/r/256190/#review263692 You are absolutely right thank you for pointing out. Tbh I did not realize it, I imported a diff file and the author information has been lost. Let me check if I can restore that now.
Comment on attachment 8991227 [details] Bug 1152401 - Augment AudioDeviceInfo with a cubeb device id. https://reviewboard.mozilla.org/r/256190/#review263990 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/AudioDeviceInfo.cpp:87 (Diff revision 2) > +Maybe<AudioDeviceID> > +AudioDeviceInfo::GetDeviceID() > +{ > + if (mDeviceId) { > + return Some(mDeviceId); > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] } else { ^~~~~~
A friendly reminder to continue that review. I think most of the issues have been addressed we could finish it soon. Thanks
Flags: needinfo?(jib)
Comment on attachment 8989924 [details] Bug 1152401 - Add MediaDeviceKind member in MediaDevice to differentiate sink devices. https://reviewboard.mozilla.org/r/254932/#review263266
Attachment #8989924 - Flags: review?(jib) → review+
Flags: needinfo?(jib)
Pushed by achronop@gmail.com: https://hg.mozilla.org/integration/autoland/rev/aef64bf5b024 Create MediaSinkEnum for speakers. r=jib https://hg.mozilla.org/integration/autoland/rev/1630f67ac197 Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources. r=jib https://hg.mozilla.org/integration/autoland/rev/93457990e40f Add MediaDeviceKind member in MediaDevice to differentiate sink devices. r=jib https://hg.mozilla.org/integration/autoland/rev/aebabaa96ad3 Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. r=jib https://hg.mozilla.org/integration/autoland/rev/10b039210911 Implement enumeration of speaker devices. r=jib https://hg.mozilla.org/integration/autoland/rev/d68ab111809f Use mKind member of MediaDevice in MediaDeviceInfo to avoid string comparisons. r=jib https://hg.mozilla.org/integration/autoland/rev/95661c6cd914 Create pref to enable enumeration of output devices. r=jib https://hg.mozilla.org/integration/autoland/rev/e45630a7c7b1 Rename SourceSet to MediaDeviceSet to help readability. r=jib https://hg.mozilla.org/integration/autoland/rev/987f2e0b2cb1 Augment AudioDeviceInfo with a cubeb device id. r=padenot
Comment on attachment 8993361 [details] Bug 1152401 - Expect the new MediaDevice types in GeckoView. https://reviewboard.mozilla.org/r/258144/#review265228
Attachment #8993361 - Flags: review?(nchen) → review+
Pushed by achronop@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2bd269c5952c Create MediaSinkEnum for speakers. r=jib https://hg.mozilla.org/integration/autoland/rev/3a3a9ea4d0d3 Use MediaDevice in MediaEngine to allow enumeration of both sinks and sources. r=jib https://hg.mozilla.org/integration/autoland/rev/d4d476a32ea8 Add MediaDeviceKind member in MediaDevice to differentiate sink devices. r=jib https://hg.mozilla.org/integration/autoland/rev/a6052c78f8cf Update enumerate methods of MediaManager and MediaEngine to accept audio sink type. r=jib https://hg.mozilla.org/integration/autoland/rev/f71f5a23b9cb Implement enumeration of speaker devices. r=jib https://hg.mozilla.org/integration/autoland/rev/4c314f9d7f3c Use mKind member of MediaDevice in MediaDeviceInfo to avoid string comparisons. r=jib https://hg.mozilla.org/integration/autoland/rev/09e2e1e8427b Create pref to enable enumeration of output devices. r=jib https://hg.mozilla.org/integration/autoland/rev/ffa412239e2a Rename SourceSet to MediaDeviceSet to help readability. r=jib https://hg.mozilla.org/integration/autoland/rev/908a5b574b39 Augment AudioDeviceInfo with a cubeb device id. r=padenot https://hg.mozilla.org/integration/autoland/rev/ecb39ca5568b Expect the new MediaDevice types in GeckoView. r=jchen
Clearing NI.
Flags: needinfo?(achronop)
Summary: enumerateDevices() should enumerate audio output devices (if we have any) → Get rid of EnumDevResolver in MediaDevices.cpp by using a promise/mozpromise/pledge solution directly.
Summary: Get rid of EnumDevResolver in MediaDevices.cpp by using a promise/mozpromise/pledge solution directly. → enumerateDevices() should enumerate audio output devices (if we have any)
Submitted PR to the BCD repository to add a note about this to the enumerateDevices() page, and updated Firefox 63 for developers.
Keywords: dev-doc-needed → dev-doc-complete
The PR to BCD has been merged and the note about this can be seen in the compatibility tables.
This should be fixed accordint to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63 On Ubuntu Linux w/ Firefox 63.0b1 I only see audio/video input, no outputs.
This feature is behind a pref ("media.setsinkid.enabled") [1] and is not enabled for 63. The rationale is there's no need for web devs to enumerate speaker devices without setSinkId(), and setSinkId (bug 934425) has been postponed due to complications. We should remove this from the release notes. [1] https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/modules/libpref/init/all.js#637
Flags: needinfo?(eshepherd)
Summary: enumerateDevices() should enumerate audio output devices (if we have any) → enumerateDevices() should enumerate audio output devices (feature behind pref)
Blocks: 1498512
Is this now expected to go live with Firefox 64 as 934425 ships?
What is the status of this feature? I am working with Firefox 64 but my enumerateDevices() still only return my audioinput devices.
The feature is behind a pref. You can go to about:config and set "media.setsinkid.enabled" to true. Then you will see the output devices.

This probably shouldn't have been closed. It's still a problem. Setting a pref flag isn't the same as having a usable API. It's useless if you need it for an app, for example.

Chris, you're right, but our engineering process is different: this bug is about implementing the feature. This is done, so we've closed the bug.

There is another bug (that you can find in the "block" section above), that is about shipping this feature (i.e. flipping the default value for the pref). This is bug 1498512. You can see that this depends on various bugs, some of which are closed, some of which are still opened. You can cc yourself to those bugs to be informed about the progress there.

The big one is called multimsg, and is being actively worked on. In particular, as noted above, we now have the capability to enumerate audio output device, but nothing finished to use this capability (this is what's being worked on).

Flags: needinfo?(eshepherd)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: