From rationale here: https://github.com/w3c/mediacapture-main/issues/387#issuecomment-243603052 We want web developers to use track.enabled for in-content audio/video muting, as that's the right API, and not play tricks with stop() and calling gUM again, which by default cause permission prompts in Firefox on "unmute". The spec says that "When all tracks connected to a source are muted or disabled, the "on-air" or "recording" indicator for that source can be turned off; when the track is no longer muted or disabled, it MUST be turned back on." We don't want to turn off our in-browser indicator, because the site still has permission to unmute at any time, but we can turn off the device itself, so the camera light goes out and users won't have to wonder if mute worked. Microphones don't have lights, so we should also consider a new UX indicator state for mute (e.g. the existing red indicator with a slash through it, something like that, but it gets tricky with cam+mic), but that would be a separate issue, but we should add support for UX to discover this muted state, which makes sense to include here.

Mass change P2->P3 to align with new Mozilla triage process.

I'm increasing priority here to match bug 1333468, and starting work.

This is a good start. Stopping the video device works, but don't flip the audio pref yet - the microphone MediaDevice is not up for the task. It should however be enough for you to get going on top of the API changes Johann. I've been using this pretty basic fiddle for manual tests so far: https://jsfiddle.net/pehrsons/89gdpkd5/

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review206086 ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:671 (Diff revision 1) > void > MediaEngineWebRTCMicrophoneSource::NotifyPull(MediaStreamGraph *aGraph, > - SourceMediaStream *aSource, > + SourceMediaStream *aStream, > TrackID aID, > StreamTime aDesiredTime, > const PrincipalHandle& aPrincipalHandle) > { > // Ignore - we push audio data > LOG_FRAMES(("NotifyPull, desired = %" PRId64, (int64_t) aDesiredTime)); > } As I said in comment 12 this is not up for the task of stopping and starting the device. It's hopefully enough to insert silence in NotifyPull however, but I will have to dig a bit deeper.

Comment on attachment 8929563 [details] Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. https://reviewboard.mozilla.org/r/200840/#review206088 ::: dom/media/MediaManager.cpp:312 (Diff revision 1) > + // true if we are allowed to stop mAudioDevice when becoming disabled. > + // MainThread only. > + bool mAudioDeviceStopOnDisable; > + > + // true if we are allowed to stop mVideoDevice when becoming disabled. > + // MainThread only. > + bool mVideoDeviceStopOnDisable; > + > + // true if this audio device is currently disabled. MainThread only. > + bool mAudioDeviceEnabled; > + > + // true if this video device is currently disabled. MainThread only. > + bool mVideoDeviceEnabled; > + > + // true if the application has currently enabled this audiodevice. > + // MainThread only. > + bool mAudioDeviceIntendedEnabled; > + > + // true if the application has currently enabled this video device. > + // MainThread only. > + bool mVideoDeviceIntendedEnabled; > + > + // true if an operation to Start() or Stop() the audio device has been > + // dispatched to the media thread and is not finished yet. > + // MainThread only. > + bool mAudioDeviceOperationInProgress; > + > + // true if an operation to Start() or Stop() the video device has been > + // dispatched to the media thread and is not finished yet. > + // MainThread only. > + bool mVideoDeviceOperationInProgress; I'm going to clean all these variables up so we don't have dupes for audio and video, but you get the general idea of how this will behave.

Comment on attachment 8929560 [details] Bug 1299515 - Remove special handling for releasing callbacks on main thread. https://reviewboard.mozilla.org/r/200834/#review206100 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/MediaManager.cpp:687 (Diff revision 1) > - onFailure->OnError(error); > + mOnFailure->OnError(error); > } > return NS_OK; > } > private: > - ~ErrorCallbackRunnable() > + ~ErrorCallbackRunnable() = default; Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override] ~ErrorCallbackRunnable() = default; ^ override

Comment on attachment 8929564 [details] Bug 1299515 - Wire up track-disabling logic to frontend APIs. https://reviewboard.mozilla.org/r/200842/#review206322 The API works for me. As Florian mentioned in bug 1333468, I'm not sure why we wouldn't want this for screensharing as well, but I'd leave that up to you :)

It's easy to add for screen-/app-/windowsharing, but since, as I understand it, there's nothing in the frontend code that will care about it I'm a bit reluctant. If you promise to add tests for this added state I will go ahead and unify the API.

(In reply to Andreas Pehrson [:pehrsons] from comment #17) > It's easy to add for screen-/app-/windowsharing, but since, as I understand > it, there's nothing in the frontend code that will care about it I'm a bit > reluctant. If you promise to add tests for this added state I will go ahead > and unify the API. In the end it's your team's call, I was just wondering why you'd make the API behavior inconsistent, but I wouldn't mind either way. It's not particularly hard for frontend code to handle this, and I've already done that in my patch for bug 1333468. And yes, we'd try to test anything that was changed in the patch :)

(In reply to Johann Hofmann [:johannh] from comment #18) > And yes, we'd try to test anything that was changed in the patch :) Cool, I'll fix the rest of the API accordingly then.

Comment on attachment 8929560 [details] Bug 1299515 - Remove special handling for releasing callbacks on main thread. https://reviewboard.mozilla.org/r/200834/#review207420 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/MediaManager.cpp:676 (Diff revision 2) > - onFailure->OnError(error); > + mOnFailure->OnError(error); > } > return NS_OK; > } > private: > - ~ErrorCallbackRunnable() > + ~ErrorCallbackRunnable() = default; Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override] ~ErrorCallbackRunnable() = default; ^ override

Comment on attachment 8932871 [details] Bug 1299515 - Use an I420BufferPool for allocations in VideoFrameConverter. https://reviewboard.mozilla.org/r/203896/#review209390 lgtm ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h:202 (Diff revision 1) > unsigned short sending_width, > unsigned short sending_height) const; > > /** > - * Function to deliver a capture video frame for encoding and transport > - * @param video_frame: pointer to captured video-frame. > + * Function to deliver a capture video frame for encoding and transport. > + * If the frame's timestamp is 0, it will be automatcally generated. typo: s/automatcally/automatically/ ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h (Diff revision 1) > - * @param captured_time: timestamp when the frame was captured. > - * if 0 timestamp is automatcally generated by the engine. > + * transmitted by the conduit. > + */ > - *NOTE: ConfigureSendMediaCodec() SHOULD be called before this function can be invoked > - * This ensures the inserted video-frames can be transmitted by the conduit > - */ > - virtual MediaConduitErrorCode SendVideoFrame(unsigned char* video_frame, You'll need to change [1] to use the other version of SendVideoFrame. [1] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/media/webrtc/signaling/gtest/videoconduit_unittests.cpp#109 ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:152 (Diff revision 1) > + if (aChunk.IsNull()) { > + aForceBlack = true; > + } else { > + aForceBlack = aChunk.mFrame.GetForceBlack(); > + } > + nit: extra whitespace ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:165 (Diff revision 1) > + } > + > + const double duplicateMinFps = 1.0; > + TimeStamp t = aChunk.mTimeStamp; > + MOZ_ASSERT(!t.IsNull()); > + if (serial == last_img_ && Should !t.IsNull() be a condition here, just in case? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:375 (Diff revision 1) > Stringify(format).c_str(), Stringify(surf->GetType()).c_str(), > Stringify(surf->GetFormat()).c_str()); > return; > } > > + MOZ_ASSERT(aImage->GetSize() == aSize); It looks like if aImage is smaller than aSize, we could end up with memory corruption when we do the copies down below. It seems like it would be safer to return early with an error if this occurs. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:376 (Diff revision 1) > Stringify(surf->GetFormat()).c_str()); > return; > } > > + MOZ_ASSERT(aImage->GetSize() == aSize); > IntSize size = aImage->GetSize(); I don't think it makes sense to preserve these checks now that the allocation is deferred to mBufferPool. We should use the values from the buffer rather than what we calculate here. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:410 (Diff revision 1) > switch (surf->GetFormat()) { > case SurfaceFormat::B8G8R8A8: > case SurfaceFormat::B8G8R8X8: > rv = libyuv::ARGBToI420(static_cast<uint8*>(map.GetData()), > map.GetStride(), > - yuv, size.width, > + buffer->MutableDataY(), size.width, Please use buffer->StrideY(), buffer->StrideU(), and buffer->StrideV() here and below.

Comment on attachment 8929556 [details] Bug 1299515 - Plumb MediaStreamTrack enabled state to sources. https://reviewboard.mozilla.org/r/200826/#review211280 Just comment nits ::: dom/media/MediaStreamTrack.h:86 (Diff revision 3) > + * temporarily stop. > + * > + * When the underlying enabled state of the sink changes, > + * call MediaStreamTrackSource::SinkEnabledStateChanged(). > + * > + * Typically MediaStreamTrack::Sink returns the track's enabled state and Do you mean MediaStreamTrack here? ::: dom/media/MediaStreamTrack.h:176 (Diff revision 3) > * KeepsSourceAlive() == true have unregistered. > */ > virtual void Stop() = 0; > > /** > + * Called by the source interface when all registered sink in sinks ::: dom/media/MediaStreamTrack.h:177 (Diff revision 3) > */ > virtual void Stop() = 0; > > /** > + * Called by the source interface when all registered sink in > + * SinkMode::AffectingSource become disabled. with KeepsSourceAlive() == true ::: dom/media/MediaStreamTrack.h:183 (Diff revision 3) > + */ > + virtual void Disable() = 0; > + > + /** > + * Called by the source interface when at least one registered sink in > + * SinkMode::AffectingSource become enabled. sink with KeepsSourceAlive() == true ::: dom/media/MediaStreamTrack.h:191 (Diff revision 3) > + > + /** > + * Called when a Sink's Enabled() state changed. Will iterate through all > + * sinks and notify the source of the aggregated enabled state. > + * > + * Note that a Sink in SinkMode::ListenOnly counts as disabled. sink with KeepsSourceAlive() == false

Comment on attachment 8929557 [details] Bug 1299515 - Remove GetUserMediaNotificationEvent. https://reviewboard.mozilla.org/r/200828/#review211284 ::: dom/media/MediaManager.cpp:4295 (Diff revision 3) > nsContentUtils::RunInStableState(runnable.forget()); > mChromeNotificationTaskPosted = true; > } > > void > GetUserMediaWindowListener::NotifyChromeOfTrackStops() NotifyChrome? ::: dom/media/MediaManager.cpp:4301 (Diff revision 3) > - GetUserMediaNotificationEvent::STOPPING, mWindowID))); > -} > + NS_DispatchToMainThread(NS_NewRunnableFunction("MediaManager::NotifyChrome", > + [windowID]() { Tip: You could use generalized lambda capture here: [windowID = mWindowID]() {

Comment on attachment 8929558 [details] Bug 1299515 - Remove ReleaseMediaOperationResource. https://reviewboard.mozilla.org/r/200830/#review211286

Comment on attachment 8929559 [details] Bug 1299515 - Remove superfluous include in MediaStreamTrack.h. https://reviewboard.mozilla.org/r/200832/#review211288

Comment on attachment 8929560 [details] Bug 1299515 - Remove special handling for releasing callbacks on main thread. https://reviewboard.mozilla.org/r/200834/#review211292

Comment on attachment 8929561 [details] Bug 1299515 - Only notify of enabled state if it actually changed. https://reviewboard.mozilla.org/r/200836/#review211294

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review211296 Nice! Nits, tips and two questions ::: dom/media/webrtc/MediaEngine.h:377 (Diff revision 3) > + class Track { > + public: > + Track() = delete; > + ~Track() = default; > + Track(const RefPtr<SourceMediaStream>& aStream, > + const TrackID& aTrackID, Nit: TrackID is an int. Pass primitive types by value. ::: dom/media/webrtc/MediaEngineCameraVideoSource.h:116 (Diff revision 3) > - // mMonitor also protects mSources[] and mPrincipalHandles[] access/changes. > - // mSources[] and mPrincipalHandles[] are accessed from webrtc threads. > + // (which are combined with EndTrack() and image changes). > + // - mTracks[] access/changes. mTracks[] is accessed on the media thread. > > // All the mMonitor accesses are from the child classes. > Monitor mMonitor; // Monitor for processing Camera frames. > - nsTArray<RefPtr<SourceMediaStream>> mSources; // When this goes empty, we shut down HW > + nsTArray<Track> mTracks; // When this has no enabled tracks, we shut down HW With all the for-loops looking up tracks in this patch, might it be faster to use std::set here? Then we could use set::find(), or does order matter in InsertInGraph? ::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:37 (Diff revision 3) > + // nullptr images are allowed, but we force it to black and retain the size. > + IntSize size(mWidth, mHeight); Before this patch we passed in 0,0 here, now we pass in mWidth,mHeight? What's the difference? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:171 (Diff revision 3) > - mSources.AppendElement(aStream); > - mPrincipalHandles.AppendElement(aPrincipalHandle); > + Track t(aStream, aTrackID, aPrincipal); > + mTracks.AppendElement(Move(t)); Inlining makes it an r-value: mTracks.AppendElement(Track(aStream, aTrackID, aPrincipal)); ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:186 (Diff revision 3) > + Track t(aStream, aTrackID, PRINCIPAL_HANDLE_NONE); > + size_t i = mTracks.IndexOf(t); inline t? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:213 (Diff revision 3) > + Track* track = nullptr; > + for (Track& t : mTracks) { > + if (t.mStream == aStream && t.mTrackID == aTrackID) { Maybe put the Track::operator== to use here? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:257 (Diff revision 3) > { > MonitorAutoLock lock(mMonitor); > > - // Drop any cached image so we don't start with a stale image on next > - // usage. Also, gfx gets very upset if these are held until this object > - // is gc'd in final-cc during shutdown (bug 1374164) > + bool found = false; > + for (Track& t : mTracks) { > + if (t.mStream != aStream || t.mTrackID != aTrackID) { ditto ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:506 (Diff revision 3) > - { > - MonitorAutoLock lock(mMonitor); > + Track t(aStream, aTrackID, aPrincipal); > + mTracks.AppendElement(Move(t)); inline ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:643 (Diff revision 3) > if (mState != kStarted) { > - return NS_ERROR_FAILURE; > + MOZ_ASSERT_UNREACHABLE("Should be started when stopping"); > + return NS_ERROR_UNEXPECTED; > } > - if (!mVoEBase) { > - return NS_ERROR_FAILURE; > + > + mAudioInput->StopRecording(aStream); You've moved this in under lock(mMonitor), which it wasn't under before. On purpose? If yes, do we need to uplift something?

Comment on attachment 8930890 [details] Bug 1299515 - Drive by touchups in MediaTrackConstraints. https://reviewboard.mozilla.org/r/202006/#review211304

Comment on attachment 8932866 [details] Bug 1299515 - Put pushPrefs in head.js. https://reviewboard.mozilla.org/r/203886/#review211306

Comment on attachment 8930889 [details] Bug 1299515 - Make `const nsMainThreadPtrHandle<T>` usable. https://reviewboard.mozilla.org/r/202004/#review211534 Seems reasonable to follow `RefPtr`'s pattern. r=me. I wonder if we should be splitting definitions for lvalues and rvalues with `const &` and `const && = delete` for the implicit conversion operator as well. Regardless, that should be done in a follow up, this is fine.

Comment on attachment 8929563 [details] Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. https://reviewboard.mozilla.org/r/200840/#review211310 r- for passing a raw this pointer to a timer callback. Otherwise looks good. ::: commit-message-a881c:6 (Diff revision 3) > +This is possible for: > +- Camera (enabled by default, controlled by pref > + "media.getusermedia.camera.stop_on_disable.enabled") > +- Microphone (disabled by default, controlled by pref > + "media.getusermedia.microphone.stop_on_disable.enabled") > + > +Screen-, app-, or windowsharing is not supported at this time. > + > +On disabling, there's a delay before the device is ordered to stop. This is > +now defaulting to 3 seconds but can be overriden by prefs > +"media.getusermedia.camera.stop_on_disable.delay_ms" and > +"media.getusermedia.microphone.stop_on_disable.delay_ms". > + > +The delay is in place to prevent misuse by malicious sites. If a track is > +re-enabled before the delay has passed, the device will not be touched until > +another disable followed by the full delay happens. Can we have this as a code comment somewhere as well? ::: commit-message-a881c:7 (Diff revision 3) > +- Camera (enabled by default, controlled by pref > + "media.getusermedia.camera.stop_on_disable.enabled") "stop" may make sense in the code (matches Start() and Stop()) but outside in a pref it seems a bit finite, given our other uses of this word in our domain, like track.stop() and transceiver.stop(). Maybe use "off" instead? "media.getusermedia.microphone.off_while_disabled.enabled" Seems more transient. ::: dom/media/MediaManager.cpp:157 (Diff revision 3) > +struct DeviceState { > +public: struct is public by default. Did you mean class? ::: dom/media/MediaManager.cpp:164 (Diff revision 3) > + , mDevice(aDevice) > + { > + } Maybe MOZ_ASSERT(mDevice)? ::: dom/media/MediaManager.cpp:184 (Diff revision 3) > + // true if we are allowed to stop the underlying source when all tracks > + // are disabled. > + bool mStopOnDisable = false; > + > + // Timer triggered by a MediaStreamTrackSource signaling that all tracks got > + // disabled. When the timer fires we initiate Stop()ing mDevice. > + // If set we allow dynamically stopping and starting mDevice. > + const nsMainThreadPtrHandle<nsITimer> mDisableTimer; > + > + // Used to resolve the callbacks for the timer above. > + MozPromiseHolder<GenericPromise> mDisableTimerPromise; > + > + // The underlying device we keep state for. Always non-null. > + const RefPtr<MediaDevice> mDevice; What are we to surmise about the fields that don't have comments saying mainthread only? I see no locking. ::: dom/media/MediaManager.cpp:184 (Diff revision 3) > + // true if we are allowed to stop the underlying source when all tracks > + // are disabled. > + bool mStopOnDisable = false; Maybe mOffWhileDisabled? ::: dom/media/MediaManager.cpp:893 (Diff revision 3) > - if (mMediaSource == MediaSourceEnum::Microphone) { > + if (GetMediaSource() == MediaSourceEnum::Microphone) { > aMediaSource.AssignLiteral(u"microphone"); > - } else if (mMediaSource == MediaSourceEnum::AudioCapture) { > + } else if (GetMediaSource() == MediaSourceEnum::AudioCapture) { > aMediaSource.AssignLiteral(u"audioCapture"); > - } else if (mMediaSource == MediaSourceEnum::Window) { // this will go away > + } else if (GetMediaSource() == MediaSourceEnum::Window) { // this will go away > aMediaSource.AssignLiteral(u"window"); > } else { // all the rest are shared How about a switch()? ::: dom/media/MediaManager.cpp:3818 (Diff revision 3) > - // Audio already stopped > - return; > - } > + const UniquePtr<DeviceState>& state = aTrackID == kAudioTrack > + ? mAudioDeviceState > + : mVideoDeviceState; auto& seems fine here. state is one or the other. No need to bring up type. Applies throughout. ::: dom/media/MediaManager.cpp:3839 (Diff revision 3) > - MOZ_ASSERT(false, "Unknown track id"); > + if (!state->mDevice) { > + MOZ_ASSERT_UNREACHABLE("Device is not stopped, so where is it?"); > - return; > + return; > - } > + } Unnecessary, since mDevice is const. ::: dom/media/MediaManager.cpp:3891 (Diff revision 3) > + if (!state) { > + MOZ_ASSERT_UNREACHABLE("DeviceState is supposed to not go away"); > - return; > + return; > - } > + } I'm for distrusting input, but for internal consistency checks, I prefer debug only (MOZ_ASSERT). ::: dom/media/MediaManager.cpp:3896 (Diff revision 3) > - break; > + uint32_t stopDelay = 0; > + if (aTrackID == kAudioTrack) { > + stopDelay = Preferences::GetUint("media.getusermedia.microphone.stop_on_disable.delay_ms", 3000); Nit: put condition inside GetUint's arg and we can use const uint_t for stopDelay ::: dom/media/MediaManager.cpp:3910 (Diff revision 3) > - // Video stopped. Disabling is pointless. > + if (!state->mDevice) { > + MOZ_ASSERT_UNREACHABLE("Device is not stopped, so where is it?"); > - return; > + return; > - } > + } Can't happen. What's the point of invariants if we can't rely on them to simplify the code? ::: dom/media/MediaManager.cpp:3929 (Diff revision 3) > + nsresult rv = state->mDisableTimer->InitWithNamedFuncCallback( > + [](nsITimer* aTimer, void* aClosure) { > + RefPtr<SourceListener> self = static_cast<SourceListener*>(aClosure); Why rely on the opaque aClosure void* for context when we have lambda capture? Passing `this` as a raw pointer seems unsafe. The refcount here happens too late, when the timer fires, right? What keeps it alive?

Comment on attachment 8929563 [details] Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. https://reviewboard.mozilla.org/r/200840/#review212192 Very nice! (I hit Publish by accident). Some more nits, a question, and I don't see where we clear inProgres in DisableTrack. ::: dom/media/MediaManager.cpp:175 (Diff revision 3) > + // true if the application has currently enabled mDevice. > + // MainThread only. > + bool mIntendedEnabled = true; "IntendedEnabled" sounds odd to me, as opposed to unintended? How about: mTrackEnabled ? ::: dom/media/MediaManager.cpp:3924 (Diff revision 3) > - default: { > - MOZ_ASSERT(false, "Unknown track id"); > + > + if (state->mOperationInProgress) { > - return; > + return; > - } > + } > + > + // All paths from here on must end in setting `inProgress` to false. And where do we do that? ::: dom/media/MediaManager.cpp:3932 (Diff revision 3) > + if (self->mAudioDeviceState && > + aTimer == self->mAudioDeviceState->mDisableTimer) { > + self->mAudioDeviceState->mDisableTimerPromise.Resolve(true, __func__); > + } else if (self->mVideoDeviceState && > + aTimer == self->mVideoDeviceState->mDisableTimer) { > + self->mVideoDeviceState->mDisableTimerPromise.Resolve(true, __func__); > + } else { > + MOZ_CRASH("invalid timer pointer. This is unexpected."); Also, why rely on aTimer? We'd know which one it is if we captured &state like you do 7 lines below. Should simplify this logic and remove the need for a MOZ_CRASH. Also, can we hide this timer init code in a DeviceState.Wait() method that returns a promise? Capturing a RefPtr to self in the .Then() callback (like you do below) should be sufficient to keep self alive I think. ::: dom/media/MediaManager.cpp:4030 (Diff revision 3) > - break; > + if (!state->mDevice) { > + MOZ_ASSERT_UNREACHABLE("Device is not stopped, so where is it?"); > + return; > - } > + } invariant ::: dom/media/MediaManager.cpp:4039 (Diff revision 3) > + state->mDisableTimer->Cancel(); > + state->mDisableTimerPromise.RejectIfExists(NS_ERROR_ABORT, __func__); Could this be put in a state->CancelWait() function? ::: dom/media/MediaManager.cpp:4098 (Diff revision 3) > + if (!state) { > + MOZ_ASSERT_UNREACHABLE("DeviceState is supposed to not go away"); > + return; > + } MOZ_ASSERT ::: dom/media/MediaManager.cpp:4111 (Diff revision 3) > + if (state->mStopped) { > + // Device was stopped on main thread during the operation. Nothing to do. > - return; > + return; > - } > + } > + > + if (!state->mDevice) { invariant ::: dom/media/MediaManager.cpp:4140 (Diff revision 3) > + // Starting the device failed. Stopping the track here will make the > + // MediaStreamTrack end after a pass through the MediaStreamGraph. > + StopTrack(aTrackID); Will this fire ended? ::: dom/media/MediaManager.cpp:4318 (Diff revision 3) > SourceListener::CapturingVideo() const > { > MOZ_ASSERT(NS_IsMainThread()); > - return Activated() && mVideoDevice && !mVideoStopped && > - !mVideoDevice->GetSource()->IsAvailable() && > + return Activated() && mVideoDeviceState && > + !mVideoDeviceState->mStopped && You've removed IsAvailable() here. Does some invariant make this the same as before, or is this a behavior change? ::: dom/media/MediaManager.cpp:4393 (Diff revision 3) > - RefPtr<AudioDevice> audioDevice = > - aTrackID == kAudioTrack ? mAudioDevice.get() : nullptr; > - RefPtr<VideoDevice> videoDevice = > + const UniquePtr<DeviceState>& state = aTrackID == kAudioTrack > + ? mAudioDeviceState > + : mVideoDeviceState; This line is repeated so much, should we put it in an accessor member function? E.g. GetStateFor(aTrackID); ::: dom/media/MediaManager.cpp:4554 (Diff revision 3) > + return; > + } > })); superfluous return

Comment on attachment 8932867 [details] Bug 1299515 - Modernize tests that disable tracks to prime for coming patches. https://reviewboard.mozilla.org/r/203888/#review212312

Comment on attachment 8932868 [details] Bug 1299515 - Set track stop_on_disable prefs to act immediately in test. https://reviewboard.mozilla.org/r/203890/#review212314 Lgtm. Would it be useful to test at least once with a non-zero delay, or is that just a slower test of the same code? On linux, is there a way to observe a difference when the delay is set, i.e. whether the device is actually stopped or not, to test that the delay does something?

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review211496 ::: dom/media/webrtc/MediaEngineCameraVideoSource.h:116 (Diff revision 3) > - // mMonitor also protects mSources[] and mPrincipalHandles[] access/changes. > - // mSources[] and mPrincipalHandles[] are accessed from webrtc threads. > + // (which are combined with EndTrack() and image changes). > + // - mTracks[] access/changes. mTracks[] is accessed on the media thread. > > // All the mMonitor accesses are from the child classes. > Monitor mMonitor; // Monitor for processing Camera frames. > - nsTArray<RefPtr<SourceMediaStream>> mSources; // When this goes empty, we shut down HW > + nsTArray<Track> mTracks; // When this has no enabled tracks, we shut down HW std::set use a red-black tree usually, which has very bad locality, and will end up slower than iterating every time on the array (that is compact). "Fancy algorithms are slow when n is small, and n is usually small" - Rob Pike.

Comment on attachment 8932869 [details] Bug 1299515 - Fix MediaEngineDefault. https://reviewboard.mozilla.org/r/203892/#review212336

Comment on attachment 8932870 [details] Bug 1299515 - Clean up MediaEngineRemoteVideoSource::FrameSizeChange. https://reviewboard.mozilla.org/r/203894/#review212340 ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:420 (Diff revision 1) > - if ((mWidth < 0) || (mHeight < 0) || > - (w != (unsigned int) mWidth) || (h != (unsigned int) mHeight)) { > - LOG(("MediaEngineRemoteVideoSource Video FrameSizeChange: %ux%u was %ux%u", w, h, mWidth, mHeight)); > - mWidth = w; > + if (aWidth == static_cast<unsigned int>(mWidth) && > + aHeight == static_cast<unsigned int>(mHeight)) { > + MOZ_ASSERT_UNREACHABLE("Unexpected frame size change that didn't change"); > + return; Calling this with unchanged values seems harmless and used to be idempotent, whereas now it asserts in debug build. That seems aggressive. Are we sure this is only called when necessary, i.e. that it won't crash debug builds from normal use? E.g. from [1] [1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/systemservices/CamerasChild.cpp#703 ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:426 (Diff revision 1) > - auto settings = mSettings; > - NS_DispatchToMainThread(media::NewRunnableFrom([settings, w, h]() mutable { > - settings->mWidth.Value() = w; > - settings->mHeight.Value() = h; > + if (aWidth == 0 || aHeight == 0) { > + MOZ_ASSERT_UNREACHABLE("We'd rather not get a frame if it's 0 pixels large"); > + return; > + } Nit: Of the two, I would put this assert (validating inputs) first.

Comment on attachment 8929564 [details] Bug 1299515 - Wire up track-disabling logic to frontend APIs. https://reviewboard.mozilla.org/r/200842/#review212352 Lgtm, except IsAvailable() is backwards now. r=me with that fixed. ::: commit-message-e9e09:4 (Diff revision 3) > +actively captured or not. Note that this is not the same as the device being > +stopped. If we don't allow a device to stop by pref, we still notify chrome "Note that this is not the same as the device being on" ::: commit-message-e9e09:6 (Diff revision 3) > +Bug 1299515 - Wire up track-disabling logic to frontend APIs. r?jib, r?johannh > + > +This modifies mediaCaptureWindowState() to say whether a camera or microphone is > +actively captured or not. Note that this is not the same as the device being > +stopped. If we don't allow a device to stop by pref, we still notify chrome > +that the capture is not active (aka disabled). Double-negative. If I read it right, it seems equivalent to: "If we disallow a device from being off while disabled, we still notify chrome that we're not actively capturing" ::: browser/modules/ContentWebRTC.jsm:312 (Diff revision 3) > - if (tabState.camera) > + if (tabState.camera == MediaManagerService.STATE_CAPTURE_ENABLED) > state.showCameraIndicator = true; > - if (tabState.microphone) > + if (tabState.camera == MediaManagerService.STATE_CAPTURE_DISABLED) > + state.showCameraIndicator = true; > + if (tabState.microphone == MediaManagerService.STATE_CAPTURE_ENABLED) > + state.showMicrophoneIndicator = true; > + if (tabState.microphone == MediaManagerService.STATE_CAPTURE_DISABLED) > state.showMicrophoneIndicator = true; Nit: Maybe use switches? ::: dom/media/MediaManager.cpp:3462 (Diff revision 3) > CaptureWindowStateCallback(MediaManager *aThis, > uint64_t aWindowID, > GetUserMediaWindowListener *aListener, > void *aData) > { > - struct CaptureWindowStateData *data = (struct CaptureWindowStateData *) aData; > + auto data = static_cast<CaptureWindowStateData*>(aData); Idea: Since aData is not supposed to be null, we could MOZ_ASSERT here and do: auto& data = *static_cast<CaptureWindowStateData*>(aData); ...here, and then: *data.mCamera = One less thing going on. ::: dom/media/MediaManager.cpp:4348 (Diff revision 3) > -bool > -SourceListener::CapturingWindow() const > -{ > - MOZ_ASSERT(NS_IsMainThread()); > + const UniquePtr<DeviceState>& state = GetAudioDevice() && > + GetAudioDevice()->GetMediaSource() == aSource > + ? mAudioDeviceState > + : mVideoDeviceState; Call me old-school, but I'd prefer some parens around the condition here. ::: dom/media/webrtc/MediaEngineDefault.h:190 (Diff revision 3) > + bool IsAvailable() const > + { > + return mState == kAllocated || mState == kStarted; Backwards ::: dom/media/webrtc/MediaEngineDefault.h:235 (Diff revision 3) > - nsTArray<RefPtr<MediaEngineVideoSource> > mVSources; > - nsTArray<RefPtr<MediaEngineAudioSource> > mASources; > + nsTArray<RefPtr<MediaEngineDefaultVideoSource> > mVSources; > + nsTArray<RefPtr<MediaEngineDefaultAudioSource> > mASources; Nit: space between >> is no longer needed. ::: dom/media/webrtc/MediaEngineDefault.cpp:523 (Diff revision 3) > MutexAutoLock lock(mMutex); > - int32_t len = mASources.Length(); > > // aMediaSource is ignored for audio devices (for now). > > - for (int32_t i = 0; i < len; i++) { > + for (const RefPtr<MediaEngineDefaultAudioSource>& source : mASources) { auto& wfm

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review211296 > Before this patch we passed in 0,0 here, now we pass in mWidth,mHeight? What's the difference? Consider this code: ``` async function disableForASecond() { let s = await navigator.mediaDevices.getUserMedia({video: true}); let t = s.getTracks()[0]; let v = document.createElement("video"); v.srcObject = s; v.play(); await new Promise(r => v.onloadeddata = r); t.enabled = false; await new Promise(r => setTimeout(r, 1000)); t.enabled = true; } ``` If we still had code to set the size to (0, 0) for null images, during the 1s the track is disabled, the media element would display black frames at 0x0 resolution (Since Stop() clears mImage). Passing the widthxheight of the last real frame makes it show black frames at widthxheight resolution instead, which is what we want. We could put the image-clearing in Deallocate() instead, but then we'd have to introduce more state, or have a glitch showing the last frame from before the stop when resuming. This warrants keeping it in Stop() IMO. > Maybe put the Track::operator== to use here? I've scrapped this model now actually. You can check this in a new patch coming up (for the mic source). > You've moved this in under lock(mMonitor), which it wasn't under before. On purpose? If yes, do we need to uplift something? The threading model here was utterly confusing and underdocumented before so this might have been going overboard. I've tried to clarify this in new patches, and adjusted code accordingly. You'll get to review them soon.

Comment on attachment 8929563 [details] Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. https://reviewboard.mozilla.org/r/200840/#review211310 > "stop" may make sense in the code (matches Start() and Stop()) but outside in a pref it seems a bit finite, given our other uses of this word in our domain, like track.stop() and transceiver.stop(). Maybe use "off" instead? > > "media.getusermedia.microphone.off_while_disabled.enabled" > > Seems more transient. I like it. > struct is public by default. Did you mean class? Oh. I meant struct but without explicitly making things public. > What are we to surmise about the fields that don't have comments saying mainthread only? I see no locking. I've clarified in comments (main thread for all except mDevice really, though the const ones are obviously threadsafe to read, but not to call into) and asserts (MediaDevice methods on owning thread, except `NotifyPull()`, which is guaranteed to happen only after setting mAllocationHandle in `Allocate()`). No easy-to-read checks for the latter sadly, but this is existing behavior. > How about a switch()? This is really out of scope, but I don't see why we can't kill the manual assignments anyway. It's not clear what "all the rest are shared" means here, but we should IMO just be able to use that path for all types, so I'll revert that code. > auto& seems fine here. state is one or the other. No need to bring up type. > > Applies throughout. "One or the other" (mAudioDeviceState, mVideoDeviceState I take it) doesn't carry type info locally though, which is great to have when reading. For one, you don't know whether you're dealing with a rawptr, refptr, uniqueptr, autoptr, or weakptr. > Can't happen. What's the point of invariants if we can't rely on them to simplify the code? OTOH, when we have invariants, how often do we have adequate testing of them not being broken? > Why rely on the opaque aClosure void* for context when we have lambda capture? > > Passing `this` as a raw pointer seems unsafe. The refcount here happens too late, when the timer fires, right? What keeps it alive? This is sadly not any lambda capture, if you actually capture something the compiler will complain. I agree this seems unsafe, but it seems to me that this is the API we have to deal with. The safety model is that we Cancel() the timer before going away and that guarantees that no further callbacks will be called. See https://searchfox.org/mozilla-central/search?q=InitWithNamedFuncCallback&path=

Comment on attachment 8932870 [details] Bug 1299515 - Clean up MediaEngineRemoteVideoSource::FrameSizeChange. https://reviewboard.mozilla.org/r/203894/#review212340 > Calling this with unchanged values seems harmless and used to be idempotent, whereas now it asserts in debug build. That seems aggressive. Are we sure this is only called when necessary, i.e. that it won't crash debug builds from normal use? E.g. from [1] > > [1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/systemservices/CamerasChild.cpp#703 This was part of the IPC layer, which is one reason why I hardened it. We don't want to send IPC messages that can be avoided. The other is that it dispatches a task to main thread. We also don't want that when it can be avoided. That said, this whole method doesn't serve a purpose anymore, since the IPC path was not used. I've inlined it into DeliverFrame which seems much cleaner.

Comment on attachment 8929563 [details] Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. https://reviewboard.mozilla.org/r/200840/#review212192 > "IntendedEnabled" sounds odd to me, as opposed to unintended? How about: > > mTrackEnabled > > ? The thinking was that this is the user's or application's intention, and since its changes are async to mEnabled (actual device enabled state), we need to keep them apart. I'm always up for a name change though, and I think mTrackEnabled makes sense with EnableTrack()/DisableTrack(). I'll also change mEnabled to mDeviceEnabled for clarity. > And where do we do that? In MediaDeviceOperationFinished(), which does a bit much checking to be inlined at the three callsites we have here. I'll note that it happens there in this comment though. > Also, why rely on aTimer? We'd know which one it is if we captured &state like you do 7 lines below. Should simplify this logic and remove the need for a MOZ_CRASH. > > Also, can we hide this timer init code in a DeviceState.Wait() method that returns a promise? Capturing a RefPtr to self in the .Then() callback (like you do below) should be sufficient to keep self alive I think. nsITimer is clunky indeed, and we can't capture anything in the lambda we give it. I'm looking at MediaTimer instead, which is promisified and based on nsITimer. Basically exactly what I want, with a patch or two to its API. > Will this fire ended? Yes. It calls into MediaEngineSource::Deallocate which ends the track in the SourceMediaStream. This then propagates the regular path through the MSG to MediaStreamTrack which fires "ended". > You've removed IsAvailable() here. Does some invariant make this the same as before, or is this a behavior change? IsAvailable() was querying the source for its internal state. On the main thread with unclear thread safety. We already keep the same state locally, so no point in asking the source. > superfluous return It's nice to have in case code gets added below this. Missing adding it is an easy mistake to make.

Comment on attachment 8929563 [details] Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. https://reviewboard.mozilla.org/r/200840/#review211310 > This is sadly not any lambda capture, if you actually capture something the compiler will complain. > I agree this seems unsafe, but it seems to me that this is the API we have to deal with. The safety model is that we Cancel() the timer before going away and that guarantees that no further callbacks will be called. > > See https://searchfox.org/mozilla-central/search?q=InitWithNamedFuncCallback&path= I've made the switch to MediaTimer now with full promisification and it's a whole lot better! Will need a new review.

Comment on attachment 8929564 [details] Bug 1299515 - Wire up track-disabling logic to frontend APIs. https://reviewboard.mozilla.org/r/200842/#review212352 > Nit: Maybe use switches? Pass. This is not keyed off on one var and nesting the switches wouldn't be pretty. I'm sure it could be improved in some way but sticking with existing behavior seems reasonable. Perhaps Johann can pick it up :-) > Backwards Not sure what I was thinking. But this has been fixed and incorporated into an earlier patch now (where the general use of IsAvailable() was removed). > auto& wfm It works, but I prefer having the type visible.

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. This will need a re-review. Sorry that the interdiff is bad. These changes came after a rebase, and interdiffs with rebases in mozreview are just bad.

Comment on attachment 8940733 [details] Bug 1299515 - Don't assert that tracks must be ended in forced shutdown. https://reviewboard.mozilla.org/r/211014/#review216764

Comment on attachment 8940732 [details] Bug 1299515 - Signal SetPullEnabled with a message. https://reviewboard.mozilla.org/r/211012/#review216762

Comment on attachment 8940726 [details] Bug 1299515 - Include nsContentUtils in TimeoutManager. https://reviewboard.mozilla.org/r/211000/#review216770

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review216996 Early feedback, as I haven't had time to complete the review. ::: dom/media/MediaManager.cpp:816 (Diff revisions 3 - 4) > mID.Assign(aID); > + > + MediaManager* mgr = MediaManager::GetIfExists(); > + MOZ_ASSERT(mgr, "If there's no manager, who's calling?"); > + mgr->PostTask(NewTaskFrom( > + [self{RefPtr<MediaDevice>(this)}, this, id{nsString(aID)}]() mutable { Won't the bare `this` trigger static analysis? ::: dom/media/MediaManager.cpp:822 (Diff revisions 3 - 4) > MediaDevice::SetRawId(const nsAString& aID) > { > mRawID.Assign(aID); > + > + MediaManager* mgr = MediaManager::GetIfExists(); > + MOZ_ASSERT(mgr, "If there's no manager, who's calling?"); > + mgr->PostTask(NewTaskFrom( > + [self{RefPtr<MediaDevice>(this)}, this, id{nsString(aID)}]() mutable { > + mRawIDMediaThread.Assign(Move(id)); > + })); What problem is being solved here? AFAICT SetId() and SetRawId() are only called by AnomymizeId() on main thread as part of enumerateDevices() initializing them before they're returned to content. Is a race possible?

Comment on attachment 8940728 [details] Bug 1299515 - Implement MediaTimer::WaitFor for convenience. https://reviewboard.mozilla.org/r/211004/#review217040 I'm not a fan of the name Wait* as it doesn't wait as such...

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. https://reviewboard.mozilla.org/r/211006/#review217042 ::: dom/media/MediaTimer.h:47 (Diff revision 1) > NS_IMETHOD_(MozExternalRefCountType) AddRef(void); > NS_IMETHOD_(MozExternalRefCountType) Release(void); > > RefPtr<MediaTimerPromise> WaitFor(const TimeDuration& aDuration, const char* aCallSite); > RefPtr<MediaTimerPromise> WaitUntil(const TimeStamp& aTimeStamp, const char* aCallSite); > + void Cancel(); // Cancel and reject any unresolve promises with false. "any unresolved" ::: dom/media/MediaTimer.cpp:102 (Diff revision 1) > } > > void > +MediaTimer::Cancel() > +{ > + MonitorAutoLock mon(mMonitor); please make Destroy() and Cancel() use common code

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review216996 > What problem is being solved here? > > AFAICT SetId() and SetRawId() are only called by AnomymizeId() on main thread as part of enumerateDevices() initializing them before they're returned to content. Is a race possible? This way of setting these strings out-of-ctor on one thread and then reading them on another thread makes thread safety harder to reason about. This was attempting to improve that, but I'll go one step further and make MediaDevice immutable. Then after the ctor these strings are inherently thread safe. There's no new racing added here, so if we're sure there was no racing before there's none now -- since the main thread `mRawID` was set just above. Had a runnable already been dispatched that might use `mRawID` we were already exposed to a race. And if there was no such runnable, then a future such runnable will read `mRawID` after it has been updated on the media thread.

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review216996 > Won't the bare `this` trigger static analysis? Apparently not: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c3c5b303411b772f6dd300c38ba60e68c4501e9

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. Please take another look -- I added a monitor lock to the Destroy path to keep the unified path simpler as to always assert the monitor is held.

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. https://reviewboard.mozilla.org/r/211006/#review217736 LGTM

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. https://reviewboard.mozilla.org/r/211006/#review218440 ::: dom/media/MediaTimer.cpp:61 (Diff revision 2) > MOZ_ASSERT(OnMediaTimerThread()); > TIMER_LOG("MediaTimer::Destroy"); > > - // Reject any outstanding entries. There's no need to acquire the monitor > - // here, because we're on the timer thread and all other references to us > - // must be gone. > + // Reject any outstanding entries. > + { > + MonitorAutoLock lock(mMonitor); Lock is not needed because no race could happen in the destructor. ::: dom/media/MediaTimer.cpp:98 (Diff revision 2) > ScheduleUpdate(); > return p; > } > > void > +MediaTimer::Cancel() Can you elaborate the use case of Cancel()? ::: dom/media/MediaTimer.cpp:103 (Diff revision 2) > +MediaTimer::Cancel() > +{ > + MonitorAutoLock mon(mMonitor); > + TIMER_LOG("MediaTimer::Cancel"); > + Reject(); > + ScheduleUpdate(); mEntries will be empty after Reject() returns. I don't see ScheduleUpdate() is needed here.

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. https://reviewboard.mozilla.org/r/211006/#review218440 > Lock is not needed because no race could happen in the destructor. I know. It's for the sake of simpler code. The alternative was to punch holes or completely remove the locking assert in Reject(). This seemed like the less bad option.

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. https://reviewboard.mozilla.org/r/211006/#review218440 > I know. It's for the sake of simpler code. The alternative was to punch holes or completely remove the locking assert in Reject(). This seemed like the less bad option. typically for that we have a BlahLocked() and Blah() method

Comment on attachment 8940729 [details] Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. https://reviewboard.mozilla.org/r/211006/#review218440 > typically for that we have a BlahLocked() and Blah() method But those would be: ``` void Blah() { MutexAutoLock lock(mMutex); BlahLocked(); } void BlahLocked() { mMutex.AssertCurrentThreadOwns(); // Do Blah } ``` ref `MediaTimer::Update/UpdateLocked`. Of course I could make BlahLocked not assert, but then what's the point? There'd be only one caller to Blah().

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review220584 Static analysis found 3 defects in this patch. - 3 defects 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/webrtc/MediaEngineRemoteVideoSource.cpp:101 (Diff revision 3) > +MediaEngineRemoteVideoSource::SetName(nsString aName) > - { > +{ > - MonitorAutoLock lock(mMonitor); > - empty = mSources.IsEmpty(); > - if (empty) { > - MOZ_ASSERT(mPrincipalHandles.IsEmpty()); > + LOG((__PRETTY_FUNCTION__)); > + AssertIsOnOwningThread(); > + > + mDeviceName = aName; Warning: Parameter 'aname' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param] ::: dom/media/webrtc/MediaEngineWebRTC.cpp:257 (Diff revision 3) > - if (mHasTabVideoSource || dom::MediaSourceEnum::Browser == aMediaSource) { > + if (mHasTabVideoSource || dom::MediaSourceEnum::Browser == aMediaSource) { > - aVSources->AppendElement(new MediaEngineTabVideoSource()); > + aSources->AppendElement(new MediaEngineTabVideoSource()); > - } > -} > + } > - > -bool > + return; > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: dom/media/webrtc/MediaTrackConstraints.cpp:519 (Diff revision 3) > > - const MediaEngineSourceType* mMediaEngineSource; > + // Then apply advanced constraints. > - nsString mDeviceId; > - }; > > - Unused << typename MockDevice::HasThreadSafeRefCnt(); > + for (int i = 0; i < int(c.mAdvanced.size()); i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment on attachment 8932866 [details] Bug 1299515 - Put pushPrefs in head.js. https://reviewboard.mozilla.org/r/203886/#review220586 Static analysis found 24 defects in this patch. - 24 defects found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/tests/mochitest/head.js:310 (Diff revision 4) > // are in place before running it. > var setTestOptions; > var testConfigured = new Promise(r => setTestOptions = r); > > +function pushPrefs(...p) { > + return SpecialPowers.pushPrefEnv({set: p}); Error: 'SpecialPowers' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:11 (Diff revision 4) > <body> > -<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1152383">ondevicechange tests</a> > <script type="application/javascript"> > +"use strict"; > > -const RESPONSE_WAIT_TIME_MS = 10000; > +createHTML({ Error: 'createHTML' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:36 (Diff revision 4) > OnDeviceChangeEvent().then(() => Promise.reject("ondevicechange event is fired unexpectedly.")), > wait(RESPONSE_WAIT_TIME_MS) > ]); > } > > -var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p}); > +runTest(async () => { Error: 'runTest' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:37 (Diff revision 4) > > -var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p}); > +runTest(async () => { > - > -var videoTracks; > - > -SimpleTest.requestCompleteLog(); > + SimpleTest.requestCompleteLog(); Error: 'SimpleTest' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:39 (Diff revision 4) > - > -function wait(time, message) { > - return new Promise(r => setTimeout(() => r(message), time)); > -} > > -pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]) > + await pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]); Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:40 (Diff revision 4) > -function wait(time, message) { > - return new Promise(r => setTimeout(() => r(message), time)); > -} > > -pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]) > -.then(() => pushPrefs(["media.navigator.permission.disabled", false])) > + await pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]); > + await pushPrefs(["media.navigator.permission.disabled", false]); Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:41 (Diff revision 4) > - return new Promise(r => setTimeout(() => r(message), time)); > -} > > -pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]) > -.then(() => pushPrefs(["media.navigator.permission.disabled", false])) > -.then(() => pushPrefs(["media.ondevicechange.enabled", true])) > + await pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]); > + await pushPrefs(["media.navigator.permission.disabled", false]); > + await pushPrefs(["media.ondevicechange.enabled", true]); Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:43 (Diff revision 4) > > -pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]) > -.then(() => pushPrefs(["media.navigator.permission.disabled", false])) > -.then(() => pushPrefs(["media.ondevicechange.enabled", true])) > -.then(() => info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => OnDeviceChangeEventNotReceived()) > + await pushPrefs(["media.ondevicechange.fakeDeviceChangeEvent.enabled", true]); > + await pushPrefs(["media.navigator.permission.disabled", false]); > + await pushPrefs(["media.ondevicechange.enabled", true]); > + > + info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); Error: 'info' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:45 (Diff revision 4) > -.then(() => pushPrefs(["media.navigator.permission.disabled", false])) > -.then(() => pushPrefs(["media.ondevicechange.enabled", true])) > -.then(() => info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => OnDeviceChangeEventNotReceived()) > -.then(() => ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => pushPrefs(['media.navigator.permission.disabled', true])) > + await pushPrefs(["media.navigator.permission.disabled", false]); > + await pushPrefs(["media.ondevicechange.enabled", true]); > + > + info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); > + await OnDeviceChangeEventNotReceived(); > + ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); Error: 'ok' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:47 (Diff revision 4) > -.then(() => info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => OnDeviceChangeEventNotReceived()) > -.then(() => ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => pushPrefs(['media.navigator.permission.disabled', true])) > -.then(() => info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => OnDeviceChangeEventReceived()) > + > + info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); > + await OnDeviceChangeEventNotReceived(); > + ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); > + > + await pushPrefs(['media.navigator.permission.disabled', true]); Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:47 (Diff revision 4) > -.then(() => info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => OnDeviceChangeEventNotReceived()) > -.then(() => ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => pushPrefs(['media.navigator.permission.disabled', true])) > -.then(() => info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => OnDeviceChangeEventReceived()) > + > + info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); > + await OnDeviceChangeEventNotReceived(); > + ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); > + > + await pushPrefs(['media.navigator.permission.disabled', true]); Error: Strings must use doublequote. [eslint: quotes] ::: dom/media/tests/mochitest/test_ondevicechange.html:49 (Diff revision 4) > -.then(() => ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted")) > -.then(() => pushPrefs(['media.navigator.permission.disabled', true])) > -.then(() => info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => OnDeviceChangeEventReceived()) > -.then(() => ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => navigator.mediaDevices.getUserMedia({video: true, fake: true})) > + await OnDeviceChangeEventNotReceived(); > + ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted"); > + > + await pushPrefs(['media.navigator.permission.disabled', true]); > + > + info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted"); Error: 'info' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:51 (Diff revision 4) > -.then(() => info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => OnDeviceChangeEventReceived()) > -.then(() => ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => navigator.mediaDevices.getUserMedia({video: true, fake: true})) > -.then(st => {videoTracks = st.getVideoTracks();}) > -.then(() => info("assure devicechange event is fired when gUM is in use")) > + > + await pushPrefs(['media.navigator.permission.disabled', true]); > + > + info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted"); > + await OnDeviceChangeEventReceived(); > + ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted"); Error: 'ok' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:52 (Diff revision 4) > -.then(() => OnDeviceChangeEventReceived()) > -.then(() => ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted")) > -.then(() => navigator.mediaDevices.getUserMedia({video: true, fake: true})) > -.then(st => {videoTracks = st.getVideoTracks();}) > -.then(() => info("assure devicechange event is fired when gUM is in use")) > -.then(() => OnDeviceChangeEventReceived()) > + await pushPrefs(['media.navigator.permission.disabled', true]); > + > + info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted"); > + await OnDeviceChangeEventReceived(); > + ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted"); > + let st = await getUserMedia({video: true, fake: true}); Error: 'getUserMedia' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:55 (Diff revision 4) > -.then(st => {videoTracks = st.getVideoTracks();}) > -.then(() => info("assure devicechange event is fired when gUM is in use")) > -.then(() => OnDeviceChangeEventReceived()) > -.then(() => ok(true, "devicechange event is fired when gUM is in use")) > -.catch(e => ok(false, "Error: " + e)) > -.then(() => { > + await OnDeviceChangeEventReceived(); > + ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted"); > + let st = await getUserMedia({video: true, fake: true}); > + let videoTracks = st.getVideoTracks(); > + > + info("assure devicechange event is fired when gUM is in use"); Error: 'info' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_ondevicechange.html:57 (Diff revision 4) > -.then(() => OnDeviceChangeEventReceived()) > -.then(() => ok(true, "devicechange event is fired when gUM is in use")) > -.catch(e => ok(false, "Error: " + e)) > -.then(() => { > - if(videoTracks) > + let st = await getUserMedia({video: true, fake: true}); > + let videoTracks = st.getVideoTracks(); > + > + info("assure devicechange event is fired when gUM is in use"); > + await OnDeviceChangeEventReceived(); > + ok(true, "devicechange event is fired when gUM is in use"); Error: 'ok' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_maxFsConstraint.html:82 (Diff revision 4) > stream.getTracks().forEach(track => track.stop()); > v1.srcObject = v2.srcObject = null; > }).catch(generateErrorCallback()); > } > > - pushPrefs(['media.peerconnection.video.lock_scaling', true]).then(() => { > + runNetworkTest(async () => { Error: 'runNetworkTest' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_maxFsConstraint.html:83 (Diff revision 4) > v1.srcObject = v2.srcObject = null; > }).catch(generateErrorCallback()); > } > > - pushPrefs(['media.peerconnection.video.lock_scaling', true]).then(() => { > + runNetworkTest(async () => { > + await pushPrefs(['media.peerconnection.video.lock_scaling', true]); Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_maxFsConstraint.html:83 (Diff revision 4) > v1.srcObject = v2.srcObject = null; > }).catch(generateErrorCallback()); > } > > - pushPrefs(['media.peerconnection.video.lock_scaling', true]).then(() => { > + runNetworkTest(async () => { > + await pushPrefs(['media.peerconnection.video.lock_scaling', true]); Error: Strings must use doublequote. [eslint: quotes] ::: dom/media/tests/mochitest/test_peerConnection_maxFsConstraint.html:89 (Diff revision 4) > - .then(networkTestFinished)); > - } else { > // No support for H.264 on Android in automation, see Bug 1355786 > - runNetworkTest(() => testScale("VP8").then(networkTestFinished)); > + await testScale("H264"); > } > + networkTestFinished(); Error: 'networkTestFinished' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:80 (Diff revision 4) > v1.srcObject = v2.srcObject = null; > pc1.close() > pc2.close() > } > > - pushPrefs(['media.peerconnection.video.lock_scaling', true]).then(() => { > + runNetworkTest(async () => { Error: 'runNetworkTest' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:81 (Diff revision 4) > pc1.close() > pc2.close() > } > > - pushPrefs(['media.peerconnection.video.lock_scaling', true]).then(() => { > + runNetworkTest(async () => { > + await pushPrefs(['media.peerconnection.video.lock_scaling', true]); Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:81 (Diff revision 4) > pc1.close() > pc2.close() > } > > - pushPrefs(['media.peerconnection.video.lock_scaling', true]).then(() => { > + runNetworkTest(async () => { > + await pushPrefs(['media.peerconnection.video.lock_scaling', true]); Error: Strings must use doublequote. [eslint: quotes] ::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:87 (Diff revision 4) > - .then(networkTestFinished)); > - } else { > // No support for H.264 on Android in automation, see Bug 1355786 > - runNetworkTest(() => testScale("VP8").then(networkTestFinished)); > + await testScale("H264"); > } > + networkTestFinished(); Error: 'networkTestFinished' is not defined. [eslint: no-undef]

Comment on attachment 8932868 [details] Bug 1299515 - Set track stop_on_disable prefs to act immediately in test. https://reviewboard.mozilla.org/r/203890/#review220590 Static analysis found 3 defects in this patch. - 3 defects found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:17 (Diff revision 4) > title: "MediaStream.clone()", > bug: "1208371" > }); > > runTest(async () => { > + await pushPrefs( Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:18 (Diff revision 4) > }); > > runNetworkTest(async () => { > var test = new PeerConnectionTest(); > > + await pushPrefs( Error: 'pushPrefs' is not defined. [eslint: no-undef] ::: dom/media/tests/mochitest/test_peerConnection_trackDisabling_clones.html:18 (Diff revision 4) > }); > > runNetworkTest(async () => { > var test = new PeerConnectionTest(); > > + await pushPrefs( Error: 'pushPrefs' is not defined. [eslint: no-undef]

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review218950 Big patch. Reviewing in pieces. Here's what I have so far. ::: dom/media/webrtc/MediaEngineSource.h:64 (Diff revision 2) > +/** > + * Returns true if the given source type is for video, false otherwise. > + * Only call with real types. > + */ > +bool IsVideo(dom::MediaSourceEnum aSource); > + > +/** > + * Returns true if the given source type maps to a real device. > + */ > +bool IsReal(dom::MediaSourceEnum aSource); Please avoid top-level (namespace mozilla) global scope functions, especially generically named ones, like these (sure name mangling probably makes them unique to MediaSourceEnum, but still). If need be, put them as static functions on a related class. In this case, it looks like IsVideo() can be inlined at its lone call site, and IsReal() appears unused and can be removed (is that right)? ::: dom/media/webrtc/MediaEngineSource.h:147 (Diff revision 2) > + * Start the device and add the track to the provided SourceMediaStream, with > + * the provided TrackID. You may start appending data to the track stray space ::: dom/media/webrtc/MediaEngineSource.h:167 (Diff revision 2) > + const MediaEnginePrefs& aPrefs, > + const nsString& aDeviceId, > + const char** aOutBadConstraint) = 0; > + > + /** > + * Stop the device and release the corresponding MediaStream. track? ::: dom/media/webrtc/MediaEngineWebRTC.cpp:135 (Diff revision 2) > > void > -MediaEngineWebRTC::EnumerateVideoDevices(dom::MediaSourceEnum aMediaSource, > - nsTArray<RefPtr<MediaEngineVideoSource> >* aVSources) > +MediaEngineWebRTC::EnumerateDevices(dom::MediaSourceEnum aMediaSource, > + nsTArray<RefPtr<MediaEngineSource> >* aSources) > { > + if (IsVideo(aMediaSource)) { AFAICT this is the lone use of IsVideo(). Please inline the function here and remove it. ::: dom/media/MediaManager.cpp:818 (Diff revision 3) > - if (mMediaSource == MediaSourceEnum::Microphone) { > + MediaSourceEnum source = GetMediaSource(); > + if (source == MediaSourceEnum::Microphone) { > aMediaSource.AssignLiteral(u"microphone"); > - } else if (mMediaSource == MediaSourceEnum::AudioCapture) { > + } else if (source == MediaSourceEnum::AudioCapture) { > aMediaSource.AssignLiteral(u"audioCapture"); > - } else if (mMediaSource == MediaSourceEnum::Window) { // this will go away > + } else if (source == MediaSourceEnum::Window) { // this will go away > aMediaSource.AssignLiteral(u"window"); > } else { // all the rest are shared > aMediaSource.Assign(NS_ConvertUTF8toUTF16( > - dom::MediaSourceEnumValues::strings[uint32_t(mMediaSource)].value)); > + dom::MediaSourceEnumValues::strings[uint32_t(source)].value)); > } Nit: Maybe a switch statement? ::: dom/media/MediaManager.cpp:1344 (Diff revision 3) > if (media_device_name && *media_device_name) { > for (auto& source : sources) { > - nsString deviceName; > + nsString deviceName = source->GetName(); > - source->GetName(deviceName); > if (deviceName.EqualsASCII(media_device_name)) { > - aResult.AppendElement(new DeviceType(source)); > + aResult.AppendElement(MakeAndAddRef<MediaDevice>( Use MakeRefPtr to avoid unnecessary detour through already_AddRefed. ::: dom/media/MediaManager.cpp:1353 (Diff revision 3) > break; > } > } > } else { > for (auto& source : sources) { > - aResult.AppendElement(new DeviceType(source)); > + aResult.AppendElement(MakeAndAddRef<MediaDevice>( ditto ::: dom/media/MediaManager.cpp:2653 (Diff revision 3) > - for (auto& device : aDevices) { > + for (RefPtr<MediaDevice>& device : aDevices) { > nsString id; > device->GetId(id); > - device->SetRawId(id); > + nsString rawId(id); > AnonymizeId(id, aOriginKey); > - device->SetId(id); > + device = MakeAndAddRef<MediaDevice>(device->mSource, Nit: Any reason to avoid device = new MediaDevice(...); ? ::: dom/media/webrtc/AllocationHandle.cpp:16 (Diff revision 3) > +uint64_t AllocationHandle::sId = 0; > + > +uint64_t > +AllocationHandle::GetUniqueId() > +{ Nit: If we put the static sId inside GetUniqueId() do we need this .cpp file? ::: dom/media/webrtc/MediaEngineDefault.h:109 (Diff revision 3) > + // For calculating fitness distances > + MediaConstraintsHelper mConstraintsHelper; MediaConstraintsHelper contains nothing but static functions, so it doesn't really need to be instantiated, the way we're using it. s/MediaConstraintsHelper./MediaConstraintsHelper::/ ? Applies throughout. ::: dom/media/webrtc/MediaEngineDefault.cpp:115 (Diff revision 3) > ); > mOpts.mWidth = std::max(160, std::min(mOpts.mWidth, 4096)) & ~1; > mOpts.mHeight = std::max(90, std::min(mOpts.mHeight, 2160)) & ~1; > - mState = kAllocated; > *aOutHandle = nullptr; > + trailing space ::: dom/media/webrtc/MediaEngineDefault.cpp:116 (Diff revision 3) > + MutexAutoLock lock(mMutex); > + mState = kAllocated; A side-note: (Maybe not a new issue but...) It makes me a little nervous that we need to lock before setting mState = kAllocated, when all state changes come from the basic control methods on the owning thread. I understand we lock because the MSG thread tests `if (mState != kStarted) return;`, but how badly are we relying on that? My concern is: If that's what's holding the dam, what were to happen if we managed to get from kAllocated to kStarted again? Would the running MSG thread suddenly resume (old) work? What controls the lifetime of this MSG setup? ::: dom/media/webrtc/MediaEngineDefault.cpp (Diff revision 3) > - // implicitly releases last image > - mImage = ycbcr_image.forget(); > - > - return NS_OK; > -} > + } > > -NS_IMETHODIMP > -MediaEngineDefaultVideoSource::GetName(nsACString& aName) > + MutexAutoLock lock(mMutex); > + mImage = do_AddRef(ycbcr_image); Extraneous addRef() + Release() compared to before. How about: mImage = Move(ycbcr_image); ::: dom/media/webrtc/MediaEngineDefault.cpp:397 (Diff revision 3) > aConstraints.mDeviceId.GetAsString().EqualsASCII("bad device")) { > return NS_ERROR_FAILURE; > } > > mFreq = aPrefs.mFreq ? aPrefs.mFreq : 1000; > mState = kAllocated; Missing `MutexAutoLock lock(mMutex);` ? I'm assuming this is meant to follow the same rules as in MediaEngineRemoteVideoSource: "Set under mMutex on the owning thread. Accessed under one of the two." ::: dom/media/webrtc/MediaEngineDefault.cpp:504 (Diff revision 3) > -MediaEngineDefault::EnumerateVideoDevices(dom::MediaSourceEnum aMediaSource, > - nsTArray<RefPtr<MediaEngineVideoSource> >* aVSources) { > - MutexAutoLock lock(mMutex); > +MediaEngineDefault::EnumerateDevices(dom::MediaSourceEnum aMediaSource, > + nsTArray<RefPtr<MediaEngineSource>>* aSources) > +{ > + AssertIsOnOwningThread(); > > - // only supports camera sources (for now). See Bug 1038241 > + if (aMediaSource == dom::MediaSourceEnum::Camera) { Nit: Maybe use a switch() here? ::: dom/media/webrtc/MediaEngineDefault.cpp:505 (Diff revision 3) > - nsTArray<RefPtr<MediaEngineVideoSource> >* aVSources) { > - MutexAutoLock lock(mMutex); > + nsTArray<RefPtr<MediaEngineSource>>* aSources) > +{ > + AssertIsOnOwningThread(); > > - // only supports camera sources (for now). See Bug 1038241 > - if (aMediaSource != dom::MediaSourceEnum::Camera) { > + if (aMediaSource == dom::MediaSourceEnum::Camera) { > + // Only supports camera video sources (for now). See Bug 1038241. I've closed bug 1038241 so we can remove "for now". ::: dom/media/webrtc/MediaEngineDefault.cpp:517 (Diff revision 3) > - aVSources->AppendElement(newSource); > + aSources->AppendElement(newSource); > + return; > -} > + } > > -void > -MediaEngineDefault::EnumerateAudioDevices(dom::MediaSourceEnum aMediaSource, > + if (aMediaSource == dom::MediaSourceEnum::Microphone) { > + for (const RefPtr<MediaEngineDefaultAudioSource>& source : mASources) { const auto& wfm fwiw ymmv ::: dom/media/webrtc/MediaEngineDefault.cpp:526 (Diff revision 3) > - mASources.AppendElement(newSource); > + mASources.AppendElement(newSource); > - aASources->AppendElement(newSource); > + aSources->AppendElement(newSource); Maybe s/mASources/mSources/ as well? ::: dom/media/webrtc/MediaEnginePrefs.h:1 (Diff revision 3) > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef MediaEnginePrefs_h > +#define MediaEnginePrefs_h > + > +namespace mozilla { > + > +/** > + * Video source and friends. > + */ > +class MediaEnginePrefs { Please consider using `hg copy` when splitting code into several files. This has several benefits: 1. It makes reviewing easier (reviewboard diffs from the original file), and 2. It preserves blame. Even though searchfox doesn't show blame right [1], it works with dxr and hg blame [2]. Until then, I'm going to assume things were mostly copied unchanged. [1] https://searchfox.org/mozilla-central/source/dom/media/systemservices/MediaTaskUtils.h [2] https://dxr.mozilla.org/mozilla-central/source/dom/media/systemservices/MediaTaskUtils.h ::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:49 (Diff revision 3) > +} > + > namespace mozilla { > > +// Fitness distance is defined in > +// https://www.w3.org/TR/2017/CR-mediacapture-streams-20171003/#dfn-selectsettings (I missed this before) Let's use https://w3c.github.io/mediacapture-main/getusermedia.html#dfn-selectsettings ::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:216 (Diff revision 3) > + // Note that these are different from the settings of the underlying device > + // since we scale frames to avoid fingerprinting. s/are/may be/ ? ::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:153 (Diff revision 3) > mWindowId = aConstraints.mBrowserWindow.WasPassed() ? > aConstraints.mBrowserWindow.Value() : -1; > *aOutHandle = nullptr; > > { > - MonitorAutoLock mon(mMonitor); > + MutexAutoLock mon(mMutex); s/mon/lock/ Applies throughout.