1299515 - Turn off camera/microphone while all tracks are muted/disabled.

Reporter

Description

•

8 years ago

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.

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Updated

•

8 years ago

Rank: 25

Priority: -- → P2

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

•

7 years ago

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

Status: NEW → ASSIGNED

Rank: 25 → 18

Priority: P3 → P2

Johann Hofmann [:johannh]

Updated

•

7 years ago

Blocks: 1333468

No longer depends on: 1333468

Comment hidden (mozreview-request)

Andreas Pehrson [:pehrsons]

Assignee

Comment 12

•

7 years ago

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/

Depends on: 1208378

Andreas Pehrson [:pehrsons]

Assignee

Comment 13

•

7 years ago

mozreview-review

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.

Andreas Pehrson [:pehrsons]

Assignee

Comment 14

•

7 years ago

mozreview-review

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.

Code Review Bot [:reviewbot]

Comment 15

•

7 years ago

mozreview-review

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

Johann Hofmann [:johannh]

Comment 16

•

7 years ago

mozreview-review

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 :)

Attachment #8929564 - Flags: review?(jhofmann) → review+

Andreas Pehrson [:pehrsons]

Assignee

Comment 17

•

7 years ago

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.

Flags: needinfo?(jhofmann)

Johann Hofmann [:johannh]

Comment 18

•

7 years ago

(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 :)

Flags: needinfo?(jhofmann)

Andreas Pehrson [:pehrsons]

Assignee

Comment 19

•

7 years ago

(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 hidden (mozreview-request)

Code Review Bot [:reviewbot]

•

7 years ago

Attachment #8930889 - Flags: review?(erahm)

Eric Rahm [:erahm]

Comment 59

•

7 years ago

mozreview-review

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.

Attachment #8930889 - Flags: review?(erahm) → review+

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Blocks: 1423991

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 60

•

7 years ago

mozreview-review

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?

Attachment #8929563 - Flags: review?(jib) → review-

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 61

•

7 years ago

mozreview-review

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

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 62

•

7 years ago

mozreview-review

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

Attachment #8932867 - Flags: review?(jib) → review+

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 63

•

7 years ago

mozreview-review

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?

Attachment #8932868 - Flags: review?(jib) → review+

Paul Adenot (:padenot)

•

7 years ago

mozreview-review

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

Attachment #8929564 - Flags: review?(jib) → review+

Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018

•

7 years ago

Blocks: 1426718

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Blocks: 1428098

Comment hidden (mozreview-request)

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Attachment #8932869 - Attachment is obsolete: true

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Attachment #8932870 - Attachment is obsolete: true

Andreas Pehrson [:pehrsons]

Assignee

•

7 years ago

mozreview-review

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

Attachment #8940726 - Flags: review?(nika) → review+

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 101

•

7 years ago

mozreview-review

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?

Jean-Yves Avenard [:jya]

Comment 102

•

7 years ago

mozreview-review

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...

Attachment #8940728 - Flags: review?(jyavenard) → review+

Jean-Yves Avenard [:jya]

Comment 103

•

7 years ago

mozreview-review

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

Attachment #8940729 - Flags: review?(jyavenard) → review+

Andreas Pehrson [:pehrsons]

Assignee

Comment 104

•

7 years ago

mozreview-review-reply

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.

Andreas Pehrson [:pehrsons]

Assignee

Comment 105

•

7 years ago

mozreview-review-reply

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 hidden (mozreview-request)

Andreas Pehrson [:pehrsons]

Assignee

Comment 129

•

7 years ago

•

7 years ago

mozreview-review-reply

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.

Jean-Yves Avenard [:jya]

Comment 133

•

7 years ago

mozreview-review-reply

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

Andreas Pehrson [:pehrsons]

Assignee

Comment 134

•

7 years ago

mozreview-review-reply

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().

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Depends on: 1351655

Comment hidden (mozreview-request)

Code Review Bot [:reviewbot]

Comment 160

•

7 years ago

mozreview-review

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]

Code Review Bot [:reviewbot]

•

7 years ago

mozreview-review

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review221054 Phew. Big cleanup, love it! Much simpler for critical code. Mostly awesome. r- for upscaling, broken tab capture (presumably), and (I assume) accidentally stopping concurrent audio. ::: dom/media/webrtc/MediaEngineDefault.cpp:131 (Diff revision 4) > MOZ_ASSERT(!aHandle); > - if (mState != kStopped && mState != kAllocated) { > - return NS_ERROR_FAILURE; > - } > + MOZ_ASSERT(mState == kStopped || mState == kAllocated); > + > + MutexAutoLock lock(mMutex); > mState = kReleased; > mImage = nullptr; must be null already (cleared in Stop()). MOZ_ASSERT? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:203 (Diff revision 4) > + // Set in ctor, reset under mMutex on the owning thread. > + // Accessed under one of the two. > + RefPtr<layers::ImageContainer> mImageContainer; Looks like you've moved this from ctor to Start()? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:41 (Diff revision 4) > - mMediaSource(aMediaSource), > - mCapEngine(aCapEngine), > - mScary(aScary) > -{ > - MOZ_ASSERT(aMediaSource != dom::MediaSourceEnum::Other); > - mSettings->mWidth.Construct(0); > + bool aScary) > + : mCaptureIndex(aIndex) > + , mMediaSource(aMediaSource) > + , mCapEngine(aCapEngine) > + , mScary(aScary) > + , mMutex("MediaEngineRemoteVideoSource::mMutex") Nit: Naming seems inconsistent. I see: mMutex("MediaEngineTabVideoSource") mMutex("MediaEngineDefaultAudioSource::mMutex") Apparently a common problem [1] [1] https://searchfox.org/mozilla-central/search?q=mMutex%28&path= ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:227 (Diff revision 4) > - if (!mRegisteredHandles.Length()) { > - if (mState != kStopped && mState != kAllocated) { > - return NS_ERROR_FAILURE; > - } > + // Stop() has stopped capture synchronously on the media thread before we get > + // here, so there are no longer any callbacks on an IPC thread accessing > + // mImageContainer. > + mImageContainer = nullptr; That may be so, but this other comment says: "Set in ctor, reset under mMutex on the owning thread." We should make code and comments match. ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:268 (Diff revision 4) > if (mozilla::camera::GetChildAndCall( > - &mozilla::camera::CamerasChild::StartCapture, > + &mozilla::camera::CamerasChild::StartCapture, > - mCapEngine, mCaptureIndex, mCapability, this)) { > + mCapEngine, mCaptureIndex, mCapability, this)) { Indent to ( ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:358 (Diff revision 4) > - if (camera::GetChildAndCall(&camera::CamerasChild::StartCapture, > - mCapEngine, mCaptureIndex, mCapability, > + NS_ENSURE_SUCCESS(Stop(mStream, mTrackID), NS_ERROR_FAILURE); > + NS_ENSURE_SUCCESS(Start(mStream, mTrackID, mPrincipal), NS_ERROR_FAILURE); The NS_ENSURE macros are out of favor, because they hide code flow (in this case they also hide the error). In new code, please use the verbose: nsresult rv = Stop(mStream, mTrackID); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:433 (Diff revision 4) > + > + // This is safe from any thread, and is safe if the track is Finished > + // or Destroyed. > + // This can fail if either a) we haven't added the track yet, or b) > + // we've removed or finished the track. > + aStream->AppendToTrack(aTrackID, &(segment)); (I know this was copied, but) aren't the ()s redundant here? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:440 (Diff revision 4) > - MonitorAutoLock lock(mMonitor); > + // Cameras IPC thread - take great care with accessing members! > - // Check for proper state. > - if (mState != kStarted || !mImageContainer) { > - LOG(("DeliverFrame: video not started")); > - return 0; > - } Looks like MediaEngineRemoteVideoSource no longer touches mState off the media thread (except MOZ_ASSERTs). Why lock around it? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:467 (Diff revision 4) > - bool needReScale = !((dst_width == mWidth && dst_height == mHeight) || > - (dst_width > mWidth || dst_height > mHeight)); > + bool needReScale = dst_width < aProps.width() || > + dst_height < aProps.height(); This may upscale in some cases, which the spec disallows. The old logic seems preferable (though it might benefit from less negation). ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:547 (Diff revision 4) > - LOGFRAME(("frame %d (%dx%d); timeStamp %u, ntpTimeMs %" PRIu64 ", renderTimeMs %" PRIu64, > + LOGFRAME(("frame %d (%dx%d); timeStamp %u, ntpTimeMs %" PRIu64 ", renderTimeMs %" PRIu64, > - frame_num++, mWidth, mHeight, > + frame_num++, dst_width, dst_height, This used to log aProps.width() and aProps.height(), now it logs dst_width and dst_height... Makes sense, just checking it's intentional. Should we maybe log both? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:561 (Diff revision 4) > - // We'll push the frame into the MSG on the next NotifyPull. This will avoid > + if (sizeChanged) { > + NS_DispatchToMainThread(NS_NewRunnableFunction( > + "MediaEngineRemoteVideoSource::FrameSizeChange", > + [settings = mSettings, dst_width, dst_height]() mutable { > + settings->mWidth.Value() = dst_width; > + settings->mHeight.Value() = dst_height; Unless I'm mistaken, this fixes a bug where track.getSettings() would return the original unscaled dimensions. That bug potentially leaks info about what camera sizes adjacent tabs may be using. Do we want a separate bug for that to uplift to beta? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:601 (Diff revision 4) > + uint64_t distance = > + uint64_t(MediaConstraintsHelper::FitnessDistance( > + aDeviceId, aConstraints.mDeviceId)) + > + uint64_t(MediaConstraintsHelper::FitnessDistance( > + mFacingMode, aConstraints.mFacingMode)) + Uha. Maybe align with ( ? This looked so pretty before. Almost makes me wanna consider: auto& fit = MediaConstraintsHelper::FitnessDistance; uint64_t distance = uint64_t(fit(aDeviceId, aConstraints.mDeviceId)) + uint64_t(fit(mFacingMode, aConstraints.mFacingMode)) + ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:654 (Diff revision 4) > +uint32_t > +MediaEngineRemoteVideoSource::GetFeasibilityDistance( Nit: GetFeasibilityDistance used to be next to GetFitnessDistance. Maybe keep them close to help people remember to update both if they add any new constraints? ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:801 (Diff revision 4) > + LogConstraints(advanced); > + } > + } > + } > + > switch(mMediaSource) { Nit: space after switch ::: dom/media/webrtc/MediaEngineSource.h:123 (Diff revision 4) > + * Override w/true if source does end-run around cross origin restrictions. > + */ > + virtual bool GetScary() const = 0; > + > + /** > + * Called by MediaEngine to allocate an instance of this source. Love the documentation! Maybe s/an instance of/a handle to/ ? Or maybe just s/an instance of// ? ::: dom/media/webrtc/MediaEngineSource.h:147 (Diff revision 4) > + /** > + * Applies new constraints to the capability selection for the underlying > + * device. > + * > + * Should the constraints lead to choosing a new capability while the device > + * is actively captured, the device will restart using the new capability. s/captured/being captured/ ::: dom/media/webrtc/MediaEngineSource.h:161 (Diff revision 4) > + * Stop the device and release the corresponding MediaStream. > + */ > + virtual nsresult Stop(SourceMediaStream *aSource, TrackID aID) = 0; > + > + /** > + * Called by MediaEngine to deallocate an instance of this source. Maybe s/an instance of/a handle to/ ? Or s/an instance of// ? ::: dom/media/webrtc/MediaEngineSource.h:197 (Diff revision 4) > + * In case of a camera where we intervene and scale frames to avoid > + * fingerprinting, GetSettings() will return the scaled resolution. I.e., the Nit: We really only intervene on concurrent use to avoid leaking info from other sites, not on single use and not because of fingerprinting. ::: dom/media/webrtc/MediaEngineSource.h:204 (Diff revision 4) > + * device settings as seen by js. > + */ > + virtual void GetSettings(dom::MediaTrackSettings& aOutSettings) const = 0; > + > + /** > + * Pulls data from the source into the track. Maybe s/source/source device/ ? Several things called source-something here, like the SourceMediaStream "destination"... ::: dom/media/webrtc/MediaEngineSource.h:244 (Diff revision 4) > + // No sharing required by default. > + bool RequiresSharing() const override; > + > + // Not fake by default. > + bool IsFake() const override; > + > + // Not scary by default. > + bool GetScary() const override; > + > + // Shutdown does nothing by default. > + void Shutdown() override; In the spirit of pure interfaces and removing inheritance, and making sure we don't forget to implement something, would it make sense to remove these defaults? Just an idea. ::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:198 (Diff revision 4) > nsresult > -MediaEngineTabVideoSource::Deallocate(AllocationHandle* aHandle) > +MediaEngineTabVideoSource::Deallocate(const RefPtr<const AllocationHandle>& aHandle) > { > + AssertIsOnOwningThread(); > MOZ_ASSERT(!aHandle); > + MOZ_ASSERT(mState == kAllocated); MOZ_ASSERT(mState == kStopped || mState == kAllocated) ::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:200 (Diff revision 4) > + MOZ_ASSERT(mStream); > + MOZ_ASSERT(IsTrackIDExplicit(mTrackID)); > + mStream->EndTrack(mTrackID); > + mStream = nullptr; > + mTrackID = TRACK_NONE; This seems wrong. You already do this in Stop(), so mStream should be nullptr here, and this assert should hit. Did you test tab capture? https://jsfiddle.net/jib1/uax4gom5/ ::: dom/media/webrtc/MediaEngineWebRTC.h:363 (Diff revision 4) > +struct Allocation { > + Allocation() = delete; > + explicit Allocation(const RefPtr<AllocationHandle>& aHandle); > + ~Allocation(); > + > + const RefPtr<AllocationHandle> mHandle; > + RefPtr<SourceMediaStream> mStream; > + TrackID mTrackID = TRACK_NONE; > + PrincipalHandle mPrincipal = PRINCIPAL_HANDLE_NONE; > +}; > + > +/** > + * Used with nsTArray<Allocation>::IndexOf to locate an Allocation by a handle. > + */ > +class AllocationHandleComparator { Allocation and AllocationHandleComparator should be nested inside MediaEngineWebRTCMicrophoneSource, since they appear to be used solely there. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:53 (Diff revision 4) > +Allocation::Allocation(const RefPtr<AllocationHandle>& aHandle) > + : mHandle(aHandle) > +{} > + > +Allocation::~Allocation() = default; why not inline? ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:115 (Diff revision 4) > bool aDelayAgnostic, > bool aExtendedFilter) > - : MediaEngineAudioSource(kReleased) > + : mAudioInput(aAudioInput) > - , mAudioInput(aAudioInput) > , mAudioProcessing(AudioProcessing::Create()) > - , mMonitor("WebRTCMic.Monitor") > + , mMutex("WebRTCMic.Mutex") s/./::/ or s/.Mutex// ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:207 (Diff revision 4) > + nsresult rv = UpdateSingleSource(aHandle, > + netConstraints, > + aPrefs, odd indent ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:457 (Diff revision 4) > - auto& source = mSources.LastElement(); > + MOZ_ASSERT(!mAllocations.IsEmpty()); > + RefPtr<SourceMediaStream> stream; > + for (const Allocation& allocation : mAllocations) { > + if (allocation.mStream) { > + stream = allocation.mStream; > + break; This would appear to be a behavior change. The old code returned the last stream, whereas the new code returns the first. Why the change, or does it not matter? ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:459 (Diff revision 4) > - MOZ_ASSERT(mSources.Length() > 0); > // If the channel count changed, tell the MSG to open a new driver with > // the correct channel count. > - auto& source = mSources.LastElement(); > + MOZ_ASSERT(!mAllocations.IsEmpty()); > + RefPtr<SourceMediaStream> stream; > + for (const Allocation& allocation : mAllocations) { or const auto& ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:549 (Diff revision 4) > bool passThrough = !(aPrefs.mAecOn || aPrefs.mAgcOn || aPrefs.mNoiseOn); > - if (!that->mSources.IsEmpty()) { > - that->mSources[0]->GraphImpl()->AppendMessage(MakeUnique<Message>(that, passThrough)); > + if (graph) { > + graph->AppendMessage(MakeUnique<Message>(that, passThrough)); Hmm, this code appears to have been added recently [1]. The runnable here exists merely to update the JS-visible settings for `track.getSettings()`. I see no reason the updating of passThrough in MSG shouldn't be done immediately here. No reason to do it from the main thread, is there? We can fix here or file a new bug if we want Paul's input. [1] https://searchfox.org/mozilla-central/diff/30d795740bf1c6c4bb80cc6736801a4e3753c871/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#557 ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:568 (Diff revision 4) > + RefPtr<AllocationHandle> handle = > + new AllocationHandle(aConstraints, aPrincipalInfo, aPrefs, aDeviceId); or: auto handle = MakeRefPtr<AllocationHandle>(aConstraints, aPrincipalInfo, aPrefs, aDeviceId); ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:668 (Diff revision 4) > // Make sure logger starts before capture > AsyncLatencyLogger::Get(true); This doesn't appear true anymore. Problem, or remove? ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:693 (Diff revision 4) > - aSource->EndTrack(aID); > + mAudioInput->StopRecording(aStream); > > - if (!mSources.IsEmpty()) { > - mAudioInput->StopRecording(aSource); > + if (!mAllocations.IsEmpty()) { > + // Another track is keeping us from stopping This looks like a change... Won't this stop recording here in spite of other concurrently open allocations, causing their audio to end when it shouldn't? ::: dom/media/webrtc/MediaTrackConstraints.cpp:409 (Diff revision 4) > + nsTArray<const NormalizedConstraintSet*> sets; > + sets.AppendElement(&aConstraints); > + > + MOZ_ASSERT(!aDevices.IsEmpty()); > + for (auto& device : aDevices) { > + if (device->GetBestFitnessDistance(sets, device) != UINT32_MAX) { The second arg here I expected to be `false`, not `device`... huh? ::: dom/media/webrtc/MediaTrackConstraints.cpp:599 (Diff revision 4) > - devices.AppendElement(new MockDevice(&aMediaEngineSource, aDeviceId)); > +MediaConstraintsHelper::FindBadConstraint( > + const NormalizedConstraints& aConstraints, > + const RefPtr<MediaEngineSource>& aMediaEngineSource, > + const nsString& aDeviceId) > +{ > + RefPtr<MediaDevice> device = MakeAndAddRef<MediaDevice>( MakeRefPtr ::: dom/media/webrtc/MediaTrackConstraints.cpp:599 (Diff revision 4) > + RefPtr<MediaDevice> device = MakeAndAddRef<MediaDevice>( > + aMediaEngineSource, > + aMediaEngineSource->GetName(), > + aDeviceId); > + AutoTArray<RefPtr<MediaDevice>, 1> devices; > + devices.AppendElement(Move(device)); Why not just an rvalue over a temporary + Move()? devices.AppendElement(new MediaDevice( aMediaEngineSource, aMediaEngineSource->GetName(), aDeviceId));

Attachment #8940727 - Flags: review?(jib) → review-

Andreas Pehrson [:pehrsons]

Assignee

•

7 years ago

mozreview-review

Comment on attachment 8944780 [details] Bug 1299515 - Make LocalTrackSource hold a WeakPtr to SourceListener. https://reviewboard.mozilla.org/r/214944/#review221552 Unsure about this fix. I have questions. Today, if I `let stream = await gUM({video: true})` and then drop the reference to `stream`, the camera light goes away after about 10 seconds (at GC/CC). If a track doesn't hold the sourceListener open, then what does? MSG? How sure are we that it isn't MSG that's holding it open past shutdown? ::: dom/media/MediaManager.cpp:1211 (Diff revision 2) > LocalTrackSource(nsIPrincipal* aPrincipal, > const nsString& aLabel, > SourceListener* aListener, If we're not going to hold a strong reference to mListener, maybe we should at least change the input arg to expect a RefPtr? I.e. const RefPtr<SourceListener>& aListener,

Andreas Pehrson [:pehrsons]

Assignee

Comment 193

•

7 years ago

mozreview-review-reply

Comment on attachment 8944780 [details] Bug 1299515 - Make LocalTrackSource hold a WeakPtr to SourceListener. https://reviewboard.mozilla.org/r/214944/#review221552 MediaManager, through the window listeners. What I wanted to achieve was to not remove any constness from the SourceListener members and descendants, and to keep the invariant that device state doesn't go away. That basically meant that the only remaining option avoiding a DOM object in CC keeping SourceListener members alive was to be sure that MediaManager controls the lifetime. It does through it's bookkeeping of windows. If a MediaStreamTrack is destroyed before shutdown it will call Stop() through to the SourceListener, which removes it from the window listener, and if it was the last track for that window listener, removes the window from MediaManager. MSG removes all listeners in its async blocker for XPCOM_WILL_SHUTDOWN, https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/dom/media/MediaStreamGraph.cpp#1481. > If we're not going to hold a strong reference to mListener, maybe we should at least change the input arg to expect a RefPtr? I.e. > > const RefPtr<SourceListener>& aListener, Can do.

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 194

•

7 years ago

mozreview-review-reply

Comment on attachment 8944780 [details] Bug 1299515 - Make LocalTrackSource hold a WeakPtr to SourceListener. https://reviewboard.mozilla.org/r/214944/#review221552 Ok, if we add that as a code comment then it lgtm.

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 195

•

7 years ago

mozreview-review

Comment on attachment 8944780 [details] Bug 1299515 - Make LocalTrackSource hold a WeakPtr to SourceListener. https://reviewboard.mozilla.org/r/214944/#review221664

Attachment #8944780 - Flags: review?(jib) → review+

Paul Adenot (:padenot)

•

7 years ago

Attachment #8944779 - Attachment is obsolete: true

Code Review Bot [:reviewbot]

Comment 218

•

7 years ago

mozreview-review

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review222042 Static 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/webrtc/MediaTrackConstraints.cpp:519 (Diff revision 5) > > - 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]

Jan-Ivar Bruaroey [:jib] (needinfo? me)

Reporter

Comment 219

•

7 years ago

mozreview-review

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review222224 Lgtm. ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:624 (Diff revision 5) > + uint64_t distance = > + uint64_t(MediaConstraintsHelper::FitnessDistance( Should probably do the same typedef MediaConstraintsHelper H; trick here as well for consistency (or in neither).

Attachment #8940727 - Flags: review?(jib) → review+

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Blocks: 1434274

Andreas Pehrson [:pehrsons]

Assignee

Comment 220

•

7 years ago

mozreview-review-reply

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review221054 > Eh, yes, StopRecording() should happen after the check. No, this was wrong. Start/StopRecording() is per *Allocation*, i.e., per SourceMediaStream, not per MediaEngineMicrophoneSource.

Comment hidden (mozreview-request)

Andreas Pehrson [:pehrsons]

Assignee

Comment 245

•

7 years ago

This should be it https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed04e68bd484e276fbdcfa267395c30d172b087

Code Review Bot [:reviewbot]

Comment 246

•

7 years ago

mozreview-review

Comment on attachment 8940727 [details] Bug 1299515 - Flatten MediaEngineSource class hierarchy. https://reviewboard.mozilla.org/r/211002/#review222494 Static 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/webrtc/MediaTrackConstraints.cpp:519 (Diff revision 6) > > - 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 hidden (mozreview-request)

Andreas Pehrson [:pehrsons]

Assignee

Comment 263

•

7 years ago

mozreview-review-reply

Comment on attachment 8929562 [details] Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. https://reviewboard.mozilla.org/r/200838/#review221532 > This stream is a placeholder. We don't need to add the track at all. I'll fix this by removing that part from SetSource() and confirm that tests are happy. And now I can (well, have to thanks to some asserts) remove the dummy stuff in MediaEngineAudioCaptureSource::Pull, nice!

•

7 years ago

Depends on: 1434628

Andreea Pavel [:apavel]

Comment 268

•

7 years ago

bugherder

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox60: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla60

Andreas Pehrson [:pehrsons]

Assignee

•

7 years ago

Depends on: 1436694

Chris Mills [:cmills]

Comment 269

•

7 years ago

Sounds like this is worth mentioning in the docs.

No longer depends on: 1436694

Keywords: dev-doc-needed

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Depends on: 1436694

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

Depends on: 1438134

Eric Shepherd [:sheppy]

Comment 270

•

7 years ago

Added a note to Firefox 60 for developers that muting the camera's track now disables the camera's "in use" indicator light: https://developer.mozilla.org/en-US/Firefox/Releases/60#Removals_from_the_web_platform

Keywords: dev-doc-needed → dev-doc-complete

Eric Shepherd [:sheppy]

Comment 271

•

7 years ago

•

7 years ago

Depends on: 1280099

Andreea Pavel [:apavel]

Updated

•

7 years ago

Depends on: 1443774

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

7 years ago

No longer depends on: 1443774

Andreas Pehrson [:pehrsons]

Assignee

Updated

•

•

4 years ago

No longer blocks: CVE-2021-29959

Bug 1299515 - Plumb MediaStreamTrack enabled state to sources. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Remove GetUserMediaNotificationEvent. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Remove ReleaseMediaOperationResource. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Remove superfluous include in MediaStreamTrack.h. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Remove special handling for releasing callbacks on main thread. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Only notify of enabled state if it actually changed. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Allow MediaEngines to Start() and Stop() without affecting tracks. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+ padenot : review+	Details
Bug 1299515 - Stop Camera and Microphone device when tracks become disabled. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Wire up track-disabling logic to frontend APIs. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	johannh : review+ jib : review+	Details
Bug 1299515 - Make `const nsMainThreadPtrHandle<T>` usable. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	erahm : review+	Details
Bug 1299515 - Drive by touchups in MediaTrackConstraints. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Put pushPrefs in head.js. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Modernize tests that disable tracks to prime for coming patches. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Set track stop_on_disable prefs to act immediately in test. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Fix MediaEngineDefault. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	padenot : review+	Details
Bug 1299515 - Clean up MediaEngineRemoteVideoSource::FrameSizeChange. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Use an I420BufferPool for allocations in VideoFrameConverter. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	dminor : review+	Details
Bug 1299515 - Include nsContentUtils in TimeoutManager. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	nika : review+	Details
Bug 1299515 - Flatten MediaEngineSource class hierarchy. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Implement MediaTimer::WaitFor for convenience. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jya : review+	Details
Bug 1299515 - Implement MediaTimer::Cancel to allow for rejecting timer promises. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jya : review+	Details
Bug 1299515 - Disable turning off camera while disabled by default on android. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Replace a rawptr handoff with generalized lambda capture. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1299515 - Signal SetPullEnabled with a message. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	padenot : review+	Details
Bug 1299515 - Don't assert that tracks must be ended in forced shutdown. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	padenot : review+	Details
Bug 1299515 - Remove enum DistanceCalculation from MediaEngineRemoteVideoSource. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review-	Details
Bug 1299515 - Make LocalTrackSource hold a WeakPtr to SourceListener. 7 years ago Andreas Pehrson [:pehrsons] 59 bytes, text/x-review-board-request	jib : review+	Details