Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 2644303002: Delete class SSRCDatabase, and its global ssrc registry. (Closed)

Created:
10 months ago by nisse-webrtc
Modified:
9 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman, tomasl_google.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

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

Patch Set 1 #

Patch Set 2 : Make tests set SSRC explicitly. #

Patch Set 3 : Delete GenerateNewSSRC. #

Patch Set 4 : Delete logic related to ssrc collisions. #

Total comments: 2

Patch Set 5 : Delete RTPSender::SetSendingStatus (it used to generate a new ssrc). #

Total comments: 17

Patch Set 6 : Addressed comments. #

Total comments: 1

Patch Set 7 : Add DCHECK to RtxSsrc. #

Patch Set 8 : Rebased. #

Patch Set 9 : Don't access SSRC of newly constructed RTPSender. #

Patch Set 10 : Rebased. #

Total comments: 2

Patch Set 11 : Fix comment typo. #

Total comments: 2

Patch Set 12 : Let webrtc:voe::Channel::CreateChannel generate a random ssrc. #

Patch Set 13 : Switch to using base/random.h. #

Patch Set 14 : Increase delay for RtpRtcpTest.RemoteRtcpCnameHasPropagatedToRemoteSide. #

Total comments: 6

Patch Set 15 : Logging and DCHECK fixes. #

Patch Set 16 : DCHECK that ssrc is set in SendOutgoingData. #

Total comments: 4

Patch Set 17 : Move VoE setting of random ssrc from Channel to ChannelManager. #

Total comments: 8

Patch Set 18 : Use remote ssrc for LogAudioPlayout. Add .proto TODO item. #

Patch Set 19 : Rebased. #

Patch Set 20 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -230 lines) Patch
M webrtc/logging/rtc_event_log/rtc_event_log.proto View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +0 lines, -1 line 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +0 lines, -37 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender.h View 1 2 3 4 5 6 7 8 9 10 5 chunks +4 lines, -8 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 21 chunks +41 lines, -63 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 7 chunks +7 lines, -1 line 0 comments Download
D webrtc/modules/rtp_rtcp/source/ssrc_database.h View 1 chunk +0 lines, -62 lines 0 comments Download
D webrtc/modules/rtp_rtcp/source/ssrc_database.cc View 1 chunk +0 lines, -51 lines 0 comments Download
M webrtc/voice_engine/channel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
M webrtc/voice_engine/channel_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +4 lines, -0 lines 0 comments Download
M webrtc/voice_engine/channel_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -1 line 0 comments Download
M webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -3 lines 0 comments Download

Messages

Total messages: 76 (34 generated)
nisse-webrtc
This passes some initial testing now. Remaining issues: Add back random-default-ssrc feature to VoE ChannelManager ...
10 months ago (2017-01-20 10:50:16 UTC) #2
nisse-webrtc
On 2017/01/20 10:50:16, nisse-webrtc wrote: > This passes some initial testing now. Remaining issues: > ...
9 months, 3 weeks ago (2017-01-31 14:22:23 UTC) #4
nisse-webrtc
And this points out the place where SetSendingStatus is called. https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (left): https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc#oldcode361 ...
9 months, 3 weeks ago (2017-01-31 14:23:42 UTC) #5
nisse-webrtc
danil: Please have a look at these changes. https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (left): https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc#oldcode361 webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:361: // ...
9 months, 3 weeks ago (2017-02-01 10:52:50 UTC) #7
danilchap
nice to see ongoing work on removing this code! https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (left): https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc#oldcode358 webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:358: ...
9 months, 3 weeks ago (2017-02-01 12:23:56 UTC) #8
nisse-webrtc
https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_sender.cc File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_sender.cc#newcode320 webrtc/modules/rtp_rtcp/source/rtp_sender.cc:320: ssrc_rtx_ = rtc::Optional<uint32_t>(ssrc); On 2017/02/01 12:23:55, danilchap wrote: > ...
9 months, 3 weeks ago (2017-02-01 14:00:51 UTC) #9
the sun
I believe there used to be a bot suppression for the SSRCDatabase, but now I ...
9 months, 3 weeks ago (2017-02-01 14:15:20 UTC) #10
nisse-webrtc
One problem is this call to SSRC, statistics_proxy_.reset(new StatisticsProxy(_rtpRtcpModule->SSRC())); see https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel.cc?q=voe::channel&sq=package:chromium&dr=Ss&l=967 This is in the ...
9 months, 2 weeks ago (2017-02-02 12:06:44 UTC) #11
the sun
lgtm https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/source/rtp_sender.h File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/source/rtp_sender.h#newcode308 webrtc/modules/rtp_rtcp/source/rtp_sender.h:308: // Must be explicitly set by the application, ...
9 months, 1 week ago (2017-02-14 12:34:11 UTC) #12
nisse-webrtc
Danil, Stefan: Any comments? The code blocking this changes is now deleted. https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/source/rtp_sender.h File webrtc/modules/rtp_rtcp/source/rtp_sender.h ...
9 months, 1 week ago (2017-02-14 13:02:03 UTC) #13
danilchap
lgtm This CL should also solve webrtc::6887 issue, can you add it to the description? ...
9 months, 1 week ago (2017-02-14 13:23:05 UTC) #18
nisse-webrtc
voe_auto_test fails in local testing too, most likely because it doesn't explicitly configure the ssrcs ...
9 months, 1 week ago (2017-02-14 13:44:21 UTC) #20
nisse-webrtc
On 2017/02/14 13:44:21, nisse-webrtc wrote: > voe_auto_test fails in local testing too, most likely because ...
9 months, 1 week ago (2017-02-14 14:16:36 UTC) #21
nisse-webrtc
On 2017/02/14 14:16:36, nisse-webrtc wrote: > On 2017/02/14 13:44:21, nisse-webrtc wrote: > > voe_auto_test fails ...
9 months, 1 week ago (2017-02-14 15:04:09 UTC) #22
stefan-webrtc
https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/source/rtp_sender.cc File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/source/rtp_sender.cc#newcode393 webrtc/modules/rtp_rtcp/source/rtp_sender.cc:393: LOG(LS_ERROR) << "SSRC unset"; End with . Same for ...
9 months, 1 week ago (2017-02-15 08:26:37 UTC) #29
nisse-webrtc
https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/source/rtp_sender.cc File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/source/rtp_sender.cc#newcode393 webrtc/modules/rtp_rtcp/source/rtp_sender.cc:393: LOG(LS_ERROR) << "SSRC unset"; On 2017/02/15 08:26:37, stefan-webrtc wrote: ...
9 months, 1 week ago (2017-02-15 08:42:03 UTC) #30
stefan-webrtc
lgtm
9 months, 1 week ago (2017-02-15 08:43:24 UTC) #31
nisse-webrtc
+ tomasl Added a DCHECK to detect more problems with unset ssrc.
9 months ago (2017-02-16 10:36:59 UTC) #36
the sun
lgtm % comments https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/channel.cc#newcode939 webrtc/voice_engine/channel.cc:939: SetLocalSSRC(random_.Rand<uint32_t>()); Leave a TODO for me ...
9 months ago (2017-02-16 12:34:16 UTC) #39
nisse-webrtc
https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/channel.cc#newcode939 webrtc/voice_engine/channel.cc:939: SetLocalSSRC(random_.Rand<uint32_t>()); On 2017/02/16 12:34:16, the sun wrote: > Leave ...
9 months ago (2017-02-16 12:58:16 UTC) #40
the sun
On 2017/02/16 12:58:16, nisse-webrtc wrote: > https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/channel.cc > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/channel.cc#newcode939 > ...
9 months ago (2017-02-16 13:00:34 UTC) #41
nisse-webrtc
On 2017/02/16 13:00:34, the sun wrote: > On 2017/02/16 12:58:16, nisse-webrtc wrote: > > Should ...
9 months ago (2017-02-16 13:12:51 UTC) #44
the sun
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); Hang on - RtcEventLog will have the same ...
9 months ago (2017-02-16 13:52:32 UTC) #46
terelius
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:52:32, the sun wrote: > Hang ...
9 months ago (2017-02-16 13:58:05 UTC) #48
nisse-webrtc
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:52:32, the sun wrote: > Hang ...
9 months ago (2017-02-16 14:01:59 UTC) #49
terelius
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:58:05, terelius wrote: > On 2017/02/16 ...
9 months ago (2017-02-16 14:04:43 UTC) #50
ivoc
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 14:04:42, terelius wrote: > On 2017/02/16 ...
9 months ago (2017-02-16 14:27:07 UTC) #51
nisse-webrtc
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 14:27:06, ivoc wrote: > On 2017/02/16 ...
9 months ago (2017-02-16 14:29:22 UTC) #52
ivoc
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 14:29:22, nisse-webrtc wrote: > On 2017/02/16 ...
9 months ago (2017-02-16 14:35:39 UTC) #53
the sun
On 2017/02/16 14:29:22, nisse-webrtc wrote: > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 > ...
9 months ago (2017-02-16 14:36:36 UTC) #54
the sun
On 2017/02/16 14:35:39, ivoc wrote: > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 > ...
9 months ago (2017-02-16 14:39:52 UTC) #55
ivoc
On 2017/02/16 14:39:52, the sun wrote: > On 2017/02/16 14:35:39, ivoc wrote: > > > ...
9 months ago (2017-02-16 14:48:45 UTC) #56
hlundin-webrtc
On 2017/02/16 14:48:45, ivoc wrote: > On 2017/02/16 14:39:52, the sun wrote: > > On ...
9 months ago (2017-02-16 14:58:19 UTC) #58
terelius
On 2017/02/16 14:48:45, ivoc wrote: > On 2017/02/16 14:39:52, the sun wrote: > > On ...
9 months ago (2017-02-16 15:00:46 UTC) #60
ivoc
On 2017/02/16 15:00:46, terelius wrote: > On 2017/02/16 14:48:45, ivoc wrote: > > On 2017/02/16 ...
9 months ago (2017-02-16 15:06:14 UTC) #61
the sun
On 2017/02/16 15:06:14, ivoc wrote: > On 2017/02/16 15:00:46, terelius wrote: > > On 2017/02/16 ...
9 months ago (2017-02-16 15:51:18 UTC) #62
nisse-webrtc
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/channel.cc#newcode579 webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:52:32, the sun wrote: > Hang ...
9 months ago (2017-02-17 09:59:47 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2644303002/350001
9 months ago (2017-02-17 11:27:14 UTC) #67
commit-bot: I haz the power
Failed to apply patch for webrtc/logging/rtc_event_log/rtc_event_log.proto: While running git apply --index -p1; error: patch failed: ...
9 months ago (2017-02-17 13:04:22 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2644303002/370001
9 months ago (2017-02-17 13:39:58 UTC) #72
commit-bot: I haz the power
Committed patchset #20 (id:370001) as https://chromium.googlesource.com/external/webrtc/+/b78d4d13835f628e722a57abae2bf06ba3655921
9 months ago (2017-02-17 16:34:43 UTC) #75
kjellander (google.com)
9 months ago (2017-02-18 19:50:11 UTC) #76
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:370001) has been created in
https://codereview.webrtc.org/2700413002/ by kjellander@google.com.

The reason for reverting is: Breaks webrtc_perf_tests reliably:
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20...
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20...

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.

Powered by Google App Engine
This is Rietveld efc10ee0f