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

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
Rank:
18
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: jib, Assigned: pehrsons)

Tracking

(Blocks 4 bugs, {dev-doc-complete})

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(24 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
padenot
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
johannh
: review+
jib
: review+
Details
59 bytes, text/x-review-board-request
erahm
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
dminor
: review+
Details
59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
(Reporter)

Description

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

Updated

3 years ago
Rank: 25
Priority: -- → P2
(Reporter)

Updated

2 years ago
Depends on: 1333468
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
(Reporter)

Updated

2 years ago
Assignee: nobody → apehrson
(Assignee)

Comment 2

2 years ago
I'm increasing priority here to match bug 1333468, and starting work.
Status: NEW → ASSIGNED
Rank: 25 → 18
Priority: P3 → P2
Blocks: 1333468
No longer depends on: 1333468
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year 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
(Assignee)

Comment 13

a year 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.
(Assignee)

Comment 14

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

Comment 15

a year 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

Comment 16

a year 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+
(Assignee)

Comment 17

a year 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)
(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)
(Assignee)

Comment 19

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

Comment 31

a year 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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 49

a year ago
mozreview-review
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.
Attachment #8932871 - Flags: review?(dminor) → review+
(Reporter)

Comment 50

a year ago
mozreview-review
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
Attachment #8929556 - Flags: review?(jib) → review+
(Reporter)

Comment 51

a year ago
mozreview-review
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]() {
Attachment #8929557 - Flags: review?(jib) → review+
(Reporter)

Comment 52

a year ago
mozreview-review
Comment on attachment 8929558 [details]
Bug 1299515 - Remove ReleaseMediaOperationResource.

https://reviewboard.mozilla.org/r/200830/#review211286
Attachment #8929558 - Flags: review?(jib) → review+
(Reporter)

Comment 53

a year ago
mozreview-review
Comment on attachment 8929559 [details]
Bug 1299515 - Remove superfluous include in MediaStreamTrack.h.

https://reviewboard.mozilla.org/r/200832/#review211288
Attachment #8929559 - Flags: review?(jib) → review+
(Reporter)

Comment 54

a year 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/#review211292
Attachment #8929560 - Flags: review?(jib) → review+
(Reporter)

Comment 55

a year ago
mozreview-review
Comment on attachment 8929561 [details]
Bug 1299515 - Only notify of enabled state if it actually changed.

https://reviewboard.mozilla.org/r/200836/#review211294
Attachment #8929561 - Flags: review?(jib) → review+
(Reporter)

Comment 56

a year 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/#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?
Attachment #8929562 - Flags: review?(jib) → review+
(Reporter)

Comment 57

a year ago
mozreview-review
Comment on attachment 8930890 [details]
Bug 1299515 - Drive by touchups in MediaTrackConstraints.

https://reviewboard.mozilla.org/r/202006/#review211304
Attachment #8930890 - Flags: review?(jib) → review+
(Reporter)

Comment 58

a year ago
mozreview-review
Comment on attachment 8932866 [details]
Bug 1299515 - Put pushPrefs in head.js.

https://reviewboard.mozilla.org/r/203886/#review211306
Attachment #8932866 - Flags: review?(jib) → review+
(Assignee)

Updated

a year ago
Attachment #8930889 - Flags: review?(erahm)

Comment 59

a year 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+
(Assignee)

Updated

a year ago
Blocks: 1423991
(Reporter)

Comment 60

a year 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-
(Reporter)

Comment 61

a year 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
(Reporter)

Comment 62

a year 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+
(Reporter)

Comment 63

a year 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+

Comment 64

a year 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/#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.
Attachment #8929562 - Flags: review?(padenot) → review+

Comment 65

a year ago
mozreview-review
Comment on attachment 8932869 [details]
Bug 1299515 - Fix MediaEngineDefault.

https://reviewboard.mozilla.org/r/203892/#review212336
Attachment #8932869 - Flags: review?(padenot) → review+
(Reporter)

Comment 66

a year ago
mozreview-review
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.
Attachment #8932870 - Flags: review?(jib) → review+
(Reporter)

Comment 67

a year 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+
(Assignee)

Comment 68

a year 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/#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.
(Assignee)

Comment 69

a year ago
mozreview-review-reply
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=
(Assignee)

Comment 70

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 71

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 72

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 73

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Blocks: 1426718
(Assignee)

Updated

a year ago
Blocks: 1428098
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8932869 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8932870 - Attachment is obsolete: true
(Assignee)

Comment 97

a year ago
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.
Attachment #8929562 - Flags: review+ → review?(jib)

Comment 98

a year ago
mozreview-review
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
Attachment #8940733 - Flags: review?(padenot) → review+

Comment 99

a year ago
mozreview-review
Comment on attachment 8940732 [details]
Bug 1299515 - Signal SetPullEnabled with a message.

https://reviewboard.mozilla.org/r/211012/#review216762
Attachment #8940732 - Flags: review?(padenot) → review+

Comment 100

a year 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+
(Reporter)

Comment 101

a year 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?

Comment 102

a year 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+

Comment 103

a year 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+
(Assignee)

Comment 104

a year 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.
(Assignee)

Comment 105

a year 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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 129

a year ago
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.
Attachment #8940729 - Flags: review+ → review?(jyavenard)
See Also: → 1429693

Comment 130

a year 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/#review217736

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

Comment 131

a year 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/#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.
(Assignee)

Comment 132

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

Comment 133

a year 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
(Assignee)

Comment 134

a year 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().
(Assignee)

Updated

a year ago
Depends on: 1351655
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 160

a year 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]

Comment 161

a year ago
mozreview-review
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 162

a year 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/#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]
(Reporter)

Comment 163

a year ago
mozreview-review
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.
(Assignee)

Comment 164

a year ago
mozreview-review-reply
Comment on attachment 8940727 [details]
Bug 1299515 - Flatten MediaEngineSource class hierarchy.

https://reviewboard.mozilla.org/r/211002/#review218950

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

IsVideo() is used in MediaManager.cpp and MediaManager.cpp. Static method it is.

> track?

That's an old text, and it changes in a later patch on this bug so I'll leave it for now.

> Nit: Maybe a switch statement?

This is getting simplified in a later patch on this bug.

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

AddListener() is external (happens in SourceListener in MediaManager.cpp) so IMHO this MediaEngineSource shouldn't make any assumptions about it's timing properties.

It's also valid to call Allocate(); Start(); Stop(); Deallocate(); Allocate(); Start(); on the same MediaEngineSource instance. Stop() and Deallocate() are responsible for cleaning up junk that can become old.

MediaEngineDefaultVideoSource::Stop() does for instance set `mImage = nullptr;` to avoid any old images lying around on the next Start().

My reasoning behind this change is simpler however. It should be easy to reason about the threading model. I.e., seeing that the mutex is locked whenever we modify mState is easy. Previously it was only held when set to kStopped or some such, for the reason you mention. But that's relying on external behavior, IMO premature optimization and a cause of real pain when debugging tricky bugs.

Does this answer your concern?

> const auto& wfm fwiw ymmv

I prefer having the type visible.

> Maybe s/mASources/mSources/ as well?

There's an iteration of mASources that does `source->IsAvailable()`, and in a later patch `IsAvailable()` moves from `MediaEngineSource` to `MediaEngineDefaultAudioSource` which is the only remaining class that needs it.

Then I prefer to keep the array of an explicit element type rather than cast them.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 183

a year ago
mozreview-review
Comment on attachment 8940727 [details]
Bug 1299515 - Flatten MediaEngineSource class hierarchy.

https://reviewboard.mozilla.org/r/211002/#review220984


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 4)
>  
> -    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]
(Reporter)

Comment 184

a year ago
mozreview-review-reply
Comment on attachment 8940727 [details]
Bug 1299515 - Flatten MediaEngineSource class hierarchy.

https://reviewboard.mozilla.org/r/211002/#review218950

> AddListener() is external (happens in SourceListener in MediaManager.cpp) so IMHO this MediaEngineSource shouldn't make any assumptions about it's timing properties.
> 
> It's also valid to call Allocate(); Start(); Stop(); Deallocate(); Allocate(); Start(); on the same MediaEngineSource instance. Stop() and Deallocate() are responsible for cleaning up junk that can become old.
> 
> MediaEngineDefaultVideoSource::Stop() does for instance set `mImage = nullptr;` to avoid any old images lying around on the next Start().
> 
> My reasoning behind this change is simpler however. It should be easy to reason about the threading model. I.e., seeing that the mutex is locked whenever we modify mState is easy. Previously it was only held when set to kStopped or some such, for the reason you mention. But that's relying on external behavior, IMO premature optimization and a cause of real pain when debugging tricky bugs.
> 
> Does this answer your concern?

Simpler makes sense. I guess I'm wondering why we need locking at all, and whether there'd be a way to write e.g. the pull code to not need to check `if (mState != kStarted) return;`. I recognize the pattern of needing a flag to neuter already-queued tasks from shutdown, but then we never clear mInShutdown booleans once we set them, so that seems safe, vs. here mState can cycle back to earlier states, so I guess I worry it might race in theory - e.g. if methods got called really fast - leading to unexpected code paths?
(Reporter)

Comment 185

a year 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-
(Assignee)

Comment 186

a year ago
mozreview-review-reply
Comment on attachment 8940727 [details]
Bug 1299515 - Flatten MediaEngineSource class hierarchy.

https://reviewboard.mozilla.org/r/211002/#review221054

> must be null already (cleared in Stop()). MOZ_ASSERT?

Hmm, I think me amending things to different patches has backfired here. The net result doesn't have this, but I'll add the assert and try to move the change here.

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

I went for efficiency and changed the comment. I also added an assert to DeliverFrame on the IPC thread to check that we only enter in state kStarted.

> Indent to (

I think I'll file a followup to rewrite the entire dir with clang-format.

> Looks like MediaEngineRemoteVideoSource no longer touches mState off the media thread (except MOZ_ASSERTs).
> 
> Why lock around it?

It's touched in Pull() on the MSG thread. Currently only in debug, but there's more in a future patch.

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

Makes sense, now that we're landing a version later than it was introduced.

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

`typedef MediaConstraintsHelper H;` and the FPS line is at 82 ... how zealous do you want to be? :-)

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

Fun fact: GetFeasibilityDistance goes away in one of the other patches. It wasn't used anymore when sharing was removed from MediaEngineRemoteVideoSource. My take is that this was only used when sharing a MediaEngineSource across multiple requests, so it has essentially moved to the parent process, since a request will be independent from other requests in the same document in that later patch.

I think we discussed this and it was fine to do.

> Maybe s/source/source device/ ?
> 
> Several things called source-something here, like the SourceMediaStream "destination"...

I went with s/source/MediaEngineSource/.

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

These defaults are typically noops that are only overridden in one subclass. No state is shared between this base class and the subclasses (which is really what I wanted to clean up). I don't think it's warranted to go all the way and remove MediaEngineSource completely.

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

Resetting the stream and track in Deallocate is not supposed to happen until a later patch. I guess that's why tests passed, this is a bad temporary state. I'll fix nevertheless.

> why not inline?

```
 0:09.32 /home/pehrsons/Dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:41:9: error: member access into incomplete type 'mozilla::AllocationHandle'
 0:09.32     aPtr->Release();
 0:09.32         ^
```

> odd indent

A tab!

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

It doesn't matter as there is code in Start() disallowing allocations of one device from multiple MediaStreamGraphs, and OpenNewAudioCallbackDriver() just calls through to the graph and its driver.

> or const auto&

I prefer to have the type staring me in the face.

> 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

My comment from bug 1397793 comment 67:

> If this changes from `false` to `true` on the media thread right in between `NotifyOutputData` and `NotifyInputData` of the same data callback; won't we leave some data in `mAudioOutputObserver`?
> 
> What are the implications? This handover seems a bit crude, even if we're planning to get rid of it with `mAudioOutputObserver`.
> 
> Can we do the handover by a message to the audio thread instead? SetLastPrefs updates the prefs on the main thread, which seems like a prime time to send a ControlMessage to MSG.

I.e., it used to be updated on the media thread (it's read on the msg/audio thread !). We can't dispatch an MSG-ControlMessage from anything else than main thread. So we piggybacked the ControlMessage dispatch on this existing main thread runnable.

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

Eh, yes, StopRecording() should happen after the check.

> The second arg here I expected to be `false`, not `device`... huh?

Good catch. I have no idea how I conceived this one.
(Reporter)

Comment 187

a year 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/#review221532

r=me with fix for MediaEngineWebRTCAudioCaptureSource::Deallocate(). Just nits otherwise.

::: commit-message-e4008:6
(Diff revision 7)
> +To allow this, this patch breaks out track-creation from Start() to SetSource()
> +and moves track-ending logic from Stop() to Deallocate().

MediaEngineSource::SetSource() sounds confusing (a "source" in my mind being MediaEngineSource).

How about SetStream() or CreateTrack() ?

::: dom/media/MediaManager.cpp:1238
(Diff revision 7)
> +        if (NS_SUCCEEDED(rv)) {
> +          rv = self->mAudioDevice->Start();
> +        }
>          if (NS_FAILED(rv)) {

else?

::: dom/media/webrtc/MediaEngineSource.h:147
(Diff revision 7)
> +                             const RefPtr<SourceMediaStream>& aStream,
> +                             TrackID aTrackID,
> +                             const PrincipalHandle& aPrincipal) = 0;
> +
> +  /**
> +   * Called by MediaEngine to start feeding data to the source associated with

s/source/track/

::: dom/media/webrtc/MediaEngineSource.h:169
(Diff revision 7)
>                                 const MediaEnginePrefs& aPrefs,
>                                 const nsString& aDeviceId,
>                                 const char** aOutBadConstraint) = 0;
>  
>    /**
> -   * Stop the device and release the corresponding MediaStream.
> +   * Called by MediaEngine to stop feeding data to the source associated with the given AllocationHandle.

s/source/track/

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:713
(Diff revision 7)
> -    bool Equals(const Allocation& aItem,
> +    return NS_OK;
> -                const RefPtr<SourceMediaStream>& aStream) const
> -    {
> -      return aItem.mStream == aStream;
> -    }
> +  }
> -  };
> +  

trailing space

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:968
(Diff revision 7)
> +  for (const Allocation& allocation : mAllocations) {
> +    if (allocation.mStream) {
> +      graph = allocation.mStream->Graph();

break; ?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:987
(Diff revision 7)
> +#ifdef DEBUG
> +  // Check that mSkipProcessing is only accessed on the graph thread.
> +  MediaStreamGraph* graph = nullptr;
> +  for (const Allocation& allocation : mAllocations) {
> +    if (allocation.mStream) {
> +      graph = allocation.mStream->Graph();

ditto

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:1217
(Diff revision 7)
> -MediaEngineWebRTCAudioCaptureSource::Stop(SourceMediaStream *aStream,
> -                                          TrackID aTrackID)
> +MediaEngineWebRTCAudioCaptureSource::Start(const RefPtr<const AllocationHandle>& aHandle)
> +{
> +  AssertIsOnOwningThread();
> +  return NS_OK;
> +}
> +
> +nsresult
> +MediaEngineWebRTCAudioCaptureSource::Stop(const RefPtr<const AllocationHandle>& aHandle)
>  {
>    AssertIsOnOwningThread();
> -  aStream->EndAllTrackAndFinish();

Where is this done now? Shouldn't it move to MediaEngineWebRTCAudioCaptureSource::Deallocate()?
Attachment #8929562 - Flags: review?(jib) → review+
(Reporter)

Comment 188

a year 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/#review218896

Lgtm. Seems doable to avoid returning a raw pointer from internal member-access function though.

::: dom/media/MediaManager.cpp:155
(Diff revision 7)
>  static Atomic<bool> sHasShutdown;
>  
>  typedef media::Pledge<bool, dom::MediaStreamError*> PledgeVoid;
>  
> +struct DeviceState {
> +  DeviceState(MediaDevice* aDevice, bool aOffWhileDisabled)

Maybe:

    const RefPtr<MediaDevice>& aDevice

?

::: dom/media/MediaManager.cpp:4169
(Diff revision 7)
> -  return Activated() && mAudioDevice && !mAudioStopped &&
> -         !mAudioDevice->mSource->IsAvailable() &&
> -         (!mAudioDevice->mSource->IsFake() ||
> +  return Activated() && mAudioDeviceState &&
> +         !mAudioDeviceState->mStopped &&
> +         GetAudioDevice()->GetMediaSource() == dom::MediaSourceEnum::Microphone &&
> +         (!GetAudioDevice()->mSource->IsFake() ||

Seems clearer to use `mAudioDeviceState->mDevice->` directly here (sibling member functions are for outside access imho).

Applies throughout.

::: dom/media/MediaManager.cpp:4227
(Diff revision 7)
> -  RefPtr<MediaDevice> videoDevice =
> -    aTrackID == kVideoTrack ? mVideoDevice.get() : nullptr;
>  
> -  if (mStopped || (!audioDevice && !videoDevice))
> -  {
> +  DeviceState* state = GetDeviceStateFor(aTrackID);
> +
> +  MOZ_ASSERT(state, "DeviceState is supposed to not go away");

Unnecessary with other advice.

::: dom/media/MediaManager.cpp:4312
(Diff revision 7)
> +DeviceState*
> +SourceListener::GetDeviceStateFor(TrackID aTrackID) const
> +{
> +  // XXX to support multiple tracks of a type in a stream, this should key off
> +  // the TrackID and not just the type
> +  switch (aTrackID) {
> +    case kAudioTrack:
> +      MOZ_ASSERT(mAudioDeviceState, "No audio device");
> +      return mAudioDeviceState.get();
> +    case kVideoTrack:
> +      MOZ_ASSERT(mVideoDeviceState, "No video device");
> +      return mVideoDeviceState.get();

How about we always return either *mAudioDeviceState or *mVideoDeviceState by reference? I.e.

    DeviceState&
    SourceListener::GetDeviceStateFor(TrackID aTrackID) const

Avoids raw pointer and assertions about nullptr that cannot happen.

::: dom/media/MediaManager.cpp:4315
(Diff revision 7)
> +  // XXX to support multiple tracks of a type in a stream, this should key off
> +  // the TrackID and not just the type

I think this comment is OBE.
Attachment #8929563 - Flags: review?(jib) → review+
(Reporter)

Comment 189

a year ago
mozreview-review
Comment on attachment 8940730 [details]
Bug 1299515 - Disable turning off camera while disabled by default on android.

https://reviewboard.mozilla.org/r/211008/#review221542
Attachment #8940730 - Flags: review?(jib) → review+
(Reporter)

Comment 190

a year ago
mozreview-review
Comment on attachment 8940731 [details]
Bug 1299515 - Replace a rawptr handoff with generalized lambda capture.

https://reviewboard.mozilla.org/r/211010/#review221544

_o/\o_
Attachment #8940731 - Flags: review?(jib) → review+
(Reporter)

Comment 191

a year ago
mozreview-review
Comment on attachment 8944779 [details]
Bug 1299515 - Remove enum DistanceCalculation from MediaEngineRemoteVideoSource.

https://reviewboard.mozilla.org/r/214942/#review221550

::: commit-message-90676:3
(Diff revision 2)
> +kFeasibility is no longer used since MediaEngineRemoteVideoSource changed to no
> +longer being shared.

I believe we still need feasibility distance.

"Feasibility distance" is a modification of the constraints algorithm for video to account for the availability of downscaling. E.g.

    {height: {max: 90}}

should effectively never fail, which it would in the stricter fitness distance algorithm when run over our discrete sets of capabilities.

If anything, we probably no longer need fitness distance, which was relegated to detecting conflicting constraints (audio constraints are all orthogonal, knock on wood).
Attachment #8944779 - Flags: review?(jib) → review-
(Reporter)

Comment 192

a year 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,
(Assignee)

Comment 193

a year 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.
(Reporter)

Comment 194

a year 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.
(Reporter)

Comment 195

a year 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+
Blocks: 1404977
(Reporter)

Comment 196

a year ago
mozreview-review-reply
Comment on attachment 8944779 [details]
Bug 1299515 - Remove enum DistanceCalculation from MediaEngineRemoteVideoSource.

https://reviewboard.mozilla.org/r/214942/#review221550

> I believe we still need feasibility distance.
> 
> "Feasibility distance" is a modification of the constraints algorithm for video to account for the availability of downscaling. E.g.
> 
>     {height: {max: 90}}
> 
> should effectively never fail, which it would in the stricter fitness distance algorithm when run over our discrete sets of capabilities.
> 
> If anything, we probably no longer need fitness distance, which was relegated to detecting conflicting constraints (audio constraints are all orthogonal, knock on wood).

(Covered in 1-1) Sorry {height: {max: 90}} was a bad example confusing things. Since we don't downscale to constraints (yet!) this should cause OverConstrainedError, so using fitness distance is correct.

So yes, technically we don't need feasibility distance anymore, since we only return "native sized" modes today (either native modes or downscaled modes made to look like native resolutions).

But once we add downscaling to constraints (bug 1286945) we'll need it again, so we agreed to keep the code around, just not call it. Hopefully that won't trigger any static analysis.
(Assignee)

Comment 197

a year ago
mozreview-review-reply
Comment on attachment 8944779 [details]
Bug 1299515 - Remove enum DistanceCalculation from MediaEngineRemoteVideoSource.

https://reviewboard.mozilla.org/r/214942/#review221550

> (Covered in 1-1) Sorry {height: {max: 90}} was a bad example confusing things. Since we don't downscale to constraints (yet!) this should cause OverConstrainedError, so using fitness distance is correct.
> 
> So yes, technically we don't need feasibility distance anymore, since we only return "native sized" modes today (either native modes or downscaled modes made to look like native resolutions).
> 
> But once we add downscaling to constraints (bug 1286945) we'll need it again, so we agreed to keep the code around, just not call it. Hopefully that won't trigger any static analysis.

As we discussed off-bug we don't need it right now, but we'll need it again once we allow scaling to a custom resolution (something not matched by a device capability). So I'll drop this patch altogether to keep this code even though there are no callers.
(Assignee)

Comment 198

a year 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

> MediaEngineSource::SetSource() sounds confusing (a "source" in my mind being MediaEngineSource).
> 
> How about SetStream() or CreateTrack() ?

SetTrack sgtm

> Where is this done now? Shouldn't it move to MediaEngineWebRTCAudioCaptureSource::Deallocate()?

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

Comment 199

a year 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

> break; ?

Oh, this also needs a lock for accessing mAllocations off the owning thread.
(Assignee)

Comment 200

a year 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

> Can you elaborate the use case of Cancel()?

The use case of Cancel() is that I set a (3 second) timer as a reaction to user input (an off toggle). If the user toggles back to on before the timer has fired, I cancel the timer so it can be restarted on the next off-toggle.

The use case for the rejections is that I have a small promise chain with inlined rejection handling in a lambda. The timer is first in the chain. With Cancel() rejecting the promises I can have a single path for handling the timer canceling gracefully and the second step of the chain succeeding.
(Assignee)

Comment 201

a year ago
mozreview-review-reply
Comment on attachment 8929563 [details]
Bug 1299515 - Stop Camera and Microphone device when tracks become disabled.

https://reviewboard.mozilla.org/r/200840/#review218896

> I think this comment is OBE.

It keys off the TrackID but it can only match them to kAudioTrack and kVideoTrack, so we're not there yet.
(Assignee)

Updated

a year ago
Depends on: 1433878
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8944779 - Attachment is obsolete: true

Comment 218

a year 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]
(Reporter)

Comment 219

a year 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+
(Assignee)

Updated

a year ago
Blocks: 1434274
(Assignee)

Comment 220

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