1290948 - Implement RTCRtpTransceiver and pc.addTransceiver

I'm going to treat this as a separate task from finishing up the sender and receiver objects. Specifically, have this NOT block on work like Bug 1307996 and Bug 1307994. We can always add a meta bug.

Byron Campen [:bwc]

Assignee

Updated

•

8 years ago

Assignee: nobody → docfaraday

•

7 years ago

mozreview-review

Comment on attachment 8855978 [details] Bug 1290948: WIP Transceivers https://reviewboard.mozilla.org/r/127840/#review165830 Publishing what I managed to review before my PTO ::: dom/media/PeerConnection.js:828 (Diff revision 23) > + // This ensures there are at least |count| |kind| transceivers that are > + // configured to receive. It will create transceivers if necessary. > + _applyOfferToReceive(kind, count) { > + this._transceivers.forEach(transceiver => { > + if (count && transceiver.getKind() == kind && !transceiver.stopped) { > + this._enableReceive(transceiver); Only count-- if _enableReceive() returned true? ::: dom/media/PeerConnection.js:1168 (Diff revision 23) > + }); > + > + if (transceiver) { > + transceiver.sender.setTrack(track); > + transceiver.sender.streams = [stream]; > + this._enableSend(transceiver); Check return value?

Byron Campen [:bwc]

Assignee

Comment 27

•

7 years ago

mozreview-review-reply

Comment on attachment 8855978 [details] Bug 1290948: WIP Transceivers https://reviewboard.mozilla.org/r/127840/#review165830 > Only count-- if _enableReceive() returned true? Nope; in our present code, offerToReceive ensures that at least that many m-sections are receiving for the type. So, calling |_enableReceive| here without checking the return is effectively an ensureReceive. > Check return value? Same answer here as above.

Comment hidden (mozreview-request)

Rick Waldron [:rwaldron]

Comment 37

•

7 years ago

At present, there are many tests in https://github.com/w3c/web-platform-tests/tree/master/webrtc that rely on addTransceiver(...) as well as tests for the method itself: https://github.com/w3c/web-platform-tests/blob/master/webrtc/RTCPeerConnection-addTransceiver.html While you're implementing, any feedback on these will be greatly appreciated. Look forward to working together

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

Updated

•

7 years ago

(In reply to Rick Waldron [:rwaldron] from comment #57) > (In reply to Byron Campen [:bwc] from comment #56) > > I seem to have broken a single test-case, > > Let me help if I can! Which test case is breaking for you? Not sure why there is a new failure in this test, but it was pretty busted to begin with. https://github.com/w3c/web-platform-tests/issues/7054

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

Updated

•

7 years ago

Attachment #8900490 - Flags: review?(jib)

Attachment #8900491 - Flags: review?(jib)

Attachment #8900491 - Flags: review?(ehsan)

Attachment #8900492 - Flags: review?(jib)

Attachment #8900493 - Flags: review?(drno)

Attachment #8900494 - Flags: review?(drno)

Attachment #8900495 - Flags: review?(drno)

Attachment #8900496 - Flags: review?(drno)

(no longer active)

Comment 87

•

7 years ago

mozreview-review

Comment on attachment 8900491 [details] Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan https://reviewboard.mozilla.org/r/171862/#review180646 ::: commit-message-4029f:1 (Diff revision 4) > +Bug 1290948 - Part 2: webidl updates Please include a proper commit message that describes the change. It's impossible to understand from this commit message for example what these updates are for, which specs I should be consulting, etc. ::: dom/webidl/RTCPeerConnection.webidl:116 (Diff revision 4) > MediaStream stream, > MediaStream... moreStreams); > void removeTrack(RTCRtpSender sender); > > + // Gets the track id that was in the SDP for this track > + DOMString mozGetWebrtcTrackId(MediaStreamTrack track); This interface is exposed to the Web, right? Our API exposure guidelines prohibit exposure of new APIs with vendor prefixes. If this is an experimental API, please consider exposing it without a prefix behind a pref. ::: dom/webidl/RTCRtpSender.webidl:79 (Diff revision 4) > RTCRtpParameters getParameters(); > - Promise<void> replaceTrack(MediaStreamTrack track); > + Promise<void> replaceTrack(MediaStreamTrack? track); > Promise<RTCStatsReport> getStats(); > [Pref="media.peerconnection.dtmf.enabled"] > readonly attribute RTCDTMFSender? dtmf; > + sequence<MediaStream> mozGetStreams(); Similar comment to the above. ::: dom/webidl/RTCRtpTransceiver.webidl:19 (Diff revision 4) > + "inactive" > +}; > + > +dictionary RTCRtpTransceiverInit { > + RTCRtpTransceiverDirection direction = "sendrecv"; > + sequence<MediaStream> streams; In the spec, this has a default value of []. ::: dom/webidl/RTCRtpTransceiver.webidl:20 (Diff revision 4) > +}; > + > +dictionary RTCRtpTransceiverInit { > + RTCRtpTransceiverDirection direction = "sendrecv"; > + sequence<MediaStream> streams; > + // sequence<RTCRtpEncodingParameters> sendEncodings; Please file a follow-up bug for enabling this and add a comment pointing to that bug here. ::: dom/webidl/RTCRtpTransceiver.webidl:34 (Diff revision 4) > + [SameObject] > + readonly attribute RTCRtpReceiver receiver; > + readonly attribute boolean stopped; > + readonly attribute RTCRtpTransceiverDirection direction; > + readonly attribute RTCRtpTransceiverDirection? currentDirection; > + readonly attribute DOMString? remoteTrackId; I don't see this attribute in http://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver... ::: dom/webidl/RTCRtpTransceiver.webidl:38 (Diff revision 4) > + readonly attribute RTCRtpTransceiverDirection? currentDirection; > + readonly attribute DOMString? remoteTrackId; > + > + void setDirection(RTCRtpTransceiverDirection direction); > + void stop(); > + // void setCodecPreferences(sequence<RTCRtpCodecCapability> codecs); Please file a follow-up bug for enabling this and add a comment pointing to that bug here. ::: dom/webidl/TransceiverImpl.webidl:16 (Diff revision 4) > + * > + * See media/webrtc/signaling/src/peerconnection/TransceiverImpl.h > + * > + */ > + > +interface nsISupports; Why this?

Attachment #8900491 - Flags: review?(ehsan) → review-

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

Reporter

Comment 88

•

7 years ago

mozreview-review

Comment on attachment 8900491 [details] Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan https://reviewboard.mozilla.org/r/171862/#review180640 Looks good, except for a couple of non-spec moz-methods. We should not add any of those without a really really good reason, and I don't see one. ::: dom/webidl/RTCPeerConnection.webidl:115 (Diff revision 4) > + // Gets the track id that was in the SDP for this track > + DOMString mozGetWebrtcTrackId(MediaStreamTrack track); We shouldn't ship non-spec moz-APIs. Is pc.getTransceivers().find(tc => tc.receiver.track == track).remoteTrackId not sufficient? ::: dom/webidl/RTCRtpSender.webidl:75 (Diff revision 4) > JSImplementation="@mozilla.org/dom/rtpsender;1"] > interface RTCRtpSender { > - readonly attribute MediaStreamTrack track; > + readonly attribute MediaStreamTrack? track; > Promise<void> setParameters (optional RTCRtpParameters parameters); > RTCRtpParameters getParameters(); > - Promise<void> replaceTrack(MediaStreamTrack track); > + Promise<void> replaceTrack(MediaStreamTrack? track); Nit: Might as well s/track/withTrack/ to match spec verbatum. ::: dom/webidl/RTCRtpSender.webidl:79 (Diff revision 4) > RTCRtpParameters getParameters(); > - Promise<void> replaceTrack(MediaStreamTrack track); > + Promise<void> replaceTrack(MediaStreamTrack? track); > Promise<RTCStatsReport> getStats(); > [Pref="media.peerconnection.dtmf.enabled"] > readonly attribute RTCDTMFSender? dtmf; > + sequence<MediaStream> mozGetStreams(); We should not add non-spec moz-APIs. Why do we need this? Can we make it [ChromeOnly]? ::: dom/webidl/RTCRtpTransceiver.webidl:19 (Diff revision 4) > + "inactive" > +}; > + > +dictionary RTCRtpTransceiverInit { > + RTCRtpTransceiverDirection direction = "sendrecv"; > + sequence<MediaStream> streams; streams = [];

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

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

Comment 96

•

7 years ago

mozreview-review

Comment on attachment 8900491 [details] Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan https://reviewboard.mozilla.org/r/171862/#review181398 ::: dom/webidl/RTCRtpTransceiver.webidl:34 (Diff revision 4) > + [SameObject] > + readonly attribute RTCRtpReceiver receiver; > + readonly attribute boolean stopped; > + readonly attribute RTCRtpTransceiverDirection direction; > + readonly attribute RTCRtpTransceiverDirection? currentDirection; > + readonly attribute DOMString? remoteTrackId; This is in the github repo, the editor's draft has not been generated since then. https://github.com/w3c/webrtc-pc/pull/1567

Byron Campen [:bwc]

Assignee

Comment 97

•

7 years ago

mozreview-review-reply

Comment on attachment 8900491 [details] Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan https://reviewboard.mozilla.org/r/171862/#review180646 > I don't see this attribute in http://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver... This change was merged here, the draft just hasn't been updated yet: https://github.com/w3c/webrtc-pc/pull/1567

(no longer active)

Comment 98

•

7 years ago

mozreview-review

Comment on attachment 8900491 [details] Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan https://reviewboard.mozilla.org/r/171862/#review181830 Thanks a lot Byron for addressing the review comments. Since the changes in this patch introduce Web-facing API changes that aren't hidden behind prefs, landing the patches effectively ships new APIs. Therefore, as per our policy for exposing new Web-facing APIs <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>, before landing the patches please send out an intent to implement and ship email to dev-platform. r=me on the WebIDL changes with that.

Attachment #8900491 - Flags: review?(ehsan) → review+

Byron Campen [:bwc]

Assignee

Comment 99

•

7 years ago

Ok, mail sent.

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

•

7 years ago

mozreview-review

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review186650 ::: dom/media/PeerConnection.js:150 (Diff revision 5) > let cleanupPcRef = function(pcref) { > let pc = pcref.get(); > if (pc) { > - pc._pc.close(); > + pc.close(); Hmm. Right now it looks like PeerConnectionImpl::Close is going to fire a signaling state change when this code is run. Is that a bug? Or is there some code that filters that event out later? ::: dom/media/PeerConnection.js:703 (Diff revision 5) > + _getTransceiverWithSender(sender) { > + let transceiver = this._transceivers.find(t => t.sender == sender); > + if (!transceiver) { > + throw new this._win.DOMException("This isn't one of my senders!", > + "InvalidAccessError"); > + } > + return transceiver; > + } So if we are asserting that this cannot fail, sender needs a backpointer to its transceiver, because there are cases where transceivers are removed from the PC. ::: dom/media/PeerConnection.js:782 (Diff revision 5) > + // Spec language implies that this needs to happen as if it were called > + // before createOffer, so we do this as early as possible. > + this._ensureTransceiversForOfferToReceive(optionsOrOnSucc); > + > // This entry-point handles both new and legacy call sig. Decipher which one > if (typeof optionsOrOnSucc == "function") { > return this._legacy(optionsOrOnSucc, onErr, () => this._createOffer(options)); We call that above. Is there something wrong with that? ::: dom/media/PeerConnection.js:1510 (Diff revision 5) > + this._queueTaskWithClosedCheck(() => { > + this.updateNegotiationNeeded(); > + }); This should be equivalent I think; let's say this is not the first datachannel created. If there is no m=application section negotiated yet, that implies we have already called updateNegotiationNeeded, so this call won't have any effect. If there is an m=application section negotiated, this check will not have any effect either. I queued this because step 19 says to run the following steps (including update the negotiation-needed step) in the background. Is there another way I should be doing this? ::: dom/media/PeerConnection.js:1579 (Diff revision 5) > onSetLocalDescriptionSuccess() { > + this._dompc._syncTransceivers(); > + this._dompc.clearNegotiationNeeded(); > + this._dompc.updateNegotiationNeeded(); updateNegotiationNeeded never causes something to fire synchronously. It is exactly the same as the "update the negotiation-needed flag" algorithm in the spec, except it will throw if the pc is closed (which you noted elsewhere, and which I will fix). The business about (duplicate) checking the signaling state before running tht algorithm, and then having a (duplicate) event dispatch, strikes me as a spec bug. Step 10 should probably just be: "10. Update the negotiation-needed flag on connection." ::: dom/media/PeerConnection.js:1762 (Diff revision 5) > - onAddTrack(track, streams) { > + _getTransceiverWithRecvTrack(webrtcTrackId) { > + return this._dompc.getTransceivers().find( > + transceiver => transceiver.remoteTrackId == webrtcTrackId); Booooo! ::: dom/media/PeerConnection.js:1779 (Diff revision 5) > + if (!streams.length) { > + throw new pc._win.DOMException( > + "No streams for receive track " + webrtcTrackId, > + "InternalError"); > + } I have not seen anything in the spec that indicates "-" as a stream id is interpreted by the other side as "no stream". I interpreted this as saying if the local side doesn't specify a stream, just pretend there is one with a placeholder. These ids are opaque unless there is spec language saying otherwise. ::: dom/media/PeerConnection.js:1785 (Diff revision 5) > + streams.forEach( > + stream => { > + stream.addTrack(matchingTransceiver.receiver.track); > + // Adding tracks from JS does not result in the stream getting > + // onaddtrack, so we need to do that here. The mediacapture spec says > + // this needs to be queued, also. > + pc._queueTaskWithClosedCheck(() => { > + stream.dispatchEvent( > + new pc._win.MediaStreamTrackEvent( > + "addtrack", { track: matchingTransceiver.receiver.track })); > + }); So, the TLDR here is the tracks need to be added, and the track events need to fire, before SRD resolves? ::: dom/media/PeerConnection.js:1934 (Diff revision 5) > + // TODO (bug XXXXX): Technically, if the transceiver is not yet associated, > + // we're supposed to synchronously set the track and return a resolved > + // promise. However, PeerConnectionImpl::ReplaceTrackNoNegotiation does > + // stuff like updating principal change observers. We might want to set > + // that stuff up later than CreateTransceiverInternal so we don't need to > + // do this for tracks that haven't sent anything yet. > + > + await this._pc._replaceTrack(this._transceiverImpl, withTrack); I'm confused. Why would calling onReplaceTrackSuccess() lead to rejection of the promise? I guess I could skip the task queuing at the bottom if we weren't associated yet, and just do it directly, and that would get the right effect... ::: dom/media/PeerConnection.js:2075 (Diff revision 5) > + // the receiver starts out without a track, so record this here > + _kind: kind, Yeah, we always know this. The tricky bit is that the spec says the an RTCRtpTransceiver always has a receiver.track, and this is the source of truth for what kind of transceiver it is. However, since the receiver's track is created in c++ _after_ creation of the RTCRtpTransceiver (but before JS gets to see it), we need this _kind breadcrumb so the right kind of track is created. ::: dom/media/PeerConnection.js:2088 (Diff revision 5) > + return; > + } > + > + this.direction = direction; > + this.sync(); > + this._pc.updateNegotiationNeeded(); updateNegotiationNeeded doesn't unconditionally fire events, it does the checking in the spec. ::: dom/media/PeerConnection.js:2101 (Diff revision 5) > + setStopped() { > + this.stopped = true; > + this.currentDirection = null; This is called from c++, which is going to cause the track to end too.

Byron Campen [:bwc]

Assignee

Comment 108

•

7 years ago

mozreview-review-reply

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review184150 > cleanupPcRef() is called when the window is torn down. > > pc.close() fires an "iceconnectionstatechange" event at the window. > > We don't want to fire events during teardown. Ugh, reviewboard submitted what I thought were replies as standalone comments. Ok, now to copy all of these over... Hmm. Right now it looks like PeerConnectionImpl::Close is going to fire a signaling state change when this code is run. Is that a bug? Or is there some code that filters that event out later? > In the spec, I believe this function corresponds to the common: > > Let transceiver be the RTCRtpTransceiver object associated with sender. > > This CANNOT FAIL. As such we should just return transceiver here I think, which will an undefined dereference (which should show in browser console only). I.e. we should not throw a content-visible "InvalidAccessError". > > To reduce pilot error, we could move this function to the sender, e.g.: > > _getTransceiver() { > return this._pc._transceivers.find(t => t.sender == this); > } So if we are asserting that this cannot fail, sender needs a backpointer to its transceiver, because there are cases where transceivers are removed from the PC. > I think we need to call > > _ensureTransceiversForOfferToReceive(options); > > here specifically in the legacy case, otherwise this lookss like it will break legacy callback users who use offerToReceive. We call that above. Is there something wrong with that? > Actually, the spec says to do this for the "first channel created" only... > > Even then it says to run the update-the-negotiation-needed-flag algorithm [1] > which in turn runs the check-if-negotiation-is-needed algorithm [2], and only then conditionally queues a task to fire an event. > > Would it make sense to align our functions closer with the spec algorithms here? > > By that I mean not just putting the queueing into the function, which would help, but perhaps even centralize the negotiation-needed logic the same way the spec does? A bit inefficient perhaps, but appealing on other levels (avoiding spec interpretation errors). Thoughts? > > If we DON'T do this, we should rename updateNegotiationNeeded() to something else so it's not mistaken for the spec algorithm of the same name (which could lead to forgetting to queue a task, as well as maybe firing in situations we shouldn't). > > [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-update-the-negotiation-needed-flag > [2] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-check-if-negotiation-is-needed This should be equivalent I think; let's say this is not the first datachannel created. If there is no m=application section negotiated yet, that implies we have already called updateNegotiationNeeded, so this call won't have any effect. If there is an m=application section negotiated, this check will not have any effect either. I queued this because step 19 says to run the following steps (including update the negotiation-needed step) in the background. Is there another way I should be doing this? > Here's an example where that might be helpful. > > This looks like it might fire the negotiatonneeded event before the SLD success callback, which seems wrong. > > The spec says "If connection's signaling state is now "stable", update the negotiation-needed flag." > > So A) we should never run this outside of stable state, and B) we should queue a task to fire an event. updateNegotiationNeeded never causes something to fire synchronously. It is exactly the same as the "update the negotiation-needed flag" algorithm in the spec, except it will throw if the pc is closed (which you noted elsewhere, and which I will fix). The business about (duplicate) checking the signaling state before running that algorithm, and then having a (duplicate) event dispatch, strikes me as a spec bug. Step 10 should probably just be: "10. Update the negotiation-needed flag on connection." > remoteTrackId got junked from the spec this morning. Booooo! > https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-20#section-5.2.1 says: > > "If no MediaStream is associated with the transceiver, a single > "a=msid" line with the special value "-" in place of the > MediaStream ID" > > so streams.length == 0 should be allowed I think, and the remote track streamless. I have not seen anything in the spec that indicates "-" as a stream id is interpreted by the other side as "no stream". I interpreted this as saying if the local side doesn't specify a stream, just pretend there is one with a placeholder. These ids are opaque unless there is spec language saying otherwise. Basically, if this happens, the c++ code has a bug. > These events happen in the wrong order I think. > > A recent PR [1] ensures event.streams are fully populated by the time the "track" event fires. But this necessitates adding tracks to streams in SRD, not onTrack: > > 1. Let trackEvents be an empty list. > 2. Run the following steps for each media description in description: > 1. Add tracks to streams, etc. > 3. For each event in trackEvents, fire event > > I also believe we shouldn't queue a task for the addtrack/remove events. I've filed an issue on the spec for that [2]. > > [1] https://github.com/w3c/webrtc-pc/pull/1519 > [2] https://github.com/w3c/webrtc-pc/issues/1599 So, the TLDR here is the tracks need to be added, then the track events need to fire, then SRD resolves? > this._pc._replaceTrack and ultimately this._pc._impl.replaceTrackNoRenegotiation() are still called synchronously here, so if the latter were to call PeerConnectionObserver.onReplaceTrackSuccess() synchronously when the transceiver is not yet associated, then the promise returned synchronously from both should already be rejected, as the spec demands, and the await here will basically (in the syntactic sugar-world of async functions) "throw" here, which basically means this async function just returns the rejected promise at this point, abandoning the remainder of the function. > > Alternatively if we fix this._pc._impl.replaceTrackNoRenegotiation() to return a promise, the c++ code could return a rejected promise directly. I'm confused. Why would calling onReplaceTrackSuccess() lead to rejection of the promise? I guess I could skip the task queuing at the bottom if we weren't associated yet, and just do it directly, and that would get the right effect... > I'm curious, does a transceiver always know its kind? If so, this might help fix bug 1376947. Yeah, we always know this. The tricky bit is that the spec says the an RTCRtpTransceiver always has a receiver.track, and this is the source of truth for what kind of transceiver it is. However, since the receiver's track is created in c++ _after_ creation of the RTCRtpTransceiver (but before JS gets to see it), we need this _kind breadcrumb so the right kind of track is created. > Again, looking at "check if negotiation is needed" in the spec [1] I'm not positive there aren't cases where we shouldn't fire an event here. I just can't tell. > > In lieu of that refactor, a comment briefly explaining why setDirection here would basically always need to trigger ONN I think would be helpful. > > [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dfn-check-if-negotiation-is-needed updateNegotiationNeeded doesn't unconditionally fire events, it does the checking in the spec. > We should queue a task to fire "ended" on receiver.track here, as described in [1] > > [1] https://w3c.github.io/mediacapture-main/getusermedia.html#track-ended This is called from c++, which is going to cause the track to end too.

Byron Campen [:bwc]

Assignee

Comment 109

•

7 years ago

mozreview-review-reply

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review184150 > Nit: redundant temporary when we have arrow functions. Huh, even when there's another non-arrow function along the way... > Even though return values are used here, this function also seems redundant, called from one place. Maybe inline? Makes it easier to compare to spec algorithms steps. > > Flat seems better here. In particular, I dislike updateNegotiationNeeded() being called both in here and again at lone call site. I'm confused about your comment about updateNegotiationNeeded. Where's the extra call? > If we do this as > > for (let transceiver of this._transceivers) { > > then we can break out of the loop. > > (Imperative for( : ) loops are also back in fashion as the dominant pattern since they work well with async/await, where forEach is a bit of a deathtrap. YMMV). Rewrote, so this is no longer a problem. > Nit: s/.getKind()/._kind/ > > Since the transceivers pushed on this._transceivers appear to be WebIDL objects, .getKind() likely calls redundant c++ binding code here. > > No need to call accessor functionss which exists for binding code only, at least for getters IMHO. False dependence. Better to call the xray ._kind directly. > > Applies throughout. This doesn't work; _kind is undefined here.

Byron Campen [:bwc]

Assignee

Comment 110

•

7 years ago

mozreview-review-reply

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review184150 > We call that above. Is there something wrong with that? Nevermind, I see the problem. Have rewritten this function to be clearer. > prefer ._hasBeenUsedToSend to avoid binding code > > (abstractions in JS are not free). Doesn't work, this is undefined here. > Bug: s/streams/_streams/ I bet this doesn't work, since we cannot access variables like this on RTCRtpTransceiver. Let me see what I can do instead. > Inlining this function would make it easier to reason about why it's firing events I think, and whether we should queue a task or not (we should not). Too much access into private members of _dompc for my taste. > This will hang if someone calls: > > let p = pc.replaceTrack(track); > pc.replaceTrack(track); > await p; // hang > > ...because the this.members storing the resolve and reject callbacks will get overwritten. > > That's why we were chaining (enqueuing) replaceTrack before even though the spec says not to. > > The proper fix would be to have replaceTrackNoRenegotiation() return a promise. If replaceTrackNoRenegotiation is synchronous, we could simply do away with the success/failure callbacks, right? > Same here. Same question as above. > I'm confused. Why would calling onReplaceTrackSuccess() lead to rejection of the promise? > > I guess I could skip the task queuing at the bottom if we weren't associated yet, and just do it directly, and that would get the right effect... Fixed I think. Please check. > Should be good once https://github.com/w3c/webrtc-pc/pull/1560 is merged. We can fix this in bug 1401592, since the test expects this async step to be here. > This is called from c++, which is going to cause the track to end too. To be more specific, when we sync with c++, the track will be ended.

Nils Ohlmeier [:drno]

Comment 111

•

7 years ago

mozreview-review

Comment on attachment 8900493 [details] Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno https://reviewboard.mozilla.org/r/171866/#review182950 Wow. Maybe I should have started with reading the tests first. Lets do one more round... ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:914 (Diff revision 7) > + info("Offer is: " + offer.sdp); > + info("Answer is: " + answer.sdp); Are these left over debug message, or do you want them to stay? ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:363 (Diff revision 7) > return 1; > } > } > return 0; Can we change these |1| and |0| to true and false please? ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:850 (Diff revision 7) > if (checkFlags & CHECK_SUCCESS) { > ASSERT_EQ(NS_OK, rv); > } > > if (checkFlags & CHECK_TRACKS) { > - auto tracks = mSessionAns->GetRemoteTracks(); > + ASSERT_EQ(types.size(), mSessionAns->GetTransceivers().size()); I guess the same limitation regarding recvonly and inactive applies here as well. Lets put that comment then here as well. ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:897 (Diff revision 7) > // These might have been in the SDP, or might have been randomly > // chosen by JsepSessionImpl I think this comment no longer applies, or? ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:947 (Diff revision 7) > // These might have been in the SDP, or might have been randomly > // chosen by JsepSessionImpl Same here. No longer applies, I think. ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1614 (Diff revision 7) > - auto offererPairs = GetTrackPairsByLevel(*mSessionOff); > - auto answererPairs = GetTrackPairsByLevel(*mSessionAns); > + std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers > + = DeepCopy(mSessionOff->GetTransceivers()); > + std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers > + = DeepCopy(mSessionAns->GetTransceivers()); auto doesn't work here for the type? ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1711 (Diff revision 7) > - ASSERT_EQ(offererPairs.size(), newAnswererPairs.size()); > - for (size_t i = 0; i < offererPairs.size(); ++i) { > - ASSERT_TRUE(Equals(offererPairs[i], newAnswererPairs[i])); > + ASSERT_EQ(offererTransceivers.size(), newAnswererTransceivers.size()); > + for (size_t i = 0; i < offererTransceivers.size(); ++i) { > + ASSERT_TRUE(Equals(*offererTransceivers[i], *newAnswererTransceivers[i])); > } > > - ASSERT_EQ(answererPairs.size(), newOffererPairs.size()); > - for (size_t i = 0; i < answererPairs.size(); ++i) { > - ASSERT_TRUE(Equals(answererPairs[i], newOffererPairs[i])); > + ASSERT_EQ(answererTransceivers.size(), newOffererTransceivers.size()); > + for (size_t i = 0; i < answererTransceivers.size(); ++i) { > + ASSERT_TRUE(Equals(*answererTransceivers[i], *newOffererTransceivers[i])); Having seen this loop the third time: how about a little helper function for doing this? ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1733 (Diff revision 7) > - auto offererPairs = GetTrackPairsByLevel(*mSessionOff); > - auto answererPairs = GetTrackPairsByLevel(*mSessionAns); > + std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers > + = DeepCopy(mSessionOff->GetTransceivers()); > + std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers > + = DeepCopy(mSessionAns->GetTransceivers()); auto? ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1788 (Diff revision 7) > - auto offererPairs = GetTrackPairsByLevel(*mSessionOff); > - auto answererPairs = GetTrackPairsByLevel(*mSessionAns); > + std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers > + = DeepCopy(mSessionOff->GetTransceivers()); > + std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers > + = DeepCopy(mSessionAns->GetTransceivers()); auto ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1855 (Diff revision 7) > - auto offererPairs = GetTrackPairsByLevel(*mSessionOff); > - auto answererPairs = GetTrackPairsByLevel(*mSessionAns); > + std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers > + = DeepCopy(mSessionOff->GetTransceivers()); > + std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers > + = DeepCopy(mSessionAns->GetTransceivers()); auto ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:1988 (Diff revision 7) > > - RefPtr<JsepTrack> removedTrack = GetTrackOff(0, types.front()); > - ASSERT_TRUE(removedTrack); > - ASSERT_EQ(NS_OK, mSessionOff->RemoveTrack(removedTrack->GetStreamId(), > - removedTrack->GetTrackId())); > + SetRemoteOffer(offer); > + > + std::vector<JsepTrack> removedTracks = mSessionAns->GetRemoteTracksRemoved(); > + std::vector<JsepTrack> addedTracks = mSessionAns->GetRemoteTracksAdded(); > + Maybe add a comment here, that point of this test is to accept a change of a MSID, but the internal APIs should still report the original IDs, if I'm not mistaken. ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:2132 (Diff revision 7) > - auto offererPairs = GetTrackPairsByLevel(*mSessionOff); > - auto answererPairs = GetTrackPairsByLevel(*mSessionAns); > - > - RefPtr<JsepTrack> removedTrack = GetTrackAns(0, types.front()); > + std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers > + = DeepCopy(mSessionOff->GetTransceivers()); > + std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers > + = DeepCopy(mSessionAns->GetTransceivers()); auto ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:2205 (Diff revision 7) > - auto offererPairs = GetTrackPairsByLevel(*mSessionOff); > - auto answererPairs = GetTrackPairsByLevel(*mSessionAns); > - > - RefPtr<JsepTrack> removedTrackAnswer = GetTrackAns(0, types.front()); > + std::vector<RefPtr<JsepTransceiver>> origOffererTransceivers > + = DeepCopy(mSessionOff->GetTransceivers()); > + std::vector<RefPtr<JsepTransceiver>> origAnswererTransceivers > + = DeepCopy(mSessionAns->GetTransceivers()); auto ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5782 (Diff revision 7) > +// 1. AddTrack either caused a transceiver to be created, or set the send > +// track on a preexisting transceiver. > +// 2. The transceiver was not created as a side-effect of AddTrack, and the > +// send track was put in place by some other means than AddTrack. Yikes ::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5823 (Diff revision 7) > + // TODO: Once nils' code for switching offer/answer roles lands, switch and > + // have the other side reoffer to negotiate the new transceivers. This has landed. ::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:123 (Diff revision 7) > - mSendAns->PopulateCodecs(mAnsCodecs.values); > - mRecvAns->PopulateCodecs(mAnsCodecs.values); > + mRecvOff.PopulateCodecs(mOffCodecs.values); > + > + mSendAns = JsepTrack(type, sdp::kSend); > + if (type != SdpMediaSection::MediaType::kApplication) { > + mSendAns.UpdateTrack( > + std::vector<std::string>(1, "stream_id"), "track_id"); If stream and track ID's don't have to match up between offer and answer in the Trasceiver world, lets please use different values for the answerer. ::: media/webrtc/signaling/gtest/jsep_track_unittest.cpp:197 (Diff revision 7) > - if (mRecvAns && GetAnswer().IsReceiving()) { > - mRecvAns->Negotiate(GetAnswer(), GetOffer()); > + if (GetAnswer().IsReceiving()) { > + mRecvAns.Negotiate(GetAnswer(), GetOffer()); > } > > - if (mSendOff && GetAnswer().IsReceiving()) { > - mSendOff->Negotiate(GetAnswer(), GetAnswer()); > + if (GetAnswer().IsReceiving()) { > + mSendOff.Negotiate(GetAnswer(), GetAnswer()); Looks like these two can be merged into the same if block now. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:197 (Diff revision 7) > - nsresult CreateReceivingTrack(size_t mline, > - const Sdp& sdp, > + JsepTransceiver* GetTransceiverForLevel(size_t level); > + JsepTransceiver* GetTransceiverForLocal(size_t level); Sigh... wouldn't it be nice if you could use mid's instead of level index some day? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:230 (Diff revision 7) > - SdpMediaSection** msection); > - nsresult CreateAnswerMSection(const JsepAnswerOptions& options, > - size_t mlineIndex, > const SdpMediaSection& remoteMsection, > Sdp* sdp); > - nsresult SetRecvonlySsrc(SdpMediaSection* msection); > + nsresult CreateReceivingTrack(const Sdp& sdp, Is this the only remaining track based function? Can we remove it, rename it? Not sure how complicated it would be. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:76 (Diff revision 7) > - track->PopulateCodecs(mSupportedCodecs.values); > - > - JsepSendingTrack strack; > - strack.mTrack = track; > > - mLocalTracks.push_back(strack); > + if (transceiver->mSending.GetMediaType() != SdpMediaSection::kApplication) { As I'm assuming that a transceiver can't support different media types for sending and receiving it might be worth adding a GetMediaType() convenience function directly to the Transceiver class. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:93 (Diff revision 7) > - if (mState != kJsepStateStable) { > - JSEP_SET_ERROR("Removing tracks outside of stable is unsupported."); > - return NS_ERROR_UNEXPECTED; > - } > + } > - > - auto track = FindTrackByIds(mLocalTracks, streamId, trackId); > + } else { > + transceiver->mJsDirection = SdpDirectionAttribute::Direction::kSendrecv; Why is |mJsDirection| only set for data channels? I get why SSRC, trackID etc don't apply to data channels. But this looks like it would/could leave transceivers in inconsistent state depending on which media type they are. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:212 (Diff revision 7) > + if (lastAnswerMsection) { > + // Use the protocol the answer used, even if it is not what we would have > + // used. > + protocol = lastAnswerMsection->GetProtocol(); > + } I think you can safely move this into the previous if {} block. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:226 (Diff revision 7) > + // Some of this stuff (eg; mid) sticks around even if disabled > + if (lastAnswerMsection) { > + nsresult rv = mSdpHelper.CopyStickyParams(*lastAnswerMsection, msection); Does this mean that in case of a re-Offer the mid from the last answers m-section N is going to copied to the re-Offer's m-section N+1? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:266 (Diff revision 7) > -// This function creates a skeleton SDP based on the old descriptions > -// (ie; all m-sections are inactive). > + msection->GetAttributeList().SetAttribute( > + new SdpStringAttribute(SdpAttribute::kMidAttribute, mid)); This logic here makes sense to set the mid based on the transceiver. But this stomps over the mid which got potentially set by mSdpHelper.CopyStickyParams(). It looks to me like mid's stick to the transceiver (instead of the indexed m-section), and therefore mSdpHelper.CopyStickyParams() probably should not attempt to copy it. Some maybe we need to split the SdpHelper's into smaller pieces here. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:365 (Diff revision 7) > NS_ENSURE_SUCCESS(rv, rv); > > - if (mCurrentLocalDescription) { > - rv = AddReofferMsections(*mCurrentLocalDescription, > - *GetAnswer(), > - sdp.get()); > + for (size_t level = 0; > + JsepTransceiver* transceiver = GetTransceiverForLocal(level); > + ++level) { > + rv = CreateOfferMsection(options, *transceiver, sdp.get()); I think you still want the NS_ENSURE_SUCCESS() check here, or? At least when generating the answer you still call that for each CreateAnswerMsection() call. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:368 (Diff revision 7) > - // Ensure that we have all the m-sections we need, and disable extras > - rv = SetupOfferMSections(options, sdp.get()); > - NS_ENSURE_SUCCESS(rv, rv); > + if (!sdp->GetMediaSectionCount()) { > + JSEP_SET_ERROR("Cannot create offer when there are no valid transceivers."); > + return NS_ERROR_UNEXPECTED; > + } Actually according to latest spec discussions (https://github.com/rtcweb-wg/jsep/issues/832) we need to be able to generate an empty SDP. But let's handle that in a new bug. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:559 (Diff revision 7) > - mPendingRemoteDescription->GetMediaSection(msection->GetLevel()), > - msection); > - } > > - return NS_OK; > -} > + // Add extmap attributes. This logic will probably be moved to the track, > + // since it can be specified on a per-sender basis in JS. Might be worth adding here that we also need an extmap registry either per bundled transport or on the session level to ensure the extension IDs are identical within the same bundled transport. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:668 (Diff revision 7) > // Check that content hasn't done anything unsupported with the SDP > rv = ValidateLocalDescription(*parsed); > NS_ENSURE_SUCCESS(rv, rv); > > - // Create transport objects. > - mOldTransports = mTransports; // Save in case we need to rollback > + if (type == kJsepSdpOffer) { > + // For rollback I found the new comment more confusing then the old one, because I remembered that in case of a rollback we had returned from this function earlier already. Can you change the comment to indicate that this is in preparation for a potential future rollback? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:820 (Diff revision 7) > if (parsed->GetAttributeList().HasAttribute( > SdpAttribute::kIceOptionsAttribute)) { > iceOptions = parsed->GetAttributeList().GetIceOptions().mValues; > } > > + // For rollback. "Prepared for future rollback"? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:884 (Diff revision 7) > - > - // Now walk through the m-sections, make sure they match, and create > - // track pairs that describe the media to be set up. > - for (size_t i = 0; i < local->GetMediaSectionCount(); ++i) { > // Skip disabled m-sections. > if (answer.GetMediaSection(i).GetPort() == 0) { Any good reason to use GetPort() instead of IsMsectionDisabled() here? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1007 (Diff revision 7) > > - trackPairOut->mRtpTransport = transport; > + if (transceiver->mTransport->mComponents == 2) { > - > - if (transport->mComponents == 2) { > // RTCP MUX or not. > // TODO(bug 1095743): verify that the PTs are consistent with mux. I just commented in that bug. I don't see how this would ever become a problem for us. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1337 (Diff revision 7) > + transceiver->Disassociate(); > + JsepTransceiver* newTransceiver = FindUnassociatedTransceiver( > + transceiver->mSending.GetMediaType(), false); > + if (newTransceiver) { > + newTransceiver->SetLevel(level); > + transceiver->ClearLevel(); > + return newTransceiver; > + } > - } > + } > > - std::vector<JsepReceivingTrack>::iterator track; > + return transceiver; So if we get in here the transceiver gets disassociated. If then the FindUnassociatedTransceiver() call returns a nullptr, this appears to return the matching transceiver for the level, but disassociated. That looks like it's probably not intended. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1384 (Diff revision 7) > + if (transceiver) { > + transceiver->SetLevel(level); > + return transceiver; > + } > + > + // Make a new transceiver Is it intentional that GetTransceiverForLocal() can return a nullptr, but GetTransceiverForRemote() never returns a nullptr because it creates a Transceiver if none is found? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1438 (Diff revision 7) > + transceiver->mReceiving.UpdateTrack(streamIds, trackId); > + } > + > + transceiver->mReceiving.UpdateRecvTrack(remote, msection); > > - track = FindTrackByIds(mRemoteTracks, streamId, trackId); > + if (!transceiver->mReceiving.GetTrackId().empty()) { Is the else branch here when the remote msection is recvonly, or is that some error scenario for which we should have a log message? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1476 (Diff revision 7) > +{ > + const Sdp* answer(GetAnswer()); > + > + if (answer && > + (level < answer->GetMediaSectionCount()) && > + mSdpHelper.MsectionIsDisabled(answer->GetMediaSection(level))) { Why not simply return mSdpHelper.MsectionIsDisabled() here directly? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:109 (Diff revision 7) > mDirection(direction), > mActive(false) > - {} > + { > + } > > - virtual mozilla::SdpMediaSection::MediaType > + void UpdateTrack(const std::vector<std::string>& streamIds, How about UpdateTrackIds()? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:116 (Diff revision 7) > { > - return mType; > + mStreamIds = streamIds; > + mTrackId = trackId; > } > > - virtual const std::string& > + void ClearTrack() Same here. How about ClearTrackIds()? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:357 (Diff revision 7) > UniquePtr<JsepTrackNegotiatedDetails> mNegotiatedDetails; > std::vector<uint32_t> mSsrcs; > bool mActive; > }; > > -// Need a better name for this. > +class JsepTransceiver { I would not expect to find JsepTransceivers to be defined in JsepTrack.h. How about moving this into it's own JsepTranceivers.h file? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:366 (Diff revision 7) > - RefPtr<JsepTransport> mRtcpTransport; > + mSending(type, sdp::kSend), > + mReceiving(type, sdp::kRecv), I though |mSending| and |mReceiving| were booleans. How about renaming them to something like |mSendingTrack| and |mReceivingTrack|? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:399 (Diff revision 7) > + void Rollback(JsepTransceiver& oldTransceiver) > + { > + *mTransport = *oldTransceiver.mTransport; > + mLevel = oldTransceiver.mLevel; > + mBundleLevel = oldTransceiver.mBundleLevel; > + mReceiving = oldTransceiver.mReceiving; The sending track never changes through an O/A and therefore does not need to be rolled back? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:401 (Diff revision 7) > + // stop() caused by a disabled m-section in a remote offer cannot be > + // rolled back. Maybe open a bug report for this and reference it here? Even if we can't fix it right now this might help people stumbling about this API limitation. ::: media/webrtc/signaling/src/jsep/JsepTrack.h:437 (Diff revision 7) > + MOZ_ASSERT(!HasLevel()); > + MOZ_ASSERT(!IsStopped()); As SIZE_MAX is used as a special value we should probably assert here that |level != SIZE_MAX| (see SetBundleLevel() below) to avoid problems with future mega SDPs. ::: media/webrtc/signaling/src/jsep/JsepTrack.h:458 (Diff revision 7) > + return mLevel; > + } > + > + void Stop() > + { > + mStopped = true; Hmm is the actual logic of stopping handled some where else? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:486 (Diff revision 7) > - MOZ_ASSERT(aBundleLevel != SIZE_MAX); > + MOZ_ASSERT(aBundleLevel != SIZE_MAX); > - mBundleLevel = aBundleLevel; > + mBundleLevel = aBundleLevel; Should this be guarded against an already set |mBundleLevel| and !IsStopped() like you did for SetLevel() above? ::: media/webrtc/signaling/src/jsep/JsepTrack.h:534 (Diff revision 7) > + OwningNonNull<JsepTransport> mTransport; > + > + private: > + // Stuff that is not negotiated > + std::string mMid; > + size_t mLevel; For consistency add the same // SIZE_MAX comment as below for |mBundleLevel| ::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:127 (Diff revision 7) > if (mDirection == sdp::kSend) { > - AddToMsection(mJsEncodeConstraints, sdp::kSend, offer); > + std::vector<JsConstraints> constraints; > + if (offer->IsSending()) { > + constraints = mJsEncodeConstraints; > + } > + AddToMsection(mJsEncodeConstraints, sdp::kSend, ssrcGenerator, offer); I'm guessing you wanted |constraints| here as the parameter for the AddToMsection() call. Otherwise you could delete the 4 lines above, because |constraints| never gets used anywhere. ::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:178 (Diff revision 7) > for (const JsepCodecDescription* codec : codecs) { > codec->AddToMediaSection(*msection); > } > > - if (mDirection == sdp::kSend) { > - if (msection->GetMediaType() != SdpMediaSection::kApplication) { > + if (mDirection == sdp::kSend && mType != SdpMediaSection::kApplication) { > + if (msection->IsSending()) { If there really is no else branch to |msection->IsSending()| then roll this condition into the if condition above. ::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:185 (Diff revision 7) > - msection->SetSending(true); > - } else { > + } else { > - msection->SetReceiving(true); > + for (const std::string& streamId : mStreamIds) { > + msection->AddMsid(streamId, mTrackId); > + // TODO() Interop hack; older Firefox barfs if there is more than one > + // msid. Remove when safe. Let's put concrete information here: at earliest this when ESR 52 is no longer supported, so after the release of ESR 59.2 or Fx 61. ::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:259 (Diff revision 7) > } > } > > - if (!rids->mRids.empty()) { > + if (rids->mRids.size() > 1) { > msection->GetAttributeList().SetAttribute(simulcast.release()); > + if (!rids->mRids.empty()) { What is this new check for? How can this be ever false if we can get here only if |mRids.size() > 1|? ::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:505 (Diff revision 7) > + streamIds->clear(); > + streamIds->push_back(i->identifier); Assuming you are going to find multiple streams which contain the same track isn't this just going to return always "only" the streamIDs of the last one found?

Attachment #8900493 - Flags: review?(drno)

Byron Campen [:bwc]

Assignee

•

7 years ago

mozreview-review

Comment on attachment 8900494 [details] Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno https://reviewboard.mozilla.org/r/171868/#review187400 I like the resulting code in the end a lot better. Look less complex to me. But I guess with the amount of comments we need another round. ::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:328 (Diff revision 9) > // TODO(bcampen@mozilla.com): Right now this does not let us test RTCP in > // both directions; only the sender's RTCP is sent, but the receiver should > // be sending it too. Looks like this comment no longer applies? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:370 (Diff revision 9) > // A specialization of pipeline for reading from the network and > // rendering video. > class MediaPipelineReceive : public MediaPipeline { Lets fix this comment. This generic PipelineReceiver is not rendering video. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2133 (Diff revision 9) > } > } > > -nsresult MediaPipelineReceiveAudio::Init() > +void MediaPipelineReceiveAudio::SetPrincipalHandle_m(const PrincipalHandle& principal_handle) > { > - ASSERT_ON_THREAD(main_thread_); > + listener_->SetPrincipalHandle_m(principal_handle); Since |listener_| can now be a nullptr when constructing, is this guaranteed to be called only when |listener_| is set? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2314 (Diff revision 9) > } > } > > -nsresult MediaPipelineReceiveVideo::Init() { > - ASSERT_ON_THREAD(main_thread_); > - MOZ_MTLOG(ML_DEBUG, __FUNCTION__); > +void MediaPipelineReceiveVideo::SetPrincipalHandle_m(const PrincipalHandle& principal_handle) > +{ > + listener_->SetPrincipalHandle_m(principal_handle); Same here: can |listener_| here be null? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2321 (Diff revision 9) > > +void > +MediaPipelineReceiveVideo::Start() > +{ > + conduit_->StartReceiving(); > listener_->AddSelf(); In MediaPipelineReceiveAudio::Start() this line is guarded with an if(listener_). Why not here? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2327 (Diff revision 9) > } > > -void MediaPipelineReceiveVideo::SetPrincipalHandle_m(const PrincipalHandle& principal_handle) > +void > +MediaPipelineReceiveVideo::Stop() > { > - listener_->SetPrincipalHandle_m(principal_handle); > + listener_->RemoveSelf(); Same question here: guard with an if? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1223 (Diff revision 9) > > if (NS_FAILED(res)) { > std::string errorString = mJsepSession->GetLastError(); > CSFLogError(logTag, "%s (%s) : pc = %s, error = %s", > __FUNCTION__, > - type == SdpMediaSection::kAudio ? "audio" : "video", > + transceiver->mSending.GetMediaType() == SdpMediaSection::kAudio ? Could the media type here also be kApplication? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1907 (Diff revision 9) > default: > MOZ_ASSERT(false); > return NS_ERROR_FAILURE; > } > > + size_t transceiverCount = mJsepSession->GetTransceivers().size(); I got confused why the count is taken before calling SetRemoteDescription(). As this appears to be the counter before SRD(), we should try to find a better name. How about originalTransceiverCount? And then used that as the initial value in a for loop below? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1964 (Diff revision 9) > + } > + > + if (jrv.Failed()) { > + nsresult rv = jrv.StealNSResult(); > + CSFLogError(logTag, "%s: pc = %s, OnTransceiverNeeded failed. " > + "This should never happen. rv = %d", Famous last words :) ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2193 (Diff revision 9) > CSFLogError(logTag, "Encountered media error! %s", aError.c_str()); > // TODO: Let content know about this somehow. > } > > nsresult > PeerConnectionImpl::AddTrack(MediaStreamTrack& aTrack, This appears to be a void operation now. Can we remove it? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2225 (Diff revision 9) > > - nsString wideTrackId; > - aTrack.GetId(wideTrackId); > + nsresult rv = NS_ERROR_INVALID_ARG; > + > + for (RefPtr<TransceiverImpl>& transceiver : transceivers) { > + if (transceiver->HasSendTrack(&aTrack)) { > + // TODO(bug XXXXX): Move DTMF stuff to TransceiverImpl Can we fill this bug number? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2368 (Diff revision 9) > break; > } > } > > return NS_OK; Not really related to this patch set, but this function return NS_OK and does not even touch |outToneBuffer| when it fails to locate the track? That looks like a bug to me. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2394 (Diff revision 9) > - CSFLogError(logTag, "Could not find stream from trackId"); > + return rv; > - return NS_ERROR_UNEXPECTED; > } > > - std::string origStreamId = info->GetId(); > - std::string newStreamId = > + for (size_t i = 0; i < mDTMFStates.Length(); ++i) { > + if (mDTMFStates[i].mTransceiver.get() == &aTransceiver) { Does this DTMF stuff also need to get moved over to the Transceivers? If so, put the same comment here as above with the bug number. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:44 (Diff revision 9) > class PCUuidGenerator; > - > -class SourceStreamInfo { > -public: > - SourceStreamInfo(DOMMediaStream* aMediaStream, > - PeerConnectionMedia *aParent, > +class MediaPipeline; > +class MediaPipelineFilter; > +class JsepSession; > + > +// TODO(bug XXXXX): If we move the TransceiverImpl stuff out of here, this will That sounds like a good idea. Fill in a bug number. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:359 (Diff revision 9) > + bool levelHasTransport = > + transport->mComponents && > + transport->mIce && > + (!transceiver->HasBundleLevel() || (transceiver->BundleLevel() == level)); > + > + if (levelHasTransport) { levelHasTransport is only the condition for the if() block, or? Let move it there. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:33 (Diff revision 9) > +#include "mozilla/dom/RTCRtpTransceiverBinding.h" > +#include "mozilla/dom/TransceiverImplBinding.h" > + > +namespace mozilla { > + > +MOZ_MTLOG_MODULE("mediapipeline") I would say lets give this it's own logging module. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:109 (Diff revision 9) > + mTransmitPipeline->UpdateSinkIdentity_m(aTrack, aPrincipal, aSinkIdentity); > + return NS_OK; > +} > + > +void > +TransceiverImpl::Shutdown_m() We don't need to do anything with mReceiveStream here? ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:154 (Diff revision 9) > + for (unsigned int ssrc : mJsepTransceiver->mReceiving.GetSsrcs()) { > + filter->AddRemoteSSRC(ssrc); > + } I'm starting to wonder if it's a good thing to restrict the filter to SSRC's from signaling only, or if we should carry forward SSRC's we (think we) learned at run time from the media side. But that is probably for another bug. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:177 (Diff revision 9) > + > +nsresult > +TransceiverImpl::UpdateConduit() > +{ > + MOZ_MTLOG(ML_DEBUG, mPCHandle << "[" << mMid << "]: " > + "In UpdateConduit"); s/UpdateConduit/__FUNCTION__ ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:189 (Diff revision 9) > + > + mReceivePipeline->Stop(); > + mTransmitPipeline->Stop(); > + > + if (mJsepTransceiver->IsStopped()) { > + MOZ_MTLOG(ML_DEBUG, this << " " << mPCHandle << " [" << mMid << "]: " The other log message don't start with |this|. Is there a special reason this one is different? I would prefer to have them consistent. Either all logging |this| or without, no strong preference from my side. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:224 (Diff revision 9) > + } else { > + rv = UpdateAudioConduit(); > + } > + > + if (NS_FAILED(rv)) { > + return rv; Lets add MOZ_MTLOG() here before returning, just in case. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:235 (Diff revision 9) > + MOZ_MTLOG(ML_WARNING, mPCHandle << "[" << mMid << "]: " > + "Starting transmit conduit without send track!"); Is there a valid use case for this, or does this indicate a bug of some kind where we should abort? ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:301 (Diff revision 9) > + WebrtcAudioConduit *audio_conduit = > + static_cast<WebrtcAudioConduit*>(mConduit.get()); > + WebrtcVideoConduit *video_conduit = > + static_cast<WebrtcVideoConduit*>(transceiver->mConduit.get()); > + > + video_conduit->SyncTo(audio_conduit); Is this intentionally not exiting after the first match? Or in other words: does it work if an audio conduit gets synced to multiple video conduits? ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:325 (Diff revision 9) > + if (!mSendTrack) { > + return false; > + } > + > + if (!aSendTrack) { > + return true; I would have expected to see false here. Or is that an intentional feature to say claim that HasSendTrack(nullptr) returns that the transceiver has AN (as in any) sending track? ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:335 (Diff revision 9) > + > +void > +TransceiverImpl::SyncWithJS(dom::RTCRtpTransceiver& aJsTransceiver, > + ErrorResult& aRv) > +{ > + MOZ_MTLOG(ML_DEBUG, "Syncing with JS transceiver"); Lets have this log the same information as the other initial function log message (mPCHandle, mMid). ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:534 (Diff revision 9) > + if (!mHaveStartedReceiving) { > + return false; > + } > + > + if (!aRecvTrack) { > + return true; Same question here: is this a special if queried with null it tells you if any receive track is set feature? ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:568 (Diff revision 9) > + MOZ_MTLOG(ML_WARNING, mPCHandle << "[" << mMid << "]: " << __FUNCTION__ << > + " returning " << mReceivePipeline.get()); Why is this at warning level? If at all, then debug I would say. But since GetSendPipeline() doesn't log anything I would suggest to remove this to make it consistent. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:690 (Diff revision 9) > + "AudioCodecConfigs (send)."); > + return rv; > + } > + > + if (configs.values.size() > 1 > + && configs.values.back()->mName == "telephone-event") { This will break if you/we turn on RED support, because then we will have three config values. I fear we need to iterate over the config values here. Can be done in new bug since RED support is not on by default yet. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:722 (Diff revision 9) > + > + return NS_OK; > +} > + > +static nsresult > +JsepCodecDescToCodecConfig(const JsepCodecDescription& aCodec, Can we please rename this to JsepCodecDescToVideoCodecConfig() and the other one to Audio? ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:752 (Diff revision 9) > + desc.mSpropParameterSets.c_str(), > + spropSize); > + h264Config->sprop_parameter_sets[spropSize - 1] = '\0'; > + h264Config->packetization_mode = desc.mPacketizationMode; > + h264Config->profile_level_id = desc.mProfileLevelId; > + h264Config->tias_bw = 0; // TODO. Issue 165. I think soon nobody will be able to link "Issue 165" to this https://github.com/unicorn-wg/gecko-dev/issues/165 any more. Let's either remove the comment, or replace it with a proper reference to a bugzilla entry. ::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:927 (Diff revision 9) > + std::vector<webrtc::RtpExtension> extmaps; > + // @@NG read extmap from track > + aDetails.ForEachRTPHeaderExtension( > + [&extmaps](const SdpExtmapAttributeList::Extmap& extmap) > + { > + extmaps.emplace_back(extmap.extensionname,extmap.entry); Missing space :)

Attachment #8900494 - Flags: review?(drno)

Nils Ohlmeier [:drno]

Comment 116

•

7 years ago

mozreview-review

Comment on attachment 8900495 [details] Bug 1290948 - Part 6: Remove some unused code. r+drno https://reviewboard.mozilla.org/r/171870/#review188460 LGTM

Attachment #8900495 - Flags: review?(drno) → review+

Byron Campen [:bwc]

Assignee

Updated

•

7 years ago

Blocks: 1402997

Nils Ohlmeier [:drno]

Comment 117

•

7 years ago

mozreview-review

Comment on attachment 8900496 [details] Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno https://reviewboard.mozilla.org/r/171872/#review188500 LGTM with some smaller tweaks. ::: dom/media/tests/mochitest/pc.js:902 (Diff revision 9) > this._pc.setIdentityProvider(provider, protocol, identity); > }, > > - ensureMediaElement : function(track, direction) { > + getMediaElementForTrack : (track, direction) => > + { > + const idPrefix = [this.label, direction].join('_'); How about putting this into a separate small function createPrefix() or something like that, instead of duplicating it? ::: dom/media/tests/mochitest/pc.js:1081 (Diff revision 9) > } > > info("Get " + constraintsList.length + " local streams"); > - return Promise.all(constraintsList.map(constraints => { > - return getUserMedia(constraints).then(stream => { > - if (constraints.audio) { > + > + return Promise.all( > + constraintsList.map(constraints => this.getUserMedia(constraints))); I think the closing ) bracket from the Promise should go on a new line here for better readability. ::: dom/media/tests/mochitest/pc.js:1089 (Diff revision 9) > - " with audio track " + track.id); > - }); > + getAllUserMediaAndAddTracks : async function(constraintsList) { > + var streams = await this.getAllUserMedia(constraintsList); > + if (!streams) { > + return; > - } > + } > - if (constraints.video) { > + return Promise.all(streams.map(stream => this.attachLocalStream(stream))); Is this on purpose calling attachLocalStream()? From the name of the function I would have expected that this calls attachLocalTrack() instead. Plus this function appears to be identical to getAllUserMediaAndAddTransceivers() below. ::: dom/media/tests/mochitest/pc.js:1285 (Diff revision 9) > this._pc.addEventListener('track', event => { > - info(this + ": 'ontrack' event fired for " + JSON.stringify(event.track)); > - > - this.checkTrackIsExpected(event.track, > - this.expectedRemoteTrackInfoById, > - this.observedRemoteTrackInfoById); > + info(this + ": 'ontrack' event fired for " + event.track.id + > + "(SDP msid is " + this.getWebrtcTrackId(event.track) + > + ")"); > + > + // TODO(bug XXXXX): Checking for remote tracks needs to be completely Fill in the bug number. ::: dom/media/tests/mochitest/pc.js:1604 (Diff revision 9) > waitForMediaFlow : function() { > return Promise.all([].concat( > this.localMediaElements.map(element => this.waitForMediaElementFlow(element)), > - Object.keys(this.expectedRemoteTrackInfoById) > - .map(id => this.remoteMediaElements > - .find(e => e.srcObject.getTracks().some(t => t.id == id))) > + this.remoteMediaElements > + .filter(elem => > + this.getExpectedActiveReceiveTracks().some(track => elem.srcObject.getTracks().some(t => t == track))) Add some line break(s) please. ::: dom/media/tests/mochitest/pc.js:1606 (Diff revision 9) > - this._pc.getSenders().map(sender => this.waitForRtpFlow(sender.track)), > - this._pc.getReceivers().map(receiver => this.waitForRtpFlow(receiver.track)))); > + this.getExpectedActiveReceiveTracks().map(track => this.waitForRtpFlow(track)), > + this.getExpectedSendTracks().map(track => this.waitForRtpFlow(track)))); Did you on purpose switch the order of checking senders and then receivers? I'm concerned that checking receivers first could slow down things (assuming the verification for sending can succeed earlier/faster). ::: dom/media/tests/mochitest/pc.js:1680 (Diff revision 9) > + if (localTransceivers[i].receiver.track.kind != "audio") { > + continue; > + } Wouldn't it make sense to have the filtering for "audio" already have part of initial gathering of the |localTransceivers| and |remoteTransceivers|? ::: dom/media/tests/mochitest/templates.js:108 (Diff revision 9) > - pc._pc.getSenders().map(sender => checkTrackStats(pc, sender, true)), > - pc._pc.getReceivers().map(receiver => checkTrackStats(pc, receiver, false)))); > + pc.getExpectedActiveReceiveTracks().map(track => checkTrackStats(pc, track, false)), > + pc.getExpectedSendTracks().map(track => checkTrackStats(pc, track, true)))); Again: any reason to switch the receiver vs sender order here? ::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:165 (Diff revision 9) > - var track = stream.getVideoTracks()[0]; > + .filter(transceiver => { > + return !transceiver.stopped && > + transceiver.receiver.track.kind == "video" && > + transceiver.sender.track; > + }) > + .forEach(transceiver => { We probably want some form of verification that the filter at least returns one transceiver here. ::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:176 (Diff revision 9) > - } catch (e) { > + } catch (e) { > - is(e.name, "InvalidParameterError", > + is(e.name, "InvalidAccessError", > - "addTrack existing track should fail"); > + "addTrack existing track should fail"); > - } > + } > - try { > + try { > - test.pcLocal._pc.addTrack(track, stream); > + test.pcLocal._pc.addTrack(track, stream); Not sure why we try to add this twice... but what ever :) ::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:75 (Diff revision 9) > + await v2loadedmetadata; > + > + await waitUntil(() => v2.currentTime > 0 && v2.srcObject.currentTime > 0); > + ok(v2.currentTime > 0, "v2.currentTime is moving (" + v2.currentTime + ")"); > + > + await wait(3000); // TODO: Bug 1248154 Bug 1248154 landed in Firefox 48. ::: dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:79 (Diff revision 9) > + > + await wait(3000); // TODO: Bug 1248154 > + > - ok(v1.videoWidth > 0, "source width is positive"); > + ok(v1.videoWidth > 0, "source width is positive"); > - ok(v1.videoHeight > 0, "source height is positive"); > + ok(v1.videoHeight > 0, "source height is positive"); > - if (v2.videoWidth == 640 && v2.videoHeight == 480) { // TODO: Bug 1248154 > + if (v2.videoWidth == 640 && v2.videoHeight == 480) { // TODO: Bug 1248154 Same here: bug 1248154 has landed.

Attachment #8900496 - Flags: review?(drno) → review+

Nils Ohlmeier [:drno]

Comment 118

•

7 years ago

mozreview-review-reply

Comment on attachment 8900493 [details] Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno https://reviewboard.mozilla.org/r/171866/#review182950 > So at the JSEP level, the track id for remote tracks matches what was signaled (if it was signaled). And, we do propogate this id down to JS, but not as the MediaStreamTrack.id; we instead put it in (pref-guarded) RTCRtpTransceiver.remoteTrackId. Does this answer your question? I think I now better understand (compared to when reviewing the patch) when this gets assinged. > RTP transceivers use the JS direction the caller specified. The logic that creates transceivers due to seeing a new m-section in a remote description uses recvonly, which is wrong for m=application. This just makes sure nothing makes that mistake. I could move this into the caller, but I would end up wanting to leave an assert here, which is why I didn't do that. Let's add a comment then here explaining why this happens here to asymmetrically. > Hmm. So we need to keep the mid the same if the transceiver is stopped/disassociated though. Maybe what we ought to do here is: > > 1. If there was a previous offer/answer, use the mid from the previous m-section, and assert it matches the transceiver (if it has one). > 2. If there was not a previous offer/answer, use the mid from the transceiver if it has one, and make one up otherwise. > > Does that sound sane to you? Yep, that sounds like what we need to do here.

Nils Ohlmeier [:drno]

•

7 years ago

Blocks: 1403238

Byron Campen [:bwc]

Assignee

Comment 124

•

7 years ago

mozreview-review-reply

Comment on attachment 8900496 [details] Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno https://reviewboard.mozilla.org/r/171872/#review188500 > Is this on purpose calling attachLocalStream()? > From the name of the function I would have expected that this calls attachLocalTrack() instead. Plus this function appears to be identical to getAllUserMediaAndAddTransceivers() below. It is calling attachLocalStream on purpose, yes. I have fixed the transceivers version though. > Did you on purpose switch the order of checking senders and then receivers? > I'm concerned that checking receivers first could slow down things (assuming the verification for sending can succeed earlier/faster). This is just elements in a Promise.all, so they run in parallel. Can't recall why the order switched, but it should not matter. > Wouldn't it make sense to have the filtering for "audio" already have part of initial gathering of the |localTransceivers| and |remoteTransceivers|? Yeah, and I need to be checking that the mid matches too. > Again: any reason to switch the receiver vs sender order here? It is a Promise.all, so shouldn't matter. > Not sure why we try to add this twice... but what ever :) Merge badness I guess? I've removed the dupe. > Bug 1248154 landed in Firefox 48. I'll see what happens if I remove this, but I am not optimistic... > Same here: bug 1248154 has landed. Same response here.

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

Comment 133

•

7 years ago

First pass is done. Every patch has at least a little work in it, please look at interdiff and review replies. I will be rebasing in the meantime, and pushing that separately.

Flags: needinfo?(jib)

Flags: needinfo?(drno)

Byron Campen [:bwc]

Assignee

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cda9a6c47ca

•

7 years ago

Blocks: 1404686

Byron Campen [:bwc]

Assignee

Comment 149

•

7 years ago

mozreview-review-reply

Comment on attachment 8900496 [details] Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno https://reviewboard.mozilla.org/r/171872/#review188500 > I'll see what happens if I remove this, but I am not optimistic... Ok, removing it seems to work fine.

Byron Campen [:bwc]

Assignee

Comment 150

•

7 years ago

mozreview-review-reply

Comment on attachment 8900494 [details] Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno https://reviewboard.mozilla.org/r/171868/#review191736 > Is it really okay to return NS_OK below if any of these two call fail? > I would assume we would want to check jrv after each of these. We were already returning NS_OK in these cases. The only places either of these fail is if a callback to JS fails for some reason, and I don't think it makes sense to throw an internal error if content JS throws in the callback or something. > This comment appears to be outdated now. This comment refers to RTCRtpTransceiver::Stop(), have updated the comment to be more clear.

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

Reporter

Comment 151

•

7 years ago

mozreview-review

Comment on attachment 8900490 [details] Bug 1290948 - Part 1: Mochitest for transceivers. r+jib https://reviewboard.mozilla.org/r/171860/#review194566 Fab. We should nominate this test for web-platform-tests. ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:23 (Diff revisions 5 - 7) > + } catch (e) { > + is(e.name, exceptionName, description + " throws " + exceptionName); > + } > + }; > + > + let stopTracks = streams => { Nit: we could write: let stopTracks = (...streams) => { ...which would let us call like this: stopTracks(audioStream, videoStream); ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:29 (Diff revisions 5 - 7) > + let oldOnTrack = pc.ontrack; > + pc.ontrack = e => trackEvents.push(e); Seems cleaner to avoid ontrack here than worry about oldOnTrack. e.g.: let listener = e => trackEvents.push(e); pc.addEventListener("track", listener); await pc.setRemoteDescription(desc); pc.removeEventListener("track", listener); Then readers don't have to wonder whether temporarily blocking firing on oldOnTrack is a feature. ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:35 (Diff revisions 5 - 7) > + pc.ontrack = e => trackEvents.push(e); > + await pc.setRemoteDescription(desc); > + pc.ontrack = oldOnTrack; > + > + // basic sanity-check, simplifies testing elsewhere > + trackEvents.forEach(e => { Nit: I recommend avoiding forEach inside async functions in general, due to the maintenance risk of someone later adding an await statement inside the loop, and expecting it to work right. The following is only two characters more: for (let e of trackEvents) { Applies throughout. ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:47 (Diff revisions 5 - 7) > + }); > + > + return trackEvents; > + }; > + > + let trickle = (pc1, pc2, stopBuffering) => { stopBuffering is unused. Remove. ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:49 (Diff revisions 5 - 7) > + return trackEvents; > + }; > + > + let trickle = (pc1, pc2, stopBuffering) => { > + pc1.onicecandidate = async e => { > + await stopBuffering; stopBuffering will always be `undefined` here AFAICT. Still, this `await` statement causes the rest of this function to happen on the microtask queue (actually on the next task until Firefox fixes its microtask queues). Specifically, this means that pc2.addIceCandidate(e.candidate); ...will be called *after* this function returns. This seems uncommon. Was the intention here to test something uncommon? ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:51 (Diff revisions 5 - 7) > + > + let trickle = (pc1, pc2, stopBuffering) => { > + pc1.onicecandidate = async e => { > + await stopBuffering; > + info("Adding ICE candidate: " + JSON.stringify(e.candidate)); > + pc2.addIceCandidate(e.candidate); Technically, we should use `await` here to catch any errors from `addIceCandidate()`, and try{ ... } catch(e){ is(e, null, "got error"); } ...around all this code since `pc1.onicecandidate` ignores the return values and thus any promise errors from this async function. That said, I confess to skimping when ICE isn't the main thing being tested, like e.g. pc1.onicecandidate = e => pc2.addIceCandidate(e.candidate); ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:836 (Diff revisions 5 - 7) > > - dictCompare(pc2.getTransceivers(), > + hasProps(pc2.getTransceivers(), > [ > { > sender: {track: {kind: "audio"}}, > receiver: {track: {kind: "audio"}}, Maybe also check track's `readyState: "ended"` and that track.onended fired? ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:844 (Diff revisions 5 - 7) > currentDirection: null, > direction: "sendrecv" > } > ]); > > pc1.close(); Maybe check that calling .stop() again doesn't throw, neither before nor after close? And finally, maybe check that transceiver.stop() throws InvalidStateError on a closed pc3 whose transceiver has not been stopped? (odd but that's what the spec says). ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:1377 (Diff revisions 5 - 7) > + ]); > + hasProps(pc1.getTransceivers(), > + [ > + { > + receiver: {track: {kind: "audio"}}, > + sender: {track: track}, {track} ::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:1632 (Diff revisions 5 - 7) > - await checkCurrentDirection(options); > - await checkAddTransceiverNoTrackDoesntPair(options); > - await checkAddTransceiverWithTrackDoesntPair(options); > + await checkSetDirection(); > + await checkCurrentDirection(); > + await checkSendrecvWithNoSendTrack(); Nit: these three are out of order.

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

Byron Campen [:bwc]

Assignee

Comment 152

•

7 years ago

mozreview-review-reply

Comment on attachment 8900490 [details] Bug 1290948 - Part 1: Mochitest for transceivers. r+jib https://reviewboard.mozilla.org/r/171860/#review194566 > Maybe also check track's `readyState: "ended"` and that track.onended fired? So the stopped transceiver's (ie; pc1's first transceiver) receive track doesn't have a readyState here. Is that a bug? Seems weird to me... > Nit: these three are out of order. I don't understand what you mean here. checkCurrentDirection and checkSendrecvWithNoSendTrack use setDirection, which means it would be nice if checkSetDirection came first.

Byron Campen [:bwc]

Assignee

Comment 153

•

7 years ago

mozreview-review-reply

Comment on attachment 8900490 [details] Bug 1290948 - Part 1: Mochitest for transceivers. r+jib https://reviewboard.mozilla.org/r/171860/#review194566 > So the stopped transceiver's (ie; pc1's first transceiver) receive track doesn't have a readyState here. Is that a bug? Seems weird to me... Huh, weird. Maybe I typoed it, because now it works.

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

Reporter

•

7 years ago

mozreview-review

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review196126 Would like to see it one more time. ::: dom/media/PeerConnection.js:785 (Diff revisions 5 - 7) > - // This entry-point handles both new and legacy call sig. Decipher which one > + // This entry-point handles both new and legacy call sig. Decipher which one > - if (typeof optionsOrOnSucc == "function") { > + return this._legacy(onSuccess, onErr, () => this._createOffer(options)); fix comment ::: dom/media/PeerConnection.js:798 (Diff revisions 5 - 7) > + if (transceiver) { > + if (transceiver.direction == "sendonly") { > - transceiver.setDirection("sendrecv"); > + transceiver.setDirection("sendrecv"); > - return true; > - } else if (transceiver.direction == "inactive") { > + } else if (transceiver.direction == "inactive") { This goes beyond the spec-requirement [1]. Is this needed for Firefox legacy? Also, calling setDirection() inside createOffer potentially does a lot of things that seem wrong, like firing negotiationneeded, which the spec says explicitly not to do, or throwing InvalidStateError if transceiver is stopped. [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcofferoptions-offertoreceiveaudio ::: dom/media/PeerConnection.js:807 (Diff revisions 5 - 7) > - count--; > - } > + } > - }); > > - while (count) { > - this._addTransceiverNoEvents(kind, {direction: "recvonly"}); > + this._addTransceiverNoEvents(kind, {direction: "recvonly"}); "recvonly" here seems right based on current behavior [1] but isn't in spec. Should we file an issue? [1] https://jsfiddle.net/c48q11jd/ ::: dom/media/PeerConnection.js:812 (Diff revisions 5 - 7) > - } > } > > - // Handles legacy offerToReceiveAudio/Video > + // Handles offerToReceiveAudio/Video > _ensureTransceiversForOfferToReceive(options) { > if (options.offerToReceiveVideo) { Speaking of Firefox legacy, today users can use {offerToReceiveVideo: false} to set sendonly or even inactive [1]. This breaks that. Are we ok with this? Should we warn people who do that? Also, we've been fairly aggressive with deprecation warnings so far. Should we perhaps warn on offerToReceive? [1] https://jsfiddle.net/c48q11jd/ ::: dom/media/PeerConnection.js:1141 (Diff revisions 5 - 7) > + transceiver.setDirection("sendrecv"); > + } else if (transceiver.direction == "inactive") { > + transceiver.setDirection("sendonly"); The spec says to set the [[Direction]] internal slot here, not call the setDirection algorithm. Since we know the transceiver isn't stopped, this would appear to not have any side-effects, but seems inefficient, since it means transceiver.sync(); this.updateNegotiationNeeded(); effectively get called twice, and before transceiver.setAddTrackMagic(); which I can't tell whether matters or not. ::: dom/media/PeerConnection.js:1146 (Diff revisions 5 - 7) > - transceiver = this._addTransceiverNoEvents( > + transceiver = this._addTransceiverNoEvents(track, { > - track, > - { > streams: [stream], > direction: "sendrecv" > - }); > + }); Nit: deep indent ::: dom/media/PeerConnection.js:1176 (Diff revisions 5 - 7) > + transceiver.setDirection("recvonly"); > + } else if (transceiver.direction == "sendonly") { > + transceiver.setDirection("inactive"); > + } > + > - transceiver.sync(); > + transceiver.sync(); > - this.updateNegotiationNeeded(); > + this.updateNegotiationNeeded(); same thing here ::: dom/media/PeerConnection.js:1875 (Diff revisions 5 - 7) > - // TODO (bug XXXXX): Technically, if the transceiver is not yet associated, > - // we're supposed to synchronously set the track and return a resolved > - // promise. However, PeerConnectionImpl::ReplaceTrackNoNegotiation does > - // stuff like updating principal change observers. We might want to set > + // Updates the track on the MediaPipeline; this is needed whether or not > + // we've associated this transceiver, the spec language notwithstanding. > + // Synchronous, and will throw on failure. > + this._pc._replaceTrack(this._transceiverImpl, withTrack); I agree the spec is a little weird here. Does this synchronous call do anything that is JS-observable? If so, then that might be a problem in the case where the transceiver is associated.

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

Byron Campen [:bwc]

Assignee

Comment 158

•

7 years ago

mozreview-review-reply

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review196126 > This goes beyond the spec-requirement [1]. Is this needed for Firefox legacy? > > Also, calling setDirection() inside createOffer potentially does a lot of things that seem wrong, like firing negotiationneeded, which the spec says explicitly not to do, or throwing InvalidStateError if transceiver is stopped. > > [1] https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcofferoptions-offertoreceiveaudio This is basically what we did before (ie; we would set the recv bit on pre-existing m-sections), but yeah we probably should change this since the spec language says something completely different. > "recvonly" here seems right based on current behavior [1] but isn't in spec. Should we file an issue? > > [1] https://jsfiddle.net/c48q11jd/ Yeah, it would be kinda weird to create a sendrecv transceiver in this case. > Speaking of Firefox legacy, today users can use {offerToReceiveVideo: false} to set sendonly or even inactive [1]. This breaks that. Are we ok with this? Should we warn people who do that? > > Also, we've been fairly aggressive with deprecation warnings so far. Should we perhaps warn on offerToReceive? > > [1] https://jsfiddle.net/c48q11jd/ Hmm. I wonder how many people use {offerToReceiveX: false}. We should probably fire warnings when someone does that. > The spec says to set the [[Direction]] internal slot here, not call the setDirection algorithm. > > Since we know the transceiver isn't stopped, this would appear to not have any side-effects, but seems inefficient, since it means > > transceiver.sync(); > this.updateNegotiationNeeded(); > > effectively get called twice, and before > > transceiver.setAddTrackMagic(); > > which I can't tell whether matters or not. Fair enough, I can set directly. > I agree the spec is a little weird here. Does this synchronous call do anything that is JS-observable? If so, then that might be a problem in the case where the transceiver is associated. I don't think it does anything that will get back to JS. There are no callbacks, and the modification to the JS objects only occurs when the sync happens.

Byron Campen [:bwc]

Assignee

Comment 159

•

7 years ago

mozreview-review-reply

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review184150 > On the second part, with https://github.com/w3c/webrtc-pc/pull/1614 the spec no longer says to do this in parallel, so we should remove the queuing. > > On the first part, are you saying the prose "If channel is the first RTCDataChannel created" in the spec is effectively redundant? > > If yes, then good. If no, then please add a comment about how our implementation deviates from the spec in a way that upholds this. The prose is fine, our underlying implementation yields the same result though. I can add a comment. > Ah, nevermind. I missed that the update-negotiation-needed algorithm itself checks for "stable", so this is good. I agree we should simplify step 10 in the spec! Cool. > Sorry, not following what you mean with "opaque". > > Are you saying the remote side should make up a stream with a random id in this case? Do you have support for this? > > From my reading, "-" is JSEP's encoding for "no MediaStream". I naively read the intent here to be to mirror this remotely. > > I find support in http://w3c.github.io/webrtc-pc/#process-remote-track-addition which says: > > "Set the associated remote streams given receiver and a list of the MSIDs that the media description indicates track is to be *associated with*." > > The key word here being "associated with", which matches the JSEP terminology when it effectively defines "-" to mean "no MediaStream is associated with the transceiver". To me, that means "-" means no mediastream is associated with this received track. > > That's the only way I make sense of http://w3c.github.io/webrtc-pc/#set-associated-remote-streams which says: > > "For each MSID in msids, unless a MediaStream object has previously been created with that id for this connection, create a MediaStream object with that id." > > Basically it says MSID and stream.id are 1-1. I see only two readings of this: > > A) "list of MSIDs" excludes "-" > B) spec says to create a stream where stream.id == "-". > > B seems undesirable. "Opaque" here means that the other end doesn't assign any special meaning to the bits in the ID; "-" is fundamentally no different from "{some-uuid}", it's just bits. If we want it to mean something special on the receiving end, we have to spell that out explicitly in the spec. I could ask deadbeef what the intent was here.

Nils Ohlmeier [:drno]

Comment 160

•

7 years ago

Just to make sure we are all on the same page: the consensus today is that we will wait with landing this for 59 to get a full Nightly cycle (rather then just the remainder of the 58 Nightly cycle).

Priority: P3 → P2

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

Comment 169

•

7 years ago

^ That was a rebase only, please ignore.

Byron Campen [:bwc]

Assignee

•

7 years ago

mozreview-review

Comment on attachment 8900495 [details] Bug 1290948 - Part 6: Remove some unused code. r+drno https://reviewboard.mozilla.org/r/171870/#review197256 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` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: media/mtransport/transportlayerice.cpp:98 (Diff revision 13) > > TransportLayerIce::~TransportLayerIce() { > // No need to do anything here, since we use smart pointers > } > > -void TransportLayerIce::SetParameters(RefPtr<NrIceCtx> ctx, > +void TransportLayerIce::SetParameters(RefPtr<NrIceMediaStream> stream, Warning: The parameter 'stream' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

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

Reporter

Comment 185

•

7 years ago

mozreview-review

Comment on attachment 8900492 [details] Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. https://reviewboard.mozilla.org/r/171864/#review197700 Lgtm! Could we add a test for the "no stream" case? ::: dom/media/PeerConnection.js:813 (Diff revisions 7 - 9) > + this.logWarning("offerToReceiveVideo: false is ignored now. If you " + > + "want to stop receiving a track, you can use " + > + "RTCRtpTransceiver.setDirection"); On wording: People probably weren't using {offerToReceiveVideo: false} to stop tracks, rather it was the way to force sendonly on the track they just added (to override the do-unto-others default) I think. RTCRtpTransceiver.setDirection sounds like a static method. Also, people need to consider backwars compat. How about: '{offerToReceiveVideo: false} is ignored now. Consider using pc.getTransceivers()[i].setDirection("sendrecv")' ? Also note I've just filed https://github.com/w3c/webrtc-pc/issues/1643 !

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

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

Reporter

Comment 186

•

7 years ago

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #185) > Also, people need to consider backwars compat. How about: I meant "backwards". Freudian slip.

Flags: needinfo?(jib)

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

Reporter

•

7 years ago

mozreview-review

Comment on attachment 8913686 [details] Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno https://reviewboard.mozilla.org/r/185086/#review202454 C/C++ static analysis found 0 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Code Review Bot [:reviewbot]

Comment 206

•

7 years ago

mozreview-review

Comment on attachment 8913686 [details] Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno https://reviewboard.mozilla.org/r/185086/#review202476 C/C++ static analysis found 0 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Pulsebot

Comment 207

•

7 years ago

Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bbe53fb92e21 Part 1: Mochitest for transceivers. r+jib r=jib https://hg.mozilla.org/integration/autoland/rev/49878c508ce6 Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan r=ehsan+251051,jib https://hg.mozilla.org/integration/autoland/rev/56c169018ceb Part 3: Implement JS part of RTCRtpTransceiver. r=jib https://hg.mozilla.org/integration/autoland/rev/ffb6e6da955f Part 4: Transceivers JSEP/SDP work. r+drno r=drno https://hg.mozilla.org/integration/autoland/rev/1a5f090502b0 Part 5: TransceiverImpl and some major refactoring. r+drno r=drno https://hg.mozilla.org/integration/autoland/rev/314675023cd5 Part 6: Remove some unused code. r+drno r=drno https://hg.mozilla.org/integration/autoland/rev/8ff38e646037 Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno r=drno https://hg.mozilla.org/integration/autoland/rev/97a271bf671e Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno r=drno

Andreea Pavel [:apavel]

Comment 208

•

7 years ago

Backed out for failing mochitest dom/tests/mochitest/general/test_interfaces.html r=backout on a CLOSED TREE Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=144668068&repo=autoland&lineNumber=8554 Push that started the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=97a271bf671eb1e8adb9378ee4052e0d93a40a80&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Backout: https://hg.mozilla.org/integration/autoland/rev/e077a6e6e8425ca49a7992e3ee0a589ebe050184

Flags: needinfo?(docfaraday)

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

Comment 210

•

7 years ago

On it. Need a DOM reviewer so I can touch test_interfaces.js.

Flags: needinfo?(docfaraday)

Byron Campen [:bwc]

Assignee

Updated

•

7 years ago

Attachment #8928253 - Flags: review?(bugs)

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 211

•

7 years ago

mozreview-review

Comment on attachment 8928253 [details] Bug 1290948 - Part 9: Add RTCRtpTransceiver to test_interfaces.js https://reviewboard.mozilla.org/r/199478/#review204572

Attachment #8928253 - Flags: review?(bugs) → review+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 212

•

7 years ago

This also fails media tests on Android and creates a mix of unexpected passes, fails and timeouts in web platform tests.

Byron Campen [:bwc]

Assignee

Comment 213

•

7 years ago

So much for try syntax... guess I'm back to running try_all.

Comment hidden (mozreview-request)

Byron Campen [:bwc]

Assignee

•

7 years ago

mozreview-review

Calixte Denizet (:calixte)

Updated

•

7 years ago

Depends on: 1421965

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

Reporter

Updated

•

7 years ago

Depends on: 1425618

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

Reporter

Updated

•

7 years ago

Depends on: 1425621

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

Reporter

Updated

•

7 years ago

Depends on: 1425873

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

Reporter

Updated

•

7 years ago

Depends on: 1425956

Nils Ohlmeier [:drno]

•

7 years ago

Depends on: 1437832

Nils Ohlmeier [:drno]

Updated

•

7 years ago

Blocks: 1437741

Byron Campen [:bwc]

Assignee

Updated

•

7 years ago

Depends on: 1449042

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

Reporter

Updated

•

7 years ago

Blocks: 1321772

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

Depends on: 1466175

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

status-firefox51: affected → ---

Byron Campen [:bwc]

Assignee

Updated

•

6 years ago

Flags: needinfo?(docfaraday)

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

Depends on: 1502899

[:philipp]

Updated

•

5 years ago

Bug 1290948: WIP Transceivers 8 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request		Details
Bug 1290948 - Part 1: Mochitest for transceivers. r+jib 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1290948 - Part 2: webidl for RTCRtpTransceiver and supporting interfaces r+jib, r+ehsan 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	ehsan.akhgari : review+ jib : review+	Details
Bug 1290948 - Part 3: Implement JS part of RTCRtpTransceiver. 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	jib : review+	Details
Bug 1290948 - Part 4: Transceivers JSEP/SDP work. r+drno 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 5: TransceiverImpl and some major refactoring. r+drno 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 6: Remove some unused code. r+drno 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 7: Update existing mochitests to track the spec fixes in previous parts. r+drno 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 8: Don't cause ICE to fail if there's no streams to establish. r+drno 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 9: Add RTCRtpTransceiver to test_interfaces.js 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1290948 - Part 9: Add RTCRtpTransceiver to test_interfaces.js r+smaug 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1290948 - Part 10: Stop expecting failure on lots of webrtc web-platform-tests. 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 11: Fix a nullptr crash that web-platform-tests is hitting. 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	drno : review+	Details
Bug 1290948 - Part 12: Disable most of the webrtc web-platform-tests until bug 1418367 is fixed. r+jib 7 years ago Byron Campen [:bwc] 59 bytes, text/x-review-board-request	jib : review+	Details