Closed Bug 1288904 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: mreavy, Assigned: jesup)

References

Details

Attachments

(5 files, 2 obsolete files)

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
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.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)
Whiteboard: [needinfo 2016/07/22 to reporter/bwc/drno]
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.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(docfaraday) → needinfo?(rjesup)
(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?
Looks like Byron is on it.
Flags: needinfo?(drno)
Rank: 21
Priority: -- → P2
(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.
(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.
Flags: needinfo?(rjesup)
(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.
Attached file log file
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
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
(In reply to Nico Grunbaum [:ng] from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e99297162da4

Added missing header, retrying on windows.
Attached patch video_rid_bug12988904.patch (obsolete) — Splinter Review
Attachment #8792669 - Flags: review?(rjesup)
Whiteboard: [needinfo 2016/07/22 to reporter/bwc/drno]
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?
Attachment #8792669 - Flags: review?(rjesup) → review+
Attached patch vid_rid_bug12988904.patch (obsolete) — Splinter Review
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.
Attachment #8792669 - Attachment is obsolete: true
Attachment #8792785 - Flags: review?(rjesup)
Attachment #8792785 - Flags: review?(rjesup) → review+
See interdiff for changes.
Attachment #8792785 - Attachment is obsolete: true
Attachment #8794409 - Flags: review?(rjesup)
Attachment #8794409 - Flags: review?(rjesup) → review+
Attachment #8794409 - Flags: checkin+
Keywords: checkin-needed
Attachment #8794409 - Flags: checkin+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cb69423014
Clean up RID header extension support r=jesup
Keywords: checkin-needed
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)
Flags: needinfo?(na-g)
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Comment on attachment 8794551 [details] [diff] [review]
fix jsep_session tests for extmap changes

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

LGTM
Attachment #8794551 - Flags: review?(drno) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b9a320f8d7
Clean up RID header extension support r=jesup,drno
https://hg.mozilla.org/mozilla-central/rev/27b9a320f8d7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1305813
Flags: needinfo?(na-g)
Blocks: 1306873
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
Attachment #8794409 - Flags: approval-mozilla-aurora?
Hi :jesup,
Can you describe more specific about user impact if declined? What's the behavior?
Flags: needinfo?(rjesup)
(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.
Flags: needinfo?(rjesup) → needinfo?(gchang)
Comment on attachment 8794409 [details] [diff] [review]
vid_rid_1_bug12988904.patch

Fix WebRTC issue related to Simulcast. Take it in 51 aurora.
Flags: needinfo?(gchang)
Attachment #8794409 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.