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

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
Rank:
12
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jib, Assigned: bwc)

Tracking

({regression})

59 Branch
mozilla59
Unspecified
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59+ fixed)

Details

Attachments

(7 attachments)

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.
Blocks: 1425873
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: → bug 1425937
(Assignee)

Updated

a year ago
Assignee: jib → docfaraday
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
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 14

a year ago
mozreview-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 15

a year ago
mozreview-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 16

a year ago
mozreview-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+
(Assignee)

Comment 17

a year ago
mozreview-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.
(Assignee)

Comment 18

a year ago
mozreview-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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 26

a year ago
mozreview-review
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+
(Reporter)

Comment 27

a year ago
mozreview-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/
(Assignee)

Comment 29

a year ago
mozreview-review-reply
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).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

a year ago
Reviewboard won't ask you for review on part 4 again for some reason, needinfoing instead.
Flags: needinfo?(jib)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

a year ago
^ That was a rebase.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

a year ago
^ 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
(Assignee)

Comment 49

a year ago
(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.
(Reporter)

Comment 50

a year ago
mozreview-review
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-
(Reporter)

Comment 51

a year ago
mozreview-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+
(Assignee)

Comment 52

a year ago
mozreview-review-reply
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.
(Reporter)

Comment 53

a year ago
mozreview-review
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+
(Reporter)

Comment 54

a year ago
mozreview-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 55

a year ago
mozreview-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 56

a year ago
mozreview-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)
(Assignee)

Comment 57

a year ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 62

a year ago
Needinfo jib again for review, because reviewboard won't do it.
Flags: needinfo?(jib)
(Reporter)

Comment 63

a year ago
mozreview-review
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+
(Assignee)

Comment 64

a year ago
mozreview-review-reply
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()))"
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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)

Comment 70

a year ago
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
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
status-firefox-esr52: --- → unaffected
(Assignee)

Updated

a year ago
Duplicate of this bug: 1425937
Duplicate of this bug: 1370006
You need to log in before you can comment on or make changes to this bug.