Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490523002:
Wire up FlexfecSender in RTPSenderVideo. (Closed)
DescriptionWire up FlexfecSender in RTPSenderVideo.
This CL adds the ability for RTPSenderVideo to generate and send
FlexFEC packets.
BUG=webrtc:5654
Committed: https://crrev.com/131bc498e65a29e4224648d6d6b72abd3de87592
Cr-Commit-Position: refs/heads/master@{#15016}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Feedback response 1. #
Total comments: 4
Patch Set 3 : Feedback response 2. #
Total comments: 6
Patch Set 4 : Rebase. #Patch Set 5 : Feedback response 3. #
Total comments: 8
Patch Set 6 : Feedback response 4. #
Dependent Patchsets: Messages
Total messages: 29 (13 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org
Please have a look :)
https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:137: uint16_t num_fec_packets = ulpfec_generator_.NumAvailableFecPackets(); probably cleaner to hide this statement (and thus next block) under if (ulpfec_enabled()) too. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:182: std::vector<std::unique_ptr<RtpPacketToSend>> RTPSenderVideo::GetFlexfecPackets( try to order functions same as in header file, i.e. move these two functions to the bottom of the .cc https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:203: constexpr StorageType kFecStorage = kDontRetransmit; the enums values probably self-explained already, extra const (specially constexpr) feels redundant. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:208: // TODO(brandtr): Wire up stats here. may be resolve this TODO right away? or is it more complicated than copying two lines from similar codepath for ulpfec https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:344: if (flexfec_enabled()) { in this file it is more common to omit {} for one-line if block. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:378: // Note that we either send non-encapsulated media packets with FlexFEC this comments feels like repeating (not explaining) code just below, i.e. redundant. May be remove it or reduce it to part that is not in code below: mentioning that ulpfec hides inside ...AsRed function. (Or rename or split SendVideoPacketAsRed to SendVideoPacketWithUlpfec) https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:392: SendFlexfecPackets(&fec_packets); may be cleaner to change SendFlexfecPackets to take fec_packets by value. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right): https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:95: std::vector<std::unique_ptr<RtpPacketToSend>> GetFlexfecPackets( this function do more than get fec packets. May be extend name to note it updates flexfec state, or merge this and flexfec block inside SendVideo into one function SendVideoPacketWithFlexFec.
Rebase.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:137: uint16_t num_fec_packets = ulpfec_generator_.NumAvailableFecPackets(); On 2016/11/08 11:45:08, danilchap wrote: > probably cleaner to hide this statement (and thus next block) under if > (ulpfec_enabled()) too. Makes sense. Done. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:182: std::vector<std::unique_ptr<RtpPacketToSend>> RTPSenderVideo::GetFlexfecPackets( On 2016/11/08 11:45:08, danilchap wrote: > try to order functions same as in header file, i.e. move these two functions to > the bottom of the .cc I changed the .h file instead :) Now at least SendVideoPacket, SendVideoPacketAsRedMaybeWithUlpfec, and SendVideoPacketWithFlexfec are locally ordered. The member functions in this .cc file are not globally ordered though :/ https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:203: constexpr StorageType kFecStorage = kDontRetransmit; On 2016/11/08 11:45:08, danilchap wrote: > the enums values probably self-explained already, extra const (specially > constexpr) feels redundant. Removed. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:208: // TODO(brandtr): Wire up stats here. On 2016/11/08 11:45:08, danilchap wrote: > may be resolve this TODO right away? or is it more complicated than copying two > lines from similar codepath for ulpfec Yes, it's more complicated, since I want to do a concerted effort to design the stats for FlexFEC. At the same time, I will probably rename the old stats. I haven't gotten that far in the design yet, so therefore I leave this TODO here. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:344: if (flexfec_enabled()) { On 2016/11/08 11:45:08, danilchap wrote: > in this file it is more common to omit {} for one-line if block. Yep, removed. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:378: // Note that we either send non-encapsulated media packets with FlexFEC On 2016/11/08 11:45:08, danilchap wrote: > this comments feels like repeating (not explaining) code just below, i.e. > redundant. May be remove it or reduce it to part that is not in code below: > mentioning that ulpfec hides inside ...AsRed function. > (Or rename or split SendVideoPacketAsRed to SendVideoPacketWithUlpfec) Agree that the comment is a bit superfluous. I have renamed SendVideoPacketAsRed to better reflect what it is doing. Note that this convoluted name is sort of necessary, since we may use RED without ULPFEC currently. (We will remove this possibility in the future, but that will take at least ~3 Chrome versions to do.) https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:392: SendFlexfecPackets(&fec_packets); On 2016/11/08 11:45:08, danilchap wrote: > may be cleaner to change SendFlexfecPackets to take fec_packets by value. Yes, that would have been nicer. Now that function doesn't exist any more though. https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right): https://codereview.webrtc.org/2490523002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:95: std::vector<std::unique_ptr<RtpPacketToSend>> GetFlexfecPackets( On 2016/11/08 11:45:08, danilchap wrote: > this function do more than get fec packets. > May be extend name to note it updates flexfec state, > or merge this and flexfec block inside SendVideo into one function > SendVideoPacketWithFlexFec. I merged it into one function, because then the code in SendVideo looks more symmetric between the three cases (FlexFEC, RED/ULPFEC, and no FEC).
lgtm https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right): https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:83: bool protect_media_packet); the name of this variable become longer each patchset :)
https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:255: (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize); this should probably be (flexfec_sender_->RtpHeaderLength() - kRtpHeaderSize) since flexfec might have less rtp headers than media
Please have a look again :) https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:255: (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize); On 2016/11/09 11:21:58, danilchap wrote: > this should probably be > (flexfec_sender_->RtpHeaderLength() - kRtpHeaderSize) > since flexfec might have less rtp headers than media Good point! This is a bit of an optimization (i.e., the current code should work fine, because it gives a bound on the overhead), but might as well do that now. Could you double check my arithmetic here? I split it up in two terms, but the end result is the same as your suggestion. (This code could be prone to sign errors, but I think it should be correct...) https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right): https://codereview.webrtc.org/2490523002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:83: bool protect_media_packet); On 2016/11/09 11:19:07, danilchap wrote: > the name of this variable become longer each patchset :) Not anymore! :)
https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:92: rtp_header_length_ = rtp_header.headers_size(); that should be simpler: rtp_extra_header_length_ = rtp_header_extension_map_.GetTotalLengthInBytes(); (since you do not use csrcs in the flexfec, the only header size over base 12 bytes are extensions) https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:259: + (media_rtp_header_length - kRtpHeaderSize) // Media header exts. though this is correct, I do not understand complexity: why add and subtract media_rtp_header_length that shouldn't be part of FEC overhead? just add flexfec->RtpExtraHeaderLength()
https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:60: size_t RtpHeaderLength() const { return rtp_header_length_; } might as well include rtp header extension size into already existent MaxPacketOverhead function
Rebase.
https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:60: size_t RtpHeaderLength() const { return rtp_header_length_; } On 2016/11/09 16:23:47, danilchap wrote: > might as well include rtp header extension size into already existent > MaxPacketOverhead function This made things much cleaner. Done. https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:92: rtp_header_length_ = rtp_header.headers_size(); On 2016/11/09 15:58:46, danilchap wrote: > that should be simpler: > rtp_extra_header_length_ = rtp_header_extension_map_.GetTotalLengthInBytes(); > (since you do not use csrcs in the flexfec, the only header size over base 12 > bytes are extensions) Nice, done. https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2490523002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:259: + (media_rtp_header_length - kRtpHeaderSize) // Media header exts. On 2016/11/09 15:58:46, danilchap wrote: > though this is correct, I do not understand complexity: > why add and subtract media_rtp_header_length that shouldn't be part of FEC > overhead? > just add flexfec->RtpExtraHeaderLength() Refer to offline discussion.
lgtm https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:47: map.Register(kRtpExtensionTransportSequenceNumber, extension.id); I would like to remove this version of Register eventually, can you write instead if (extension.uri == TransportSequenceNumber::kUri) { map.Register<TransportSequenceNumber>(extension.id); } (template version also slightly faster, thought that is negligible) https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:81: FilterRtpHeaderExtensions(rtp_header_extensions)) { maybe call this function 'RegisterBweExtensions' so that it would fit on one line. https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:164: // For FlexFEC, the overhead is the FEC header, as well as the BWE header exts. may be instead of shortering word remove redundant words: // The overhead is BWE rtp header extensions and FlexFEC header. https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:167: rtp_header_extension_map_.GetTotalLengthInBytes(); really tiny nit: may be sum other way around since in a raw packet extensions go before flexfec header.
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/...
Patchset #6 (id:140001) has been deleted
https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:47: map.Register(kRtpExtensionTransportSequenceNumber, extension.id); On 2016/11/10 11:29:23, danilchap wrote: > I would like to remove this version of Register eventually, > can you write instead > if (extension.uri == TransportSequenceNumber::kUri) { > map.Register<TransportSequenceNumber>(extension.id); > } > (template version also slightly faster, thought that is negligible) Done. https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:81: FilterRtpHeaderExtensions(rtp_header_extensions)) { On 2016/11/10 11:29:23, danilchap wrote: > maybe call this function 'RegisterBweExtensions' > so that it would fit on one line. Done. https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:164: // For FlexFEC, the overhead is the FEC header, as well as the BWE header exts. On 2016/11/10 11:29:23, danilchap wrote: > may be instead of shortering word remove redundant words: > // The overhead is BWE rtp header extensions and FlexFEC header. Done. https://codereview.webrtc.org/2490523002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:167: rtp_header_extension_map_.GetTotalLengthInBytes(); On 2016/11/10 11:29:23, danilchap wrote: > really tiny nit: may be sum other way around since in a raw packet extensions go > before flexfec header. I like that! These things make the code easier to interpret, IMO. Thanks for the comment.
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 danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2490523002/#ps160001 (title: "Feedback response 4.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Wire up FlexfecSender in RTPSenderVideo. This CL adds the ability for RTPSenderVideo to generate and send FlexFEC packets. BUG=webrtc:5654 ========== to ========== Wire up FlexfecSender in RTPSenderVideo. This CL adds the ability for RTPSenderVideo to generate and send FlexFEC packets. BUG=webrtc:5654 Committed: https://crrev.com/131bc498e65a29e4224648d6d6b72abd3de87592 Cr-Commit-Position: refs/heads/master@{#15016} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/131bc498e65a29e4224648d6d6b72abd3de87592 Cr-Commit-Position: refs/heads/master@{#15016} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
