A follow-up from bug 1213517: Share new constraints code added for cameras with microphone stack as well, so it'll work for echoCancellation et al.

Jib, can you give my an idea of the priority of this ask.

Yes sorry, same as bug 1213517. Just need to land that patch queue before it swallows me.

Review commit: https://reviewboard.mozilla.org/r/64992/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64992/

Review commit: https://reviewboard.mozilla.org/r/64994/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64994/

Review commit: https://reviewboard.mozilla.org/r/64996/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64996/

Review commit: https://reviewboard.mozilla.org/r/64998/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64998/

Review commit: https://reviewboard.mozilla.org/r/65000/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65000/

Review commit: https://reviewboard.mozilla.org/r/65002/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65002/

Review commit: https://reviewboard.mozilla.org/r/65004/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65004/

Some of this overlaps with bug 1088621, but the patch queue worked out this way, so I'm thinking I'll save that bug for writing the actual tests.

Why we want or need mozNoiseSuppression and mozAutoGainControl?

(In reply to Olli Pettay [:smaug] from comment #11) > Why we want or need mozNoiseSuppression and mozAutoGainControl? Because those make important modifications to the captured audio - which some people very much do not want (think music recording, people who run fanless computers or are using a real mic on a cable, and gain control we default off (unlike Chrome), but for some usecases it could be very useful, where some talkers are close to the mic and some very far away. Google gangs control of them with the AEC (turn AEC off, and those turn off). Since we don't default them all on that doesn't quite fit - and it makes it very hard to do certain things (like record a video for distribution on a laptop - you don't need the AEC (hurts quality, or can), but if you turn it off you lose noise suppression. I hope we can add them to the standard eventually.

(In reply to Olli Pettay [:smaug] from comment #11) > Why we want or need mozNoiseSuppression and mozAutoGainControl? We already support mozNoiseSuppression and mozAutoGainControl as constraints in getUserMedia() and track.applyConstraints() [1] The webidl in this bug is for track.getSettings() which is a complement method to those. (Unfortunately for code maintenance, the spec people decided to split up what used to be a single dictionary into three: MediaTrackConstraints, MediaTrackCapabilities, and MediaTrackSettings). [1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MediaStreamTrack.webidl#58-59

ok. So has someone at least filed a spec bug to get these properties into some spec? Exposing moz-prefixed stuff is against our rules, so reviewing this kinds of changes makes me feel a bit unease.

Comment on attachment 8772093 [details] Bug 1286096 - Wire up audio getSettings(). https://reviewboard.mozilla.org/r/65004/#review62016

(In reply to Olli Pettay [:smaug] from comment #14) > ok. So has someone at least filed a spec bug to get these properties into > some spec? We need a spec to file a bug on at this point, since mediacapture is near frozen and not accepting any more constraints. There's also supposed to be a registry for constraints, to reserve the name at least [1], but I don't know if it's up and running yet. [1] https://tools.ietf.org/html/draft-ietf-rtcweb-constraints-registry-03

Comment on attachment 8772089 [details] Bug 1286096 - Remove fakeTracks constraint. https://reviewboard.mozilla.org/r/64996/#review62008 LGTM with one Nit ::: dom/media/test/test_multiple_mediastreamtracks.html:17 (Diff revision 1) > function startTest() { > - navigator.mediaDevices.getUserMedia({audio:true, video:true, fake:true, fakeTracks:true}) > + navigator.mediaDevices.getUserMedia({audio:true, video:true, fake:true}) > .then(function(stream) { > + var a = stream.getAudioTracks()[0]; > + var v = stream.getVideoTracks()[0]; > + stream = new MediaStream([a, a, a, a, v, v, v].map(track => track.clone())); Nit: I would prefer to create a new variable for the new MediaStream here rather then overloading the input argument.

(In reply to Nils Ohlmeier [:drno] from comment #17) > Nit: I would prefer to create a new variable for the new MediaStream here > rather then overloading the input argument. OK, but doesn't having two variables named stream-something just increase the risk of using the wrong one?

Comment on attachment 8772089 [details] Bug 1286096 - Remove fakeTracks constraint. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64996/diff/1-2/

Comment on attachment 8772090 [details] Bug 1286096 - Move AllocationHandle used for cameras to MediaEngineSource base class to reuse for microphones. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64998/diff/1-2/

Comment on attachment 8772091 [details] Bug 1286096 - Move UpdateSingleSource pattern to MediaEngine base class for reuse. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65000/diff/1-2/

Comment on attachment 8772092 [details] Bug 1286096 - Consider competing audio constraints as well. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65002/diff/1-2/

Comment on attachment 8772093 [details] Bug 1286096 - Wire up audio getSettings(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65004/diff/1-2/

Addressed feedback, and simplified the UpdateSingleSource pattern slightly.

Comment on attachment 8772089 [details] Bug 1286096 - Remove fakeTracks constraint. https://reviewboard.mozilla.org/r/64996/#review62242

(In reply to Jan-Ivar Bruaroey [:jib] from comment #18) > OK, but doesn't having two variables named stream-something just increase > the risk of using the wrong one? I guess it is one of these things which you can prefer one way or the other, because there are no real hard arguments for having it to do either way. So I'll leave it up to you :-)

Comment on attachment 8772090 [details] Bug 1286096 - Move AllocationHandle used for cameras to MediaEngineSource base class to reuse for microphones. https://reviewboard.mozilla.org/r/64998/#review64482 Edit the commit message to tell why you're doing this move.

Comment on attachment 8772088 [details] Bug 1286096 - Enable width/height constraints on fake device. https://reviewboard.mozilla.org/r/64994/#review64492 Update the commit message to explain why you're doing this.

https://reviewboard.mozilla.org/r/64992/#review64460 ::: dom/media/webrtc/MediaEngineDefault.cpp:47 (Diff revision 1) > /** > * Default video source. > */ > > MediaEngineDefaultVideoSource::MediaEngineDefaultVideoSource() > - : MediaEngineVideoSource(kReleased) > + : MediaEngineCameraVideoSource(0, "FakeVideo.Monitor") Magic numbers and strings are not ideal. In particular, nothing tells me why passing in `0` here is okay.

Comment on attachment 8772087 [details] Bug 1286096 - Have MediaEngineDefaultVideoSource inherit from MediaEngineCameraVideoSource. https://reviewboard.mozilla.org/r/64992/#review64498

Comment on attachment 8772091 [details] Bug 1286096 - Move UpdateSingleSource pattern to MediaEngine base class for reuse. https://reviewboard.mozilla.org/r/65000/#review64500 ::: dom/media/webrtc/MediaEngine.h:385 (Diff revision 2) > + * aDeviceId - As passed in (origin-dependent id). > + * aOutBadConstraint - Result: nonzero if failed to apply. Name of culprit. > + */ > + > + nsresult > + UpdateShareOfSingleSource(AllocationHandle* aHandle, Maybe rename this to make it clear that the intent is to reevaluate the current allocation considering a new constraint set for a shared device ?

Comment on attachment 8772092 [details] Bug 1286096 - Consider competing audio constraints as well. https://reviewboard.mozilla.org/r/65002/#review64484 ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:232 (Diff revision 2) > const char** aOutBadConstraint) > { > AssertIsOnOwningThread(); > - if (mState == kReleased) { > + nsresult rv = Super::Allocate(aConstraints, aPrefs, aDeviceId, aOrigin, > + aOutHandle, aOutBadConstraint); > + if (NS_FAILED(rv)) { Can you not just return rv ?

Comment on attachment 8772087 [details] Bug 1286096 - Have MediaEngineDefaultVideoSource inherit from MediaEngineCameraVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64992/diff/1-2/

Comment on attachment 8772088 [details] Bug 1286096 - Enable width/height constraints on fake device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64994/diff/1-2/

Comment on attachment 8772089 [details] Bug 1286096 - Remove fakeTracks constraint. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64996/diff/2-3/

Comment on attachment 8772090 [details] Bug 1286096 - Move AllocationHandle used for cameras to MediaEngineSource base class to reuse for microphones. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64998/diff/2-3/

Comment on attachment 8772091 [details] Bug 1286096 - Move UpdateSingleSource pattern to MediaEngine base class for reuse. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65000/diff/2-3/

Comment on attachment 8772092 [details] Bug 1286096 - Consider competing audio constraints as well. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65002/diff/2-3/

Comment on attachment 8772093 [details] Bug 1286096 - Wire up audio getSettings(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65004/diff/2-3/

Thanks Paul! Feedback addressed. Note, I'm still missing a review from you on the last patch I think: https://reviewboard.mozilla.org/r/65004/diff/3/ MozReview's UI shows it as green because smaug r+'ed it, but I think he only looked at the webidl. Did you miss it?

https://reviewboard.mozilla.org/r/64994/#review64492 This patch really belongs in Bug 1088621 is the real issue. I'll see if I can rebase it.

Comment on attachment 8772093 [details] Bug 1286096 - Wire up audio getSettings(). https://reviewboard.mozilla.org/r/65004/#review64772

Comment on attachment 8772087 [details] Bug 1286096 - Have MediaEngineDefaultVideoSource inherit from MediaEngineCameraVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64992/diff/2-3/

Comment on attachment 8772088 [details] Bug 1286096 - Enable width/height constraints on fake device. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64994/diff/2-3/

Comment on attachment 8772089 [details] Bug 1286096 - Remove fakeTracks constraint. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64996/diff/3-4/

Comment on attachment 8772090 [details] Bug 1286096 - Move AllocationHandle used for cameras to MediaEngineSource base class to reuse for microphones. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64998/diff/3-4/

Comment on attachment 8772091 [details] Bug 1286096 - Move UpdateSingleSource pattern to MediaEngine base class for reuse. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65000/diff/3-4/

Comment on attachment 8772092 [details] Bug 1286096 - Consider competing audio constraints as well. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65002/diff/3-4/

Comment on attachment 8772093 [details] Bug 1286096 - Wire up audio getSettings(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65004/diff/3-4/

Rebased.

Pushed by jbruaroey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ca07b7baf290 Have MediaEngineDefaultVideoSource inherit from MediaEngineCameraVideoSource. r=padenot https://hg.mozilla.org/integration/autoland/rev/8a75cd08366e Enable width/height constraints on fake device. r=padenot https://hg.mozilla.org/integration/autoland/rev/54d171fd763a Remove fakeTracks constraint. r=drno,smaug https://hg.mozilla.org/integration/autoland/rev/3b9c5f9d8a42 Move AllocationHandle used for cameras to MediaEngineSource base class to reuse for microphones. r=padenot https://hg.mozilla.org/integration/autoland/rev/e3354c949659 Move UpdateSingleSource pattern to MediaEngine base class for reuse. r=padenot https://hg.mozilla.org/integration/autoland/rev/274a3c105eb5 Consider competing audio constraints as well. r=padenot https://hg.mozilla.org/integration/autoland/rev/26085bd0d428 Wire up audio getSettings(). r=padenot,smaug

https://hg.mozilla.org/mozilla-central/rev/ca07b7baf290 https://hg.mozilla.org/mozilla-central/rev/8a75cd08366e https://hg.mozilla.org/mozilla-central/rev/54d171fd763a https://hg.mozilla.org/mozilla-central/rev/3b9c5f9d8a42 https://hg.mozilla.org/mozilla-central/rev/e3354c949659 https://hg.mozilla.org/mozilla-central/rev/274a3c105eb5 https://hg.mozilla.org/mozilla-central/rev/26085bd0d428