Project: webrtc Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 3 users
Status: Fixed
Owner:
Closed: Mar 8
Cc:
Components:
OS: ----
Pri: 2
Type: Enhancement



Sign in to add a comment
Remove shared global SSRC database
Project Member Reported by pbos@webrtc.org, Feb 16 2015 Back to list
Remove SSRCDatabase from RTPSender.

SSRCDatabase uses a static instance that shares SSRCs globally. Work is
ongoing to remove static instances that aren't neccessary. SSRCs should
be negotiated outside of the RTP module either way, so having a global
database just complicates the code.

Initial CL (not at all done) here: https://review.webrtc.org/39969004/
 
Project Member Comment 1 by pbos@webrtc.org, Oct 6 2015
Cc: solenberg@webrtc.org
+solenberg, this couldawouldashoulda (needs to) make sure that all SSRCs on VoiceEngine are explicitly configured from the outside, preferably on RTP module construction, but not mandatory.
Project Member Comment 2 by pbos@webrtc.org, Apr 26 2016
Cc: pbos@webrtc.org
Owner: ----
Status: Available
Project Member Comment 3 by solenberg@webrtc.org, Apr 26 2016
IIRC, on the audio side, SSRCs are explicitly configured in all but one case - for outgoing RTCP in the case where we're only receiving. In that case a random SSRC will be used for the sender (until a send stream is also configured, which will then "take over" the RTCP stream).

Since we only have one outgoing stream in this case, I guess the SSRC could just as well be randomized? Is that what you do on the video side?

Stefan - do you have any input?

Project Member Comment 4 by pbos@webrtc.org, Apr 26 2016
We have no default outgoing stream for video as far as I'm aware.
Project Member Comment 5 by solenberg@webrtc.org, Apr 26 2016
No RRs being sent?
Project Member Comment 6 by pbos@webrtc.org, Apr 26 2016
Ah, we just set it to 1 by default, which is probably incorrect, but a valid "random" value, for some really crappy PRNG.

https://chromium.googlesource.com/external/webrtc/+/555604a7463c6a6677f54aff340e7852a16b3c58/webrtc/media/engine/webrtcvideoengine2.cc#686
Project Member Comment 7 by anatolid@webrtc.org, Dec 11 2016
Components: Video
Project Member Comment 8 by solenberg@webrtc.org, Jan 20 2017
Cc: nisse@webrtc.org
Project Member Comment 9 by nisse@webrtc.org, Jan 20 2017
Owner: nisse@webrtc.org
Status: Started
I'm going to have a look at this. I hope there should be code elsewhere which keeps track of ssrc allocation, so we can just delete this.

For the case in comment #3, would it work for the time being to use a hard-coded ssrc, as is done for video?

We aim to design a class which is responsible for all RTP transport to one endpoint. When we get there, that class should know which ssrcs are in use, and it should be straight forward with a method to return a random unused ssrc.
Project Member Comment 10 by solenberg@webrtc.org, Jan 20 2017
For the PeerConnection use case, the SSRC is already hard coded: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/engine/webrtcvoiceengine.h#275

The situation where the SSRC database could still play role is for clients using the VoE interfaces directly. E.g. it is perfectly possible to create voice channels and never explicitly set SSRC via VoERTP_RTCP::SetLocalSSRC(), even for channels used for sending.

Project Member Comment 11 by nisse@webrtc.org, Jan 20 2017
So if we need to generate a random ssrc for that case, where should that be done?

We could let the constructor for webrtc::voe::Channel generate a random number in some way and call SetSSRC. In this use case, is there any other object which knows all ssrcs/channels/rtp modules? We'd need that to avoid accidental ssrc collisions.

A less drastic change might be to keep SSRCDatabase, but pass it in RtpRtcpConfiguration intead of using a global. But it would be nicer to remove all SSRC allocation and book-keeping from the rtp module.
Project Member Comment 12 by nisse@webrtc.org, Jan 20 2017
Hmm, looks like the voe::ChannelManager::CreateChannel could generate a random ssrc and check for collisions with other channels. Does that sound reasonable?

BTW, can/should the ChannelOwner class be replaced with rtc::scoped_refptr<Channel>?
Project Member Comment 13 by solenberg@webrtc.org, Jan 20 2017
1. ChannelManager sounds like the right place for this functionality.

2. Not right now.
Project Member Comment 14 by tommi@webrtc.org, Jan 22 2017
If ChannelOwner is being modified then there are threading issues (and
lifetime iirc) with that design that would be great if we could address at
the same time.
Project Member Comment 15 by bugdroid1@chromium.org, Feb 17 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/b78d4d13835f628e722a57abae2bf06ba3655921

commit b78d4d13835f628e722a57abae2bf06ba3655921
Author: nisse <nisse@webrtc.org>
Date: Fri Feb 17 16:34:35 2017

Delete class SSRCDatabase, and its global ssrc registry,
and the method RTPSender::GenerateNewSSRC.

It's now mandatory for higher layers to call SetSSRC, RTPSender
no longer allocates any ssrc by default.

BUG= webrtc:4306 ,webrtc:6887

Review-Url: https://codereview.webrtc.org/2644303002
Cr-Commit-Position: refs/heads/master@{#16670}

[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/logging/rtc_event_log/rtc_event_log.proto
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/modules/rtp_rtcp/BUILD.gn
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/modules/rtp_rtcp/source/rtp_sender.h
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
[delete] https://crrev.com/1088248a4e6151dfbe8cc733fd771a79a80ad19d/webrtc/modules/rtp_rtcp/source/ssrc_database.cc
[delete] https://crrev.com/1088248a4e6151dfbe8cc733fd771a79a80ad19d/webrtc/modules/rtp_rtcp/source/ssrc_database.h
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/voice_engine/channel.cc
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/voice_engine/channel_manager.cc
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/voice_engine/channel_manager.h
[modify] https://crrev.com/b78d4d13835f628e722a57abae2bf06ba3655921/webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_test.cc

Project Member Comment 16 by bugdroid1@chromium.org, Feb 18 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/b5848ecbf5f7b310108546ec6b858fe93452f58e

commit b5848ecbf5f7b310108546ec6b858fe93452f58e
Author: kjellander <kjellander@google.com>
Date: Sat Feb 18 20:00:50 2017

Revert of Delete class SSRCDatabase, and its global ssrc registry. (patchset #20 id:370001 of https://codereview.webrtc.org/2644303002/ )

Reason for revert:
Breaks webrtc_perf_tests reliably:
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20%28L%20Nexus5%29/builds/1780
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20%28L%20Nexus4%29/builds/178

We're actively working on getting a quick version of webrtc_perf_tests up on the trybots again to prevent breakages like this: https://bugs.chromium.org/p/webrtc/issues/detail?id=7101

Original issue's description:
> Delete class SSRCDatabase, and its global ssrc registry,
> and the method RTPSender::GenerateNewSSRC.
>
> It's now mandatory for higher layers to call SetSSRC, RTPSender
> no longer allocates any ssrc by default.
>
> BUG= webrtc:4306 ,webrtc:6887
>
> Review-Url: https://codereview.webrtc.org/2644303002
> Cr-Commit-Position: refs/heads/master@{#16670}
> Committed: https://chromium.googlesource.com/external/webrtc/+/b78d4d13835f628e722a57abae2bf06ba3655921

TBR=solenberg@webrtc.org,stefan@webrtc.org,danilchap@webrtc.org,ivoc@webrtc.org,nisse@webrtc.org
NOTRY=True
BUG= webrtc:4306 ,webrtc:6887

Review-Url: https://codereview.webrtc.org/2700413002
Cr-Commit-Position: refs/heads/master@{#16693}

[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/logging/rtc_event_log/rtc_event_log.proto
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/BUILD.gn
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/rtp_sender.h
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
[add] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/ssrc_database.cc
[add] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/modules/rtp_rtcp/source/ssrc_database.h
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/voice_engine/channel.cc
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/voice_engine/channel_manager.cc
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/voice_engine/channel_manager.h
[modify] https://crrev.com/b5848ecbf5f7b310108546ec6b858fe93452f58e/webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_test.cc

Project Member Comment 17 by bugdroid1@chromium.org, Feb 21 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/7d59f6b1c46cf551d2b55849ed061756ba25f1b0

commit 7d59f6b1c46cf551d2b55849ed061756ba25f1b0
Author: nisse <nisse@webrtc.org>
Date: Tue Feb 21 11:40:24 2017

Reland of Delete class SSRCDatabase, and its global ssrc registry. (patchset #1 id:1 of https://codereview.webrtc.org/2700413002/ )

Reason for revert:
Intend to fix perf problem and reland.

Original issue's description:
> Revert of Delete class SSRCDatabase, and its global ssrc registry. (patchset #20 id:370001 of https://codereview.webrtc.org/2644303002/ )
>
> Reason for revert:
> Breaks webrtc_perf_tests reliably:
> https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20%28L%20Nexus5%29/builds/1780
> https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20%28L%20Nexus4%29/builds/178
>
> We're actively working on getting a quick version of webrtc_perf_tests up on the trybots again to prevent breakages like this: https://bugs.chromium.org/p/webrtc/issues/detail?id=7101
>
> Original issue's description:
> > Delete class SSRCDatabase, and its global ssrc registry,
> > and the method RTPSender::GenerateNewSSRC.
> >
> > It's now mandatory for higher layers to call SetSSRC, RTPSender
> > no longer allocates any ssrc by default.
> >
> > BUG= webrtc:4306 ,webrtc:6887
> >
> > Review-Url: https://codereview.webrtc.org/2644303002
> > Cr-Commit-Position: refs/heads/master@{#16670}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/b78d4d13835f628e722a57abae2bf06ba3655921
>
> TBR=solenberg@webrtc.org,stefan@webrtc.org,danilchap@webrtc.org,ivoc@webrtc.org,nisse@webrtc.org
> NOTRY=True
> BUG= webrtc:4306 ,webrtc:6887
>
> Review-Url: https://codereview.webrtc.org/2700413002
> Cr-Commit-Position: refs/heads/master@{#16693}
> Committed: https://chromium.googlesource.com/external/webrtc/+/b5848ecbf5f7b310108546ec6b858fe93452f58e

TBR=solenberg@webrtc.org,stefan@webrtc.org,danilchap@webrtc.org,ivoc@webrtc.org,kjellander@webrtc.org,kjellander@google.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= webrtc:4306 ,webrtc:6887

Review-Url: https://codereview.webrtc.org/2702203002
Cr-Commit-Position: refs/heads/master@{#16737}

[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/logging/rtc_event_log/rtc_event_log.proto
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/BUILD.gn
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/source/rtp_sender.h
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
[delete] https://crrev.com/531100dc7a9c58d8cbe200cf3a65d1254070861d/webrtc/modules/rtp_rtcp/source/ssrc_database.cc
[delete] https://crrev.com/531100dc7a9c58d8cbe200cf3a65d1254070861d/webrtc/modules/rtp_rtcp/source/ssrc_database.h
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/voice_engine/channel.cc
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/voice_engine/channel_manager.cc
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/voice_engine/channel_manager.h
[modify] https://crrev.com/7d59f6b1c46cf551d2b55849ed061756ba25f1b0/webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_test.cc

Project Member Comment 18 by anatolid@webrtc.org, Mar 6
Is there any more work planned for this?
Project Member Comment 19 by nisse@webrtc.org, Mar 8
Labels: M-59
Status: Fixed
Closing, and setting M-59 milestone (it didn't make it to M-58, if I read https://chromiumdash.appspot.com/commits correctly).
Project Member Comment 20 by anatolid@chromium.org, Mar 21
Labels: -M-59 M-58
Setting to M-58 since last CL (in #17) from this bug landed in M-58. Nisse, why do you think it didn't make it to M-58?
Project Member Comment 21 by nisse@webrtc.org, Mar 22
I was confused by chromiumdash. When I followed the instructions and logged in using my chromium account, I couldn't find the cl. When I retry now, and sign out and login using my webrtc account instead, I get a list of my webrtc commits, and I agree the cl made it to M-58.

Sign in to add a comment