Closed Bug 1208378 Opened 9 years ago Closed 7 years ago

Implement MediaStreamTrack.muted/onmute/onunmute

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Version:
33 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

Bug 1208378 - Store MediaStreamTrackSource::Sink in a WeakPtr.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1208378 - Make HTMLMediaElement register its Sink.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1208378 - Implement MediaStreamTrack's muted state and events.
59 bytes, text/x-review-board-request
smaug
: review+
jib
: review+
Details
Bug 1208378 - Set up basic tests with muted state.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1208378 - Distinguish track sinks on whether their presence allows a source to stop.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1208378 - Fix WPT expectations.
59 bytes, text/x-review-board-request
jib
: review+
Details 
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.
Rank: 25
Priority: -- → P2
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.
Blocks: 1290948
Rank: 25 → 15
Component: Audio/Video: MediaStreamGraph → WebRTC: Audio/Video
Priority: P2 → P1
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.
Flags: needinfo?(apehrson)
I think so. I can take a look at it.
Assignee: nobody → apehrson
Flags: needinfo?(apehrson)
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Blocks: 1400363
No longer blocks: 1290948
Status: NEW → ASSIGNED
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.
Attachment #8925972 - Flags: review?(bugs) → review+
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/?
Attachment #8925970 - Flags: review?(jib) → review+
Comment on attachment 8925971 [details]
Bug 1208378 - Make HTMLMediaElement register its Sink.

https://reviewboard.mozilla.org/r/197172/#review204200
Attachment #8925971 - Flags: review?(jib) → review+
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 ? :
Attachment #8925972 - Flags: review?(jib) → review+
Comment on attachment 8925973 [details]
Bug 1208378 - Set up basic tests with muted state.

https://reviewboard.mozilla.org/r/197190/#review204226
Attachment #8925973 - Flags: review?(jib) → review+
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.
Attachment #8929591 - Flags: review?(jib) → review+
Comment on attachment 8930018 [details]
Bug 1208378 - Fix WPT expectations.

https://reviewboard.mozilla.org/r/201224/#review206380

lgtm
Attachment #8930018 - Flags: review?(jib) → review+
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! :)
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.
Flags: needinfo?(apehrson)
Keywords: dev-doc-needed → dev-doc-complete
Looking great, thanks!
Flags: needinfo?(apehrson)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: