Closed
Bug 1400363
Opened 7 years ago
Closed 7 years ago
Update muted state on tracks when negotiation happens
Categories
(Core :: WebRTC: Signaling, enhancement, P2)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: bwc, Assigned: bwc)
References
Details
Attachments
(5 files)
(deleted),
text/x-review-board-request
|
jib
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
drno
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
drno
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
drno
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
pehrsons
:
review+
|
Details |
No description provided.
Updated•7 years ago
|
Rank: 15
Priority: -- → P2
Updated•7 years ago
|
Assignee: nobody → docfaraday
Comment 1•7 years ago
|
||
There are now patches on bug 1208378 that you can use to start implementing this.
Flags: needinfo?(docfaraday)
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 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209564
Looks good with the two concerns below addresses.
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1934
(Diff revision 2)
> + NS_DispatchToMainThread(NewRunnableMethod(
> + "GenericReceiveListener::OnRtpReceived_m",
> + this,
> + &GenericReceiveListener::OnRtpReceived_m));
Do we need to hold a temporary reference to this to avoid the object getting destroyed before that runnable gets executed?
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1934
(Diff revision 2)
> }
> }
>
> + void OnRtpReceived()
> + {
> + NS_DispatchToMainThread(NewRunnableMethod(
Does this mean we are going to dispatch a runnable to main thread on every single RTP packet we receive?
If so then we should probably only do this on the first received RTP packet.
Attachment #8933098 -
Flags: review?(drno) → review+
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8933099 [details]
Bug 1400363 - Part 3: Start webrtc receive tracks as muted.
https://reviewboard.mozilla.org/r/204068/#review209566
LGTM
Attachment #8933099 -
Flags: review?(drno) → review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8933100 [details]
Bug 1400363 - Part 4: Mute webrtc receive tracks when they are negotiated to stop receiving.
https://reviewboard.mozilla.org/r/204070/#review209576
LGTM after making that comment less confusing.
::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:515
(Diff revision 2)
> } else {
> - currentDirection = dom::RTCRtpTransceiverDirection::Inactive;
> + aJsTransceiver.SetCurrentDirection(
> + dom::RTCRtpTransceiverDirection::Inactive, aRv);
> }
> +
> + // If negotiation stops a remote track from sending, we mark the track muted.
This comment confused me. I think it would be easier to match to the code above if it says "If negotiation stops a track from receiving, we mark the track muted."
Attachment #8933100 -
Flags: review?(drno) → review+
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209564
> Do we need to hold a temporary reference to this to avoid the object getting destroyed before that runnable gets executed?
My understanding is that NewRunnableMethod does this, since it requires the second arg to have AddRef() and Release() methods. I can double-check.
> Does this mean we are going to dispatch a runnable to main thread on every single RTP packet we receive?
>
> If so then we should probably only do this on the first received RTP packet.
Hmm. Easier said than done. This code-path needs to be exercised every time an RTP packet is received when the track is muted (which can happen any number of times), but it is only safe to check that on main. We'd probably have to cache the muted state in an Atomic or something. I'll look into it.
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209564
> My understanding is that NewRunnableMethod does this, since it requires the second arg to have AddRef() and Release() methods. I can double-check.
Ok, this does acquire a reference, so it should be good.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review209688
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:82
(Diff revision 2)
> + let onmute = track => {
> + return new Promise(resolve => track.onmute = () => {
> + if (track.muted) {
> + resolve();
> + } else {
> + ok(false, "onmute fired, but track isn't muted!");
> + reject();
> + }
> + });
> + };
> +
> + let onunmute = track => {
> + return new Promise(resolve => track.onunmute = () => {
> + if (track.muted) {
> + ok(false, "onunmute fired, but track is still muted!");
> + reject();
> + } else {
> + resolve();
> + }
> + });
> + };
Since you have the full test framework here, you could use `haveEvent()` instead. Perhaps even `haveEventsButNoMore()`.
https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/dom/media/tests/mochitest/head.js#678
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:824
(Diff revision 2)
> + // Check that receive tracks start muted
> + hasProps(pc1.getTransceivers(),
> + [
> + {receiver: {track: {kind: "audio", muted: true}}},
> + {receiver: {track: {kind: "video", muted: true}}}
> + ]);
> +
> + hasProps(pc1.getTransceivers(),
> + [
> + {receiver: {track: {kind: "audio", muted: true}}},
> + {receiver: {track: {kind: "video", muted: true}}}
> + ]);
We should check that video is black and audio is silent when muted.
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:855
(Diff revision 2)
> + // Check that receive tracks are unmuted when RTP starts flowing
> + await onunmuteaudio1;
> + await onunmutevideo1;
> + await onunmuteaudio2;
> + await onunmutevideo2;
Should check that audio and video are flowing when unmuted.
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:872
(Diff revision 2)
> + await onmuteaudio1;
> + await onmutevideo1;
Should check that audio1, video1 are silent/black, after mute. Likewise we should check that audio2, video2 are still flowing.
and so on for the remaining two cases too.
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209694
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1896
(Diff revision 3)
> };
>
> class GenericReceiveListener : public MediaStreamListener
> {
> public:
> - GenericReceiveListener(SourceMediaStream *source, TrackID track_id)
> + GenericReceiveListener(DOMMediaStream *stream, TrackID track_id)
Can we replace the `DOMMediaStream*` and `TrackID` here with just a `MediaStreamTrack*`?
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1947
(Diff revision 3)
> + }
> + }
> +
> + void OnRtpReceived_m()
> + {
> + if (listening_ && track_ && track_->Muted()) {
This check should really happen on the thread that calls `OnRtpReceived()`. Then you can just dispatch `MutedChanged()` straight to main thread.
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8933100 [details]
Bug 1400363 - Part 4: Mute webrtc receive tracks when they are negotiated to stop receiving.
https://reviewboard.mozilla.org/r/204070/#review209704
::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:515
(Diff revision 3)
> + // If negotiation stops a track from receiving (ie; m-section is
> + // negotiated "sendonly" or "inactive"), we mark the track muted. We do
> + // _not_ do the reverse; we need to wait for RTP to unmute according to
> + // the spec. That happens in MediaPipeline.
I think it would simplify things if this signaled the muted state to MediaPipeline on the thread that receives rtp packets.
The main thread muted state change should then probably be handled by MediaPipeline as well, to keep state in sync.
::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:520
(Diff revision 3)
> + // If negotiation stops a track from receiving (ie; m-section is
> + // negotiated "sendonly" or "inactive"), we mark the track muted. We do
> + // _not_ do the reverse; we need to wait for RTP to unmute according to
> + // the spec. That happens in MediaPipeline.
> + nsTArray<RefPtr<dom::MediaStreamTrack>> tracks;
> + mReceiveStream->GetTracks(tracks);
Ouch. I think `mReceiveTrack` would be better. I'd prefer poking a hole to `MediaStreamTrack::GetInputStream` rather than you having to keep a `DOMMediaStream` here.
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209694
> Can we replace the `DOMMediaStream*` and `TrackID` here with just a `MediaStreamTrack*`?
Is there a way to get the SourceMediaStream from the MediaStreamTrack? If so, that would be fine.
> This check should really happen on the thread that calls `OnRtpReceived()`. Then you can just dispatch `MutedChanged()` straight to main thread.
None of these variables are safe to touch from STS. It is simpler to dispatch and then check (with an Atomic flag that tells us when we _might_ need to check) than it is to make all of these things threadsafe.
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933100 [details]
Bug 1400363 - Part 4: Mute webrtc receive tracks when they are negotiated to stop receiving.
https://reviewboard.mozilla.org/r/204070/#review209704
> I think it would simplify things if this signaled the muted state to MediaPipeline on the thread that receives rtp packets.
>
> The main thread muted state change should then probably be handled by MediaPipeline as well, to keep state in sync.
I could live with that.
> Ouch. I think `mReceiveTrack` would be better. I'd prefer poking a hole to `MediaStreamTrack::GetInputStream` rather than you having to keep a `DOMMediaStream` here.
That would be really nice to have, and simplify quite a bit of code. Probably a large enough change to merit its own bug, honestly.
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209694
> Is there a way to get the SourceMediaStream from the MediaStreamTrack? If so, that would be fine.
We could expose [1] one way or the other. It's not exposed today because the producer of the track is supposed to know the SourceMediaStream already. However now with certain things (muted state in this case) signaled directly from the producer to the track, I think it makes sense to open this up. Doing it in another bug or here doesn't really matter, as long as it gets dealt with soonish.
[1] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/dom/media/MediaStreamTrack.h#523
> None of these variables are safe to touch from STS. It is simpler to dispatch and then check (with an Atomic flag that tells us when we _might_ need to check) than it is to make all of these things threadsafe.
Would it be enough to make `track_` a `const RefPtr<MediaStreamTrack>` to ensure it doesn't go away?
And to do message passing with mirrored muted state (from signaling) on the STS thread?
If this is the only place setting the muted state on the MediaStreamTrack there's nothing else that can modify it. So you'd never have to read `track_->Muted()` here - maintaining a local copy would be fine.
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209694
> Would it be enough to make `track_` a `const RefPtr<MediaStreamTrack>` to ensure it doesn't go away?
> And to do message passing with mirrored muted state (from signaling) on the STS thread?
>
> If this is the only place setting the muted state on the MediaStreamTrack there's nothing else that can modify it. So you'd never have to read `track_->Muted()` here - maintaining a local copy would be fine.
track_ needs to be released on main (in EndTrack), otherwise we hit assertions due to the last PipelineListener ref going away on some webrtc.org thread. |listening_| is also updated on main, and I would prefer to avoid races on it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933098 [details]
Bug 1400363 - Part 2: Unmute webrtc receive tracks when RTP is received.
https://reviewboard.mozilla.org/r/204066/#review209694
> We could expose [1] one way or the other. It's not exposed today because the producer of the track is supposed to know the SourceMediaStream already. However now with certain things (muted state in this case) signaled directly from the producer to the track, I think it makes sense to open this up. Doing it in another bug or here doesn't really matter, as long as it gets dealt with soonish.
>
> [1] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/dom/media/MediaStreamTrack.h#523
Ok, I have done this in part 0.
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933100 [details]
Bug 1400363 - Part 4: Mute webrtc receive tracks when they are negotiated to stop receiving.
https://reviewboard.mozilla.org/r/204070/#review209704
> I could live with that.
I think this is more trouble than it is worth. Dispatching this to STS in order to update a cached value, and then dispatching back to main again to perform the actual muting, seems a bit much to me.
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review209934
Lgtm, just nits and a question.
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:82
(Diff revision 2)
> + let onmute = track => {
> + return new Promise(resolve => track.onmute = () => {
> + if (track.muted) {
> + resolve();
> + } else {
> + ok(false, "onmute fired, but track isn't muted!");
> + reject();
> + }
> + });
> + };
> +
> + let onunmute = track => {
> + return new Promise(resolve => track.onunmute = () => {
> + if (track.muted) {
> + ok(false, "onunmute fired, but track is still muted!");
> + reject();
> + } else {
> + resolve();
> + }
> + });
> + };
Agree. Otherwise, please rewrite to have minimal code inside the Promise constructor executor function. E.g.
await new Promise(resolve => track.onunmute = resolve);
is(track.muted, false, "onunmute fired, but track is still muted!");
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:82
(Diff revision 2)
>
> let negotiationNeeded = pc => {
> return new Promise(resolve => pc.onnegotiationneeded = resolve);
> };
>
> + let onmute = track => {
nit: onMute and onUnmute
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:794
(Diff revision 2)
> ]);
>
> trickle(pc2, pc1);
> await pc2.setLocalDescription(answer);
>
> + let onunmutePc1 = onunmute(pc1.getTransceivers()[0].receiver.track);
Maybe s/onunmutePc1/haveUnmutePc1/ ?
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:800
(Diff revision 2)
> +
> await iceConnected(pc1);
> await iceConnected(pc2);
>
> + await onunmutePc1;
> + await wait(1000);
Why this?
Attachment #8933097 -
Flags: review?(jib) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8933463 [details]
Bug 1400363 - Part 0: Expose the SourceMediaStream of MediaStreamTrack, and store tracks instead of the source streams.
https://reviewboard.mozilla.org/r/204384/#review210160
Great! Mostly nits to fix.
FWIW I think in the long term we'll need an equivalent of NotifyPull on MediaStreamTrackListener. That way we'll be able to get rid of the direct use of SourceMediaStream here and code can be simplified further.
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1894
(Diff revision 1)
> - GenericReceiveListener(SourceMediaStream *source, TrackID track_id)
> - : source_(source),
> + GenericReceiveListener(dom::MediaStreamTrack* track, TrackID track_id)
> + : track_(track),
> track_id_(track_id),
track_id == track->mInputTrackID.
Maybe we can make `MediaStreamTrack::mInputTrackID` and `MediaStreamTrack::mTrackID` public and const?
That would get rid of `track_id_` too.
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1956
(Diff revision 1)
>
> - RefPtr<SourceMediaStream> source_;
> + RefPtr<MediaStreamTrack> track_;
> const TrackID track_id_;
> };
>
> - source_->GraphImpl()->AppendMessage(MakeUnique<Message>(source_, track_id_));
> + track_->GraphImpl()->AppendMessage(MakeUnique<Message>(track_, track_id_));
Grab the SourceMediaStream on main thread to avoid passing the MediaStreamTrack to the MSG.
Also, no need to keep `source_`. There's `MediaStream* ControlMessage::GetStream();`.
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1968
(Diff revision 1)
> {
> class Message : public ControlMessage
> {
> public:
> Message(GenericReceiveListener* listener,
> - MediaStream* stream,
> + dom::MediaStreamTrack* track,
It's ok to pass `nullptr` to ControlMessage if you're not going to use the stream (`ControlMessage::GetStream() like above).
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2142
(Diff revision 1)
> nsCOMPtr<nsIEventTarget> main_thread,
> nsCOMPtr<nsIEventTarget> sts_thread,
> RefPtr<AudioSessionConduit> conduit,
> - SourceMediaStream* aStream) :
> + dom::MediaStreamTrack* aTrack) :
> MediaPipelineReceive(pc, main_thread, sts_thread, conduit),
> - listener_(aStream ? new PipelineListener(aStream, conduit_) : nullptr)
> + listener_(aTrack ? new PipelineListener(aTrack, conduit_) : nullptr)
Is it legal to not have a track here?
::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2326
(Diff revision 1)
> -OwningNonNull<DOMMediaStream>
> -PeerConnectionImpl::CreateReceiveStreamWithTrack(
> +OwningNonNull<dom::MediaStreamTrack>
> +PeerConnectionImpl::CreateReceiveTrack(
> SdpMediaSection::MediaType type) {
>
> - OwningNonNull<DOMMediaStream> stream = MakeMediaStream();
> + MediaStreamGraph* graph =
> + MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, GetWindow());
Note that AUDIO_THREAD_DRIVER is the driver that will be created if there's no existing graph.
This should then be AUDIO_THREAD_DRIVER if we're creating an audio track, or SYSTEM_THREAD_DRIVER if we're creating a video track.
::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:120
(Diff revision 1)
> - DOMMediaStream& aReceiveStream,
> + dom::MediaStreamTrack& aReceiveTrack,
> dom::MediaStreamTrack* aSendTrack,
Any reason for the receive and send tracks to be of different types - ref vs ptr?
::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1144
(Diff revision 1)
> - DOMMediaStream& aReceiveStream,
> + dom::MediaStreamTrack& aReceiveTrack,
> dom::MediaStreamTrack* aSendTrack,
Also here
Attachment #8933463 -
Flags: review?(apehrson) → review+
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933463 [details]
Bug 1400363 - Part 0: Expose the SourceMediaStream of MediaStreamTrack, and store tracks instead of the source streams.
https://reviewboard.mozilla.org/r/204384/#review210160
> Is it legal to not have a track here?
The mediapipeline_unittest does this :(
> Any reason for the receive and send tracks to be of different types - ref vs ptr?
Transceivers always have a receive track, but might not have a send track. Just enforcing that invariant everywhere.
Assignee | ||
Comment 36•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933463 [details]
Bug 1400363 - Part 0: Expose the SourceMediaStream of MediaStreamTrack, and store tracks instead of the source streams.
https://reviewboard.mozilla.org/r/204384/#review210160
> It's ok to pass `nullptr` to ControlMessage if you're not going to use the stream (`ControlMessage::GetStream() like above).
Is that something I can count on being true in the future?
Comment 37•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933463 [details]
Bug 1400363 - Part 0: Expose the SourceMediaStream of MediaStreamTrack, and store tracks instead of the source streams.
https://reviewboard.mozilla.org/r/204384/#review210160
> Is that something I can count on being true in the future?
Sure. There are already plenty of examples in the tree:
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla14ControlMessageC1EPNS_11MediaStreamE&redirect=false
Assignee | ||
Comment 38•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review209934
> nit: onMute and onUnmute
I've renamed to gotMuteEvent and gotUnmuteEvent.
> Why this?
Making sure that onunmute doesn't fire for pc2's receive track. I can add a comment.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review209688
> We should check that video is black and audio is silent when muted.
It isn't possible to do this on a naked track as far as I know, and checking for frames feels a little out-of-scope for this test-case anyhow.
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8933463 [details]
Bug 1400363 - Part 0: Expose the SourceMediaStream of MediaStreamTrack, and store tracks instead of the source streams.
https://reviewboard.mozilla.org/r/204384/#review210248
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`
::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1939
(Diff revision 2)
> - : ControlMessage(stream),
> + track_id_(track->GetInputTrackId())
> - source_(stream),
> - track_id_(track_id)
> {}
>
> + virtual ~Message() {}
Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
virtual ~Message() {}
^
= default;
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review209688
> It isn't possible to do this on a naked track as far as I know, and checking for frames feels a little out-of-scope for this test-case anyhow.
Testing this in other test cases now.
Comment 52•7 years ago
|
||
mozreview-review |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review210958
IMHO these issues still need fixing before landing this.
::: dom/media/tests/mochitest/test_peerConnection_removeThenAddVideoTrack.html:64
(Diff revision 5)
> "pcRemote should have two transceivers");
> const track = test.pcRemote._pc.getTransceivers()[0].receiver.track;
>
> const vAdded = test.pcRemote.remoteMediaElements.find(
> elem => elem.id.includes(track.id));
> return helper.checkVideoPaused(vAdded, 10, 10, 16, 5000);
This checks that the video is paused while muted, but spec says explicitly that it should be black, [1] (there's certainly ambiguity in there. however for a media element I interpret it as rendering black).
[1] https://w3c.github.io/mediacapture-main/getusermedia.html#life-cycle-and-media-flow
::: dom/media/tests/mochitest/test_peerConnection_removeThenAddVideoTrack.html:69
(Diff revision 5)
> + test.chain.insertBefore("PC_REMOTE_SET_LOCAL_DESCRIPTION", [
> + function PC_REMOTE_SETUP_ONMUTE(test) {
> + haveMuteEvent = haveEvent(test.pcRemote._pc.getReceivers()[0].track, "mute");
> + },
> + function PC_REMOTE_SETUP_ONUNMUTE(test) {
> + haveUnmuteEvent = haveEvent(test.pcRemote._pc.getReceivers()[1].track, "unmute");
> + }
This checks that removing a track and adding another works.
A more important (as in where I could see issues happening) case would be where a transceiver goes from receiving to not receiving and back to receiving again. I.e., re-using the receiver.
The transceivers test tests muted events for the re-use case but not that media is flowing, and the other tests don't cover the re-use case at all :/
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:829
(Diff revision 5)
> + let gotUnmuteAudio1 = gotUnmuteEvent(pc1.getTransceivers()[0].receiver.track);
> + let gotUnmuteVideo1 = gotUnmuteEvent(pc1.getTransceivers()[1].receiver.track);
> +
> + let gotUnmuteAudio2 = gotUnmuteEvent(pc2.getTransceivers()[0].receiver.track);
> + let gotUnmuteVideo2 = gotUnmuteEvent(pc2.getTransceivers()[1].receiver.track);
This is fine and all but also kind of a happy-path test. Would you mind adding checks that we don't get unexpected "mute" or "unmute" events in between the expected ones?
::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:879
(Diff revision 5)
> + gotUnmuteAudio1 = gotUnmuteEvent(pc1.getTransceivers()[0].receiver.track);
> + gotUnmuteVideo1 = gotUnmuteEvent(pc1.getTransceivers()[1].receiver.track);
> + gotUnmuteAudio2 = gotUnmuteEvent(pc2.getTransceivers()[0].receiver.track);
> + gotUnmuteVideo2 = gotUnmuteEvent(pc2.getTransceivers()[1].receiver.track);
> + await pc1.setRemoteDescription(answer);
> + await pc2.setLocalDescription(answer);
What about ordering here? It should take until finishing sRD and sLD before "unmute" can fire, no? Should enforce that by checking muted and setting up the `gotUnmuteEvent` after them?
Same question goes for the other cases in this test.
Comment 53•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review210958
Agree. My review was meant as "in addition to" Andreas's comments.
Assignee | ||
Comment 54•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review210958
> This checks that the video is paused while muted, but spec says explicitly that it should be black, [1] (there's certainly ambiguity in there. however for a media element I interpret it as rendering black).
>
> [1] https://w3c.github.io/mediacapture-main/getusermedia.html#life-cycle-and-media-flow
We have never done this in response to negotiation. That would be a new bug.
> This checks that removing a track and adding another works.
>
> A more important (as in where I could see issues happening) case would be where a transceiver goes from receiving to not receiving and back to receiving again. I.e., re-using the receiver.
>
> The transceivers test tests muted events for the re-use case but not that media is flowing, and the other tests don't cover the re-use case at all :/
Then this is a pre-existing problem, and not within scope of this bug. I can open a new one.
> What about ordering here? It should take until finishing sRD and sLD before "unmute" can fire, no? Should enforce that by checking muted and setting up the `gotUnmuteEvent` after them?
>
> Same question goes for the other cases in this test.
Current spec has onmute firing before resolution of sRD.
Comment 55•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review210958
> We have never done this in response to negotiation. That would be a new bug.
We have also never implemented muted before. Now that we do it's non-compliant.
> Then this is a pre-existing problem, and not within scope of this bug. I can open a new one.
Current lack of tests doesn't warrant future lack of tests IMHO.
It'll be much easier to write these tests now that you're doing related stuff here, than at some arbitrary time in the future when the edge cases of the API are forgotten.
> Current spec has onmute firing before resolution of sRD.
That's not how I read the spec just now.
sRD do these steps sync:
- 2.2.8.2.6. process the removal of a remote track [1]
- 2.2.11. Resolve p with undefined
[1] then does
- 4. If track.muted is false, update the muted state [2] of track with the value true.
[2] then does (in mediacapture-main)
"To update a track's muted state to newState, the User Agent MUST queue a task to run the following steps"
[1] http://w3c.github.io/webrtc-pc/#process-remote-track-removal
[2] http://w3c.github.io/webrtc-pc/#update-track-muted
Comment 56•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8933097 [details]
Bug 1400363 - Part 1: Test muted/onmuted/onunmuted for webrtc cases.
https://reviewboard.mozilla.org/r/204064/#review210958
> That's not how I read the spec just now.
>
> sRD do these steps sync:
> - 2.2.8.2.6. process the removal of a remote track [1]
> - 2.2.11. Resolve p with undefined
>
> [1] then does
> - 4. If track.muted is false, update the muted state [2] of track with the value true.
>
> [2] then does (in mediacapture-main)
> "To update a track's muted state to newState, the User Agent MUST queue a task to run the following steps"
>
>
> [1] http://w3c.github.io/webrtc-pc/#process-remote-track-removal
> [2] http://w3c.github.io/webrtc-pc/#update-track-muted
Scratch this. I was looking at the latest draft, which 40 minutes ago didn't include jib's latest PR. Now it does, and there's no task-queueing to fire "muted" in sRD anymore.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 62•7 years ago
|
||
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be10a74ec95d
Part 0: Expose the SourceMediaStream of MediaStreamTrack, and store tracks instead of the source streams. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/787ddacd432d
Part 1: Test muted/onmuted/onunmuted for webrtc cases. r=jib
https://hg.mozilla.org/integration/autoland/rev/6cf1b98c8e77
Part 2: Unmute webrtc receive tracks when RTP is received. r=drno
https://hg.mozilla.org/integration/autoland/rev/89067522fb61
Part 3: Start webrtc receive tracks as muted. r=drno
https://hg.mozilla.org/integration/autoland/rev/c8c2e98d91fe
Part 4: Mute webrtc receive tracks when they are negotiated to stop receiving. r=drno
Comment 63•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be10a74ec95d
https://hg.mozilla.org/mozilla-central/rev/787ddacd432d
https://hg.mozilla.org/mozilla-central/rev/6cf1b98c8e77
https://hg.mozilla.org/mozilla-central/rev/89067522fb61
https://hg.mozilla.org/mozilla-central/rev/c8c2e98d91fe
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•