Closed Bug 1425621 Opened 7 years ago Closed 7 years ago

Lost ability to detect remote track removal (Remove remote tracks from their streams when negotiated away)

Categories

(Core :: WebRTC: Signaling, defect, P2)

Product:
Core
Component:
WebRTC: Signaling
Version:
59 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 + fixed

People

(Reporter: jib, Assigned: bwc)

References

Details

(Keywords: regression)

Attachments

(7 files)

Bug 1425621 - Part 1: Test-case for removeTrack and MediaStream.onremovetrack.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1425621 - Part 2: webidl for MediaStream.onremovetrack.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1425621 - Part 3: Implementation for MediaStream.onremovetrack.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1425621 - Part 4: Move track event logic to JS.
59 bytes, text/x-review-board-request
smaug
: review+
drno
: review+
jib
: review+
Details
Bug 1425621 - Part 5: Handle transceiver removal caused by rollback after track events.
59 bytes, text/x-review-board-request
smaug
: review+
drno
: review+
jib
: review+
Details
Bug 1425621 - Part 6: Update mochitests to match new behavior.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1425621 - Part 7: Update wpt to expect onremovetrack.
59 bytes, text/x-review-board-request
jib
: review+
Details 
Users need this to detect removal of remote tracks, an ability that was lost with transceivers model no longer firing the ended event. We should get this into 59.

Mandated by spec [1]

[1] http://w3c.github.io/webrtc-pc/#process-remote-track-removal
[Tracking Requested - why for this release]:
Assignee: nobody → docfaraday
tracking-firefox59: --- → ?
Rank: 11
Likewise, we should fire adtrack on addition of remote track as well, if we aren't already.
Spec change for 59, tracking is fine. 
I don't understand the consequences if it doesn't get in, but it sounds like we should comply with the spec changes.
tracking-firefox59: ? → +
Taking this so Byron can focus on bug 1425956.
Assignee: docfaraday → jib
Rank: 11 → 12
See Also: → 1425937
Assignee: jib → docfaraday
Comment on attachment 8938430 [details]
Bug 1425621 - Part 2: webidl for MediaStream.onremovetrack.

https://reviewboard.mozilla.org/r/209138/#review214898

Seems to follow the spec.
Attachment #8938430 - Flags: review?(bugs) → review+
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review214904

r+ for the webidl.
Attachment #8938432 - Flags: review?(bugs) → review+
Comment on attachment 8938433 [details]
Bug 1425621 - Part 5: Handle transceiver removal caused by rollback after track events.

https://reviewboard.mozilla.org/r/209144/#review214906

::: dom/webidl/RTCRtpTransceiver.webidl:53
(Diff revision 1)
>  
>      [ChromeOnly]
>      void setAddTrackMagic();
>      [ChromeOnly]
>      readonly attribute boolean addTrackMagic;
> +    attribute boolean shouldRemove;

I don't see shouldRemove property in the spec.
Please add [ChromeOnly]
Attachment #8938433 - Flags: review?(bugs) → review+
Comment on attachment 8938431 [details]
Bug 1425621 - Part 3: Implementation for MediaStream.onremovetrack.

https://reviewboard.mozilla.org/r/209140/#review214910
Attachment #8938431 - Flags: review?(apehrson) → review+
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review214972

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:545
(Diff revision 1)
> +    receiveStreamIds.AppendElement(NS_ConvertUTF8toUTF16(id.c_str()),
> +                                   fallible);
> +  }
> +  receiver->SetStreamIds(receiveStreamIds, aRv);
> -    if (aRv.Failed()) {
> +  if (aRv.Failed()) {
> +    MOZ_CRASH();

I'm removing this MOZ_CRASH

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:552
(Diff revision 1)
> -    }
> +  }
> +
> +  receiver->SetRemoteSendBit(mJsepTransceiver->mRecvTrack.GetRemoteSetSendBit(),
> +                             aRv);
> +  if (aRv.Failed()) {
> +    MOZ_CRASH();

I'm removing this MOZ_CRASH.
Comment on attachment 8938433 [details]
Bug 1425621 - Part 5: Handle transceiver removal caused by rollback after track events.

https://reviewboard.mozilla.org/r/209144/#review214974

::: media/webrtc/signaling/src/jsep/JsepTrack.h:10
(Diff revision 1)
>  #ifndef _JSEPTRACK_H_
>  #define _JSEPTRACK_H_
>  
>  #include <functional>
>  #include <algorithm>
> +#include <iostream>

Some debug crud snuck in here. Will remove this.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:132
(Diff revision 1)
>      std::string error;
>      SdpHelper helper(&error);
>  
>      if (msection.IsSending()) {
>        (void)helper.GetIdsFromMsid(sdp, msection, &mStreamIds, &mTrackId);
> +      std::cout << "remote send bit " << mRemoteSetSendBit << " -> " << true << std::endl;

And these.
Comment on attachment 8937532 [details]
Bug 1425621 - Part 1: Test-case for removeTrack and MediaStream.onremovetrack.

https://reviewboard.mozilla.org/r/208224/#review214994
Attachment #8937532 - Flags: review?(jib) → review+
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review214996

::: dom/media/PeerConnection.js:1330
(Diff revision 2)
> +  _fireReceiverEvents() {
> +    for (let transceiver of this._transceivers) {
> +      transceiver.receiver.fireEvents(transceiver);

fireEvents seems like a misnomer, since it does more than fire events, like adding/removing tracks to/from streams.

::: dom/media/PeerConnection.js:2179
(Diff revision 2)
> +    // Spec says to batch these up and fire them after all of the streams have
> +    // gotten their callbacks. Maybe need to split this off into its own
> +    // function?
> +    if (!this._remoteSetSendBit) {
> +      // remote used "recvonly" or "inactive"
> +      this._ontrackFired = false;
> +      if (!this.track.muted) {
> +        this.track.mutedChanged(true);
> +      }
> +    } else if (!this._ontrackFired) {
> +      // remote used "sendrecv" or "sendonly", and we haven't fired ontrack
> +      let ev = new this._pc._win.RTCTrackEvent("track", {

Not done reviewing yet, but wanted to leave early feedback.

I'm finding this code a bit confusing, especially all the temporary states. Are they here to avoid crossing c++ to js too many times?

Importantly, we must not fire track events until all track additions and removals have been done, as all stream<->track relationships need to be in their final state then, as they may be examined by listeners of the track events, as the spec requires. We should test this as well.

I'd prefer this to be organized more like the spec is organized, calling sub-functions with arrays passed in to lift out the events, as the spec suggests. This is also needed to make sure we iterate copies of lists and not the JS observable lists, to avoid malicious javaScript messing with state from under our algorithm.

I'm working on a PR to fix the last bits to lift tracks out into an addList and removeList as I mention here [1].

[1] https://github.com/w3c/webrtc-pc/issues/1691#issuecomment-352919912
Attachment #8938432 - Flags: review?(jib) → review-
s/lift out the events/lift out the tracks/
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review214996

> Not done reviewing yet, but wanted to leave early feedback.
> 
> I'm finding this code a bit confusing, especially all the temporary states. Are they here to avoid crossing c++ to js too many times?
> 
> Importantly, we must not fire track events until all track additions and removals have been done, as all stream<->track relationships need to be in their final state then, as they may be examined by listeners of the track events, as the spec requires. We should test this as well.
> 
> I'd prefer this to be organized more like the spec is organized, calling sub-functions with arrays passed in to lift out the events, as the spec suggests. This is also needed to make sure we iterate copies of lists and not the JS observable lists, to avoid malicious javaScript messing with state from under our algorithm.
> 
> I'm working on a PR to fix the last bits to lift tracks out into an addList and removeList as I mention here [1].
> 
> [1] https://github.com/w3c/webrtc-pc/issues/1691#issuecomment-352919912

I _really_ don't want to have to pass events back to c++ to have them fire, but I can reorganize this function to accumulate a list of events, and then fire them all at the end. I should note that that would break spec for the onaddtrack/onremovetrack events on the MediaStream, however (since the spec says to fire those immediately).
Reviewboard won't ask you for review on part 4 again for some reason, needinfoing instead.
Flags: needinfo?(jib)
^ That was a rebase.
^ And _that_ was an actual fix.
(In reply to Byron Campen [:bwc] from comment #29)
> I _really_ don't want to have to pass events back to c++ to have them fire,
> but I can reorganize this function to accumulate a list of events, and then
> fire them all at the end. I should note that that would break spec for the
> onaddtrack/onremovetrack events on the MediaStream, however (since the spec
> says to fire those immediately).

I hope to have that fixed with https://github.com/w3c/webrtc-pc/pull/1720.

Basically https://github.com/w3c/webrtc-pc/issues/1691#issuecomment-355026902
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #48)
> (In reply to Byron Campen [:bwc] from comment #29)
> > I _really_ don't want to have to pass events back to c++ to have them fire,
> > but I can reorganize this function to accumulate a list of events, and then
> > fire them all at the end. I should note that that would break spec for the
> > onaddtrack/onremovetrack events on the MediaStream, however (since the spec
> > says to fire those immediately).
> 
> I hope to have that fixed with https://github.com/w3c/webrtc-pc/pull/1720.
> 
> Basically https://github.com/w3c/webrtc-pc/issues/1691#issuecomment-355026902

Yeah, I saw that go by. I _think_ the latest changesets are consistent with that in terms of observable behavior.
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review216814

::: dom/media/PeerConnection.js:2163
(Diff revisions 2 - 5)
>  
>    setRemoteSendBit(sendBit) {
>      this._remoteSetSendBit = sendBit;
>    }
>  
> -  fireEvents(transceiver) {
> +  processTrackAdditionsAndRemovals(transceiver, tracksToMuteAndTrackEvents) {

I'd recommend destructuring here:

    processTrackAdditionsAndRemovals(transceiver, {muteTracks, trackEvents}) {

Avoids argument order reliance without losing readability.

::: dom/media/PeerConnection.js:2172
(Diff revisions 2 - 5)
>      addedStreams.forEach(stream => {
>        stream.addTrack(this.track);

This still adds and removes tracks while walking transceivers, which doesn't match https://github.com/w3c/webrtc-pc/pull/1720

Seems fixable the same way, by passing in `addList` and `removeList`:

    processTrackAdditionsAndRemovals(transceiver, {muteTracks, trackEvents, addList, removeList}) {

and moving the actual adding/removing out to the caller.

::: dom/media/PeerConnection.js:2212
(Diff revisions 2 - 5)
>        ev = new this._pc._win.MediaStreamTrackEvent("addtrack",
>            { track: this.track });
> -      this._pc.dispatchEvent(ev);
> +      tracksToMuteAndTrackEvents.trackEvents.push(ev);
>      }
> +
> +    return tracksToMuteAndTrackEvents;

On first read, this looked redundant to me, since we've modified the passed-in object here already, so no need to return it.

But then I see this is necessary because we use webidl dictionaries, which requires copying back and forth...

::: dom/webidl/RTCRtpReceiver.webidl:27
(Diff revisions 2 - 5)
>    [ChromeOnly]
> -  void fireEvents(RTCRtpTransceiver transceiver);
> +  TracksToMuteAndTrackEvents processTrackAdditionsAndRemovals(
> +      RTCRtpTransceiver transceiver,
> +      TracksToMuteAndTrackEvents tracksAndEvents);

Since this is ChromeOnly and same-file JS-to-JS I think we should simplify to:

    [ChromeOnly]
    void processTrackAdditionsAndRemovals(
        RTCRtpTransceiver transceiver,
        object tracksAndEvents);

That way we avoid unnecessary copying back and forth.

Come to think of it, are you positive a ChromeOnly method here is needed at all? It may be that xrays do this for you automatically.
Attachment #8938432 - Flags: review?(jib) → review-
Comment on attachment 8938433 [details]
Bug 1425621 - Part 5: Handle transceiver removal caused by rollback after track events.

https://reviewboard.mozilla.org/r/209144/#review216854
Attachment #8938433 - Flags: review?(jib) → review+
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review216814

> Since this is ChromeOnly and same-file JS-to-JS I think we should simplify to:
> 
>     [ChromeOnly]
>     void processTrackAdditionsAndRemovals(
>         RTCRtpTransceiver transceiver,
>         object tracksAndEvents);
> 
> That way we avoid unnecessary copying back and forth.
> 
> Come to think of it, are you positive a ChromeOnly method here is needed at all? It may be that xrays do this for you automatically.

I've tried to get xrays to work in several other cases, with no luck.
Comment on attachment 8938434 [details]
Bug 1425621 - Part 6: Update mochitests to match new behavior.

https://reviewboard.mozilla.org/r/209146/#review216898

::: dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html:17
(Diff revision 5)
>      let haveMuteEvent;
>      let haveUnmuteEvent;

Maybe initialize:

    let haveMuteEvent = new Promise(() => {});
    let haveUnmuteEvent = new Promise(() => {});

Given how addRenegotiation() works, I find it hard to reason about when these are initialized later, and if they're initialized too late then tests would silently succeed.

This should protect against maintenance mistakes.

::: dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html:67
(Diff revision 5)
> -    test.chain.insertBefore("PC_REMOTE_SET_LOCAL_DESCRIPTION", [
> +    test.chain.insertBefore("PC_REMOTE_SET_REMOTE_DESCRIPTION", [
>        function PC_REMOTE_SETUP_ONMUTE(test) {
>          haveMuteEvent = haveEvent(test.pcRemote._pc.getReceivers()[0].track, "mute");
> -      },
> +      }
> +    ], false, 1);

A comment about why we insert before the *second* instance of PC_REMOTE_SET_REMOTE_DESCRIPTION might save the next reviewer some time.
Attachment #8938434 - Flags: review?(jib) → review+
Comment on attachment 8938479 [details]
Bug 1425621 - Part 7: Update wpt to expect onremovetrack.

https://reviewboard.mozilla.org/r/209178/#review216912
Attachment #8938479 - Flags: review?(jib) → review+
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review216960

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:4586
(Diff revision 5)
>    SetRemoteOffer(offer);
>  
>    ASSERT_EQ(NS_OK,
>              mSessionAns->SetRemoteDescription(kJsepSdpRollback, ""));
>    ASSERT_EQ(kJsepStateStable, mSessionAns->GetState());
> -  ASSERT_EQ(CountRtpTypes(), mSessionAns->GetRemoteTracksRemoved().size());
> +  for (const auto& transceiver : mSessionAns->GetTransceivers()) {

Are there any references to CountRtpTypes() left, or can we remove that code?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:131
(Diff revision 5)
>      std::string error;
>      SdpHelper helper(&error);
>  
>      if (msection.IsSending()) {
>        (void)helper.GetIdsFromMsid(sdp, msection, &mStreamIds, &mTrackId);
> +      mRemoteSetSendBit = true;

mRemoteSetSendBit = msection.IsSending(); ?
Attachment #8938432 - Flags: review?(drno) → review+
Comment on attachment 8938433 [details]
Bug 1425621 - Part 5: Handle transceiver removal caused by rollback after track events.

https://reviewboard.mozilla.org/r/209144/#review216980
Attachment #8938433 - Flags: review?(drno) → review+
Flags: needinfo?(jib)
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review216960

> mRemoteSetSendBit = msection.IsSending(); ?

Seems like a wash to me, but ok.
Needinfo jib again for review, because reviewboard won't do it.
Flags: needinfo?(jib)
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review218198

Lgtm. I'd like smaug to bless the webidl again. I don't like to add `object` to WebIDL without a second set of eyes (though it should be fine in a [ChromeOnly] method in).

::: dom/media/PeerConnection.js:2177
(Diff revisions 5 - 6)
> -    addedStreams.forEach(stream => {
> +    updateStreamFunctions.push(...addedStreams.map(stream => () => {
>        stream.addTrack(this.track);
>        // Adding tracks from JS does not result in the stream getting
>        // onaddtrack, so we need to do that here.
>        stream.dispatchEvent(
>            new this._pc._win.MediaStreamTrackEvent(
>              "addtrack", { track: this.track }));

Spec [1] says to check if track is already added here, which we should do. addTrack may be idempotent, but we still need to check to avoid firing the event.

This is important, because these steps will run after synchronous event firings, which means JS may have fiddled with the state since.

On the flip-side, this means JS can influence whether we fire all events or not, by adding or removing tracks from the stream in the callbacks (which is what synchronous event dispatches are), but I think it's more important for the events to reflect actual additions and removals done by the browser rather than JS, in case the JS is keeping count. So I think the spec has it right.

[1] https://rawgit.com/w3c/mediacapture-main/master/getusermedia.html#add-track

::: dom/media/PeerConnection.js:2186
(Diff revisions 5 - 6)
> -    removedStreams.forEach(stream => {
> +    updateStreamFunctions.push(...removedStreams.map(stream => () => {
>        stream.removeTrack(this.track);
>        // Removing tracks from JS does not result in the stream getting
>        // onremovetrack, so we need to do that here.
>        stream.dispatchEvent(
>            new this._pc._win.MediaStreamTrackEvent(
>              "removetrack", { track: this.track }));

Same here.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:129
(Diff revisions 5 - 6)
> +    mRemoteSetSendBit = msection.IsSending();
> +
>      if (msection.IsSending()) {

Or

    mRemoteSetSendBit = msection.IsSending();
    if (mRemoteSetSendBit) {

:)
Attachment #8938432 - Flags: review?(jib) → review+
Comment on attachment 8938432 [details]
Bug 1425621 - Part 4: Move track event logic to JS.

https://reviewboard.mozilla.org/r/209142/#review218198

> Or
> 
>     mRemoteSetSendBit = msection.IsSending();
>     if (mRemoteSetSendBit) {
> 
> :)

I am afraid if I change this again, someone else will ask for "if ((mRemoteSendBit = msection.IsSending()))"
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #63)
> I'd like smaug to bless the webidl again. I don't like to add `object`
> to WebIDL without a second set of eyes (though it should be fine in a
> [ChromeOnly] method in).

Ran this by smaug and bz on irc [1] who said it looked safe.

[1] https://mozilla.logbot.info/content/20180112#c14122829-c14122855
Flags: needinfo?(jib)
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ecc116e85fb2
Part 1: Test-case for removeTrack and MediaStream.onremovetrack. r=jib
https://hg.mozilla.org/integration/autoland/rev/eb93d67c5747
Part 2: webidl for MediaStream.onremovetrack. r=smaug
https://hg.mozilla.org/integration/autoland/rev/463ebca87767
Part 3: Implementation for MediaStream.onremovetrack. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/6fc395c1746d
Part 4: Move track event logic to JS. r=drno,jib,smaug
https://hg.mozilla.org/integration/autoland/rev/2cdb3560d089
Part 5: Handle transceiver removal caused by rollback after track events. r=drno,jib,smaug
https://hg.mozilla.org/integration/autoland/rev/5399d7e4dc9b
Part 6: Update mochitests to match new behavior. r=jib
https://hg.mozilla.org/integration/autoland/rev/c58bce6abcf2
Part 7: Update wpt to expect onremovetrack. r=jib
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: