Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553863003:
Parse FlexFEC RTP headers in Call and add integration with BWE. (Closed)
DescriptionParse FlexFEC RTP headers in Call and add integration with BWE.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2553863003
Cr-Commit-Position: refs/heads/master@{#15709}
Committed: https://chromium.googlesource.com/external/webrtc/+/ab2ffa3b28b55ef359232723049fb88b2dcd807a
Patch Set 1 : Work in progress. #
Total comments: 23
Patch Set 2 : Rebase and changes, including adressing danilchap's early comments. #
Total comments: 16
Patch Set 3 : nisse comments 1. #
Total comments: 12
Patch Set 4 : philipel comments 1 + danilchap comments 2. LACKING TESTS. #
Total comments: 14
Patch Set 5 : Rebase onto master. #Patch Set 6 : danilchap comments 2. #
Total comments: 8
Patch Set 7 : danilchap comments 3. #Patch Set 8 : Rebase and update to new interface of RtpPacketReceived. #Patch Set 9 : Rebase fixes: flexfec_receive_stream.h moved out of api/call/. #Patch Set 10 : More rebase fixes in flexfec_receive_stream_impl.h #Patch Set 11 : holmer comments 1. #Patch Set 12 : Add NotifyBweOfReceivedPacket to mock. #
Total comments: 5
Patch Set 13 : holmer comments 2. #Patch Set 14 : Polish. #Patch Set 15 : Fix fuzzer. #Patch Set 16 : Rebase. #Patch Set 17 : Add basic CongestionController unit test, based on nisse's suggestion. #Dependent Patchsets: Messages
Total messages: 65 (33 generated)
https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode274 webrtc/call/call.cc:274: RtpHeaderExtensionMap CreateRtpHeaderExtensionMap( this function implemented as a constructor of the RtpHeaderExtensionMap. i.e. instead of CreateRtpHeaderExtensionMap(extensions) it should be possible to use RtpHeaderExtensionMap(extensions) https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode395 webrtc/call/call.cc:395: parsed_packet.Parse(packet, length); what if Parse fails? https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode750 webrtc/call/call.cc:750: std::move(rtp_header_extensions); do not move: RtpHeaderExtensionMap doesn't have a move constructor because copy is as expensive/cheap as move. https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode1172 webrtc/call/call.cc:1172: // not be parsed, as FlexFEC is oblivious to the semantic meaning of the what about BWE header extensions? https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/flexfec_receive_s... File webrtc/call/flexfec_receive_stream.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/flexfec_receive_s... webrtc/call/flexfec_receive_stream.h:38: bool AddAndProcessReceivedPacket(RtpPacketReceived packet); to pass by value forward declaration is not enough. include header with the definition or pass by const&. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/include/congestion_controller.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/include/congestion_controller.h:106: bool transport_cc, what does transport_cc mean? https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:19: #include "webrtc/modules/rtp_rtcp/include/flexfec_receiver.h" unrelated question: is it a self-include? https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:50: bool AddAndProcessReceivedPacket(RtpPacketReceived packet); to pass by value forward declaration is not enough. include header with the definition or pass by const&. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:35: if (packet.HasExtension<TransportSequenceNumber>()) { may be return packet.HasExtension<TransportSequenceNumber>() || packet.HasExtension<AbsoluteSendTime>() || packet.HasExtension<TransmissionOffset>(); https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:86: if (packet.size() < kRtpHeaderSize) { feel free to DCHECK now instead: RtpPacket should guarantee validity (including size). https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:502: TransportSequenceNumber::kId); sorry for confusing, but it is better not to use TransportSequenceNumber::kId as an extension id to avoid more confusion. Use some (any) number instead (between 1 and 14).
Description was changed from ========== Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 R=philipel@webrtc.org, stefan@webrtc.org, danilchap@webrtc.org, nisse@webrtc.org ========== to ========== Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 R=philipel@webrtc.org, stefan@webrtc.org, danilchap@webrtc.org, nisse@webrtc.org ==========
Description was changed from ========== Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 R=philipel@webrtc.org, stefan@webrtc.org, danilchap@webrtc.org, nisse@webrtc.org ========== to ========== Work in progress. Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 ==========
brandtr@webrtc.org changed reviewers: - danilchap@webrtc.org, nisse@webrtc.org, philipel@webrtc.org, stefan@webrtc.org
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Work in progress. Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 ========== to ========== Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 ==========
Patchset #2 (id:20001) has been deleted
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, nisse@webrtc.org, philipel@webrtc.org, stefan@webrtc.org
This is another take on CLs 2, 3, 4, in the previous batch(*). Instead of propagating lots of objects and information down to the FlexfecReceiver, in this CL I propagate down a parsed RtpPacketReceived to FlexfecReceiver instead. This means that the Call layer is responsible for the RTP parsing. It further means that I include stuff from rtp_rtcp/source in call.cc, but I believe that these things will be moved to the packetization module at some point in the future. Please have a look and see if this idea and implementation is reasonable. philipel/holmer: General review. danilchap: Is RtpPacketReceived used correctly? Would this approach hinder how you would like to use RtpPacketReceived for the media pipeline on the receive side? nisse: I changed a TODO of yours in call.cc, which affects the bitrate estimator sent to the audio receive stream. Is this done correctly? (*) CLs 0 and 1 are still needed, as they relate to expanding the FlexFEC configs to contain more information. They have landed. https://codereview.webrtc.org/2553863003/diff/1/webrtc/api/call/flexfec_recei... File webrtc/api/call/flexfec_receive_stream.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/api/call/flexfec_recei... webrtc/api/call/flexfec_receive_stream.h:68: std::vector<RtpExtension> rtp_header_extensions; Prior CLs required that this be named 'extensions'. That is no longer required, so a more descriptive name is used here. https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode274 webrtc/call/call.cc:274: RtpHeaderExtensionMap CreateRtpHeaderExtensionMap( On 2016/12/06 15:04:30, danilchap wrote: > this function implemented as a constructor of the RtpHeaderExtensionMap. > i.e. instead of > CreateRtpHeaderExtensionMap(extensions) > it should be possible to use > RtpHeaderExtensionMap(extensions) Great! https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode395 webrtc/call/call.cc:395: parsed_packet.Parse(packet, length); On 2016/12/06 15:04:30, danilchap wrote: > what if Parse fails? Fixed by using optional. https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode750 webrtc/call/call.cc:750: std::move(rtp_header_extensions); On 2016/12/06 15:04:30, danilchap wrote: > do not move: > RtpHeaderExtensionMap doesn't have a move constructor because copy is as > expensive/cheap as move. Done. https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/call.cc#newcode1172 webrtc/call/call.cc:1172: // not be parsed, as FlexFEC is oblivious to the semantic meaning of the On 2016/12/06 15:04:30, danilchap wrote: > what about BWE header extensions? Extended comment to why this is not relevant for media packets. https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/flexfec_receive_s... File webrtc/call/flexfec_receive_stream.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/call/flexfec_receive_s... webrtc/call/flexfec_receive_stream.h:38: bool AddAndProcessReceivedPacket(RtpPacketReceived packet); On 2016/12/06 15:04:30, danilchap wrote: > to pass by value forward declaration is not enough. > include header with the definition or pass by const&. It did compile though? Moved the include to the header from the definition file. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/include/congestion_controller.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/include/congestion_controller.h:106: bool transport_cc, On 2016/12/06 15:04:30, danilchap wrote: > what does transport_cc mean? Before this comment, I actually didn't know. Now I know that it stands for "transport-wide congestion control". The variable name |transport_cc| is used in the configs of AudioReceiveStream and VideoReceiveStream, so I just reused the same here. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:19: #include "webrtc/modules/rtp_rtcp/include/flexfec_receiver.h" On 2016/12/06 15:04:30, danilchap wrote: > unrelated question: is it a self-include? Yes! This was removed in another CL, that this CL replaces. So I missed to remove it here. Thanks for finding! (This is from the days when there was both an interface and an implementation. That is no longer the case.) https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:50: bool AddAndProcessReceivedPacket(RtpPacketReceived packet); On 2016/12/06 15:04:30, danilchap wrote: > to pass by value forward declaration is not enough. > include header with the definition or pass by const&. Done. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:35: if (packet.HasExtension<TransportSequenceNumber>()) { On 2016/12/06 15:04:30, danilchap wrote: > may be > return packet.HasExtension<TransportSequenceNumber>() || > packet.HasExtension<AbsoluteSendTime>() || > packet.HasExtension<TransmissionOffset>(); Done. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:86: if (packet.size() < kRtpHeaderSize) { On 2016/12/06 15:04:30, danilchap wrote: > feel free to DCHECK now instead: RtpPacket should guarantee validity (including > size). Done. https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:502: TransportSequenceNumber::kId); On 2016/12/06 15:04:30, danilchap wrote: > sorry for confusing, but it is better not to use TransportSequenceNumber::kId as > an extension id to avoid more confusion. Use some (any) number instead (between > 1 and 14). > Done. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:522: CongestionController::UseSendSideBwe( Nisse: please check.
https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:522: CongestionController::UseSendSideBwe( On 2016/12/12 13:51:08, brandtr wrote: > Nisse: please check. Maybe the TODO item was badly worded. The thing is that in case UseSendSideBwe(config) is false, this argument is ignored by the callee (the AudioReceiveStream constructor), and the call to GetRemoteBitrateEstimator is useless. But it's no big deal because the call is cheap. unless you have made some related changes, I think it's still fine to always pass true to the GetRemoteBitrateEstimator method. And CongestionController::UseSendSideBwe is an internal function which shouldn't be used here, and if I remember correctly, it's deleted in one of my cl:s.
https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:522: CongestionController::UseSendSideBwe( On 2016/12/13 10:32:32, nisse-webrtc wrote: > On 2016/12/12 13:51:08, brandtr wrote: > > Nisse: please check. > > Maybe the TODO item was badly worded. The thing is that in case > UseSendSideBwe(config) is false, this argument is ignored by the callee (the > AudioReceiveStream constructor), and the call to GetRemoteBitrateEstimator is > useless. But it's no big deal because the call is cheap. > > unless you have made some related changes, I think it's still fine to always > pass true to the GetRemoteBitrateEstimator method. And > CongestionController::UseSendSideBwe is an internal function which shouldn't be > used here, and if I remember correctly, it's deleted in one of my cl:s. Got it, thanks. I've reverted this part of the patch. CongestionController::UseSendSideBwe is added by this CL :) See https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/congestion....
https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:272: // Has the RTP header extension been negotiated? Do you still need it? The version I deleted didn't live here, but in audio_receive_stream.cc, see https://codereview.webrtc.org/2516983004/diff/140001/webrtc/audio/audio_recei... For that change, we concluded that there wasn't any need to check the list of negotiated extensions, but instead just check what's in the parsed headers of each rtp packet. And then trust the parser to never insert anything which wasn't properly negotiated.
Thanks for quick feedback! https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:272: // Has the RTP header extension been negotiated? On 2016/12/13 11:46:42, nisse-webrtc wrote: > Do you still need it? The version I deleted didn't live here, but in > audio_receive_stream.cc, see > > https://codereview.webrtc.org/2516983004/diff/140001/webrtc/audio/audio_recei... > > For that change, we concluded that there wasn't any need to check the list of > negotiated extensions, but instead just check what's in the parsed headers of > each rtp packet. And then trust the parser to never insert anything which wasn't > properly negotiated. Ok, I understand. In this CL, I moved the implementation that existed in video_receive_stream.cc: https://codereview.webrtc.org/2553863003/diff/100001/webrtc/video/video_recei... If I understand this function correctly, we use it to make sure that we only return the RemoteEstimatorProxy whenever both the RTCP report block and the RTP header extensions have been negotiated properly. Do you mean that it's ok to only check that the RTCP report block has been negotiated instead? Or is the difference to your CL that if we do audio BWE, we only ever use send-side BWE? Whereas for video, we might be using some of the older BWEs. I also rely on the parsing to decide if a packet should go into the BWE, or not: https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/....
https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:272: // Has the RTP header extension been negotiated? On 2016/12/13 12:37:36, brandtr wrote: > On 2016/12/13 11:46:42, nisse-webrtc wrote: > > Do you still need it? The version I deleted didn't live here, but in > > audio_receive_stream.cc, see > > > > > https://codereview.webrtc.org/2516983004/diff/140001/webrtc/audio/audio_recei... > > > > For that change, we concluded that there wasn't any need to check the list of > > negotiated extensions, but instead just check what's in the parsed headers of > > each rtp packet. And then trust the parser to never insert anything which > wasn't > > properly negotiated. > > Ok, I understand. In this CL, I moved the implementation that existed in > video_receive_stream.cc: > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/video/video_recei... > > If I understand this function correctly, we use it to make sure that we only > return the RemoteEstimatorProxy whenever both the RTCP report block and the RTP > header extensions have been negotiated properly. Do you mean that it's ok to > only check that the RTCP report block has been negotiated instead? > > Or is the difference to your CL that if we do audio BWE, we only ever use > send-side BWE? Whereas for video, we might be using some of the older BWEs. > > I also rely on the parsing to decide if a packet should go into the BWE, or not: > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/.... I'm afraid I don't understand why the logic is different in audio_receive_stream.cc and video_receive_stream.cc. The former uses a check like if (config_.rtp.transport_cc && header.extension.hasTransportSequenceNumber) { in the DeliverRtp method. And that's the only use of the transport_cc flag in that file.
On 2016/12/13 12:59:14, nisse-webrtc wrote: > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... > File webrtc/modules/congestion_controller/congestion_controller.cc (right): > > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... > webrtc/modules/congestion_controller/congestion_controller.cc:272: // Has the > RTP header extension been negotiated? > On 2016/12/13 12:37:36, brandtr wrote: > > On 2016/12/13 11:46:42, nisse-webrtc wrote: > > > Do you still need it? The version I deleted didn't live here, but in > > > audio_receive_stream.cc, see > > > > > > > > > https://codereview.webrtc.org/2516983004/diff/140001/webrtc/audio/audio_recei... > > > > > > For that change, we concluded that there wasn't any need to check the list > of > > > negotiated extensions, but instead just check what's in the parsed headers > of > > > each rtp packet. And then trust the parser to never insert anything which > > wasn't > > > properly negotiated. > > > > Ok, I understand. In this CL, I moved the implementation that existed in > > video_receive_stream.cc: > > > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/video/video_recei... > > > > If I understand this function correctly, we use it to make sure that we only > > return the RemoteEstimatorProxy whenever both the RTCP report block and the > RTP > > header extensions have been negotiated properly. Do you mean that it's ok to > > only check that the RTCP report block has been negotiated instead? > > > > Or is the difference to your CL that if we do audio BWE, we only ever use > > send-side BWE? Whereas for video, we might be using some of the older BWEs. > > > > I also rely on the parsing to decide if a packet should go into the BWE, or > not: > > > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/.... > > I'm afraid I don't understand why the logic is different in > audio_receive_stream.cc and video_receive_stream.cc. The former uses a check > like > > if (config_.rtp.transport_cc && > header.extension.hasTransportSequenceNumber) { > > in the DeliverRtp method. And that's the only use of the transport_cc flag in > that file. It's my understanding that the audio path only uses send-side BWE, and therefore the logic can be simpler there. The video path supports multiple BWEs however (send-side, abs send time, ..), and therefore we need to ask the congestion controller for the right object, using the helper method UseSendSideBwe. Then we only feed that object with the RTP headers if the appropriate header extensions are present.
https://codereview.webrtc.org/2553863003/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:378: arrival_time_ms = clock_->TimeInMilliseconds(); It looks like Call::DeliverRtp takes a PacketTime argument, which I think should be used to set the arrival time of the packet. https://codereview.webrtc.org/2553863003/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:707: config, recovered_packet_receiver, remote_bitrate_estimator); As we concluded in our offline discussion, the FlexFecReceiveStream does not need to know about the RemoteBitrateEstimator. https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:266: bool CongestionController::UseSendSideBwe( If this function is still used, could it be moved to that .cc file?
for RtpPacketReceived that is a step towards my thoughts where it should be used. Next step would be to parse it at the start of the Call::DeliverRtp and update Audio/VideoStreams to take parsed packet instead of unparsed packet with packet_time. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... File webrtc/call/flexfec_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_unittest.cc:31: RtpPacketReceived ParsePacket(const uint8_t* data, size_t length) { taking rtc::ArrayView<const uint8_t> as a single parameter would make tests slightly shorter and safer. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_unittest.cc:33: packet.Parse(data, length); EXPECT_TRUE(packet.Parse(data, length)); to have better error message on malformed test. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_unittest.cc:172: kFlexfecTs[0], kFlexfecTs[1], kFlexfecTs[2], kFlexfecTs[3], 0xaa, 0xbb, 0xcc, 0xdd, // Flexfex rtp timestamp. probably more descriptive. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_unittest.cc:176: 0xbe, 0xde, /* magic constant */ 0x00, 0x01, /* length = 1 */ may be instead of inline comments use more lines: 0xbe, 0xde, // Magic constant for rtp header extensions. 0x00, 0x01 // Length of 1x32bit word. 0x71, // ID = 7, Length is (1+1) bytes. 0xab, 0xcd, // kTransportSequenceNumber. 0x00, // Padding. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:69: RemoteBitrateEstimator* remote_bitrate_estimator_; * const https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:45: RtpPacketReceived ParsePacket(const std::unique_ptr<T>& packet) { probably better to use const reference to object than reference to the smart pointer ParsePacket(const T& packet) { parsed_packet.Parse(packet.data, packet.length); } https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:501: auto media_packet_raw = media_packets.front().get(); may be use "auto*" to stress it is a raw pointer https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:503: media_packet.Parse(media_packet_raw->data, media_packet_raw->length); may be RtpPacketReceived media_packet = ParsePacket(*media_packets.front()); i.e. reuse ParsePacket helper function introduced in this cl. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:514: RtpPacketReceived fec_packet; ditto https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:95: // Notify BWE. why is it done all the way down here rather than in call?
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Please have a second look :) I have moved the BWE notification from FlexfecReceiver to Call. This is a better place for it. The downside is that I don't know how to test it currently. I would like to inject a MockRemoteBitrateEstimator somewhere, but AFAICT there is no place to do that. (*) Looking at the RtpStreamReceiver (where the BWE is notified of media packets), I can't find any unit tests...? Suggestions are welcome. (*) I could add a RemoteBitrateEstimator* to Call::Config, just for testing, but that doesn't feel right. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... File webrtc/call/flexfec_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_unittest.cc:31: RtpPacketReceived ParsePacket(const uint8_t* data, size_t length) { On 2016/12/13 14:11:24, danilchap wrote: > taking rtc::ArrayView<const uint8_t> as a single parameter would make tests > slightly shorter and safer. Done. Added a convenience ctor to rtp::Packet too. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_unittest.cc:33: packet.Parse(data, length); On 2016/12/13 14:11:24, danilchap wrote: > EXPECT_TRUE(packet.Parse(data, length)); > to have better error message on malformed test. Done. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:69: RemoteBitrateEstimator* remote_bitrate_estimator_; On 2016/12/13 14:11:24, danilchap wrote: > * const N/A. https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:45: RtpPacketReceived ParsePacket(const std::unique_ptr<T>& packet) { On 2016/12/13 14:11:24, danilchap wrote: > probably better to use const reference to object than reference to the smart > pointer > > ParsePacket(const T& packet) { > parsed_packet.Parse(packet.data, packet.length); > } Done. Could remove the templatization then too :) (std::unique_ptr is not covariant, so it was needed before.) https://codereview.webrtc.org/2553863003/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:378: arrival_time_ms = clock_->TimeInMilliseconds(); On 2016/12/13 13:58:27, philipel wrote: > It looks like Call::DeliverRtp takes a PacketTime argument, which I think should > be used to set the arrival time of the packet. Line 376 :) https://codereview.webrtc.org/2553863003/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:707: config, recovered_packet_receiver, remote_bitrate_estimator); On 2016/12/13 13:58:27, philipel wrote: > As we concluded in our offline discussion, the FlexFecReceiveStream does not > need to know about the RemoteBitrateEstimator. Yes, it makes more sense to put this in internal::Call. Not sure how I will test it though; there is currently no way to insert a mock BWE into internal::Call. https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:266: bool CongestionController::UseSendSideBwe( On 2016/12/13 13:58:27, philipel wrote: > If this function is still used, could it be moved to that .cc file? Moved it back to video_receive_stream.cc. When the receive media pipeline is upgrade to use RtpPacketReceived, it can be removed from there. https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:272: // Has the RTP header extension been negotiated? On 2016/12/13 12:59:14, nisse-webrtc wrote: > On 2016/12/13 12:37:36, brandtr wrote: > > On 2016/12/13 11:46:42, nisse-webrtc wrote: > > > Do you still need it? The version I deleted didn't live here, but in > > > audio_receive_stream.cc, see > > > > > > > > > https://codereview.webrtc.org/2516983004/diff/140001/webrtc/audio/audio_recei... > > > > > > For that change, we concluded that there wasn't any need to check the list > of > > > negotiated extensions, but instead just check what's in the parsed headers > of > > > each rtp packet. And then trust the parser to never insert anything which > > wasn't > > > properly negotiated. > > > > Ok, I understand. In this CL, I moved the implementation that existed in > > video_receive_stream.cc: > > > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/video/video_recei... > > > > If I understand this function correctly, we use it to make sure that we only > > return the RemoteEstimatorProxy whenever both the RTCP report block and the > RTP > > header extensions have been negotiated properly. Do you mean that it's ok to > > only check that the RTCP report block has been negotiated instead? > > > > Or is the difference to your CL that if we do audio BWE, we only ever use > > send-side BWE? Whereas for video, we might be using some of the older BWEs. > > > > I also rely on the parsing to decide if a packet should go into the BWE, or > not: > > > https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/.... > > I'm afraid I don't understand why the logic is different in > audio_receive_stream.cc and video_receive_stream.cc. The former uses a check > like > > if (config_.rtp.transport_cc && > header.extension.hasTransportSequenceNumber) { > > in the DeliverRtp method. And that's the only use of the transport_cc flag in > that file. The audio path has simpler logic because only send-side BWE is used there, if any BWE is used at all. (To my understanding.) In the video path, we need to support the old BWEs as well. Anyway, I have used your idea in internal::Call:NotifyBweOfReceivedPacket :) https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/2553863003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:95: // Notify BWE. On 2016/12/13 14:11:24, danilchap wrote: > why is it done all the way down here rather than in call? I forgot that Call demuxes based on SSRC, so we could notify the BWE at that level. The reason that I put it here originally is that FlexfecReceiver also demuxes based on SSRC, whereas FlexfecReceiveStream does not.
Rebase onto master.
On 2016/12/14 12:55:49, brandtr wrote:
> Please have a second look :)
>
> I have moved the BWE notification from FlexfecReceiver to Call. This is a
better
> place for it.
>
> The downside is that I don't know how to test it currently. I would like to
> inject a MockRemoteBitrateEstimator somewhere, but AFAICT there is no place to
> do that. (*) Looking at the RtpStreamReceiver (where the BWE is notified of
> media packets), I can't find any unit tests...? Suggestions are welcome.
I guess it ought to be passed to the CongestionController constructor. It
currently gets a RemoteBitrateObserver and constructs a the
REmoteBitrateEstimator using
remote_bitrate_estimator_(
new WrappingBitrateEstimator(remote_bitrate_observer, clock_)),
I don't understand the distinction between observer and estimator here, but it's
not enough to pass in a mock RemoteBitrateObserver?
On 2016/12/14 13:12:56, nisse-webrtc wrote: > On 2016/12/14 12:55:49, brandtr wrote: > > Please have a second look :) > > > > I have moved the BWE notification from FlexfecReceiver to Call. This is a > better > > place for it. > > > > The downside is that I don't know how to test it currently. I would like to > > inject a MockRemoteBitrateEstimator somewhere, but AFAICT there is no place to > > do that. (*) Looking at the RtpStreamReceiver (where the BWE is notified of > > media packets), I can't find any unit tests...? Suggestions are welcome. > > I guess it ought to be passed to the CongestionController constructor. It > currently gets a RemoteBitrateObserver and constructs a the > REmoteBitrateEstimator using > > remote_bitrate_estimator_( > new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), > > I don't understand the distinction between observer and estimator here, but it's > not enough to pass in a mock RemoteBitrateObserver? Right, so you mean that I would pass in a partially mocked CongestionController here: https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?q=call.c... ? It might be hard to instantiate internal::Call outside of call.cc though, as it is declared in there :( I guess I could create a special Call::Create method for that.
https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:379: uint32_t ssrc = ByteReader<uint32_t>::ReadBigEndian(&packet[8]); there is no need to hacky-extract ssrc before parsing, There is IdentifyExtensions function exactly for this usecase: RtpPacketReceived parsed_packet(nullptr); if (!parsed_packet.Parse(packet, length)) return; auto it = extensions_.find(parsed_packet->Ssrc()); if (it != extensions_.end()) { parsed_packet.IdentifyExtensions(&it->second); } https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:384: // The existence of |it->second| is guaranteed during the lifetime of if it is a problem, file a bug and assign it to me. I'll then adjust RtpPacket not to hold pointer to the RtpHeaderExtensionMap. https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1246: // Other BWE. may be 'Receive-side' instead of 'Other' https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... File webrtc/call/flexfec_receive_stream.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream.cc:113: return receiver_->AddAndProcessReceivedPacket(packet); either move or pass by const& https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... File webrtc/call/flexfec_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream_unittest.cc:110: rtc::ArrayView<const uint8_t>(kFlexfecPacket, kFlexfecPacketLength))); just ParsePacket(kFlexfecPacket) ArrayView would do the rest. https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream_unittest.cc:116: receive_stream.AddAndProcessReceivedPacket(ParsePacket( ditto https://codereview.webrtc.org/2553863003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: if (!AddReceivedPacket(packet)) { either move or pass by const&
https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:379: uint32_t ssrc = ByteReader<uint32_t>::ReadBigEndian(&packet[8]); On 2016/12/14 13:39:02, danilchap wrote: > there is no need to hacky-extract ssrc before parsing, > There is IdentifyExtensions function exactly for this usecase: > > RtpPacketReceived parsed_packet(nullptr); > if (!parsed_packet.Parse(packet, length)) return; > auto it = extensions_.find(parsed_packet->Ssrc()); > if (it != extensions_.end()) { > parsed_packet.IdentifyExtensions(&it->second); > } Great! Then I don't need all the stuff with the pointer below. https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:384: // The existence of |it->second| is guaranteed during the lifetime of On 2016/12/14 13:39:02, danilchap wrote: > if it is a problem, file a bug and assign it to me. I'll then adjust RtpPacket > not to hold pointer to the RtpHeaderExtensionMap. Shouldn't be a problem right now, due to the lock and the implementation of rtp::Packet. It is a little brittle to rely on that for the future though, so if it isn't too much work I think it would be nicer if IdentifyExtensions didn't store the supplied pointer internally. (I think it's only used in the Parse and IdentifyExtensions methods?) https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1246: // Other BWE. On 2016/12/14 13:39:02, danilchap wrote: > may be 'Receive-side' instead of 'Other' Done. https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... File webrtc/call/flexfec_receive_stream.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream.cc:113: return receiver_->AddAndProcessReceivedPacket(packet); On 2016/12/14 13:39:02, danilchap wrote: > either move or pass by const& I was going to ask you about that in the email, but forgot. After studying rtp_packet.h, I got the feeling that rtp::Packet should be treated as a value type, since there are no explicitly defined constructors and since the main data is carried by the COW buffer. Anyway, I've added back the std::move's! I like std::move better than const& here, since when the packet has been passed down to FlexfecReceiver, a copy is made of it, and the original packet is thrown away. With the move's, it's clear where the packet is thrown away. https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... File webrtc/call/flexfec_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream_unittest.cc:110: rtc::ArrayView<const uint8_t>(kFlexfecPacket, kFlexfecPacketLength))); On 2016/12/14 13:39:02, danilchap wrote: > just ParsePacket(kFlexfecPacket) > ArrayView would do the rest. Nice! https://codereview.webrtc.org/2553863003/diff/180001/webrtc/call/flexfec_rece... webrtc/call/flexfec_receive_stream_unittest.cc:116: receive_stream.AddAndProcessReceivedPacket(ParsePacket( On 2016/12/14 13:39:02, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2553863003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/2553863003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:51: if (!AddReceivedPacket(packet)) { On 2016/12/14 13:39:02, danilchap wrote: > either move or pass by const& Done.
RtpPacketReceived usage lgtm %nit https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:379: RtpPacketReceived parsed_packet(nullptr); sorry for incomplete previous comment, there is actually a default constructor for RtpPacketReceived that looks more suitable here.
lgtm
https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:379: RtpPacketReceived parsed_packet(nullptr); On 2016/12/15 10:23:47, danilchap wrote: > sorry for incomplete previous comment, there is actually a default constructor > for RtpPacketReceived that looks more suitable here. Done. https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:386: // |parsed_packet|, due to us holding the |receive_crit_| lock in the Will update here when the RtpPacket CL has landed.
Rebase fixes: flexfec_receive_stream.h moved out of api/call/.
https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1222: void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet) { Should we make this a method on CongestionController instead so that we can make the RBE and the REP internal to the CongestionController later?
https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1222: void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet) { On 2016/12/19 11:03:42, stefan-webrtc (holmer) wrote: > Should we make this a method on CongestionController instead so that we can make > the RBE and the REP internal to the CongestionController later? Sounds good to me. https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1228: if (!transport_wide && !abs_send_time && !t_offset) Is this too strict for the older BWEs? (For send-side BWE, we should check that the transport_wide extension is set before notifying the BWE. For the older BWEs, this is not always strictly necessary.)
https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1228: if (!transport_wide && !abs_send_time && !t_offset) On 2016/12/19 13:26:17, brandtr wrote: > Is this too strict for the older BWEs? > > (For send-side BWE, we should check that the transport_wide extension is set > before notifying the BWE. For the older BWEs, this is not always strictly > necessary.) Yes, too strict. The old BWE can take RTP timestamps without header extensions at all. https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:202: void CongestionController::NotifyBweOfReceivedPacket( Call this method "OnReceivedPacket" https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:206: const bool t_offset = packet.HasExtension<TransmissionOffset>(); One more tiny request... Could we move out the rtp specific stuff here and only pass in arrival time, payload size and, for now, the header?
Patchset #13 (id:360001) has been deleted
https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2553863003/diff/220001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1228: if (!transport_wide && !abs_send_time && !t_offset) On 2016/12/19 14:03:31, stefan-webrtc (holmer) wrote: > On 2016/12/19 13:26:17, brandtr wrote: > > Is this too strict for the older BWEs? > > > > (For send-side BWE, we should check that the transport_wide extension is set > > before notifying the BWE. For the older BWEs, this is not always strictly > > necessary.) > > Yes, too strict. The old BWE can take RTP timestamps without header extensions > at all. Done. https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:202: void CongestionController::NotifyBweOfReceivedPacket( On 2016/12/19 14:03:31, stefan-webrtc (holmer) wrote: > Call this method "OnReceivedPacket" Done. Alternatively, it could also be called "IncomingPacket", and would then have the same signature as RemoteBitrateEstimator::IncomingPacket. https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:206: const bool t_offset = packet.HasExtension<TransmissionOffset>(); On 2016/12/19 14:03:31, stefan-webrtc (holmer) wrote: > One more tiny request... > > Could we move out the rtp specific stuff here and only pass in arrival time, > payload size and, for now, the header? Done.
lgtm https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2553863003/diff/340001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:202: void CongestionController::NotifyBweOfReceivedPacket( On 2016/12/19 14:26:53, brandtr wrote: > On 2016/12/19 14:03:31, stefan-webrtc (holmer) wrote: > > Call this method "OnReceivedPacket" > > Done. Alternatively, it could also be called "IncomingPacket", and would then > have the same signature as RemoteBitrateEstimator::IncomingPacket. Agree, but now it resembles OnSentPacket below, which I also find nice. :)
The CQ bit was checked by brandtr@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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by brandtr@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, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15767) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/21127) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20439) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11598)
Rebase.
The CQ bit was checked by brandtr@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.
On 2016/12/14 13:12:56, nisse-webrtc wrote: > On 2016/12/14 12:55:49, brandtr wrote: > > Please have a second look :) > > > > I have moved the BWE notification from FlexfecReceiver to Call. This is a > better > > place for it. > > > > The downside is that I don't know how to test it currently. I would like to > > inject a MockRemoteBitrateEstimator somewhere, but AFAICT there is no place to > > do that. (*) Looking at the RtpStreamReceiver (where the BWE is notified of > > media packets), I can't find any unit tests...? Suggestions are welcome. > > I guess it ought to be passed to the CongestionController constructor. It > currently gets a RemoteBitrateObserver and constructs a the > REmoteBitrateEstimator using > > remote_bitrate_estimator_( > new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), > > I don't understand the distinction between observer and estimator here, but it's > not enough to pass in a mock RemoteBitrateObserver? I used this idea to make a rudimentary CongestionController unit test. It's not super clean, but at least it gives some coverage to CongestionController::OnReceivedPacket.
On 2016/12/20 09:50:51, brandtr wrote: > On 2016/12/14 13:12:56, nisse-webrtc wrote: > > On 2016/12/14 12:55:49, brandtr wrote: > > > Please have a second look :) > > > > > > I have moved the BWE notification from FlexfecReceiver to Call. This is a > > better > > > place for it. > > > > > > The downside is that I don't know how to test it currently. I would like to > > > inject a MockRemoteBitrateEstimator somewhere, but AFAICT there is no place > to > > > do that. (*) Looking at the RtpStreamReceiver (where the BWE is notified of > > > media packets), I can't find any unit tests...? Suggestions are welcome. > > > > I guess it ought to be passed to the CongestionController constructor. It > > currently gets a RemoteBitrateObserver and constructs a the > > REmoteBitrateEstimator using > > > > remote_bitrate_estimator_( > > new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), > > > > I don't understand the distinction between observer and estimator here, but > it's > > not enough to pass in a mock RemoteBitrateObserver? > > I used this idea to make a rudimentary CongestionController unit test. It's not > super clean, but at least it gives some coverage to > CongestionController::OnReceivedPacket. lgtm
The CQ bit was checked by brandtr@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.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2553863003/#ps460001 (title: "Add basic CongestionController unit test, based on nisse's suggestion.")
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": 460001, "attempt_start_ts": 1482233507261840,
"parent_rev": "b36ee8d498be2fa58fde3f3f3d69a74e4d3b817d", "commit_rev":
"ab2ffa3b28b55ef359232723049fb88b2dcd807a"}
Message was sent while issue was closed.
Description was changed from ========== Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 ========== to ========== Parse FlexFEC RTP headers in Call and add integration with BWE. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2553863003 Cr-Commit-Position: refs/heads/master@{#15709} Committed: https://chromium.googlesource.com/external/webrtc/+/ab2ffa3b28b55ef3592327230... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:460001) as https://chromium.googlesource.com/external/webrtc/+/ab2ffa3b28b55ef3592327230...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:460001) has been created in https://codereview.webrtc.org/2589393002/ by brandtr@webrtc.org. The reason for reverting is: Unexpected perf regressions..
Message was sent while issue was closed.
Original patch.
Message was sent while issue was closed.
Patchset #18 (id:480001) has been deleted |
