Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by brandtr Modified:
4 years, 1 month ago Reviewers:
danilchap, stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd FlexfecSender.
This class will interface RTPSenderVideo with the underlying
erasure code. It is functionally similar to ProducerFec
(to be renamed UlpfecGenerator). In fact, the FlexfecSender is a
friend of ProducerFec, and reuses most of its implementation.
Besides the fact that FlexfecSender outputs FlexFEC packets,
the main difference with ProducerFec is that FlexfecSender
allocates RTP sequence numbers, whereas ProducerFec does not
do this for the RED-encapsulated ULPFEC packets.
This class is split as interface/implementation, since it will
be owned by VideoSendStream initially. Further along, it may be
owned by PacedSender.
BUG=webrtc:5654
Committed: https://crrev.com/c295e00fa0a9774acbadf5d54389b46b1c17965e
Cr-Commit-Position: refs/heads/master@{#14922}
Patch Set 1 #
Total comments: 38
Patch Set 2 : Feedback response 1. #
Total comments: 4
Patch Set 3 : Feedback response 2. #
Total comments: 18
Patch Set 4 : Feedback response 3. #Patch Set 5 : Rename ulpfec_sender_ -> ulpfec_generator_. #Patch Set 6 : DCHECK on unprotected media SSRC added instead. #
Total comments: 6
Patch Set 7 : Feedback response 4. #Patch Set 8 : Rebase. #Patch Set 9 : Rebase + fixes. #
Total comments: 10
Patch Set 10 : Feedback response 5. #Patch Set 11 : Remove interface. #Patch Set 12 : Fix GYP file. #Patch Set 13 : Rebase. #
Created: 4 years, 1 month ago
Dependent Patchsets: Messages
Total messages: 37 (12 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
Please have a look :) https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:20: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h" I guess the RtpPacket classes will become "public" when the packetization module project is done?
https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:20: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h" On 2016/10/20 13:19:42, brandtr wrote: > I guess the RtpPacket classes will become "public" when the packetization module > project is done? Either it will go into /include/ of some module (rtp_rtcp or new one) or all rtp specific code (including this FlexfecSender) will gather inside rtp_rtcp. Either way this include should be fine. https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:27: class FlexfecSender { this class doesn't send anything, it only generates fec packets. Does name reflect future plans, i.e. do you plan to emit packets via some kind of transport? https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:39: virtual void SetFecParameters(const FecProtectionParams* params) = 0; may be pass by const& (since internally it is assumed to be not null and not stored by pointer) https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:44: virtual int AddRtpPacketAndGenerateFec(RtpPacketToSend* packet) = 0; what does this function return? https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:27: constexpr int kMsToRtpTimestamp = 90; does the frequency has to be same as for video stream? https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:45: rtp_header_extensions, clock)); std::move(rtp_header_extensions) (otherwise vector will be copied) or pass that vector by const& (since you do not store it) https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:70: // Register RTP header extensions. Does FlexfecSender has to know full list of bwe extensions? RTPSender need to know that list since it is RTPSender that set them. Not sure if it is currently avoidable (without creating dedicated RTPSender just for flexfec and keeping that logic there) look like TODO for me to provide better interface for supported extensions lists. https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:98: int FlexfecSenderImpl::AddRtpPacketAndGenerateFec(RtpPacketToSend* packet) { does parameter need to be non-const? May be it can it be const& https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:129: static_cast<uint32_t>(clock_->CurrentNtpInMilliseconds())); Does it have to be based on ntp? may be base it on TimeInMilliseconds (or microseconds) (advantage: that one is monotonic, ntp is not) either way do cast after multiplication for proper wrapping. Or (in case you want to keep it ntp based) use function from rtp_rtcp/source/time_util.h: NtpToRtp(NtpTime(*clock_), kSToRtpTimestamp) or CurrentRtp(*clock_, kSToRtpTimestamp) (frequency should be per second then, i.e. 90000) https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:131: // Reserve extensions, if registered. These will be set by the RtpSender. fec_packet_to_send->set_capture_time_ms(...) otherwise TransmissionOffset will not be set by RTPSender https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:148: std::stringstream ss; probably better to avoid stringstream: Currently string will be always generated, even if later discarded. if logged directly to LOG(LS_INFO) and log disabled, all operator<< will be mostly noop. https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:36: // Implements FlexfecSender. add override keyword to functions below https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right): https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:94: EXPECT_GE(fec_packet->payload_size(), kPayloadLength); constant 1st for EXPECT macros (i.e. EXPECT_LE(kPayloadLength, fec_packet->payload_size()); https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:101: FecProtectionParams params = {15, 3, kFecMaskRandom}; Are values 15 and 3 important? If they are, may be initialize params in more explicit way: params.member1 = 15; params.member2 = 3; params.mask_type = kFecMaskRandom; If they are not, may be create a global constant above const FecProtectionParams kFecParamsMayBeWithDescription = {15, 3, kFecMaskRandom}; https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:195: EXPECT_TRUE(fec_packet->ReserveExtension<AbsoluteSendTime>()); may be fec_packet->HasExtension<AbsoluteSendTime>() (ReserveExtension return true if it can be reserved. HasExtension return true if it was reserved). https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec.h:45: friend class FlexfecSenderImpl; do you plan to remove this friendship later, after renaming class to UlpfecSender? e.g. by moving commong part of ProducerFec and FlexfecSender into a different (base or helper) class.
Rebase.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Addressed comments from danilchap. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:20: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h" On 2016/10/20 15:12:57, danilchap wrote: > On 2016/10/20 13:19:42, brandtr wrote: > > I guess the RtpPacket classes will become "public" when the packetization > module > > project is done? > > Either it will go into /include/ of some module (rtp_rtcp or new one) or all rtp > specific code (including this FlexfecSender) will gather inside rtp_rtcp. > Either way this include should be fine. Right. But in the latter case (all RTP code in the rtp_rtcp module), this interface will still be included from outside the rtp_rtcp module. Specifically, either from VideoSendStream (with current design) or from PacedSender (with future design). Or do you mean that even those classes would be part of the expanded rtp_rtcp module? https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:27: class FlexfecSender { On 2016/10/20 15:12:57, danilchap wrote: > this class doesn't send anything, it only generates fec packets. > Does name reflect future plans, i.e. do you plan to emit packets via some kind > of transport? If your definition of "sending packets" is "emitting packets through a transport", then no, this class does not currently send packets. The purpose of this class is to obtain RTP media packets, decide when it's time to generate FEC packets, and then forward the generated FEC packets to some other class that will push them to the network. Currently, I'm reusing parts of the RTPSender pipeline for the latter stages (forwarding the FEC packets to the network). The reason is that then I can easily reuse our current logic for setting the FEC rates, which happens through RTPSender. In the future, FlexfecSender will be connected to PacedSender and the API will change: the PacedSender will issue a call to FlexfecSender when it's time to generate FEC, and FlexfecSender will forward generated FlexFEC packets (with a suitable FEC rate) directly to a transport. So then, yes, this class will "send" packets. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:39: virtual void SetFecParameters(const FecProtectionParams* params) = 0; On 2016/10/20 15:12:57, danilchap wrote: > may be pass by const& (since internally it is assumed to be not null and not > stored by pointer) Agree. I just replicated the interface from ProducerFec here. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:44: virtual int AddRtpPacketAndGenerateFec(RtpPacketToSend* packet) = 0; On 2016/10/20 15:12:57, danilchap wrote: > what does this function return? Added description. Also made parameter const& based on other comment. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:27: constexpr int kMsToRtpTimestamp = 90; On 2016/10/20 15:12:57, danilchap wrote: > does the frequency has to be same as for video stream? It doesn't have to be, but it's "RECOMMENDED" that the FlexFEC timestamp rate matches that of the protected media stream, which currently only is video. (When we support protecting audio, we might do that in combination with protecting video, so then the RTP timestamp rate should probably still be 90 kHz.) I have updated the comment and replaced the "90" literal with a constant from rtp_rtcp_defines.h. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:45: rtp_header_extensions, clock)); On 2016/10/20 15:12:57, danilchap wrote: > std::move(rtp_header_extensions) > (otherwise vector will be copied) > or pass that vector by const& (since you do not store it) Yep, this should be const&. Thanks! https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:70: // Register RTP header extensions. On 2016/10/20 15:12:57, danilchap wrote: > Does FlexfecSender has to know full list of bwe extensions? > RTPSender need to know that list since it is RTPSender that set them. > Not sure if it is currently avoidable (without creating dedicated RTPSender just > for flexfec and keeping that logic there) > look like TODO for me to provide better interface for supported extensions > lists. Yes, it needs to know about all BWE extensions, since we are not reusing the RTP header extension map from the RTPSender here, but we do call ReserveExtension on the newly create RtpPackets. I agree that it's not super nice to list a bunch of header extensions here however, but that seemed like a simple solution for me. With the current design (FlexfecSender connected to RTPSender), we _could_ have a method "RegisterExtension" in the FlexfecSender interface, which would be called from RTPSender when that registers an extension. I'm not sure its worth it though, because with the new design (FlexfecSender connected to PacedSender), the FlexfecSender object would probably be created by Call, where the RTP header extensions are available as a std::vector<RtpExtension> in the VideoSendStream config. What do you think? Creating a dedicated RTPSender for FlexFEC wouldn't work, since the FlexfecSender would then not have access to the RTP media packets created by the "media RTPSender". https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:98: int FlexfecSenderImpl::AddRtpPacketAndGenerateFec(RtpPacketToSend* packet) { On 2016/10/20 15:12:57, danilchap wrote: > does parameter need to be non-const? May be it can it be const& Good idea, done! https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:129: static_cast<uint32_t>(clock_->CurrentNtpInMilliseconds())); On 2016/10/20 15:12:57, danilchap wrote: > Does it have to be based on ntp? may be base it on TimeInMilliseconds (or > microseconds) (advantage: that one is monotonic, ntp is not) > either way do cast after multiplication for proper wrapping. > Or (in case you want to keep it ntp based) use function from > rtp_rtcp/source/time_util.h: > NtpToRtp(NtpTime(*clock_), kSToRtpTimestamp) > or > CurrentRtp(*clock_, kSToRtpTimestamp) > (frequency should be per second then, i.e. 90000) Cool, didn't think about the monotonicity. It can be based on TimeInMilliseconds, so changing to that. Also moving the multiplication inside the cast. Note that the multiplication is outside the cast in the place where I got inspired: https://cs.chromium.org/chromium/src/third_party/webrtc/video/vie_encoder.cc?... https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:131: // Reserve extensions, if registered. These will be set by the RtpSender. On 2016/10/20 15:12:57, danilchap wrote: > fec_packet_to_send->set_capture_time_ms(...) > otherwise TransmissionOffset will not be set by RTPSender Done. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:148: std::stringstream ss; On 2016/10/20 15:12:57, danilchap wrote: > probably better to avoid stringstream: > Currently string will be always generated, even if later discarded. > if logged directly to LOG(LS_INFO) and log disabled, all operator<< will be > mostly noop. Done. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:36: // Implements FlexfecSender. On 2016/10/20 15:12:57, danilchap wrote: > add override keyword to functions below Done! (Usually, the compiler complains about missing overrides. But not here...?) https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:94: EXPECT_GE(fec_packet->payload_size(), kPayloadLength); On 2016/10/20 15:12:58, danilchap wrote: > constant 1st for EXPECT macros (i.e. > EXPECT_LE(kPayloadLength, fec_packet->payload_size()); Done. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:101: FecProtectionParams params = {15, 3, kFecMaskRandom}; On 2016/10/20 15:12:58, danilchap wrote: > Are values 15 and 3 important? > If they are, may be initialize params in more explicit way: > params.member1 = 15; > params.member2 = 3; > params.mask_type = kFecMaskRandom; > If they are not, may be create a global constant above > const FecProtectionParams kFecParamsMayBeWithDescription = {15, 3, > kFecMaskRandom}; Yes, they are important. Clarified according to your suggestion. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:195: EXPECT_TRUE(fec_packet->ReserveExtension<AbsoluteSendTime>()); On 2016/10/20 15:12:58, danilchap wrote: > may be fec_packet->HasExtension<AbsoluteSendTime>() > (ReserveExtension return true if it can be reserved. HasExtension return true if > it was reserved). Of course! Fixed. https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/producer_fec.h:45: friend class FlexfecSenderImpl; On 2016/10/20 15:12:58, danilchap wrote: > do you plan to remove this friendship later, after renaming class to > UlpfecSender? > e.g. by moving commong part of ProducerFec and FlexfecSender into a different > (base or helper) class. Yes, this friendship will be removed in the future. But this will happen when FlexfecSender is connected to PacedSender, together with the corresponding API change. So there will never be a base class that both ProducerFec and FlexfecSender derives from. In fact, ULPFEC will eventually be deprecated, and ProducerFec will be removed. Btw, is it appropriate to place this friend line at the top of the class declaration, or should it be placed somewhere else?
https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:20: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h" On 2016/10/24 12:52:07, brandtr wrote: > On 2016/10/20 15:12:57, danilchap wrote: > > On 2016/10/20 13:19:42, brandtr wrote: > > > I guess the RtpPacket classes will become "public" when the packetization > > module > > > project is done? > > > > Either it will go into /include/ of some module (rtp_rtcp or new one) or all > rtp > > specific code (including this FlexfecSender) will gather inside rtp_rtcp. > > Either way this include should be fine. > > Right. But in the latter case (all RTP code in the rtp_rtcp module), this > interface will still be included from outside the rtp_rtcp module. Specifically, > either from VideoSendStream (with current design) or from PacedSender (with > future design). Or do you mean that even those classes would be part of the > expanded rtp_rtcp module? In the latter case: If paced sender would be designed just for rtp packets, it will make sense to move it into rtp_rtcp. If paced sender would be for generic packets, it will make sense to update current interface to return rtc::CopyOnWriteBuffer instead of RtpPacketToSend. (this case is behind two large ifs, so it is preliminary to make changes for it) https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:27: class FlexfecSender { On 2016/10/24 12:52:07, brandtr wrote: > On 2016/10/20 15:12:57, danilchap wrote: > > this class doesn't send anything, it only generates fec packets. > > Does name reflect future plans, i.e. do you plan to emit packets via some kind > > of transport? > > If your definition of "sending packets" is "emitting packets through a > transport", then no, this class does not currently send packets. The purpose of > this class is to obtain RTP media packets, decide when it's time to generate FEC > packets, and then forward the generated FEC packets to some other class that > will push them to the network. > > Currently, I'm reusing parts of the RTPSender pipeline for the latter stages > (forwarding the FEC packets to the network). The reason is that then I can > easily reuse our current logic for setting the FEC rates, which happens through > RTPSender. In the future, FlexfecSender will be connected to PacedSender and the > API will change: the PacedSender will issue a call to FlexfecSender when it's > time to generate FEC, and FlexfecSender will forward generated FlexFEC packets > (with a suitable FEC rate) directly to a transport. So then, yes, this class > will "send" packets. Since it plan to put packet to transport itself, 'Sender' is proper name. My view that generator/sender hints interface: You give something to generator, then you need take result back. (current interface) If you give something to sender, you may forget about it - class will forward it somewhere else eventually. (that what you plan) https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/sou... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:27: constexpr int kMsToRtpTimestamp = 90; On 2016/10/24 12:52:08, brandtr wrote: > On 2016/10/20 15:12:57, danilchap wrote: > > does the frequency has to be same as for video stream? > > It doesn't have to be, but it's "RECOMMENDED" that the FlexFEC timestamp rate > matches that of the protected media stream, which currently only is video. (When > we support protecting audio, we might do that in combination with protecting > video, so then the RTP timestamp rate should probably still be 90 kHz.) > > I have updated the comment and replaced the "90" literal with a constant from > rtp_rtcp_defines.h. Sorry used too strong wording, I meant 'is there a reason' instead of 'has to'. There is! https://codereview.chromium.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.chromium.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:138: fec_packet_to_send->set_capture_time_ms(clock_->CurrentNtpInMilliseconds()); sorry for not beeing detailed enough. it is expected TimeInMilliseconds() is used to generate capture time. (there are conversions when capture_time might come from different source)
Feedback response 2.
https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:20: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h" On 2016/10/24 13:40:31, danilchap wrote: > On 2016/10/24 12:52:07, brandtr wrote: > > On 2016/10/20 15:12:57, danilchap wrote: > > > On 2016/10/20 13:19:42, brandtr wrote: > > > > I guess the RtpPacket classes will become "public" when the packetization > > > module > > > > project is done? > > > > > > Either it will go into /include/ of some module (rtp_rtcp or new one) or all > > rtp > > > specific code (including this FlexfecSender) will gather inside rtp_rtcp. > > > Either way this include should be fine. > > > > Right. But in the latter case (all RTP code in the rtp_rtcp module), this > > interface will still be included from outside the rtp_rtcp module. > Specifically, > > either from VideoSendStream (with current design) or from PacedSender (with > > future design). Or do you mean that even those classes would be part of the > > expanded rtp_rtcp module? > > In the latter case: > If paced sender would be designed just for rtp packets, it will make sense to > move it into rtp_rtcp. > If paced sender would be for generic packets, it will make sense to update > current interface to return rtc::CopyOnWriteBuffer instead of RtpPacketToSend. > (this case is behind two large ifs, so it is preliminary to make changes for it) Got it! https://codereview.chromium.org/2441613002/diff/1/webrtc/modules/rtp_rtcp/inc... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:27: class FlexfecSender { On 2016/10/24 13:40:31, danilchap wrote: > On 2016/10/24 12:52:07, brandtr wrote: > > On 2016/10/20 15:12:57, danilchap wrote: > > > this class doesn't send anything, it only generates fec packets. > > > Does name reflect future plans, i.e. do you plan to emit packets via some > kind > > > of transport? > > > > If your definition of "sending packets" is "emitting packets through a > > transport", then no, this class does not currently send packets. The purpose > of > > this class is to obtain RTP media packets, decide when it's time to generate > FEC > > packets, and then forward the generated FEC packets to some other class that > > will push them to the network. > > > > Currently, I'm reusing parts of the RTPSender pipeline for the latter stages > > (forwarding the FEC packets to the network). The reason is that then I can > > easily reuse our current logic for setting the FEC rates, which happens > through > > RTPSender. In the future, FlexfecSender will be connected to PacedSender and > the > > API will change: the PacedSender will issue a call to FlexfecSender when it's > > time to generate FEC, and FlexfecSender will forward generated FlexFEC packets > > (with a suitable FEC rate) directly to a transport. So then, yes, this class > > will "send" packets. > > Since it plan to put packet to transport itself, 'Sender' is proper name. > My view that generator/sender hints interface: You give something to generator, > then you need take result back. (current interface) > If you give something to sender, you may forget about it - class will forward it > somewhere else eventually. (that what you plan) Makes sense! So I should probably rename ProducerFec -> UlpfecGenerator. https://codereview.chromium.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.chromium.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:138: fec_packet_to_send->set_capture_time_ms(clock_->CurrentNtpInMilliseconds()); On 2016/10/24 13:40:31, danilchap wrote: > sorry for not beeing detailed enough. > it is expected TimeInMilliseconds() is used to generate capture time. > (there are conversions when capture_time might come from different source) Done :)
lgtm (with bunch of suggestions) https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:44: // Returns 0 on success, and a negative value if the erasure coding failed. can it return different negative values? may be change return value to bool https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:51: virtual std::vector<std::unique_ptr<RtpPacketToSend>> GetFecPackets() = 0; actually since RtpPacketToSend is inside unique_ptr, you may be can forward-declare instead of include. (will make slightly easier for me to move RtpPacketToSend to different module) https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:24: constexpr uint16_t kMaxInitRtpSeqNumber = 32767; // 2^15 -1. may be 0x7fff instead of 32767 and the comment. https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:35: constexpr int kPacketLogIntervalMs = 10000; int64_t for time_ms variables https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:45: // Don't instantiate this object if FlexFEC is disabled. that is TODO? https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:99: RTC_DCHECK(sequence_checker_.CalledSequentially()); RTC_DCHECK_CALLED_SEQUENTIALLY(&sequence_checker_) would allow to write in .h file ProducerFec ulpfec_sender_ GUARDED_BY(sequence_checker_); https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:106: if (packet.Ssrc() != protected_media_ssrc_) { Is there plan to insert packets with different SSRC? i.e. is there a reason it is a runtime check instead of DCHECK? https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:154: int64_t now_ms = clock_->TimeInMilliseconds(); may be move this call to the top of the function and use same now_ms inside the loop to reduce number of calls to clock_ https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.chromium.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:52: uint16_t seq_num_; may be move non-const member outside of the const group, e.g. after protected_media_ssrc_
Description was changed from ========== Add FlexfecSender. This class will interface RTPSenderVideo with the underlying erasure code. It is functionally similar to ProducerFec (to be renamed UlpfecSender). In fact, the FlexfecSender is a friend of ProducerFec, and reuses most of its implementation. Besides the fact that FlexfecSender outputs FlexFEC packets, the main difference with ProducerFec is that FlexfecSender allocates RTP sequence numbers, whereas ProducerFec does not do this for the RED-encapsulated ULPFEC packets. This class is split as interface/implementation, since it will be owned by VideoSendStream initially. Further along, it may be owned by PacedSender. BUG=webrtc:5654 ========== to ========== Add FlexfecSender. This class will interface RTPSenderVideo with the underlying erasure code. It is functionally similar to ProducerFec (to be renamed UlpfecGenerator). In fact, the FlexfecSender is a friend of ProducerFec, and reuses most of its implementation. Besides the fact that FlexfecSender outputs FlexFEC packets, the main difference with ProducerFec is that FlexfecSender allocates RTP sequence numbers, whereas ProducerFec does not do this for the RED-encapsulated ULPFEC packets. This class is split as interface/implementation, since it will be owned by VideoSendStream initially. Further along, it may be owned by PacedSender. BUG=webrtc:5654 ==========
Patchset #4 (id:100001) has been deleted
https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:44: // Returns 0 on success, and a negative value if the erasure coding failed. On 2016/10/24 15:29:12, danilchap wrote: > can it return different negative values? > may be change return value to bool Nope, can only return from the set {-1,0}. Changing to bool. I should probably change the underlying code as well. https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:51: virtual std::vector<std::unique_ptr<RtpPacketToSend>> GetFecPackets() = 0; On 2016/10/24 15:29:12, danilchap wrote: > actually since RtpPacketToSend is inside unique_ptr, you may be can > forward-declare instead of include. > (will make slightly easier for me to move RtpPacketToSend to different module) Done. https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:24: constexpr uint16_t kMaxInitRtpSeqNumber = 32767; // 2^15 -1. On 2016/10/24 15:29:12, danilchap wrote: > may be 0x7fff instead of 32767 and the comment. Done. https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:35: constexpr int kPacketLogIntervalMs = 10000; On 2016/10/24 15:29:12, danilchap wrote: > int64_t for time_ms variables Done. https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:45: // Don't instantiate this object if FlexFEC is disabled. On 2016/10/24 15:29:12, danilchap wrote: > that is TODO? No. Clarified comment. https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:99: RTC_DCHECK(sequence_checker_.CalledSequentially()); On 2016/10/24 15:29:12, danilchap wrote: > RTC_DCHECK_CALLED_SEQUENTIALLY(&sequence_checker_) > would allow to write in .h file > ProducerFec ulpfec_sender_ GUARDED_BY(sequence_checker_); Done. https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:106: if (packet.Ssrc() != protected_media_ssrc_) { On 2016/10/24 15:29:12, danilchap wrote: > Is there plan to insert packets with different SSRC? > i.e. is there a reason it is a runtime check instead of DCHECK? With simulcast there will be several RTPSenders referencing a single FlexfecSender, since the FlexfecSender will be owned by VideoSendStream. Since we (currently) only support protecting a single media SSRC, we need to throw the other SSRCs away here. My plan was to do that using a runtime check, together with a LS_WARNING somewhere higher up. Given this situation, do you think it should be a DCHECK instead? https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:154: int64_t now_ms = clock_->TimeInMilliseconds(); On 2016/10/24 15:29:12, danilchap wrote: > may be move this call to the top of the function and use same now_ms inside the > loop to reduce number of calls to clock_ I think you missed the } on line 150; this part of the code is already outside the loop :) https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.webrtc.org/2441613002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:52: uint16_t seq_num_; On 2016/10/24 15:29:13, danilchap wrote: > may be move non-const member outside of the const group, e.g. after > protected_media_ssrc_ Done.
holmer, any thoughts?
lgtm % nits https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:17: #include "webrtc/config.h" alphabetical order https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:112: RTC_DCHECK(packet.Ssrc() == protected_media_ssrc_); RTC_DCHECK_EQ https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:46: Clock* const clock_ GUARDED_BY(sequence_checker_); no need to guard const member.
Thanks! Done. https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:17: #include "webrtc/config.h" On 2016/10/26 12:08:41, danilchap wrote: > alphabetical order Done. https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:112: RTC_DCHECK(packet.Ssrc() == protected_media_ssrc_); On 2016/10/26 12:08:41, danilchap wrote: > RTC_DCHECK_EQ Done. https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.webrtc.org/2441613002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:46: Clock* const clock_ GUARDED_BY(sequence_checker_); On 2016/10/26 12:08:41, danilchap wrote: > no need to guard const member. Fixed!
Rebase.
Rebase + fixes.
lg https://codereview.webrtc.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.webrtc.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:27: class FlexfecSenderImpl : public FlexfecSender { I'm thinking that maybe we should skip having an interface, and just expose the impl. If we ever want to have two implementations we could add the interface later, WDYT? https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:32: uint32_t flexfec_ssrc, I would consider removing flexfec prefix here. https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:67: // Initialize the timestamp offset and RTP sequence numbers and randomly. -and https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:166: } Will this be valuable, or mostly confusing when we don't do the same thing for audio/video? https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:99: UlpfecGenerator::UlpfecGenerator(std::unique_ptr<ForwardErrorCorrection> fec) This is a bit weird. Why not have a single constructor and initialize like this: fec_(ForwardErrorCorrection::CreateUlpfec())?
https://codereview.webrtc.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h (right): https://codereview.webrtc.org/2441613002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.h:27: class FlexfecSenderImpl : public FlexfecSender { On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > I'm thinking that maybe we should skip having an interface, and just expose the > impl. If we ever want to have two implementations we could add the interface > later, WDYT? I think having the interface is cumbersome, and if we can avoid it that would be nice. The reason for having it is that this class will be owned by objects outside the rtp_rtcp module, however. With the current design, the FlexfecSender will be owned by VideoSendStream(*). With the future design, the FlexfecSender will be owned by either Call or PacedSender. (*) The FlexfecSender could also conceivably be owned by the RTP module, but let's discuss the benefits/drawbacks of that setup offline. https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:32: uint32_t flexfec_ssrc, On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > I would consider removing flexfec prefix here. Done. https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:67: // Initialize the timestamp offset and RTP sequence numbers and randomly. On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > -and Done. https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:166: } On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > Will this be valuable, or mostly confusing when we don't do the same thing for > audio/video? It's valueable for now, when I'm still wiring things up, because then it's easy to see whenever the FlexfecSender is active. Long-term there's probably no need for it, and as you say, it may be confusing the the media streams do not output logs like this. I'll add a TODO to remove it, when things are properly wired up. == Second thought; what is our opinion about logging things like this to the VERBOSE level? https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:99: UlpfecGenerator::UlpfecGenerator(std::unique_ptr<ForwardErrorCorrection> fec) On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > This is a bit weird. Why not have a single constructor and initialize like this: > fec_(ForwardErrorCorrection::CreateUlpfec())? Yes, it's a bit weird. With your suggestion however, the "normal user" of UlpfecGenerator (i.e. RTPSenderVideo when it's sending RED+ULPFEC) will have to be aware that UlpfecGenerator under the hood supports multiple FEC header formatting routines. With this setup, the "normal user" will use the default constructor, whereas the "friend user" (i.e., FlexfecSenderImpl) will use the other constructor. (Note that the single-argument constructor is private.) I don't see a need to make this a more general solution, where UlpfecGenerator and FlexfecSenderImpl inherit from some common parent, for two reasons: 1) it's our intention to completely change the interface of FlexfecSender when we move it to PacedSender, and 2) eventually UlpfecGenerator will be deprecated.
Removed interface.
lgtm https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:166: } On 2016/11/02 14:04:37, brandtr wrote: > On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > > Will this be valuable, or mostly confusing when we don't do the same thing for > > audio/video? > > It's valueable for now, when I'm still wiring things up, because then it's easy > to see whenever the FlexfecSender is active. Long-term there's probably no need > for it, and as you say, it may be confusing the the media streams do not output > logs like this. I'll add a TODO to remove it, when things are properly wired up. > > == > > Second thought; what is our opinion about logging things like this to the > VERBOSE level? I don't have an opinion. If it's used for things like this in other places it's fine. I just want to keep the number of lines of code down, and if it's not very useful it should probably not be there. :) Keep for now if you use it. https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:99: UlpfecGenerator::UlpfecGenerator(std::unique_ptr<ForwardErrorCorrection> fec) On 2016/11/02 14:04:37, brandtr wrote: > On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > > This is a bit weird. Why not have a single constructor and initialize like > this: > > fec_(ForwardErrorCorrection::CreateUlpfec())? > > Yes, it's a bit weird. With your suggestion however, the "normal user" of > UlpfecGenerator (i.e. RTPSenderVideo when it's sending RED+ULPFEC) will have to > be aware that UlpfecGenerator under the hood supports multiple FEC header > formatting routines. With this setup, the "normal user" will use the default > constructor, whereas the "friend user" (i.e., FlexfecSenderImpl) will use the > other constructor. (Note that the single-argument constructor is private.) > > I don't see a need to make this a more general solution, where UlpfecGenerator > and FlexfecSenderImpl inherit from some common parent, for two reasons: 1) it's > our intention to completely change the interface of FlexfecSender when we move > it to PacedSender, and 2) eventually UlpfecGenerator will be deprecated. I see, you do need both constructors? I thought this second constructor was only used from the first constructor, but if it's used elsewhere I understand.
On 2016/11/03 13:42:40, stefan-webrtc (holmer) wrote: > lgtm > > https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc (right): > > https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc:166: } > On 2016/11/02 14:04:37, brandtr wrote: > > On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > > > Will this be valuable, or mostly confusing when we don't do the same thing > for > > > audio/video? > > > > It's valueable for now, when I'm still wiring things up, because then it's > easy > > to see whenever the FlexfecSender is active. Long-term there's probably no > need > > for it, and as you say, it may be confusing the the media streams do not > output > > logs like this. I'll add a TODO to remove it, when things are properly wired > up. > > > > == > > > > Second thought; what is our opinion about logging things like this to the > > VERBOSE level? > > I don't have an opinion. If it's used for things like this in other places it's > fine. I just want to keep the number of lines of code down, and if it's not very > useful it should probably not be there. :) > > Keep for now if you use it. I'll keep for now and remove later. My thinking about the VERBOSE level was inspired by all the logging that goes on in the ICE/STUN systems to the VERBOSE level. Don't think there is a need or reason for this log output to go there though. > > https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc (right): > > https://codereview.webrtc.org/2441613002/diff/220001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/ulpfec_generator.cc:99: > UlpfecGenerator::UlpfecGenerator(std::unique_ptr<ForwardErrorCorrection> fec) > On 2016/11/02 14:04:37, brandtr wrote: > > On 2016/11/02 13:02:42, stefan-webrtc (holmer) wrote: > > > This is a bit weird. Why not have a single constructor and initialize like > > this: > > > fec_(ForwardErrorCorrection::CreateUlpfec())? > > > > Yes, it's a bit weird. With your suggestion however, the "normal user" of > > UlpfecGenerator (i.e. RTPSenderVideo when it's sending RED+ULPFEC) will have > to > > be aware that UlpfecGenerator under the hood supports multiple FEC header > > formatting routines. With this setup, the "normal user" will use the default > > constructor, whereas the "friend user" (i.e., FlexfecSenderImpl) will use the > > other constructor. (Note that the single-argument constructor is private.) > > > > I don't see a need to make this a more general solution, where UlpfecGenerator > > and FlexfecSenderImpl inherit from some common parent, for two reasons: 1) > it's > > our intention to completely change the interface of FlexfecSender when we move > > it to PacedSender, and 2) eventually UlpfecGenerator will be deprecated. > > I see, you do need both constructors? I thought this second constructor was only > used from the first constructor, but if it's used elsewhere I understand. It's used here: https://codereview.webrtc.org/2441613002/diff/260001/webrtc/modules/rtp_rtcp/... Not a super nice solution, but it works for now.
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, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2441613002/#ps280001 (title: "Fix GYP file.")
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
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2805)
Rebase.
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, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2441613002/#ps300001 (title: "Rebase.")
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.
Description was changed from ========== Add FlexfecSender. This class will interface RTPSenderVideo with the underlying erasure code. It is functionally similar to ProducerFec (to be renamed UlpfecGenerator). In fact, the FlexfecSender is a friend of ProducerFec, and reuses most of its implementation. Besides the fact that FlexfecSender outputs FlexFEC packets, the main difference with ProducerFec is that FlexfecSender allocates RTP sequence numbers, whereas ProducerFec does not do this for the RED-encapsulated ULPFEC packets. This class is split as interface/implementation, since it will be owned by VideoSendStream initially. Further along, it may be owned by PacedSender. BUG=webrtc:5654 ========== to ========== Add FlexfecSender. This class will interface RTPSenderVideo with the underlying erasure code. It is functionally similar to ProducerFec (to be renamed UlpfecGenerator). In fact, the FlexfecSender is a friend of ProducerFec, and reuses most of its implementation. Besides the fact that FlexfecSender outputs FlexFEC packets, the main difference with ProducerFec is that FlexfecSender allocates RTP sequence numbers, whereas ProducerFec does not do this for the RED-encapsulated ULPFEC packets. This class is split as interface/implementation, since it will be owned by VideoSendStream initially. Further along, it may be owned by PacedSender. BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Add FlexfecSender. This class will interface RTPSenderVideo with the underlying erasure code. It is functionally similar to ProducerFec (to be renamed UlpfecGenerator). In fact, the FlexfecSender is a friend of ProducerFec, and reuses most of its implementation. Besides the fact that FlexfecSender outputs FlexFEC packets, the main difference with ProducerFec is that FlexfecSender allocates RTP sequence numbers, whereas ProducerFec does not do this for the RED-encapsulated ULPFEC packets. This class is split as interface/implementation, since it will be owned by VideoSendStream initially. Further along, it may be owned by PacedSender. BUG=webrtc:5654 ========== to ========== Add FlexfecSender. This class will interface RTPSenderVideo with the underlying erasure code. It is functionally similar to ProducerFec (to be renamed UlpfecGenerator). In fact, the FlexfecSender is a friend of ProducerFec, and reuses most of its implementation. Besides the fact that FlexfecSender outputs FlexFEC packets, the main difference with ProducerFec is that FlexfecSender allocates RTP sequence numbers, whereas ProducerFec does not do this for the RED-encapsulated ULPFEC packets. This class is split as interface/implementation, since it will be owned by VideoSendStream initially. Further along, it may be owned by PacedSender. BUG=webrtc:5654 Committed: https://crrev.com/c295e00fa0a9774acbadf5d54389b46b1c17965e Cr-Commit-Position: refs/heads/master@{#14922} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/c295e00fa0a9774acbadf5d54389b46b1c17965e Cr-Commit-Position: refs/heads/master@{#14922} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
