1288904 - Firefox sending unannounced SSRCs in simulcast, may not be sending RID

Status: RESOLVED FIXED

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

Description

[Maire Reavy [:mreavy]]

Quoting from Dmitriy Solovey from m.dev.media:

We are testing FF/Simulcast with our SFU, expecting FF to send RTP packets with the same SSRC  (announced in SDP/offer)  and RIDs in RTP header extension

It does not. Instead we observe FF sends RTP/RTCP with unannounced SSRCs without any information about them…

How to make FF send RIDs instead of unknown SSRC? 

....
Could you please confirm if FF sends RIDs in RTP header  extension after A/O SDPs (with send/recv  “a=rid…” & a=simulcast…”) are set?

Our observation - for some reason FF sends unannounced, related to nothing SSRCs, and no rtp header extensions for video at all…

We can’t tell - is it a bug or do we miss something?

From the Offer from Firefox:
  ...
  a=simulcast: send rid=aaa;bbb;ccc
  a=ssrc:798642356 cname:{045c1672-89d5-44c2-b914-e2049ce595b4}

Answer from SFU:
  ...
  a=rid:aaa recv
  a=rid:bbb recv
  a=rid:ccc recv
  a=simulcast: recv rid=aaa;bbb;ccc

Comment 1

[Maire Reavy [:mreavy]]

Dmitriy - Marking unconfirmed until we can investigate.  Can you provide a PCAP of the network traffic after starting this call, so we can see what FF is sending in your case?

Also, please set the environment variable MOZ_LOG to signaling:5 and MOZ_LOG_FILE to some temp file before starting firefox, run your test, then save the file and attach it here?  This is likely more useful than a PCAP.

Comment 2

[Byron Campen [:bwc]]

With simulcast, the SSRCs of the RIDs are not signaled in SDP. The ssrc attr we're signaling is a fallback in case rid/simulcast is not used. As for RID RTP headers/RTCP SDES being missing, this is not expected. Let me look into it.

Comment 3

[Byron Campen [:bwc]]

It does seem that no RID RTP header extensions are being used, nor any RTCP SDES RID. It seems that the API surface we're using to set up the extensions on the webrtc.org code does not work.

Randell, any ideas here?

Comment 4

[dsolovey]

(In reply to Byron Campen [:bwc] from comment #2)
> With simulcast, the SSRCs of the RIDs are not signaled in SDP. The ssrc attr
> we're signaling is a fallback in case rid/simulcast is not used. As for RID
> RTP headers/RTCP SDES being missing, this is not expected. Let me look into
> it.

I would suggest to signal the same SSRC in SDP which can be used with and without RID. The other side can explicitly signal via SDP if it does support RID or not... RID spec covers such scenarios. SDES should be used for informational purposes, not for negotiations. This way all things are clear and predictable, no guessing, less coding, testing, no race conditions, and no detection of missng rtp/rtcp packets during negotiation. What do you think?

Comment 5

[Nils Ohlmeier [:drno]]

Looks like Byron is on it.

Comment 6

[Byron Campen [:bwc]]

(In reply to dsolovey from comment #4)
> (In reply to Byron Campen [:bwc] from comment #2)
> > With simulcast, the SSRCs of the RIDs are not signaled in SDP. The ssrc attr
> > we're signaling is a fallback in case rid/simulcast is not used. As for RID
> > RTP headers/RTCP SDES being missing, this is not expected. Let me look into
> > it.
> 
> I would suggest to signal the same SSRC in SDP which can be used with and
> without RID. The other side can explicitly signal via SDP if it does support
> RID or not... RID spec covers such scenarios. SDES should be used for
> informational purposes, not for negotiations. This way all things are clear
> and predictable, no guessing, less coding, testing, no race conditions, and
> no detection of missng rtp/rtcp packets during negotiation. What do you
> think?

   This is not how the specifications are written, so this won't interop even if we did it.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Byron Campen [:bwc] from comment #3)
> It does seem that no RID RTP header extensions are being used, nor any RTCP
> SDES RID. It seems that the API surface we're using to set up the extensions
> on the webrtc.org code does not work.
> 
> Randell, any ideas here?

My belief was that RID was being sent (and that the tests were using this to check simulcast).  I'll check into it.  I don't remember off the top of my head if SDES was being added, but can check that too.

Comment 8

[Byron Campen [:bwc]]

(In reply to [:jesup] on pto until 2016/8/1 Randell Jesup from comment #7)
> (In reply to Byron Campen [:bwc] from comment #3)
> > It does seem that no RID RTP header extensions are being used, nor any RTCP
> > SDES RID. It seems that the API surface we're using to set up the extensions
> > on the webrtc.org code does not work.
> > 
> > Randell, any ideas here?
> 
> My belief was that RID was being sent (and that the tests were using this to
> check simulcast).  I'll check into it.  I don't remember off the top of my
> head if SDES was being added, but can check that too.

The tests just use SSRCs; webrtc.org doesn't make it easy to get the RID for incoming traffic.

Comment 9

[dsolovey]

Attachment: log file

Comment 10

[dsolovey]

log file attached... btw,

MOZ_LOG + MOZ_LOG_FILE -- is not working, but
NSPR_LOG_FILE + NSPR_LOG_MODULES -- works

Windows 7 64bit, FF 47.0.1, 32bit, up to date

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: simulcast mochitest logs @97a52326b06a

Here are some breadcrumbs that fell out during the webrtc.prg 49 merge.
* There is no rtp extmap for RTPStreamId in the SDP
* webrtc::RTPSender::BuildRIDExtension isn't being called during the mochitest https://dxr.mozilla.org/mozilla-central/search?q=%2Bfunction-decl%3A%22webrtc%3A%3ARTPSender%3A%3ABuildRIDExtension%28uint8_t+%2A%29+const%22
* webrtc::ViERTP_RTCPImpl::SetSendRIDStatus isn't being called by anything https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22webrtc%3A%3AViERTP_RTCPImpl%3A%3ASetSendRIDStatus%28int%2C+bool%2C+int%2C+const+char+%2A%29%22

mochitest run log attached

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 13

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Nico Grunbaum [:ng] from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e99297162da4

Added missing header, retrying on windows.

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 16

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 17

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: video_rid_bug12988904.patch

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8792669 [details] [diff] [review]
video_rid_bug12988904.patch

Review of attachment 8792669 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +862,5 @@
>        MOZ_MTLOG(ML_ERROR, "External codec not available");
>        return NS_ERROR_FAILURE;
>      }
>  
> +

remove extra blank line (nit)

::: media/webrtc/trunk/webrtc/config.h
@@ +72,5 @@
>    int max_bitrate_bps;
>  
>    int max_qp;
>  
> +  char rid[kRIDSize+1]; // MOZ addition mRid

get rid of comment

::: media/webrtc/trunk/webrtc/video/video_send_stream.cc
@@ +140,5 @@
>        CHECK_EQ(0, rtp_rtcp_->SetSendAbsoluteSendTimeStatus(channel_, true, id));
>      } else if (extension == RtpExtension::kVideoRotation) {
>        CHECK_EQ(0, rtp_rtcp_->SetSendVideoRotationStatus(channel_, true, id));
> +    } else if (extension == RtpExtension::kRtpStreamId) {
> +      RTC_CHECK_EQ(0, vie_channel_->SetSendRtpStreamId(true,id));

The others here are CHECK_EQ, I think this should be the same, right?

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: vid_rid_bug12988904.patch

RTC_CHECK_* and CHECK_* are the same, and after the merge that block all moves to RTC_CHECK_*. I am leaving it in hopes of creating less noise when Paul rebases again.

Comment 20

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: vid_rid_1_bug12988904.patch

See interdiff for changes.

Comment 21

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: interdiff_vid_rid.patch

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cb69423014
Clean up RID header extension support r=jesup

Comment 23

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

Backed out for failing cpp unit test jsep_session_unittest:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d04a39c71956

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=07cb6942301414cf99972c76c3a0bc433e649fb0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=36441169&repo=mozilla-inbound

21:05:56     INFO -  [ RUN      ] JsepSessionTest.TestExtmap
21:05:56     INFO -  OFFER: v=0
21:05:56     INFO -  o=mozilla...THIS_IS_SDPARTA-52.0a1 3198412495940532565 0 IN IP4 0.0.0.0
21:05:56     INFO -  s=-
21:05:56     INFO -  t=0 0
21:05:56     INFO -  a=fingerprint:sha-1 4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F
21:05:56     INFO -  a=fingerprint:sha-256 4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F:4F
21:05:56     INFO -  a=ice-options:trickle
21:05:56     INFO -  a=msid-semantic:WMS *
21:05:56     INFO -  m=audio 9 UDP/TLS/RTP/SAVPF 109 9 0 8
21:05:56     INFO -  c=IN IP4 0.0.0.0
21:05:56     INFO -  a=sendrecv
21:05:56     INFO -  a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
21:05:56     INFO -  a=extmap:2 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
21:05:56     INFO -  a=extmap:3 foo
21:05:56     INFO -  a=extmap:4 bar
21:05:56     INFO -  a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=0
21:05:56     INFO -  a=ice-pwd:Offerer-1234567890
21:05:56     INFO -  a=ice-ufrag:Offerer-ufrag
21:05:56     INFO -  a=mid:sdparta_0
21:05:56     INFO -  a=msid:FAKE_UUID_1127 FAKE_UUID_1128
21:05:56     INFO -  a=rtcp-mux
21:05:56     INFO -  a=rtpmap:109 opus/48000/2
21:05:56     INFO -  a=rtpmap:9 G722/8000/1
21:05:56     INFO -  a=rtpmap:0 PCMU/8000
21:05:56     INFO -  a=rtpmap:8 PCMA/8000
21:05:56     INFO -  a=setup:actpass
21:05:56     INFO -  a=ssrc:1970559680 cname:FAKE_UUID_1124
21:05:56     INFO -  ANSWER: v=0
21:05:56     INFO -  o=mozilla...THIS_IS_SDPARTA-52.0a1 3116521854353999107 0 IN IP4 0.0.0.0
21:05:56     INFO -  s=-
21:05:56     INFO -  t=0 0
21:05:56     INFO -  a=fingerprint:sha-1 41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41
21:05:56     INFO -  a=fingerprint:sha-256 41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41:41
21:05:56     INFO -  a=ice-options:trickle
21:05:56     INFO -  a=msid-semantic:WMS *
21:05:56     INFO -  m=audio 9 UDP/TLS/RTP/SAVPF 109
21:05:56     INFO -  c=IN IP4 0.0.0.0
21:05:56     INFO -  a=sendrecv
21:05:56     INFO -  a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
21:05:56     INFO -  a=extmap:2 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
21:05:56     INFO -  a=extmap:4 bar
21:05:56     INFO -  a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=0
21:05:56     INFO -  a=ice-pwd:Answerer-1234567890
21:05:56     INFO -  a=ice-ufrag:Answerer-ufrag
21:05:56     INFO -  a=mid:sdparta_0
21:05:56     INFO -  a=msid:FAKE_UUID_1129 FAKE_UUID_1130
21:05:56     INFO -  a=rtcp-mux
21:05:56     INFO -  a=rtpmap:109 opus/48000/2
21:05:56     INFO -  a=setup:active
21:05:56     INFO -  a=ssrc:3851146584 cname:FAKE_UUID_1126
21:05:56     INFO -  Track pair 0
21:05:56     INFO -  Sending-->
21:05:56     INFO -    type=audio
21:05:56     INFO -    encodings=
21:05:56     INFO -      id=      opus
21:05:56     INFO -  Receiving-->
21:05:56     INFO -    type=audio
21:05:56     INFO -    encodings=
21:05:56     INFO -      id=      opus
21:05:56     INFO -  Track pair 0
21:05:56     INFO -  Sending-->
21:05:56     INFO -    type=audio
21:05:56     INFO -    encodings=
21:05:56     INFO -      id=      opus
21:05:56     INFO -  Receiving-->
21:05:56     INFO -    type=audio
21:05:56     INFO -    encodings=
21:05:56     INFO -      id=      opus
21:05:56     INFO -  /builds/slave/m-in-l64-000000000000000000000/build/src/media/webrtc/signaling/test/jsep_session_unittest.cpp:3493: Failure
21:05:56     INFO -  Value of: offerExtmap.size()
21:05:56     INFO -    Actual: 4
21:05:56     INFO -  Expected: 3U
21:05:56     INFO -  Which is: 3
21:05:56     INFO -  [  FAILED  ] JsepSessionTest.TestExtmap (7 ms)

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix jsep_session tests for extmap changes

Comment 25

[Nils Ohlmeier [:drno]]

Comment on attachment 8794551 [details] [diff] [review]
fix jsep_session tests for extmap changes

Review of attachment 8794551 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b9a320f8d7
Clean up RID header extension support r=jesup,drno

Comment 27

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/27b9a320f8d7

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8794409 [details] [diff] [review]
vid_rid_1_bug12988904.patch

This is for the set of patches as-landed and the followup patch in bug 1306873

Approval Request Comment
[Feature/regressing bug #]: RID support (new feature (simulcast) in 48)

[User impact if declined]: We would only do simulcast with SSRC differences and no RID sent to identify the streams.

[Describe test coverage new/current, TreeHerder]: RID is not accessible from JS code without some work,and receiving RID is not normally needed by a client, just a server.  We've used manual testing and wireshark to verify.

[Risks and why]: moderately-low risk.  A moderate amount of fairly simple new code/paths.  A lot of the change is name-changes, which makes it look larger.  Some of the RID paths in the RTP code we added previously will get exercised now.

[String/UUID change made/needed]: none

Comment 29

[Gerry Chang [:gchang]]

Hi :jesup,
Can you describe more specific about user impact if declined? What's the behavior?

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Gerry Chang [:gchang] from comment #29)
> Hi :jesup,
> Can you describe more specific about user impact if declined? What's the
> behavior?

Simulcast in WebRTC allows sending multiple resolutions/framerates of the same source.  They're supposed to be distinguished with a "RID" header-extension value, so the server (and SFU, selective-forwarding-unit) can forward the right stream without having to try to infer which of the simulcast streams it is.

Without this, RID won't be sent, and the SFU won't work, or will have to fall back on heuristics to infer the identity of each stream, which would only be distinguished by SSRC.

Since we want to push forward the implementation of SFUs and RID, we really want Firefox to properly support it.

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8794409 [details] [diff] [review]
vid_rid_1_bug12988904.patch

Fix WebRTC issue related to Simulcast. Take it in 51 aurora.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2235c46ad1be