1449042 - bytesReceived but no video after adding video in second round of negotiation

Status: RESOLVED FIXED

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

Description

[mroberts]

Attachment: Screen Shot 2018-03-26 at 4.04.41 PM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

Please try this JSFiddle: https://jsfiddle.net/g00n3qca/

Basically,

1. Two RTCPeerConnections (P1 and P2) connect to each other. P1 and P2 both share two audio MediaStreamTracks each. P2 offers with offerToReceiveVideo set to true. The result is two audio m=sections and one video m=section (BUNDLE group is "sdparta_0 sdparta_1 sdparta_2"). P1's m=video section has a=inactive.
2. P1 adds a video MediaStreamTrack and re-offers (BUNDLE group is still "sdparta_0 sdparta_1 sdparta_2").


Actual results:

P2 receives a video MediaStreamTrack from P1. getStats shows that P2 receives bytes for this track.

However, depending on the delay between Steps 1 and 2, the video may not play back correctly when attached to a video element on the DOM. Initially, I thought this might be a user gesture issue (so I added the "Mute video" checkbox), but I don't think it's related to user gesture.

In the JSFiddle, checking the delay checkbox introduces a delay of 15 s between the first and second rounds of negotiation.

Tested with Firefox 59.0.1 and Firefox 59.0.2. It takes a few times to (maybe 4 or 5) to trigger the failure. When the failure occurs, I can right click the video element and see playback options (like "Pause") in the menu (see screenshot).


Expected results:

P2 receives a video MediaStreamTrack from P1. getStats shows that P2 receives bytes for this track. The received video MediaStreamTrack plays back correctly when attached to a video element on the DOM.

Comment 1

[Nils Ohlmeier [:drno]]

Byron, can you have a look if this is fall out from the Transceivers work?

Comment 2

[Byron Campen [:bwc]]

Does delaying the second negotiation tend to cause the failure, or prevent it?

Comment 3

[Byron Campen [:bwc]]

Ok, so delaying the second negotiation seems to prevent the video stream from being displayed. The bytes received counter goes up, and more notably it _starts_ with a value as if the video stream was being received since the first negotiation took place!

I've experimented with the fiddle, and it seems that we also run into trouble if we create the new video track immediately following the first negotiation. This happens even if I attach the video track to a new stream (to rule out problems caused by A/V sync).

https://jsfiddle.net/fgo865cf/26/

The problem goes away if I remove the {offerToReceiveVideo: true}. The problem also goes away if I use only one audio track in each direction.

Comment 4

[mroberts]

> Does delaying the second negotiation tend to cause the failure, or prevent it?

Yes, AFAICT, delaying the second negotiation tends to cause the failure, as you noticed in #3.

> The problem goes away if I remove the {offerToReceiveVideo: true}.

This is effectively convenience for adding a video transceiver, so we shouldn't expect a difference in behavior if `addTransceiver('video')` is used instead?

> The problem also goes away if I use only one audio track in each direction.

I also noticed this.

Comment 5

[Byron Campen [:bwc]]

Looking at the logging, something is getting very confused about updating SSRCs (I see a VideoConduit updating the receive SSRC to the SSRC negotiated for an audio track!). I wonder if this is related to bug 1394602?

Comment 6

[Byron Campen [:bwc]]

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

Comment 7

[Byron Campen [:bwc]]

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

Comment 8

[Andreas Pehrson [:pehrsons]]

I don't think I can be of assistance here, but I'll follow along.

Comment 9

[Byron Campen [:bwc]]

Attachment: Bug 1449042: Negotiate tracks even when they're not active.

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

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8963272 [details]
Bug 1449042: Negotiate tracks even when they're not active.

https://reviewboard.mozilla.org/r/232168/#review237686

LGTM

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9c4bf921486
Negotiate tracks even when they're not active. r=drno

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a9c4bf921486

Comment 13

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

This was reported in 59, and works in 58 for me, so I'm marking this as a regression. As such we should try getting it into 60, unless someone disagrees.

[Tracking Requested - why for this release]: A bit of an edge-case perhaps, but video failing to appear in a WebRTC call is bad.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Sounds like this needs a Beta approval request given comment 13.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8963272 [details]
Bug 1449042: Negotiate tracks even when they're not active.

Approval Request Comment
[Feature/Bug causing the regression]:

   Bug 1290948

[User impact if declined]:

   Some webrtc mute scenarios will not work properly.

[Is this code covered by automated tests?]:

   Not really.

[Has the fix been verified in Nightly?]:

   Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

   No.

[List of other uplifts needed for the feature/fix]:

   None.

[Is the change risky?]:

   Not very.

[Why is the change risky/not risky?]:

   We're just invoking the most common code path in an additional case.

[String changes made/needed]:

   None.

Comment 16

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8963272 [details]
Bug 1449042: Negotiate tracks even when they're not active.

webrtc regression fix, approved for 60.0b10

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/df8ec314e637