1309886 - Received tracks don't end after removal and renegotiation

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

MediaStreamTrack's "ended" event landed in bug 1208373 but I just noticed a track doesn't end at the receiver after being removed at the sender and renegotiated.

Comment 1

[Andreas Pehrson [:pehrsons]]

I noticed this because the patches on bug 1305949 started to permafail test_peerConnection_addtrack_removetrack_events.html.

Turned out we were hooking up the MediaStreamVideoSink in media element to a track that was marked as live but didn't receive any more data.

Ending it properly moves the sink to the first live track.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1309886 - Check that pre-renegotiation track ends in test_pc_addtrack_removetrack_events.html.

Review commit: https://reviewboard.mozilla.org/r/85532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85532/

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1309886 - End received tracks when MediaPipelineReceive is detached.

Review commit: https://reviewboard.mozilla.org/r/85534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85534/

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8800656 [details]
Bug 1309886 - End received tracks when MediaPipelineReceive is detached.

https://reviewboard.mozilla.org/r/85534/#review84222

LGTM

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8800655 [details]
Bug 1309886 - Check that pre-renegotiation track ends in test_pc_addtrack_removetrack_events.html.

https://reviewboard.mozilla.org/r/85532/#review84232

Looks good after if fixed or explained why no fixing needed :-)

::: dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html:35
(Diff revision 1)
>            test.pcLocal.removeSender(videoSenderIndex);
>            test.pcLocal.attachLocalTrack(stream.getTracks()[0], localStream);

If I'm not mistaken these two lines are causing the actual swapping. Shouldn't that be moved to the end of the function below the eventsPromise line?

::: dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html:58
(Diff revision 1)
> +              .find(t => t.kind == "video");
> +          ok(remoteTrack, "Should have received remote track");
> +          const endedPromise = haveEvent(remoteTrack, "ended",
> +              wait(50000, new Error("No ended event")));
> +
> +          eventsPromise = Promise.all([addTrackPromise, endedPromise]);

Is it on purpose that the promise is not returned here?
I would have expected that we wait for both promises to get resolved, before the next test step gets executed. I think without returning this there is no guarantee that any of these actually get resolved before the test finishes.

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8800655 [details]
Bug 1309886 - Check that pre-renegotiation track ends in test_pc_addtrack_removetrack_events.html.

https://reviewboard.mozilla.org/r/85532/#review84232

> If I'm not mistaken these two lines are causing the actual swapping. Shouldn't that be moved to the end of the function below the eventsPromise line?

Not needed since it takes the whole renegotiation before we start seeing things on the remote side.

> Is it on purpose that the promise is not returned here?
> I would have expected that we wait for both promises to get resolved, before the next test step gets executed. I think without returning this there is no guarantee that any of these actually get resolved before the test finishes.

It's checked after the renegotiation is complete further down in the file.

Comment 7

[Andreas Pehrson [:pehrsons]]

Recent try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fba8ee056e8805d9f3409b7d17ced5ae7f5ca430
And without that debug commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c9687fc01e1

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8800655 [details]
Bug 1309886 - Check that pre-renegotiation track ends in test_pc_addtrack_removetrack_events.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85532/diff/1-2/

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8800656 [details]
Bug 1309886 - End received tracks when MediaPipelineReceive is detached.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85534/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47cf46bd3e3e
Check that pre-renegotiation track ends in test_pc_addtrack_removetrack_events.html. r=drno
https://hg.mozilla.org/integration/autoland/rev/227c9d7813e6
End received tracks when MediaPipelineReceive is detached. r=drno

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/47cf46bd3e3e
https://hg.mozilla.org/mozilla-central/rev/227c9d7813e6