Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644303002:
Delete class SSRCDatabase, and its global ssrc registry. (Closed)
|
Created:
3 years, 11 months ago by nisse-webrtc Modified:
3 years, 10 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. |
DescriptionDelete 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. #Messages
Total messages: 76 (34 generated)
nisse@webrtc.org changed reviewers: + solenberg@webrtc.org, stefan@webrtc.org
This passes some initial testing now. Remaining issues: Add back random-default-ssrc feature to VoE ChannelManager (unless we decide to stop supporting it after M57). Consider deleting the GenerateNewSSRC method. We could keep it and let it generate a random number, but responsibility for checking for collisions would have to be elsewhere. Consider adding the ssrc to the RtpSender constructor, now that explicit configuration is mandatory. We could then drop the rtc::Optional which would make the code a little cleaner.
Description was changed from ========== Delete class SSRCDatabase, and its global ssrc registry. BUG=webrtc:4306 ========== to ========== Delete class SSRCDatabase, and its global ssrc registry, and the method RTPSender::GenerateNewSSRC. It's now mandatory for upper layers to call SetSSRC, RTPSender no longer allocates a default ssrc. This breaks the old VoE api, which is going to be deleted. BUG=webrtc:4306 ==========
On 2017/01/20 10:50:16, nisse-webrtc wrote: > This passes some initial testing now. Remaining issues: > > Add back random-default-ssrc feature to VoE ChannelManager (unless we decide to > stop supporting it after M57). > > Consider deleting the GenerateNewSSRC method. We could keep it and let it > generate a random number, but responsibility for checking for collisions would > have to be elsewhere. > > Consider adding the ssrc to the RtpSender constructor, now that explicit > configuration is mandatory. We could then drop the rtc::Optional which would > make the code a little cleaner. I've tried to clean this up and make it ready. I lean towards not adding an ssrc to the RTPSender constructor now, that can be a followup change, and it already has a bit too many arguments... I have broken RTPSender::SetSendingStatus, in that it no longer allocates a new ssrc. Can anyone explain if that features was used, and where?
And this points out the place where SetSendingStatus is called. https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (left): https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:361: // Generate a new SSRC for the next "call" if false This is no longer true, the SetSendingStatus is a nop. Is this feature used for anything? If we need to select a new ssrc, where should that allocation move?
nisse@webrtc.org changed reviewers: + danilchap@webrtc.org
danil: Please have a look at these changes. https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (left): https://codereview.webrtc.org/2644303002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:361: // Generate a new SSRC for the next "call" if false On 2017/01/31 14:23:42, nisse-webrtc wrote: > This is no longer true, the SetSendingStatus is a nop. > > Is this feature used for anything? If we need to select a new ssrc, where should > that allocation move? I just deleted this function and the below logic. It was used only for automatic ssrc selection.
nice to see ongoing work on removing this code! https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (left): https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:358: I also think this block is obsolete, tried remove it myself for sligtly different reason (https://codereview.webrtc.org/2660253002/) but that turn one bot red. i.e. pay attention to android_arm64_rel bot since you did this. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:320: ssrc_rtx_ = rtc::Optional<uint32_t>(ssrc); ssrc_rtx_.emplace(ssrc); is another, shorter syntax to put value into optional. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:325: // TODO(nisse): Is it better to crash if ssrc_rtx_ is unset? check how RtxSsrc is used. It might be better to crash, might be better to reteurn optional and make caller to decide what to do with absent value. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:985: packet->SetSsrc(ssrc_.value_or(0)); there is a DCHECK just above, do not try to recover, write *ssrc_ https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1004: RTC_DCHECK_EQ(packet->Ssrc(), *ssrc_); RTC_DCHECK_EQ(packet->Ssrc(), ssrc_) should work. it is an error to call AssignSequenceNumber before ssrc was assigned. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1060: if (ssrc_ && *ssrc_== ssrc) { if (ssrc_ == ssrc) (it is legal to compare optional with value) https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1063: ssrc_ = rtc::Optional<uint32_t>(ssrc); ssrc_.emplace(ssrc) https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1071: // TODO(nisse): Is it better to crash if ssrc_ is unset? likely better to crash than return unusable value. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1151: if (!ssrc_rtx_) feel free to DCHECK: no point to try to build rtx packet when rtx is not configured.
https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... 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: > ssrc_rtx_.emplace(ssrc); is another, shorter syntax to put value into optional. Done. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:325: // TODO(nisse): Is it better to crash if ssrc_rtx_ is unset? On 2017/02/01 12:23:55, danilchap wrote: > check how RtxSsrc is used. It might be better to crash, might be better to > reteurn optional and make caller to decide what to do with absent value. There's a single call, in ModuleRtpRtcpImpl::SetRtcpReceiverSsrcs. After a check of RtxStatus() != kRtxOff. And SetRtxSendStatus and SetRtxSsrc are called in VideoSendStreamImpl::ConfigureSsrcs. So hopefully, we can trust that code to not enable Rtx without configuring an ssrc. I think this could be simplified. But for this cl, it might make sense to just add a DCHECK. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:985: packet->SetSsrc(ssrc_.value_or(0)); On 2017/02/01 12:23:55, danilchap wrote: > there is a DCHECK just above, do not try to recover, write *ssrc_ Done. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1004: RTC_DCHECK_EQ(packet->Ssrc(), *ssrc_); On 2017/02/01 12:23:55, danilchap wrote: > RTC_DCHECK_EQ(packet->Ssrc(), ssrc_) > should work. > it is an error to call AssignSequenceNumber before ssrc was assigned. Comparing with RTC_DCHECK_EQ fails. But RTC_DCHECK(packet->Ssrc() == ssrc_) works. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1060: if (ssrc_ && *ssrc_== ssrc) { On 2017/02/01 12:23:56, danilchap wrote: > if (ssrc_ == ssrc) > (it is legal to compare optional with value) Done, I didn't know that was allowed. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1063: ssrc_ = rtc::Optional<uint32_t>(ssrc); On 2017/02/01 12:23:55, danilchap wrote: > ssrc_.emplace(ssrc) Done. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1071: // TODO(nisse): Is it better to crash if ssrc_ is unset? On 2017/02/01 12:23:55, danilchap wrote: > likely better to crash than return unusable value. Done. https://codereview.webrtc.org/2644303002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1151: if (!ssrc_rtx_) On 2017/02/01 12:23:55, danilchap wrote: > feel free to DCHECK: no point to try to build rtx packet when rtx is not > configured. Done.
I believe there used to be a bot suppression for the SSRCDatabase, but now I can't seem to find it. https://codereview.webrtc.org/2644303002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:325: // TODO(nisse): Is it better to crash if ssrc_rtx_ is unset? You could at least add a DCHECK.
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.... This is in the voe::Channel constructor, before SetLocalSSRC is called. I lso wonder what the StatisticsProxy use the ssrc for? As far as I see, it's not updated if SetLocalSSRC is called later. Advice?
lgtm https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:308: // Must be explicitly set by the application, use of rtc:Optional nit: "::" in rtc:Optional
Danil, Stefan: Any comments? The code blocking this changes is now deleted. https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2644303002/diff/170001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:308: // Must be explicitly set by the application, use of rtc:Optional On 2017/02/14 12:34:11, the sun wrote: > nit: "::" in rtc:Optional Done.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL)
lgtm This CL should also solve webrtc::6887 issue, can you add it to the description? https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:330: return ssrc_rtx_.value_or(0); there is a DCHECK above, so no need to handle empty optional case: return *ssrc_rtx_;
Description was changed from ========== Delete class SSRCDatabase, and its global ssrc registry, and the method RTPSender::GenerateNewSSRC. It's now mandatory for upper layers to call SetSSRC, RTPSender no longer allocates a default ssrc. This breaks the old VoE api, which is going to be deleted. BUG=webrtc:4306 ========== to ========== Delete class SSRCDatabase, and its global ssrc registry, and the method RTPSender::GenerateNewSSRC. It's now mandatory for upper layers to call SetSSRC, RTPSender no longer allocates a default ssrc. This breaks the old VoE api, which is going to be deleted. BUG=webrtc:4306,webrtc:6887 ==========
voe_auto_test fails in local testing too, most likely because it doesn't explicitly configure the ssrcs to use. Should I try to fix that, or just wait for the code to be deleted? https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:330: return ssrc_rtx_.value_or(0); On 2017/02/14 13:23:05, danilchap wrote: > there is a DCHECK above, so no need to handle empty optional case: > return *ssrc_rtx_; Done. (Then the DCHECK is a bit redundant too, we'll always crash one way or the other if the member is unset. But let's keep the DCHECK for clarity.)
On 2017/02/14 13:44:21, nisse-webrtc wrote: > voe_auto_test fails in local testing too, most likely because it doesn't > explicitly configure the ssrcs to use. Should I try to fix that, or just wait > for the code to be deleted? > > https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): > > https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtp_sender.cc:330: return ssrc_rtx_.value_or(0); > On 2017/02/14 13:23:05, danilchap wrote: > > there is a DCHECK above, so no need to handle empty optional case: > > return *ssrc_rtx_; > > Done. (Then the DCHECK is a bit redundant too, we'll always crash one way or the > other if the member is unset. But let's keep the DCHECK for clarity.) Trying a hack to fix voe tests. We'll see if it is enough.
On 2017/02/14 14:16:36, nisse-webrtc wrote: > On 2017/02/14 13:44:21, nisse-webrtc wrote: > > voe_auto_test fails in local testing too, most likely because it doesn't > > explicitly configure the ssrcs to use. Should I try to fix that, or just wait > > for the code to be deleted? > > > > > https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... > > File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): > > > > > https://codereview.webrtc.org/2644303002/diff/190001/webrtc/modules/rtp_rtcp/... > > webrtc/modules/rtp_rtcp/source/rtp_sender.cc:330: return > ssrc_rtx_.value_or(0); > > On 2017/02/14 13:23:05, danilchap wrote: > > > there is a DCHECK above, so no need to handle empty optional case: > > > return *ssrc_rtx_; > > > > Done. (Then the DCHECK is a bit redundant too, we'll always crash one way or > the > > other if the member is unset. But let's keep the DCHECK for clarity.) > > Trying a hack to fix voe tests. We'll see if it is enough. The test RtpRtcpTest.RemoteRtcpCnameHasPropagatedToRemoteSide still failing. Possibly this cl could make it ask for the wrong ssrc, but when I stepped through Channel::GetRemoteRTCP_CNAME in gdb, it got the right value. And if I increase the timeout from 1 to 10 seconds it succeeds locally.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/16209)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:393: LOG(LS_ERROR) << "SSRC unset"; End with . Same for the logs below. Should we also DCHECK? Seems like incorrect usage to me. https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1021: RTC_DCHECK(packet->Ssrc() == ssrc_); DCHECK_EQ exists, right? https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1088: return *ssrc_; DCHECK that ssrc_ is set here first.
https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:393: LOG(LS_ERROR) << "SSRC unset"; On 2017/02/15 08:26:37, stefan-webrtc wrote: > End with . > > Same for the logs below. Done for all log messages in this file. > Should we also DCHECK? Seems like incorrect usage to me. Maybe. I'm aiming for making the ssrc a construction time const, but we're not there yet. https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1021: RTC_DCHECK(packet->Ssrc() == ssrc_); On 2017/02/15 08:26:37, stefan-webrtc wrote: > DCHECK_EQ exists, right? Tried it earlier, but it doesn't support comparing an optional to a value. https://codereview.webrtc.org/2644303002/diff/250001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1088: return *ssrc_; On 2017/02/15 08:26:37, stefan-webrtc wrote: > DCHECK that ssrc_ is set here first. Added here, and a couple of other places dereferencing ssrc_ or ssrc_rtx_.
lgtm
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+ tomasl Added a DCHECK to detect more problems with unset ssrc.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm % comments https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:939: SetLocalSSRC(random_.Rand<uint32_t>()); Leave a TODO for me to remove this. https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:524: webrtc::Random random_; You're only using this member in the ctor, why do you need it here?
https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:939: SetLocalSSRC(random_.Rand<uint32_t>()); On 2017/02/16 12:34:16, the sun wrote: > Leave a TODO for me to remove this. Will do in next upload. https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:524: webrtc::Random random_; On 2017/02/16 12:34:16, the sun wrote: > You're only using this member in the ctor, why do you need it here? Good catch. I think I originally put it in the ChannelManager, and then that didn't seem to work out. Should I move it to ChannelManager, and move the SetLocalSsrc call one level up, to ChannelManager::CreateChannel?
On 2017/02/16 12:58:16, nisse-webrtc wrote: > https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.cc:939: SetLocalSSRC(random_.Rand<uint32_t>()); > On 2017/02/16 12:34:16, the sun wrote: > > Leave a TODO for me to remove this. > > Will do in next upload. > > https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.h (right): > > https://codereview.webrtc.org/2644303002/diff/290001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.h:524: webrtc::Random random_; > On 2017/02/16 12:34:16, the sun wrote: > > You're only using this member in the ctor, why do you need it here? > > Good catch. I think I originally put it in the ChannelManager, and then that > didn't seem to work out. > > Should I move it to ChannelManager, and move the SetLocalSsrc call one level up, > to ChannelManager::CreateChannel? sgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/16 13:00:34, the sun wrote: > On 2017/02/16 12:58:16, nisse-webrtc wrote: > > Should I move it to ChannelManager, and move the SetLocalSsrc call one level > up, > > to ChannelManager::CreateChannel? > > sgtm Done now.
solenberg@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. Further, this doesn't look right - I believe it should be using the remote SSRC. terelius@, do you remember if there was any particular reason grouping by local SSRC for incoming streams was selected?
terelius@webrtc.org changed reviewers: + ivoc@webrtc.org - terelius@webrtc.org
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:52:32, the sun wrote: > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. > > Further, this doesn't look right - I believe it should be using the remote SSRC. > terelius@, do you remember if there was any particular reason grouping by local > SSRC for incoming streams was selected? I've never used the SSRC for audio playouts. ivoc@, can you answer the question above?
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:52:32, the sun wrote: > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. > > Further, this doesn't look right - I believe it should be using the remote SSRC. > terelius@, do you remember if there was any particular reason grouping by local > SSRC for incoming streams was selected? Ooops. I misunderstood the GetLocalSSRC call completely. Background for this change was that ssrcs wasn't set at this point. I could try undoing this part of the change and maybe it will now work, thanks to the random default ssrc set by the ChannelManager.
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:58:05, terelius wrote: > On 2017/02/16 13:52:32, the sun wrote: > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. > > > > Further, this doesn't look right - I believe it should be using the remote > SSRC. > > terelius@, do you remember if there was any particular reason grouping by > local > > SSRC for incoming streams was selected? > > I've never used the SSRC for audio playouts. ivoc@, can you answer the question > above? If the ssrc is no longer considered useful, please send me an email so I can remove the field from any future versions of the proto.
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 14:04:42, terelius wrote: > On 2017/02/16 13:58:05, terelius wrote: > > On 2017/02/16 13:52:32, the sun wrote: > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. > > > > > > Further, this doesn't look right - I believe it should be using the remote > > SSRC. > > > terelius@, do you remember if there was any particular reason grouping by > > local > > > SSRC for incoming streams was selected? > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer the > question > > above? > > If the ssrc is no longer considered useful, please send me an email so I can > remove the field from any future versions of the proto. The SSRC was added in https://codereview.webrtc.org/1340283002/. If I recall correctly we had the problem that playout events were being logged for multiple channels, and it was impossible to tell them apart without a unique ID. We decided to use the SSRC for this purpose, so I don't think it should be removed.
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 14:27:06, ivoc wrote: > On 2017/02/16 14:04:42, terelius wrote: > > On 2017/02/16 13:58:05, terelius wrote: > > > On 2017/02/16 13:52:32, the sun wrote: > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. > > > > > > > > Further, this doesn't look right - I believe it should be using the remote > > > SSRC. > > > > terelius@, do you remember if there was any particular reason grouping by > > > local > > > > SSRC for incoming streams was selected? > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer the > > question > > > above? > > > > If the ssrc is no longer considered useful, please send me an email so I can > > remove the field from any future versions of the proto. > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I recall > correctly we had the problem that playout events were being logged for multiple > channels, and it was impossible to tell them apart without a unique ID. We > decided to use the SSRC for this purpose, so I don't think it should be removed. But if it's a received stream, we should be using the "remote" ssrc, not the "local" one? Since in this code, local really means sending, if I've understood it correctly.
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... 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 14:27:06, ivoc wrote: > > On 2017/02/16 14:04:42, terelius wrote: > > > On 2017/02/16 13:58:05, terelius wrote: > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout > events. > > > > > > > > > > Further, this doesn't look right - I believe it should be using the > remote > > > > SSRC. > > > > > terelius@, do you remember if there was any particular reason grouping > by > > > > local > > > > > SSRC for incoming streams was selected? > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer the > > > question > > > > above? > > > > > > If the ssrc is no longer considered useful, please send me an email so I can > > > remove the field from any future versions of the proto. > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I recall > > correctly we had the problem that playout events were being logged for > multiple > > channels, and it was impossible to tell them apart without a unique ID. We > > decided to use the SSRC for this purpose, so I don't think it should be > removed. > > But if it's a received stream, we should be using the "remote" ssrc, not the > "local" one? Since in this code, local really means sending, if I've understood > it correctly. We only used it to tell the different channels apart, so we don't care about the actual value. Feel free to change it to something that makes more sense.
On 2017/02/16 14:29:22, nisse-webrtc wrote: > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); > On 2017/02/16 14:27:06, ivoc wrote: > > On 2017/02/16 14:04:42, terelius wrote: > > > On 2017/02/16 13:58:05, terelius wrote: > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout > events. > > > > > > > > > > Further, this doesn't look right - I believe it should be using the > remote > > > > SSRC. > > > > > terelius@, do you remember if there was any particular reason grouping > by > > > > local > > > > > SSRC for incoming streams was selected? > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer the > > > question > > > > above? > > > > > > If the ssrc is no longer considered useful, please send me an email so I can > > > remove the field from any future versions of the proto. > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I recall > > correctly we had the problem that playout events were being logged for > multiple > > channels, and it was impossible to tell them apart without a unique ID. We > > decided to use the SSRC for this purpose, so I don't think it should be > removed. > > But if it's a received stream, we should be using the "remote" ssrc, not the > "local" one? Since in this code, local really means sending, if I've understood > it correctly. Yes, for a received stream, the local SSRC is the SSRC we use for RTCP, which defaults to this: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/engin... It is reset to use an SSRC of one of the outgoing streams, as soon as we have one: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/engin...
On 2017/02/16 14:35:39, ivoc wrote: > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > 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 14:27:06, ivoc wrote: > > > On 2017/02/16 14:04:42, terelius wrote: > > > > On 2017/02/16 13:58:05, terelius wrote: > > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout > > events. > > > > > > > > > > > > Further, this doesn't look right - I believe it should be using the > > remote > > > > > SSRC. > > > > > > terelius@, do you remember if there was any particular reason grouping > > by > > > > > local > > > > > > SSRC for incoming streams was selected? > > > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer the > > > > question > > > > > above? > > > > > > > > If the ssrc is no longer considered useful, please send me an email so I > can > > > > remove the field from any future versions of the proto. > > > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I recall > > > correctly we had the problem that playout events were being logged for > > multiple > > > channels, and it was impossible to tell them apart without a unique ID. We > > > decided to use the SSRC for this purpose, so I don't think it should be > > removed. > > > > But if it's a received stream, we should be using the "remote" ssrc, not the > > "local" one? Since in this code, local really means sending, if I've > understood > > it correctly. > > We only used it to tell the different channels apart, so we don't care about the > actual value. Feel free to change it to something that makes more sense. Are we not using it anymore? If we are using it for distinguishing log info for streams, it should be the remove SSRC. (And the proto field should probably be replaced.)
On 2017/02/16 14:39:52, the sun wrote: > On 2017/02/16 14:35:39, ivoc wrote: > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > File webrtc/voice_engine/channel.cc (right): > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > 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 14:27:06, ivoc wrote: > > > > On 2017/02/16 14:04:42, terelius wrote: > > > > > On 2017/02/16 13:58:05, terelius wrote: > > > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout > > > events. > > > > > > > > > > > > > > Further, this doesn't look right - I believe it should be using the > > > remote > > > > > > SSRC. > > > > > > > terelius@, do you remember if there was any particular reason > grouping > > > by > > > > > > local > > > > > > > SSRC for incoming streams was selected? > > > > > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer the > > > > > question > > > > > > above? > > > > > > > > > > If the ssrc is no longer considered useful, please send me an email so I > > can > > > > > remove the field from any future versions of the proto. > > > > > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I > recall > > > > correctly we had the problem that playout events were being logged for > > > multiple > > > > channels, and it was impossible to tell them apart without a unique ID. We > > > > decided to use the SSRC for this purpose, so I don't think it should be > > > removed. > > > > > > But if it's a received stream, we should be using the "remote" ssrc, not the > > > "local" one? Since in this code, local really means sending, if I've > > understood > > > it correctly. > > > > We only used it to tell the different channels apart, so we don't care about > the > > actual value. Feel free to change it to something that makes more sense. > > Are we not using it anymore? > > If we are using it for distinguishing log info for streams, it should be the > remove SSRC. (And the proto field should probably be replaced.) Well, I do not regularly use event logs to debug issues with audio playout events myself, but maybe hlundin does (added hlundin to give input)? I think it's a useful feature that we should keep around. In this case the useful information that can be gotten from the event log is that the events (for each channel) are happening more or less evenly spaced in time, otherwise it can indicate that the processor is overloaded and strange things can happen in NetEq.
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
On 2017/02/16 14:48:45, ivoc wrote: > On 2017/02/16 14:39:52, the sun wrote: > > On 2017/02/16 14:35:39, ivoc wrote: > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > File webrtc/voice_engine/channel.cc (right): > > > > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > 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 14:27:06, ivoc wrote: > > > > > On 2017/02/16 14:04:42, terelius wrote: > > > > > > On 2017/02/16 13:58:05, terelius wrote: > > > > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout > > > > events. > > > > > > > > > > > > > > > > Further, this doesn't look right - I believe it should be using > the > > > > remote > > > > > > > SSRC. > > > > > > > > terelius@, do you remember if there was any particular reason > > grouping > > > > by > > > > > > > local > > > > > > > > SSRC for incoming streams was selected? > > > > > > > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer > the > > > > > > question > > > > > > > above? > > > > > > > > > > > > If the ssrc is no longer considered useful, please send me an email so > I > > > can > > > > > > remove the field from any future versions of the proto. > > > > > > > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I > > recall > > > > > correctly we had the problem that playout events were being logged for > > > > multiple > > > > > channels, and it was impossible to tell them apart without a unique ID. > We > > > > > decided to use the SSRC for this purpose, so I don't think it should be > > > > removed. > > > > > > > > But if it's a received stream, we should be using the "remote" ssrc, not > the > > > > "local" one? Since in this code, local really means sending, if I've > > > understood > > > > it correctly. > > > > > > We only used it to tell the different channels apart, so we don't care about > > the > > > actual value. Feel free to change it to something that makes more sense. > > > > Are we not using it anymore? > > > > If we are using it for distinguishing log info for streams, it should be the > > remove SSRC. (And the proto field should probably be replaced.) > > Well, I do not regularly use event logs to debug issues with audio playout > events myself, but maybe hlundin does (added hlundin to give input)? I think > it's a useful feature that we should keep around. In this case the useful > information that can be gotten from the event log is that the events (for each > channel) are happening more or less evenly spaced in time, otherwise it can > indicate that the processor is overloaded and strange things can happen in > NetEq. What Ivo says. I use the audio playout events to assess the impacts of uneven GetAudio call patterns (which happens under CPU crunch). Please, don't remove this feature. And it must be possible to distinguish different channels.
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
On 2017/02/16 14:48:45, ivoc wrote: > On 2017/02/16 14:39:52, the sun wrote: > > On 2017/02/16 14:35:39, ivoc wrote: > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > File webrtc/voice_engine/channel.cc (right): > > > > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > 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 14:27:06, ivoc wrote: > > > > > On 2017/02/16 14:04:42, terelius wrote: > > > > > > On 2017/02/16 13:58:05, terelius wrote: > > > > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > > > > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout > > > > events. > > > > > > > > > > > > > > > > Further, this doesn't look right - I believe it should be using > the > > > > remote > > > > > > > SSRC. > > > > > > > > terelius@, do you remember if there was any particular reason > > grouping > > > > by > > > > > > > local > > > > > > > > SSRC for incoming streams was selected? > > > > > > > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer > the > > > > > > question > > > > > > > above? > > > > > > > > > > > > If the ssrc is no longer considered useful, please send me an email so > I > > > can > > > > > > remove the field from any future versions of the proto. > > > > > > > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I > > recall > > > > > correctly we had the problem that playout events were being logged for > > > > multiple > > > > > channels, and it was impossible to tell them apart without a unique ID. > We > > > > > decided to use the SSRC for this purpose, so I don't think it should be > > > > removed. > > > > > > > > But if it's a received stream, we should be using the "remote" ssrc, not > the > > > > "local" one? Since in this code, local really means sending, if I've > > > understood > > > > it correctly. > > > > > > We only used it to tell the different channels apart, so we don't care about > > the > > > actual value. Feel free to change it to something that makes more sense. > > > > Are we not using it anymore? > > > > If we are using it for distinguishing log info for streams, it should be the > > remove SSRC. (And the proto field should probably be replaced.) > > Well, I do not regularly use event logs to debug issues with audio playout > events myself, but maybe hlundin does (added hlundin to give input)? I think > it's a useful feature that we should keep around. In this case the useful > information that can be gotten from the event log is that the events (for each > channel) are happening more or less evenly spaced in time, otherwise it can > indicate that the processor is overloaded and strange things can happen in > NetEq. My experience is that audio playout events often don't occur at regular intervals on mobile devices. Instead, a "burst" of audio playouts is followed by a blank period before the next sequence of back-to-back playouts.
On 2017/02/16 15:00:46, terelius wrote: > On 2017/02/16 14:48:45, ivoc wrote: > > On 2017/02/16 14:39:52, the sun wrote: > > > On 2017/02/16 14:35:39, ivoc wrote: > > > > > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > > File webrtc/voice_engine/channel.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > > 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 14:27:06, ivoc wrote: > > > > > > On 2017/02/16 14:04:42, terelius wrote: > > > > > > > On 2017/02/16 13:58:05, terelius wrote: > > > > > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > > > > > Hang on - RtcEventLog will have the same SSRC for all > AudioPlayout > > > > > events. > > > > > > > > > > > > > > > > > > Further, this doesn't look right - I believe it should be using > > the > > > > > remote > > > > > > > > SSRC. > > > > > > > > > terelius@, do you remember if there was any particular reason > > > grouping > > > > > by > > > > > > > > local > > > > > > > > > SSRC for incoming streams was selected? > > > > > > > > > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you answer > > the > > > > > > > question > > > > > > > > above? > > > > > > > > > > > > > > If the ssrc is no longer considered useful, please send me an email > so > > I > > > > can > > > > > > > remove the field from any future versions of the proto. > > > > > > > > > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If I > > > recall > > > > > > correctly we had the problem that playout events were being logged for > > > > > multiple > > > > > > channels, and it was impossible to tell them apart without a unique > ID. > > We > > > > > > decided to use the SSRC for this purpose, so I don't think it should > be > > > > > removed. > > > > > > > > > > But if it's a received stream, we should be using the "remote" ssrc, not > > the > > > > > "local" one? Since in this code, local really means sending, if I've > > > > understood > > > > > it correctly. > > > > > > > > We only used it to tell the different channels apart, so we don't care > about > > > the > > > > actual value. Feel free to change it to something that makes more sense. > > > > > > Are we not using it anymore? > > > > > > If we are using it for distinguishing log info for streams, it should be the > > > remove SSRC. (And the proto field should probably be replaced.) > > > > Well, I do not regularly use event logs to debug issues with audio playout > > events myself, but maybe hlundin does (added hlundin to give input)? I think > > it's a useful feature that we should keep around. In this case the useful > > information that can be gotten from the event log is that the events (for each > > channel) are happening more or less evenly spaced in time, otherwise it can > > indicate that the processor is overloaded and strange things can happen in > > NetEq. > > My experience is that audio playout events often don't occur at regular > intervals > on mobile devices. Instead, a "burst" of audio playouts is followed by a blank > period before the next sequence of back-to-back playouts. Depending on how big these bursts are, that may be fine or may cause issues in NetEq. Either way I think we should continue to log this so we have the information available when problems pop up. Also I fully agree with changing the SSRC to the remote one.
On 2017/02/16 15:06:14, ivoc wrote: > On 2017/02/16 15:00:46, terelius wrote: > > On 2017/02/16 14:48:45, ivoc wrote: > > > On 2017/02/16 14:39:52, the sun wrote: > > > > On 2017/02/16 14:35:39, ivoc wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > > > File webrtc/voice_engine/channel.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... > > > > > 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 14:27:06, ivoc wrote: > > > > > > > On 2017/02/16 14:04:42, terelius wrote: > > > > > > > > On 2017/02/16 13:58:05, terelius wrote: > > > > > > > > > On 2017/02/16 13:52:32, the sun wrote: > > > > > > > > > > Hang on - RtcEventLog will have the same SSRC for all > > AudioPlayout > > > > > > events. > > > > > > > > > > > > > > > > > > > > Further, this doesn't look right - I believe it should be > using > > > the > > > > > > remote > > > > > > > > > SSRC. > > > > > > > > > > terelius@, do you remember if there was any particular reason > > > > grouping > > > > > > by > > > > > > > > > local > > > > > > > > > > SSRC for incoming streams was selected? > > > > > > > > > > > > > > > > > > I've never used the SSRC for audio playouts. ivoc@, can you > answer > > > the > > > > > > > > question > > > > > > > > > above? > > > > > > > > > > > > > > > > If the ssrc is no longer considered useful, please send me an > email > > so > > > I > > > > > can > > > > > > > > remove the field from any future versions of the proto. > > > > > > > > > > > > > > The SSRC was added in https://codereview.webrtc.org/1340283002/. If > I > > > > recall > > > > > > > correctly we had the problem that playout events were being logged > for > > > > > > multiple > > > > > > > channels, and it was impossible to tell them apart without a unique > > ID. > > > We > > > > > > > decided to use the SSRC for this purpose, so I don't think it should > > be > > > > > > removed. > > > > > > > > > > > > But if it's a received stream, we should be using the "remote" ssrc, > not > > > the > > > > > > "local" one? Since in this code, local really means sending, if I've > > > > > understood > > > > > > it correctly. > > > > > > > > > > We only used it to tell the different channels apart, so we don't care > > about > > > > the > > > > > actual value. Feel free to change it to something that makes more sense. > > > > > > > > Are we not using it anymore? > > > > > > > > If we are using it for distinguishing log info for streams, it should be > the > > > > remove SSRC. (And the proto field should probably be replaced.) > > > > > > Well, I do not regularly use event logs to debug issues with audio playout > > > events myself, but maybe hlundin does (added hlundin to give input)? I think > > > it's a useful feature that we should keep around. In this case the useful > > > information that can be gotten from the event log is that the events (for > each > > > channel) are happening more or less evenly spaced in time, otherwise it can > > > indicate that the processor is overloaded and strange things can happen in > > > NetEq. > > > > My experience is that audio playout events often don't occur at regular > > intervals > > on mobile devices. Instead, a "burst" of audio playouts is followed by a blank > > period before the next sequence of back-to-back playouts. > > Depending on how big these bursts are, that may be fine or may cause issues in > NetEq. Either way I think we should continue to log this so we have the > information available when problems pop up. Also I fully agree with changing the > SSRC to the remote one. I suggest in this CL that nisse@ picks the remote SSRC, but add a TODO in the proto definition for ivoc@ to either rename or replace (not sure which is best ATM) the "local_ssrc" field.
https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2644303002/diff/310001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:579: event_log_proxy_->LogAudioPlayout(0); On 2017/02/16 13:52:32, the sun wrote: > Hang on - RtcEventLog will have the same SSRC for all AudioPlayout events. > > Further, this doesn't look right - I believe it should be using the remote SSRC. > terelius@, do you remember if there was any particular reason grouping by local > SSRC for incoming streams was selected? Undid this change, and then changed GetLocalSsrc to GetRemoteSsrc.
Description was changed from ========== Delete class SSRCDatabase, and its global ssrc registry, and the method RTPSender::GenerateNewSSRC. It's now mandatory for upper layers to call SetSSRC, RTPSender no longer allocates a default ssrc. This breaks the old VoE api, which is going to be deleted. BUG=webrtc:4306,webrtc:6887 ========== to ========== 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 ==========
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2644303002/#ps350001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for webrtc/logging/rtc_event_log/rtc_event_log.proto:
While running git apply --index -p1;
error: patch failed: webrtc/logging/rtc_event_log/rtc_event_log.proto:251
error: webrtc/logging/rtc_event_log/rtc_event_log.proto: patch does not apply
Patch: webrtc/logging/rtc_event_log/rtc_event_log.proto
Index: webrtc/logging/rtc_event_log/rtc_event_log.proto
diff --git a/webrtc/logging/rtc_event_log/rtc_event_log.proto
b/webrtc/logging/rtc_event_log/rtc_event_log.proto
index
e80772297997ce2b088404d8ff83395b71fab4f8..3559f844e7c8904984840360d12c49f47a63850d
100644
--- a/webrtc/logging/rtc_event_log/rtc_event_log.proto
+++ b/webrtc/logging/rtc_event_log/rtc_event_log.proto
@@ -99,6 +99,8 @@ message RtcpPacket {
}
message AudioPlayoutEvent {
+ // TODO(ivoc): Rename, we currently use the "remote" ssrc, i.e. identifying
+ // the receive stream, while local_ssrc identifies the send stream, if any.
// required - The SSRC of the audio stream associated with the playout event.
optional uint32 local_ssrc = 2;
}
@@ -251,4 +253,4 @@ message AudioNetworkAdaptation {
// Number of audio channels that each encoded packet consists of.
optional uint32 num_channels = 6;
-}
\ No newline at end of file
+}
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2644303002/#ps370001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 370001, "attempt_start_ts": 1487338792493990,
"parent_rev": "1088248a4e6151dfbe8cc733fd771a79a80ad19d", "commit_rev":
"b78d4d13835f628e722a57abae2bf06ba3655921"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/b78d4d13835f628e722a57aba... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:370001) as https://chromium.googlesource.com/external/webrtc/+/b78d4d13835f628e722a57aba...
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. |
