New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
NextAction: ----
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

Issue description

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 (was: Assigned)
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 (was: Available)
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 2017

Is there any more work planned for this?
Project Member

Comment 19 by nisse@webrtc.org, Mar 8 2017

Labels: M-59
Status: Fixed (was: Started)
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 2017

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 2017

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