From the spec: Muted refers to the input to the MediaStreamTrack. If live samples are not made available to the MediaStreamTrack it is muted. Muted is out of control for the application, but can be observed by the application by reading the muted attribute and listening to the associated events mute and unmute. There can be several reasons for a MediaStreamTrack to be muted: the user pushing a physical mute button on the microphone, the user toggling a control in the operating system, the user clicking a mute button in the browser chrome, the User Agent (on behalf of the user) mutes, etc. --- I don't think we have low-level support for this yet. I picture it working either: * the same way as `enabled` on MSG-tracks, or * the source puts null data into the track and the rest is automatic - null Image for VideoChunks (already supported) - AudioChunks will have to be refactored to support something nullable The latter might interfere with removing audio and video buffering from MediaStreamGraph? Perhaps the `enabled` route is simpler.

Muted applies to video as well, so I'm changing the component. We'll likely need this for WebRTC first, and on remote tracks only. We should put in the webidl and the code, so webrtc can call it on remote tracks. We'll need mute events in the same release transceivers land, because transceivers likely mean the end of ended events, leaving JS with no way to detect renegotiation causing removal of remote tracks. This is assuming https://github.com/w3c/webrtc-pc/pull/1402 merges soon. I can't think of any cases where Firefox chrome would fire muted on a local track atm, but open to hear use cases.

Andreas, is this something that would be quick to add assuming the only user for now will be RTCPeerConnection? We'll need this for bug 1290948.

I think so. I can take a look at it.

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

Comment on attachment 8925972 [details] Bug 1208378 - Implement MediaStreamTrack's muted state and events. https://reviewboard.mozilla.org/r/197174/#review202390 r+ for the .webidl, but please consider to not use auto so heavily. ::: dom/media/MediaStreamTrack.h:206 (Diff revision 1) > + * Notifies all sinks. > + */ > + void MutedChanged(bool aNewState) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + auto sinks(mSinks); You make me cry. What does this code do? What is the type of auto here? This makes code totally unreadable. Please don't use auto so heavily. We have had security critical issues because of it, and we have had memory leaks because of it. Write code to be easy to read and review. auto should be in general used only when it is obvious from the surrounding code what the type is, or the special case when iterating some data structure and the iterator type would be something complicated. ::: dom/media/MediaStreamTrack.h:212 (Diff revision 1) > + for (auto& sink : sinks) { > + if (!sink) { > + MOZ_ASSERT_UNREACHABLE("Sink was not explicitly removed"); > + mSinks.RemoveElement(sink); > + } > + sink->MutedChanged(aNewState); I assume this can't delete any sink objects, right? sinks seems to keep just raw pointers to objects.

Comment on attachment 8925972 [details] Bug 1208378 - Implement MediaStreamTrack's muted state and events. https://reviewboard.mozilla.org/r/197174/#review202390 > You make me cry. What does this code do? > What is the type of auto here? > This makes code totally unreadable. > Please don't use auto so heavily. We have had security critical issues because of it, and we have had memory leaks because of it. Write code to be easy to read and review. > auto should be in general used only when it is obvious from the surrounding code what the type is, or the special case when iterating some data structure and the iterator type would be something complicated. Please don't cry! I can make it a full type. The auto is to make a copy of an array of pointers so we don't run into issues if the listener decides to remove the sink in the handler (mid-iteration). We had issues like that in the past. > I assume this can't delete any sink objects, right? > sinks seems to keep just raw pointers to objects. Which raw pointers? Note that the first patch changed the raw pointers in mSinks to WeakPtrs.

(In reply to Andreas Pehrson [:pehrsons] from comment #10) > Which raw pointers? > > Note that the first patch changed the raw pointers in mSinks to WeakPtrs. You see, auto makes the code hard to read. If one had used non-auto, the types would be clear.

(In reply to Olli Pettay [:smaug] from comment #15) > You see, auto makes the code hard to read. If one had used non-auto, the > types would be clear. Point taken, thanks.

Comment on attachment 8925970 [details] Bug 1208378 - Store MediaStreamTrackSource::Sink in a WeakPtr. https://reviewboard.mozilla.org/r/197170/#review204198 ::: dom/media/MediaStreamTrack.h:187 (Diff revision 2) > * Notifies all sinks. > */ > void PrincipalChanged() > { > - for (Sink* sink : mSinks) { > + MOZ_ASSERT(NS_IsMainThread()); > + nsTArray<WeakPtr<Sink>> sinks(mSinks); Maybe s/sinks/copy/?

Comment on attachment 8925971 [details] Bug 1208378 - Make HTMLMediaElement register its Sink. https://reviewboard.mozilla.org/r/197172/#review204200

Comment on attachment 8925972 [details] Bug 1208378 - Implement MediaStreamTrack's muted state and events. https://reviewboard.mozilla.org/r/197174/#review204208 Lgtm, modulo removing the queueing of a task. ::: dom/media/MediaStreamTrack.h:398 (Diff revision 2) > + /** > + * 4.3.1 Life-cycle and Media flow - Media flow > + * To update a track's muted state to `newState`, the User Agent MUST queue > + * a task to run the following steps: > + * 1. Let track be the MediaStreamTrack in question. > + * 2. Set track's muted attribute to newState. > + * 3. If newState is true let eventName be mute, otherwise unmute. > + * 4. Fire a simple event named eventName on track. > + */ Note this might change with https://github.com/w3c/webrtc-pc/issues/1568 ::: dom/media/MediaStreamTrack.cpp:370 (Diff revision 2) > + MOZ_ASSERT(NS_IsMainThread()); > + RefPtr<MediaStreamTrack> track = this; > + NS_DispatchToMainThread(NewRunnableFrom([track, aNewState]() mutable { I need to write a PR for https://github.com/w3c/webrtc-pc/issues/1568 Basically, I think we should anticipate a resolution to that issue and not queue a task here. We're already on the main task, which I think is what the mediacapture language was trying to ensure. We want to enable peer connection to fire these events before the SRD promise resolves, and the extra queueing of a task here makes that hard. ::: dom/media/MediaStreamTrack.cpp:378 (Diff revision 2) > + nsString eventName; > + if (aNewState) { > + eventName = NS_LITERAL_STRING("mute"); > + } else { > + eventName = NS_LITERAL_STRING("unmute"); nit: why not ? :

Comment on attachment 8925973 [details] Bug 1208378 - Set up basic tests with muted state. https://reviewboard.mozilla.org/r/197190/#review204226

PR up at https://github.com/w3c/webrtc-pc/pull/1667 regarding comment 19.

The next push is just a rebase, and will go up to try.

Comment on attachment 8929591 [details] Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop. https://reviewboard.mozilla.org/r/200858/#review206374 Patch does the right thing, so r+. Only the terminology may need some cleanup. ::: commit-message-f0dad:3 (Diff revision 3) > +We have two types of sinks in-tree. MediaStreamTracks and > +HTMLMediaElement::StreamCaptureTrackSource. A source relies on the presence of > +sinks to not call stop() on the underlying source. However, a > +StreamCaptureTrackSource should *not* keep a sink alive. If it does, Stop()ing > +a track that is also played and captured in a media element would not stop the > +source. Can I suggest this reword of the commit message? "There are two types of sinks for a MediaStreamTrackSource. Actual MediaStreamTracks and HTMLMediaElement::StreamCaptureTrackSource. A source needs actual tracks as sinks to not call stop() on the underlying source. A StreamCaptureTrackSource, however, should *not* count toward keeping a sorce alive (otherwise mozCaptureStream() would prevent track.stop() from working on the track feeding the media element)." ::: commit-message-f0dad:6 (Diff revision 3) > +Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop. r?jib > + > +We have two types of sinks in-tree. MediaStreamTracks and > +HTMLMediaElement::StreamCaptureTrackSource. A source relies on the presence of > +sinks to not call stop() on the underlying source. However, a > +StreamCaptureTrackSource should *not* keep a sink alive. If it does, Stop()ing I think you mean s/sink/source/ here. ::: dom/media/MediaStreamTrack.h:48 (Diff revision 3) > + ListenOnly, // A Sink that is a passive listener to events from a Source > + AffectingSource, // A Sink that keeps a Source alive and affects its enabled state Not sure passive vs. non-passive (actively modifying?), or affective non-affective are the right terms. It's already confusing that the "sink" of a (circularly defined) "track source" can be anything but a track, so how about: SinkMode::RealTrack SinkMode::NonTrack This relies on the concept of tracks keeping track "sources" alive as being understood, and marks StreamCaptureTrackSource as exceptional. I think the right abstraction to lean on is whether a sink "counts" or not as a first-order sink. That way we don't have to define new abilities. (From IRC with pehrsons) or something like Sink::AsMediaStreamTrack() (From IRC with pehrsons) or something like Sink::KeepAlive() Then we don't need the enum.

Comment on attachment 8930018 [details] Bug 1208378 - Fix WPT expectations. https://reviewboard.mozilla.org/r/201224/#review206380 lgtm

Comment on attachment 8929591 [details] Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop. https://reviewboard.mozilla.org/r/200858/#review206374 > Can I suggest this reword of the commit message? > > "There are two types of sinks for a MediaStreamTrackSource. Actual MediaStreamTracks and HTMLMediaElement::StreamCaptureTrackSource. A source needs actual tracks as sinks to not call stop() on the underlying source. > > A StreamCaptureTrackSource, however, should *not* count toward keeping a sorce alive (otherwise mozCaptureStream() would prevent track.stop() from working on the track feeding the media element)." Taken, with a few modifications to the first paragraph.

Comment on attachment 8929591 [details] Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop. https://reviewboard.mozilla.org/r/200858/#review206462 ::: commit-message-f0dad:3 (Diff revisions 3 - 4) > Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop. r?jib > > -We have two types of sinks in-tree. MediaStreamTracks and > +There are currently two types of sinks for a MediaStreamTrackSource. maybe a colon?

Comment on attachment 8929591 [details] Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop. https://reviewboard.mozilla.org/r/200858/#review206462 > maybe a colon? In all fairness, you should have suggested the colon last time :-P

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/83d5a772d5b4 Store MediaStreamTrackSource::Sink in a WeakPtr. r=jib https://hg.mozilla.org/integration/autoland/rev/29b6b2c4855d Make HTMLMediaElement register its Sink. r=jib https://hg.mozilla.org/integration/autoland/rev/b9333a4fe45b Implement MediaStreamTrack's muted state and events. r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/b28d805676dd Set up basic tests with muted state. r=jib https://hg.mozilla.org/integration/autoland/rev/a13e81bd6443 Distinguish track sinks on whether their presence allows a source to stop. r=jib https://hg.mozilla.org/integration/autoland/rev/b2cc4de35eb1 Fix WPT expectations. r=jib

(In reply to Andreas Pehrson [:pehrsons] from comment #41) > In all fairness, you should have suggested the colon last time :-P True that! :)

https://hg.mozilla.org/mozilla-central/rev/83d5a772d5b4 https://hg.mozilla.org/mozilla-central/rev/29b6b2c4855d https://hg.mozilla.org/mozilla-central/rev/b9333a4fe45b https://hg.mozilla.org/mozilla-central/rev/b28d805676dd https://hg.mozilla.org/mozilla-central/rev/a13e81bd6443 https://hg.mozilla.org/mozilla-central/rev/b2cc4de35eb1

Work on doc updates is complete. A review of the new and updated content would be helpful. Hitting up Andreas [:pehrsons] for that. Documentation updated: https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/onmute https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/onunmute https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/muted Updated this page to mention that enabled, note muted, is the way to go for implementing track muting as users understand it: https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/enabled New articles written: https://developer.mozilla.org/en-US/docs/Web/Events/mute https://developer.mozilla.org/en-US/docs/Web/Events/unmute Updated Firefox 59 for developers. Finally, submitted a PR (#572 on the KumaScript repo) to remove the nonexistent "muted" event.

Looking great, thanks!