Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1479023002:
Prepare the AudioSendStream to be hooked up to send-side BWE. (Closed)
|
Created:
5 years ago by stefan-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, henrika_webrtc, hlundin-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPrepare the AudioSendStream to be hooked up to send-side BWE.
This CL contains three changes as a preparation for adding audio send streams
to the send-side BWE:
1. Audio packets are passed through the pacer with high priority. This
is needed to be able to set transport sequence numbers on the packets.
2. A feedback observer is passed to the audio stream's rtcp receiver so
that the BWE can get notified of any BWE feedback being received on the
audio feedback channel.
3. Support for the transport sequence number header extension is added
to audio send streams.
BUG=webrtc:5263, webrtc:5307
R=mflodman@webrtc.org, solenberg@webrtc.org
Committed: https://crrev.com/b86d4e4a8dec1eb1b801244a2a97cda66f561d8e
Cr-Commit-Position: refs/heads/master@{#10909}
Patch Set 1 #Patch Set 2 : Fixed tests. #Patch Set 3 : Only configure rtp module with pacing if channel configured to do it. #Patch Set 4 : Improve class name. #Patch Set 5 : Rebase #Patch Set 6 : . #Patch Set 7 : Fixes and cleanups. #Patch Set 8 : Fix GN bots. #
Total comments: 46
Patch Set 9 : Comments addressed. #
Total comments: 10
Patch Set 10 : All requested changes except in channel.h/.cc which needs discussion. #Patch Set 11 : Comments addressed. #
Total comments: 17
Patch Set 12 : Magnus' comments addressed. #
Total comments: 42
Patch Set 13 : Reverted back to using a proxy and fixed an issue related to audio nack. #
Total comments: 1
Patch Set 14 : Remove anonymous namespace due to compile warning. #
Total comments: 11
Patch Set 15 : Allow sending high prio (audio) packets when network down to work around broken peer connection beh… #Patch Set 16 : . #
Total comments: 3
Patch Set 17 : Addressed comment #
Total comments: 2
Patch Set 18 : Moved method. #
Total comments: 7
Patch Set 19 : Magnus comments addressed. #Patch Set 20 : Rebase #Patch Set 21 : Fix merge #Patch Set 22 : Remove incorrect thread check. #Messages
Total messages: 71 (20 generated)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5574)
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/5941) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/11096) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/9751) mac_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_x64_dbg/bui...) mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/5547)
Rebase
Fixes and cleanups.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/5548)
Fix GN bots.
stefan@webrtc.org changed reviewers: + mflodman@webrtc.org, solenberg@webrtc.org
This change is fairly well covered by the audio/video sync tests. When audio is fully hooked up to send-side BWE I plan to adapt one of the existing BWE end-to-end tests to also include audio streams. Let me know if you still prefer to have separate tests in this CL.
Bunch of nits basically. Biggest concern is with the PacketSenderProxy. I see the point but is it the right approach? Another option would of course be to add mutator functions to RTPSender. Is the critsect in PacketSenderProxy really needed? What threads are the interfaces actually called on? I'm also a bit concerned about test coverage since there are some obvious mishaps. https://codereview.webrtc.org/1479023002/diff/140001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:540: voe_config_.Set<webrtc::VoicePacing>(new webrtc::VoicePacing(true)); If this is always enabled, don't put it in the config. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:76: RTC_DCHECK(congestion_controller); since you dereference below. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:82: } else if (extension.name == RtpExtension::kTransportSequenceNumber) { You have 2x "if (extension.name == RtpExtension::kTransportSequenceNumber)" A unit test should have caught this? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:92: channel_proxy_->SetCongestionControlObjects( Would it be easier to just call this SetCongestionControl() and send in the cc*? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:20: #include "webrtc/video_engine/call_stats.h" Should call_stats be moved to webrtc/call? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:74: SetSendAudioLevelIndicationStatus(true, kAudioLevelId)).Times(1); You're missing an EXPECT_CALL(*channel_proxy, SetSendTransportSequenceNumber(kTransportSequenceNumberId)).Times(1); https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:75: EXPECT_CALL(*channel_proxy_, SetCongestionControlObjects(_, _, _)) By moving the common config stuff in here you could verify the inputs as well. NullBitrateObserver could even be private to ConfigHelper. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:154: rtc::scoped_ptr<ProcessThread> thread( Put the common configuration stuff in ConfigHelper https://codereview.webrtc.org/1479023002/diff/140001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:38: MOCK_CONST_METHOD0(GetDelayEstimate, uint32_t()); Use same declaration order as voe::ChannelProxy You're not mocking SetSendTransportSequenceNumber() - is it lacking a test? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:778: packet_sender_proxy_(config.Get<VoicePacing>().enabled By making PacketSenderProxy construct-and-config you could leave this as nullptr and get rid of the Config option (you could have a bool in AudioSendStream::Config instead if you wanted it optional). https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:835: packet_router_->RemoveRtpModule(_rtpRtcpModule.get()); Seems like the PacketSenderProxy should handle this on destruction https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:80: class PacketSenderProxy : public RtpPacketSender, You can put this class in an anonymous namespace in the .cc https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:102: if (packet_sender_ == nullptr) Looks like you have a crash bug here; packet_sender_ may be uninitialized. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, I don't see where this is called from? Where is it hooked up? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:132: rtc::CriticalSection crit_; So what are the threads this can be called on? Please document using a ThreadChecker. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:14: #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" not needed? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:119: channel_owner_.channel()->SetSendTransportSequenceNumber(id); Use the channel() helper instead of the DCHECK+call on channel_owner_ Move these methods to their appropriate place (follow declaration order)
Comments addressed.
Thanks for the thorough review. Most comments should have been addressed. We may need to discuss how/if the VoicePacing config should be removed. I currently don't see how that is possible while still supporting the old voice engine API. https://codereview.webrtc.org/1479023002/diff/140001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:540: voe_config_.Set<webrtc::VoicePacing>(new webrtc::VoicePacing(true)); On 2015/11/30 12:37:20, the sun wrote: > If this is always enabled, don't put it in the config. It's a question of whether we want to keep supporting the old API or not, since it isn't possible to set the congestion control objects (and RtpPacketSender in particular) to the voice channel in the old API (and it doesn't make sense to try to do it with the old API either). https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:76: On 2015/11/30 12:37:20, the sun wrote: > RTC_DCHECK(congestion_controller); > since you dereference below. Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:82: } else if (extension.name == RtpExtension::kTransportSequenceNumber) { On 2015/11/30 12:37:20, the sun wrote: > You have 2x "if (extension.name == RtpExtension::kTransportSequenceNumber)" > A unit test should have caught this? Ah, merge problem. Adding the same test that you have added for the other extensions. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:92: channel_proxy_->SetCongestionControlObjects( On 2015/11/30 12:37:20, the sun wrote: > Would it be easier to just call this SetCongestionControl() and send in the cc*? It would be a bit more tricky since voice engine can't depend on the "webrtc" target in which files in webrtc/call/ are included. I could break it out into a separate lib, but I'm not sure it's really worth it. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:20: #include "webrtc/video_engine/call_stats.h" On 2015/11/30 12:37:20, the sun wrote: > Should call_stats be moved to webrtc/call? Yes, it definitely should. I plan on doing that separately, is that fine with you? https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:74: SetSendAudioLevelIndicationStatus(true, kAudioLevelId)).Times(1); On 2015/11/30 12:37:20, the sun wrote: > You're missing an EXPECT_CALL(*channel_proxy, > SetSendTransportSequenceNumber(kTransportSequenceNumberId)).Times(1); Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:75: EXPECT_CALL(*channel_proxy_, SetCongestionControlObjects(_, _, _)) On 2015/11/30 12:37:20, the sun wrote: > By moving the common config stuff in here you could verify the inputs as well. > NullBitrateObserver could even be private to ConfigHelper. Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:154: rtc::scoped_ptr<ProcessThread> thread( On 2015/11/30 12:37:20, the sun wrote: > Put the common configuration stuff in ConfigHelper Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:38: MOCK_CONST_METHOD0(GetDelayEstimate, uint32_t()); On 2015/11/30 12:37:20, the sun wrote: > Use same declaration order as voe::ChannelProxy > > You're not mocking SetSendTransportSequenceNumber() - is it lacking a test? Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:778: packet_sender_proxy_(config.Get<VoicePacing>().enabled On 2015/11/30 12:37:20, the sun wrote: > By making PacketSenderProxy construct-and-config you could leave this as nullptr > and get rid of the Config option (you could have a bool in > AudioSendStream::Config instead if you wanted it optional). What do you mean with construct-and-config? The problem here is that if I pass in a non-null paced_sender to the RTP module, it will be used. If someone uses the old voice engine API (which I assumed we still support), there will be no way of setting the paced sender (and it's not feasible to wire it up for the old API). I also don't want to add a pacer-setter to the RTP module as that will increase the complexity of the module. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:835: packet_router_->RemoveRtpModule(_rtpRtcpModule.get()); On 2015/11/30 12:37:20, the sun wrote: > Seems like the PacketSenderProxy should handle this on destruction It could, but that would require it to keep around its own pointer to the rtp module. I'll make that change if that's what you prefer. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:80: class PacketSenderProxy : public RtpPacketSender, On 2015/11/30 12:37:20, the sun wrote: > You can put this class in an anonymous namespace in the .cc Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:102: if (packet_sender_ == nullptr) On 2015/11/30 12:37:20, the sun wrote: > Looks like you have a crash bug here; packet_sender_ may be uninitialized. Definitely could crash here, good catch. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, On 2015/11/30 12:37:20, the sun wrote: > I don't see where this is called from? Where is it hooked up? Here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:132: rtc::CriticalSection crit_; On 2015/11/30 12:37:20, the sun wrote: > So what are the threads this can be called on? Please document using a > ThreadChecker. Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:14: #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" On 2015/11/30 12:37:20, the sun wrote: > not needed? Done. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:119: channel_owner_.channel()->SetSendTransportSequenceNumber(id); On 2015/11/30 12:37:20, the sun wrote: > Use the channel() helper instead of the DCHECK+call on channel_owner_ > > Move these methods to their appropriate place (follow declaration order) Ah, I don't think channel() was available when I worked on this. :)
https://codereview.webrtc.org/1479023002/diff/140001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:540: voe_config_.Set<webrtc::VoicePacing>(new webrtc::VoicePacing(true)); On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > On 2015/11/30 12:37:20, the sun wrote: > > If this is always enabled, don't put it in the config. > > It's a question of whether we want to keep supporting the old API or not, since > it isn't possible to set the congestion control objects (and RtpPacketSender in > particular) to the voice channel in the old API (and it doesn't make sense to > try to do it with the old API either). Given the time frame for deprecating the old API isn't astronomical, I think this can be a feature that's only available by hooking up an AudioSendStream to a channel. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:92: channel_proxy_->SetCongestionControlObjects( On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > On 2015/11/30 12:37:20, the sun wrote: > > Would it be easier to just call this SetCongestionControl() and send in the > cc*? > > It would be a bit more tricky since voice engine can't depend on the "webrtc" > target in which files in webrtc/call/ are included. I could break it out into a > separate lib, but I'm not sure it's really worth it. No, likely not. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:20: #include "webrtc/video_engine/call_stats.h" On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > On 2015/11/30 12:37:20, the sun wrote: > > Should call_stats be moved to webrtc/call? > > Yes, it definitely should. I plan on doing that separately, is that fine with > you? Acknowledged. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:127: private: Hey, I don't want my privates public! https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:778: packet_sender_proxy_(config.Get<VoicePacing>().enabled On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > On 2015/11/30 12:37:20, the sun wrote: > > By making PacketSenderProxy construct-and-config you could leave this as > nullptr > > and get rid of the Config option (you could have a bool in > > AudioSendStream::Config instead if you wanted it optional). > > What do you mean with construct-and-config? The problem here is that if I pass > in a non-null paced_sender to the RTP module, it will be used. If someone uses > the old voice engine API (which I assumed we still support), there will be no > way of setting the paced sender (and it's not feasible to wire it up for the old > API). > > I also don't want to add a pacer-setter to the RTP module as that will increase > the complexity of the module. Sorry, I mean construct-from-config; that is, make it an immutable class, but I guess that defeats the whole purpose of having a proxy in this case. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:835: packet_router_->RemoveRtpModule(_rtpRtcpModule.get()); On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > On 2015/11/30 12:37:20, the sun wrote: > > Seems like the PacketSenderProxy should handle this on destruction > > It could, but that would require it to keep around its own pointer to the rtp > module. I'll make that change if that's what you prefer. No, that would only make sense if the proxy class was immutable. https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > On 2015/11/30 12:37:20, the sun wrote: > > I don't see where this is called from? Where is it hooked up? > > Here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Yes, but I don't see how transport_feedback_observer_ is being configured. You don't set it in the RtpRtcp::Configuration. https://codereview.webrtc.org/1479023002/diff/160001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:76: kTransportSequenceNumberId)) if git cl format did this I guess I'm, ok with it, but the formatting looks funny... https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:130: private: Wow! So with this critsect, this class becomes a common choke for contention across all of these 4 threads? To me, that is sign that the rtp module should instead be blessed with separate CSs for the three pointers that can be mutated. Then there'd only be contention between the worker thread and either of the others. Not between all of them. Another option would be to recreate the rtp module (and keep it in Channel as a scoped_ptr). The recreation will happen very shortly after the channel is created, so any state the rtp module has collected won't matter much, if at all. And thanks for adding the TCs, they really revealed stuff I wasn't fully aware of. https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:67: void ChannelProxy::SetSendTransportSequenceNumber(int id) { soo nit: but can you please move me with the other senders, I feel so lonely (and in the .h, and in the mock)? https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:68: RTC_DCHECK(channel_owner_.channel()); no need for this DCHECK; already done in channel()
https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:127: private: On 2015/12/01 10:25:35, the sun wrote: > Hey, I don't want my privates public! Ok, fine... :) https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, On 2015/12/01 10:25:36, the sun wrote: > On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > > On 2015/11/30 12:37:20, the sun wrote: > > > I don't see where this is called from? Where is it hooked up? > > > > Here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Yes, but I don't see how transport_feedback_observer_ is being configured. You > don't set it in the RtpRtcp::Configuration. Gooood catch. :) It would have been caught later when we actually hook up voice to the BWE, currently there is no test for that. https://codereview.webrtc.org/1479023002/diff/160001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:76: kTransportSequenceNumberId)) On 2015/12/01 10:25:36, the sun wrote: > if git cl format did this I guess I'm, ok with it, but the formatting looks > funny... It did :) https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:130: private: On 2015/12/01 10:25:36, the sun wrote: > Wow! So with this critsect, this class becomes a common choke for contention > across all of these 4 threads? Right, not very nice. I redid it to only copy the pointers in the choke, but that will only work if we know that audio stops being encoded before we tear down the audio send stream. Do we know that? In that case this choke is cheap. > > To me, that is sign that the rtp module should instead be blessed with separate > CSs for the three pointers that can be mutated. Then there'd only be contention > between the worker thread and either of the others. Not between all of them. Basically same thing but with three locks? I'd prefer not to introduce complexity in the rtp module which we have worked hard to get rid of. > > Another option would be to recreate the rtp module (and keep it in Channel as a > scoped_ptr). The recreation will happen very shortly after the channel is > created, so any state the rtp module has collected won't matter much, if at all. I like this option, and that is basically what we do in video engine. If you think that should be fine for voice I wouldn't mind doing that. But won't there be a problem where the channel can be started (and therefore is already sending packets) when we try to tear down the rtp module? I could possibly stop and restart the channel, but I'm not very comfortable doing things like that since I don't know voice engine too well... > > And thanks for adding the TCs, they really revealed stuff I wasn't fully aware > of. https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:67: void ChannelProxy::SetSendTransportSequenceNumber(int id) { On 2015/12/01 10:25:36, the sun wrote: > soo nit: but can you please move me with the other senders, I feel so lonely > (and in the .h, and in the mock)? Done. https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:68: RTC_DCHECK(channel_owner_.channel()); On 2015/12/01 10:25:36, the sun wrote: > no need for this DCHECK; already done in channel() Done.
https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, On 2015/12/01 16:19:33, stefan-webrtc (holmer) wrote: > On 2015/12/01 10:25:36, the sun wrote: > > On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > > > On 2015/11/30 12:37:20, the sun wrote: > > > > I don't see where this is called from? Where is it hooked up? > > > > > > Here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > Yes, but I don't see how transport_feedback_observer_ is being configured. You > > don't set it in the RtpRtcp::Configuration. > > Gooood catch. :) > > It would have been caught later when we actually hook up voice to the BWE, > currently there is no test for that. Is it possible to add a test? At some level? https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:130: private: On 2015/12/01 16:19:33, stefan-webrtc (holmer) wrote: > On 2015/12/01 10:25:36, the sun wrote: > > Wow! So with this critsect, this class becomes a common choke for contention > > across all of these 4 threads? > > Right, not very nice. I redid it to only copy the pointers in the choke, but > that will only work if we know that audio stops being encoded before we tear > down the audio send stream. Do we know that? In that case this choke is cheap. > > > > > To me, that is sign that the rtp module should instead be blessed with > separate > > CSs for the three pointers that can be mutated. Then there'd only be > contention > > between the worker thread and either of the others. Not between all of them. > > Basically same thing but with three locks? I'd prefer not to introduce > complexity in the rtp module which we have worked hard to get rid of. > > > > > Another option would be to recreate the rtp module (and keep it in Channel as > a > > scoped_ptr). The recreation will happen very shortly after the channel is > > created, so any state the rtp module has collected won't matter much, if at > all. > > I like this option, and that is basically what we do in video engine. If you > think that should be fine for voice I wouldn't mind doing that. But won't there > be a problem where the channel can be started (and therefore is already sending > packets) when we try to tear down the rtp module? I could possibly stop and > restart the channel, but I'm not very comfortable doing things like that since I > don't know voice engine too well... I think we might be able to get away with it in this case. Look at WebRtcVoiceMediaChannel::AddSendStream(). The VoE channel is created: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... then just a few lines after the AudioSendStream is created: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... The stream has not been started yet. So if you could break out the creation/config of the RtpRtcp module and call that from the ctor and SetCongestionControlObjects() I *think* we should be fine. Just leave a BIG TODO for me, and RTC_DCHECK(!channel_state_.Get().sending). > > > > And thanks for adding the TCs, they really revealed stuff I wasn't fully aware > > of. >
Comments addressed.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/200001
https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, On 2015/12/01 16:48:23, the sun wrote: > On 2015/12/01 16:19:33, stefan-webrtc (holmer) wrote: > > On 2015/12/01 10:25:36, the sun wrote: > > > On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > > > > On 2015/11/30 12:37:20, the sun wrote: > > > > > I don't see where this is called from? Where is it hooked up? > > > > > > > > Here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > > > Yes, but I don't see how transport_feedback_observer_ is being configured. > You > > > don't set it in the RtpRtcp::Configuration. > > > > Gooood catch. :) > > > > It would have been caught later when we actually hook up voice to the BWE, > > currently there is no test for that. > > Is it possible to add a test? At some level? It is, but it requires setting up a call and monitoring the outgoing packets. At least that is the way I was planning on doing it. We already have such a test for video and my plan is to adapt those tests to include audio. The tests can be found here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Do you think it is worthwhile to write a smaller version of this now before adapting the above test? https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/160001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:130: private: On 2015/12/01 16:48:23, the sun wrote: > On 2015/12/01 16:19:33, stefan-webrtc (holmer) wrote: > > On 2015/12/01 10:25:36, the sun wrote: > > > Wow! So with this critsect, this class becomes a common choke for contention > > > across all of these 4 threads? > > > > Right, not very nice. I redid it to only copy the pointers in the choke, but > > that will only work if we know that audio stops being encoded before we tear > > down the audio send stream. Do we know that? In that case this choke is cheap. > > > > > > > > To me, that is sign that the rtp module should instead be blessed with > > separate > > > CSs for the three pointers that can be mutated. Then there'd only be > > contention > > > between the worker thread and either of the others. Not between all of them. > > > > Basically same thing but with three locks? I'd prefer not to introduce > > complexity in the rtp module which we have worked hard to get rid of. > > > > > > > > Another option would be to recreate the rtp module (and keep it in Channel > as > > a > > > scoped_ptr). The recreation will happen very shortly after the channel is > > > created, so any state the rtp module has collected won't matter much, if at > > all. > > > > I like this option, and that is basically what we do in video engine. If you > > think that should be fine for voice I wouldn't mind doing that. But won't > there > > be a problem where the channel can be started (and therefore is already > sending > > packets) when we try to tear down the rtp module? I could possibly stop and > > restart the channel, but I'm not very comfortable doing things like that since > I > > don't know voice engine too well... > > I think we might be able to get away with it in this case. Look at > WebRtcVoiceMediaChannel::AddSendStream(). > > The VoE channel is created: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > then just a few lines after the AudioSendStream is created: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > The stream has not been started yet. So if you could break out the > creation/config of the RtpRtcp module and call that from the ctor and > SetCongestionControlObjects() I *think* we should be fine. Just leave a BIG TODO > for me, and RTC_DCHECK(!channel_state_.Get().sending). > > > > > > > And thanks for adding the TCs, they really revealed stuff I wasn't fully > aware > > > of. > > > Done.
https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:109: void AddPacket(uint16_t sequence_number, On 2015/12/02 16:14:17, stefan-webrtc (holmer) wrote: > On 2015/12/01 16:48:23, the sun wrote: > > On 2015/12/01 16:19:33, stefan-webrtc (holmer) wrote: > > > On 2015/12/01 10:25:36, the sun wrote: > > > > On 2015/11/30 15:22:02, stefan-webrtc (holmer) wrote: > > > > > On 2015/11/30 12:37:20, the sun wrote: > > > > > > I don't see where this is called from? Where is it hooked up? > > > > > > > > > > Here: > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > > > > > Yes, but I don't see how transport_feedback_observer_ is being configured. > > You > > > > don't set it in the RtpRtcp::Configuration. > > > > > > Gooood catch. :) > > > > > > It would have been caught later when we actually hook up voice to the BWE, > > > currently there is no test for that. > > > > Is it possible to add a test? At some level? > > It is, but it requires setting up a call and monitoring the outgoing packets. At > least that is the way I was planning on doing it. We already have such a test > for video and my plan is to adapt those tests to include audio. The tests can be > found here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Do you think it is worthwhile to write a smaller version of this now before > adapting the above test? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... that link is probably a better example.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/11241) mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/10876)
https://codereview.webrtc.org/1479023002/diff/200001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:20: class PacketRouter; Alphabetic order. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:823: if (packet_router_ != nullptr) I prefer 'if (packet_router)', but I'm not sure about the gerneal usage of this expression in the voice code. Same further down in this file. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2802: void Channel::SetSendTransportSequenceNumber(int id) { This is a bit ambiguous, might be interpreted as settting a sequence number and not enabling the extension. And since there is no way to disable, only enable, maybe change 'Set' to 'Enable'? https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2805: RTC_DCHECK_EQ(0, ret); Do we expect errors for other extensions? If not, it would be a nicer to make SetSendRtpHeaderExtension a void-function and DCHECK there. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2821: _rtpRtcpModule.reset(CreateRtpRtcp(rtp_packet_sender, packet_router, Can we check we haven't sent anything before this point? Just resetting the RTP module might cause SRTP issues. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:25: class PacketRouter; Alphabetic order.
Magnus' comments addressed.
https://codereview.webrtc.org/1479023002/diff/200001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:20: class PacketRouter; On 2015/12/03 09:24:21, mflodman wrote: > Alphabetic order. Done. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:823: if (packet_router_ != nullptr) On 2015/12/03 09:24:21, mflodman wrote: > I prefer 'if (packet_router)', but I'm not sure about the gerneal usage of this > expression in the voice code. > > Same further down in this file. I also prefer that actually. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2802: void Channel::SetSendTransportSequenceNumber(int id) { On 2015/12/03 09:24:21, mflodman wrote: > This is a bit ambiguous, might be interpreted as settting a sequence number and > not enabling the extension. And since there is no way to disable, only enable, > maybe change 'Set' to 'Enable'? Done. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2805: RTC_DCHECK_EQ(0, ret); On 2015/12/03 09:24:21, mflodman wrote: > Do we expect errors for other extensions? > > If not, it would be a nicer to make SetSendRtpHeaderExtension a void-function > and DCHECK there. I'd prefer to not touch too much of the existing VoE API since it's supposed to still be supported. For instance, I don't know if other code relies on voice engine checking the range of the header extension id (libjingle does, but what other code is there?). I'll leave it as is for now if you don't mind. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2821: _rtpRtcpModule.reset(CreateRtpRtcp(rtp_packet_sender, packet_router, On 2015/12/03 09:24:21, mflodman wrote: > Can we check we haven't sent anything before this point? > > Just resetting the RTP module might cause SRTP issues. We check that the channel_state_ is not sending on line 2818. I don't think that's enough to be 100% sure this is safe, but maybe it's a good enough check? Otherwise we'll have to add a lock to check if a packet has been sent or not. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:25: class PacketRouter; On 2015/12/03 09:24:21, mflodman wrote: > Alphabetic order. Done.
LGTM https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2805: RTC_DCHECK_EQ(0, ret); On 2015/12/03 10:18:27, stefan-webrtc (holmer) wrote: > On 2015/12/03 09:24:21, mflodman wrote: > > Do we expect errors for other extensions? > > > > If not, it would be a nicer to make SetSendRtpHeaderExtension a void-function > > and DCHECK there. > > I'd prefer to not touch too much of the existing VoE API since it's supposed to > still be supported. For instance, I don't know if other code relies on voice > engine checking the range of the header extension id (libjingle does, but what > other code is there?). I'll leave it as is for now if you don't mind. Acknowledged. https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2821: _rtpRtcpModule.reset(CreateRtpRtcp(rtp_packet_sender, packet_router, On 2015/12/03 10:18:27, stefan-webrtc (holmer) wrote: > On 2015/12/03 09:24:21, mflodman wrote: > > Can we check we haven't sent anything before this point? > > > > Just resetting the RTP module might cause SRTP issues. > > We check that the channel_state_ is not sending on line 2818. I don't think > that's enough to be 100% sure this is safe, but maybe it's a good enough check? > Otherwise we'll have to add a lock to check if a packet has been sent or not. I was thinking of checking number of sent frames, or similar, in the RTP module we're about to delete, but I didn't see a good counter for audio, only video.
https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:823: if (packet_router_ != nullptr) On 2015/12/03 10:18:27, stefan-webrtc (holmer) wrote: > On 2015/12/03 09:24:21, mflodman wrote: > > I prefer 'if (packet_router)', but I'm not sure about the gerneal usage of > this > > expression in the voice code. > > > > Same further down in this file. > > I also prefer that actually. +1 https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2805: RTC_DCHECK_EQ(0, ret); On 2015/12/03 10:18:27, stefan-webrtc (holmer) wrote: > On 2015/12/03 09:24:21, mflodman wrote: > > Do we expect errors for other extensions? > > > > If not, it would be a nicer to make SetSendRtpHeaderExtension a void-function > > and DCHECK there. > > I'd prefer to not touch too much of the existing VoE API since it's supposed to > still be supported. For instance, I don't know if other code relies on voice > engine checking the range of the header extension id (libjingle does, but what > other code is there?). I'll leave it as is for now if you don't mind. +1 https://codereview.webrtc.org/1479023002/diff/200001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2821: _rtpRtcpModule.reset(CreateRtpRtcp(rtp_packet_sender, packet_router, On 2015/12/03 10:18:27, stefan-webrtc (holmer) wrote: > On 2015/12/03 09:24:21, mflodman wrote: > > Can we check we haven't sent anything before this point? > > > > Just resetting the RTP module might cause SRTP issues. > > We check that the channel_state_ is not sending on line 2818. I don't think > that's enough to be 100% sure this is safe, but maybe it's a good enough check? > Otherwise we'll have to add a lock to check if a packet has been sent or not. We should be fine; the ambiguity here is transient - once AudioSendStream creates its own VoE::Channel there will be no way for a client to mess things up. https://codereview.webrtc.org/1479023002/diff/220001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:527: voe_config_.Set<webrtc::VoicePacing>(new webrtc::VoicePacing(true)); Remove https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:21: #include "webrtc/modules/pacing/paced_sender.h" I believe these 3 includes are unnecessary: paced_sender.h packet_router.h rtp_rtcp_defines.h https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:21: #include "webrtc/modules/pacing/paced_sender.h" What do you need paced_sender.h for? https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:78: EXPECT_CALL(*channel_proxy_, SetSendTransportSequenceNumber( EnableSend....() https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:237: int send_channel = voe_base->CreateChannel(voe_config); send_channel_id recv_channel_id https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:247: receiver_config.audio_state = sender_config.audio_state; Create a separate audio state; it will make us cry less down the road... https://codereview.webrtc.org/1479023002/diff/220001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/config.h#newcode141 webrtc/config.h:141: struct VoicePacing { Remove https://codereview.webrtc.org/1479023002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:472: LOG(LS_WARNING) << "Payload type " << static_cast<int>(payload_type) int8_t couldn't be serialized? https://codereview.webrtc.org/1479023002/diff/220001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:19: class PacketRouter; you can avoid this list, since it is already present in channel_proxy.h https://codereview.webrtc.org/1479023002/diff/220001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:31: MOCK_METHOD1(SetSendTransportSequenceNumber, void(int id)); The name is now EnableSendTransportSequenceNumber. Is the test gone again? https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:24: #include "webrtc/modules/pacing/paced_sender.h" Not needed? https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:778: packet_router_(nullptr) { Set to null in .h instead https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:823: if (packet_router_) nit: {}, like the surrounding code https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:16: #include "webrtc/base/thread_checker.h" Not needed. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:17: #include "webrtc/call/congestion_controller.h" Not needed, in fact VoE::Channel shouldn't rely on anything in call/ https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:24: #include "webrtc/modules/pacing/paced_sender.h" Not needed? https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:471: RtpRtcp* CreateRtpRtcp( Please indent like the rest of this class, it'll make it easier to clean up later. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:56: RTC_DCHECK(channel_owner_.channel()); Remove this DCHECK and add RTC_DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:75: PacketRouter* packet_router) { RTC_DCHECK(thread_checker_.CalledOnValidThread());
Slowly getting there... Now we're back to using a proxy. In fact, I chose to split it into three proxies to make the locking more clear. https://codereview.webrtc.org/1479023002/diff/220001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:527: voe_config_.Set<webrtc::VoicePacing>(new webrtc::VoicePacing(true)); On 2015/12/03 11:10:28, the sun wrote: > Remove Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:21: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/03 11:10:28, the sun wrote: > I believe these 3 includes are unnecessary: > paced_sender.h > packet_router.h > rtp_rtcp_defines.h Why do you think that? I can't remove them because we're calling pacer(), packet_router() etc on congestion_controller below. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:21: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/03 11:10:28, the sun wrote: > What do you need paced_sender.h for? pacer() call below. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:78: EXPECT_CALL(*channel_proxy_, SetSendTransportSequenceNumber( On 2015/12/03 11:10:28, the sun wrote: > EnableSend....() Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:237: int send_channel = voe_base->CreateChannel(voe_config); On 2015/12/03 11:10:28, the sun wrote: > send_channel_id > recv_channel_id Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:247: receiver_config.audio_state = sender_config.audio_state; On 2015/12/03 11:10:28, the sun wrote: > Create a separate audio state; it will make us cry less down the road... This turned out to be a bit of work voice engine instances, so I'd prefer to leave that for now. If you really don't like the state this test will be in after this change, an option would be to set VoicePacing(false) and leave everything else as is for now. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/config.h#newcode141 webrtc/config.h:141: struct VoicePacing { On 2015/12/03 11:10:28, the sun wrote: > Remove Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:472: LOG(LS_WARNING) << "Payload type " << static_cast<int>(payload_type) On 2015/12/03 11:10:28, the sun wrote: > int8_t couldn't be serialized? It's interpreted as char. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:19: class PacketRouter; On 2015/12/03 11:10:28, the sun wrote: > you can avoid this list, since it is already present in channel_proxy.h Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:31: MOCK_METHOD1(SetSendTransportSequenceNumber, void(int id)); On 2015/12/03 11:10:28, the sun wrote: > The name is now EnableSendTransportSequenceNumber. Is the test gone again? I don't know why this didn't trigger. Maybe I didn't build the test somehow. The test definitely should be there. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:24: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/03 11:10:28, the sun wrote: > Not needed? Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:778: packet_router_(nullptr) { On 2015/12/03 11:10:28, the sun wrote: > Set to null in .h instead Done, although it doesn't follow the style of this file. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:823: if (packet_router_) On 2015/12/03 11:10:28, the sun wrote: > nit: {}, like the surrounding code Code gone. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:16: #include "webrtc/base/thread_checker.h" On 2015/12/03 11:10:28, the sun wrote: > Not needed. Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:17: #include "webrtc/call/congestion_controller.h" On 2015/12/03 11:10:28, the sun wrote: > Not needed, in fact VoE::Channel shouldn't rely on anything in call/ Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:24: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/03 11:10:28, the sun wrote: > Not needed? Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:471: RtpRtcp* CreateRtpRtcp( On 2015/12/03 11:10:28, the sun wrote: > Please indent like the rest of this class, it'll make it easier to clean up > later. Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:56: RTC_DCHECK(channel_owner_.channel()); On 2015/12/03 11:10:29, the sun wrote: > Remove this DCHECK and add > RTC_DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:75: PacketRouter* packet_router) { On 2015/12/03 11:10:29, the sun wrote: > RTC_DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.webrtc.org/1479023002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1011: return 0; Added this to make sure we don't send padding on the audio stream since I'm not sure how well neteq would handle padding-only packets.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:21: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/04 10:31:42, stefan-webrtc (holmer) wrote: > On 2015/12/03 11:10:28, the sun wrote: > > I believe these 3 includes are unnecessary: > > paced_sender.h > > packet_router.h > > rtp_rtcp_defines.h > > Why do you think that? I can't remove them because we're calling pacer(), > packet_router() etc on congestion_controller below. Anything needed to make calls *on* CC should be provided by congestion_controller.h. You're just passing on the pointers, not making calls on the packet_router, etc, itself. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:247: receiver_config.audio_state = sender_config.audio_state; On 2015/12/04 10:31:43, stefan-webrtc (holmer) wrote: > On 2015/12/03 11:10:28, the sun wrote: > > Create a separate audio state; it will make us cry less down the road... > > This turned out to be a bit of work voice engine instances, so I'd prefer to > leave that for now. If you really don't like the state this test will be in > after this change, an option would be to set VoicePacing(false) and leave > everything else as is for now. Fine. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:48: I like this split into three. It avoids most of the potential for congestion and is not invasive on the RtpRtcp module (like adding the locks there would be). Good compromise! https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:68: rtc::CritScope lock(&crit_); Don't you need to if (feedback_observer_) { feedback_observer_->AddPacket(...) } ?? here and below? https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:102: RTC_DCHECK(seq_num_allocator_ != nullptr); nit: drop " != nullptr" https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:140: packet_sender->InsertPacket(priority, ssrc, sequence_number, You can't do this reliably since someone else owns the packet_sender. You need to call the method on packet_sender_ while holding the lock. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:148: RtpPacketSender* packet_sender_ GUARDED_BY(&crit_); nit: might as well be consistent and prefix "rtp_"
Allow sending high prio (audio) packets when network down to work around broken peer connection behavior. Also switch video to low priority.
Description was changed from ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=5263 ========== to ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=5263,5307 ==========
.
I had to work around this issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=5307 by changing the pacer so that audio packets always are sent even if the network is signaled as being down. This should not be a change in behavior compared to before this CL. https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:21: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/04 12:04:48, the sun wrote: > On 2015/12/04 10:31:42, stefan-webrtc (holmer) wrote: > > On 2015/12/03 11:10:28, the sun wrote: > > > I believe these 3 includes are unnecessary: > > > paced_sender.h > > > packet_router.h > > > rtp_rtcp_defines.h > > > > Why do you think that? I can't remove them because we're calling pacer(), > > packet_router() etc on congestion_controller below. > > Anything needed to make calls *on* CC should be provided by > congestion_controller.h. You're just passing on the pointers, not making calls > on the packet_router, etc, itself. Ah, the problem is that I'm getting a PacedSender* but the method takes an RtpPacketSender*. Without this include it doesn't know that PacedSender is in fact an RtpPacketSender. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:48: On 2015/12/04 12:04:48, the sun wrote: > I like this split into three. It avoids most of the potential for congestion and > is not invasive on the RtpRtcp module (like adding the locks there would be). > Good compromise! Acknowledged. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:68: rtc::CritScope lock(&crit_); On 2015/12/04 12:04:48, the sun wrote: > Don't you need to > if (feedback_observer_) { > feedback_observer_->AddPacket(...) > } > ?? > here and below? Not in practice since we only use these methods if we are also using AudioSendStream, and then we always set these. This is at least true if we never send packets before we have created the audio send stream. But maybe it's good to add the ifs anyway... https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:102: RTC_DCHECK(seq_num_allocator_ != nullptr); On 2015/12/04 12:04:48, the sun wrote: > nit: drop " != nullptr" Done. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:140: packet_sender->InsertPacket(priority, ssrc, sequence_number, On 2015/12/04 12:04:48, the sun wrote: > You can't do this reliably since someone else owns the packet_sender. You need > to call the method on packet_sender_ while holding the lock. Ack, I accidentally left this. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:148: RtpPacketSender* packet_sender_ GUARDED_BY(&crit_); On 2015/12/04 12:04:48, the sun wrote: > nit: might as well be consistent and prefix "rtp_" Done. https://codereview.webrtc.org/1479023002/diff/300001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/300001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:395: if ((!paused_ || packet.priority == kHighPriority) && SendPacket(packet)) { I had to add this and change lines 362 and 408 to work around issue 5307. This basically means that audio packets will be sent even if the network is reported as down by libjingle, which is the same behavior as before this CL. I'm also changing priorities so that audio packets are the only packets sent with high priority. Video packets will be sent with low priority and retransmissions with normal priority.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2187)
https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/1479023002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:21: #include "webrtc/modules/pacing/paced_sender.h" On 2015/12/04 13:12:39, stefan-webrtc (holmer) wrote: > On 2015/12/04 12:04:48, the sun wrote: > > On 2015/12/04 10:31:42, stefan-webrtc (holmer) wrote: > > > On 2015/12/03 11:10:28, the sun wrote: > > > > I believe these 3 includes are unnecessary: > > > > paced_sender.h > > > > packet_router.h > > > > rtp_rtcp_defines.h > > > > > > Why do you think that? I can't remove them because we're calling pacer(), > > > packet_router() etc on congestion_controller below. > > > > Anything needed to make calls *on* CC should be provided by > > congestion_controller.h. You're just passing on the pointers, not making calls > > on the packet_router, etc, itself. > > Ah, the problem is that I'm getting a PacedSender* but the method takes an > RtpPacketSender*. Without this include it doesn't know that PacedSender is in > fact an RtpPacketSender. Acknowledged. https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/260001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:68: rtc::CritScope lock(&crit_); On 2015/12/04 13:12:39, stefan-webrtc (holmer) wrote: > On 2015/12/04 12:04:48, the sun wrote: > > Don't you need to > > if (feedback_observer_) { > > feedback_observer_->AddPacket(...) > > } > > ?? > > here and below? > > Not in practice since we only use these methods if we are also using > AudioSendStream, and then we always set these. This is at least true if we never > send packets before we have created the audio send stream. > > But maybe it's good to add the ifs anyway... Well, you're setting them to nullptr when the AudioSendStream is destroyed, which is a little before the channel is deleted, so a context switch could happen in between. https://codereview.webrtc.org/1479023002/diff/300001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/300001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:104: RTC_DCHECK(seq_num_allocator_); if (seq_num_allocator)
Addressed comment
https://codereview.webrtc.org/1479023002/diff/300001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/300001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:104: RTC_DCHECK(seq_num_allocator_); On 2015/12/04 14:01:57, the sun wrote: > if (seq_num_allocator) Done.
lgtm https://codereview.webrtc.org/1479023002/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2914: void Channel::SetCongestionControlObjects( nit: Move up above SetRTCPStatus() - use same order as in declaration file.
Moved method.
Description was changed from ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=5263,5307 ========== to ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=webrtc:5263,webrtc:5307 ==========
Two nits and one question. https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:375: target_bitrate_kbps = min_bitrate_needed_kbps; Indentation. https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1010: if (audio_configured_) Maybe do bytes == 0 || audio_configured_ to reduce number of returns. https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:482: dtmfbuffer, 4, 12, TickTime::MillisecondTimestamp(), There is a risk this DTMF packet will get the same capture time as the real audio packet on line 371. Will that be a problem?
Magnus comments addressed.
Thanks for noticing those nits. Reformatted. https://codereview.webrtc.org/1479023002/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1479023002/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2914: void Channel::SetCongestionControlObjects( On 2015/12/04 14:21:19, the sun wrote: > nit: Move up above SetRTCPStatus() - use same order as in declaration file. Done. https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/pacing/pa... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/pacing/pa... webrtc/modules/pacing/paced_sender.cc:375: target_bitrate_kbps = min_bitrate_needed_kbps; On 2015/12/04 15:25:19, mflodman wrote: > Indentation. Done. https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1010: if (audio_configured_) On 2015/12/04 15:25:19, mflodman wrote: > Maybe do bytes == 0 || audio_configured_ to reduce number of returns. Done. https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:482: dtmfbuffer, 4, 12, TickTime::MillisecondTimestamp(), On 2015/12/04 15:25:19, mflodman wrote: > There is a risk this DTMF packet will get the same capture time as the real > audio packet on line 371. Will that be a problem? Not that I'm aware. The capture time is ignored for audio except for when we compute how long the oldest packet has been in the pacer, and when we compute the send-side delay (which isn't used by audio). This shouldn't affect sending of dtmf or audio.
LGTM https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc (right): https://codereview.webrtc.org/1479023002/diff/340001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:482: dtmfbuffer, 4, 12, TickTime::MillisecondTimestamp(), On 2015/12/04 15:30:22, stefan-webrtc (holmer) wrote: > On 2015/12/04 15:25:19, mflodman wrote: > > There is a risk this DTMF packet will get the same capture time as the real > > audio packet on line 371. Will that be a problem? > > Not that I'm aware. The capture time is ignored for audio except for when we > compute how long the oldest packet has been in the pacer, and when we compute > the send-side delay (which isn't used by audio). This shouldn't affect sending > of dtmf or audio. Great, I couldn't find anything either but wanted to double check.
Rebase
Fix merge
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1479023002/#ps400001 (title: "Fix merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1479023002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1479023002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/11335) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/11164)
Remove incorrect thread check.
Message was sent while issue was closed.
Description was changed from ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=webrtc:5263,webrtc:5307 ========== to ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=webrtc:5263,webrtc:5307 R=mflodman@webrtc.org, solenberg@webrtc.org Committed: https://crrev.com/b86d4e4a8dec1eb1b801244a2a97cda66f561d8e Cr-Commit-Position: refs/heads/master@{#10909} ==========
Message was sent while issue was closed.
Description was changed from ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=webrtc:5263,webrtc:5307 R=mflodman@webrtc.org, solenberg@webrtc.org Committed: https://crrev.com/b86d4e4a8dec1eb1b801244a2a97cda66f561d8e Cr-Commit-Position: refs/heads/master@{#10909} ========== to ========== Prepare the AudioSendStream to be hooked up to send-side BWE. This CL contains three changes as a preparation for adding audio send streams to the send-side BWE: 1. Audio packets are passed through the pacer with high priority. This is needed to be able to set transport sequence numbers on the packets. 2. A feedback observer is passed to the audio stream's rtcp receiver so that the BWE can get notified of any BWE feedback being received on the audio feedback channel. 3. Support for the transport sequence number header extension is added to audio send streams. BUG=webrtc:5263,webrtc:5307 R=mflodman@webrtc.org, solenberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/b86d4e4a8dec1eb1b801244a2... ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/b86d4e4a8dec1eb1b801244a2a97cda66f561d8e Cr-Commit-Position: refs/heads/master@{#10909}
Message was sent while issue was closed.
Committed patchset #22 (id:420001) manually as b86d4e4a8dec1eb1b801244a2a97cda66f561d8e (presubmit successful). |
