Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110763002:
Style updates to ProducerFec/FecReceiver. (Closed)
|
Created:
4 years, 5 months ago by brandtr Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStyle updates to ProducerFec/FecReceiver.
- Make more use of std::unique_ptr.
- Auto type deduction for iterator type names.
- More extensive comments.
- Variable renaming.
- Make ProducerFec::BuildRedPacket() static.
- Avoid dynamic allocation of ProducerFec::fec_.
BUG=webrtc:5654
Committed: https://crrev.com/74811e5fa36b25f7c2c9e567223c1366fa1f52b9
Cr-Commit-Position: refs/heads/master@{#13700}
Patch Set 1 : Initial. #
Total comments: 24
Patch Set 2 : Response to feedback. #
Total comments: 6
Patch Set 3 : Rebase on top of master + lint fixes. #Patch Set 4 : Response to feedback. #Patch Set 5 : Fix memory leak due to incorrect rebase. (CL "1b" will remove this code, thanks to std::unique_ptr.… #
Total comments: 2
Patch Set 6 : Rename kNumPacketsPerFrameThresholdForMinNumMediaPacketsAdditiveAdaptation #
Total comments: 24
Patch Set 7 : Rebase. #Patch Set 8 : Response to feedback. #
Total comments: 1
Patch Set 9 : Final nit. #Patch Set 10 : Rebase + 'git cl format'. #Messages
Total messages: 46 (27 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== FlexFEC pt. 1e: Mini-fixes in FEC producer/receiver. - Make more use of std::unique_ptr. - auto instead of explicit iterator type names. - More comments. - Some renames. - Make BuildRedPacket a static member function of ProducerFec. BUG=webrtc:5654 ========== to ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - auto instead of explicit iterator type names. - More comments. - Variable renaming. - Make BuildRedPacket a static member function of ProducerFec. - Remove ForwardErrorCorrection::ResetState(). BUG=webrtc:5654 ==========
Description was changed from ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - auto instead of explicit iterator type names. - More comments. - Variable renaming. - Make BuildRedPacket a static member function of ProducerFec. - Remove ForwardErrorCorrection::ResetState(). BUG=webrtc:5654 ========== to ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - auto instead of explicit iterator type names. - More extensive comments. - Variable renaming. - Make BuildRedPacket a static member function of ProducerFec. - Remove ForwardErrorCorrection::ResetState(). BUG=webrtc:5654 ==========
Description was changed from ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - auto instead of explicit iterator type names. - More extensive comments. - Variable renaming. - Make BuildRedPacket a static member function of ProducerFec. - Remove ForwardErrorCorrection::ResetState(). BUG=webrtc:5654 ========== to ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. - Remove ForwardErrorCorrection::ResetState(). BUG=webrtc:5654 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. - Remove ForwardErrorCorrection::ResetState(). BUG=webrtc:5654 ========== to ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. BUG=webrtc:5654 ==========
brandtr@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
Some smaller updates to the buffer classes wrapping the actual FEC.
https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:72: uint8_t RED_header_length = 1; red_header_length https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:109: block_length = (0x03 & incoming_rtp_packet[header.headerLength + 2]) << 8; 0x3 https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:126: std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> received_packet2; Not 100% sure what this variable is suppose to hold, but I prefer the old name, or do you think the old name misleading? https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:11: #include "webrtc/base/basictypes.h" Should be with the rest of the webrtc includes, in alphabetic order. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:14: #include "webrtc/modules/rtp_rtcp/source/producer_fec.h" Should be on top, before basictypes.h https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:21: constexpr size_t kREDForFECHeaderLength = 1; kRedForFecHeaderLength https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:47: RedPacket::~RedPacket() = default; Remove destructor. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:99: ProducerFec::~ProducerFec() = default; Remove destructor. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:118: // Number of important packets (in unequal protection parlance) cannot Wut? Is parlence some established term when it comes to FEC? https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:215: RTC_DCHECK_GT(media_packets_.size(), static_cast<size_t>(0)); RTC_DCHECK(!media_packets_.empty()) https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.h:89: int num_frames_protected_; num_protected_frames_ https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.h:91: int minimum_num_media_packets_; min_num_media_packets_
Patchset #3 (id:120001) has been deleted
Addressed comments. The sporadic bot errors are due to the upstream patch application problem. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:72: uint8_t RED_header_length = 1; On 2016/07/04 15:35:03, philipel wrote: > red_header_length Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:109: block_length = (0x03 & incoming_rtp_packet[header.headerLength + 2]) << 8; On 2016/07/04 15:35:03, philipel wrote: > 0x3 Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:126: std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> received_packet2; On 2016/07/04 15:35:04, philipel wrote: > Not 100% sure what this variable is suppose to hold, but I prefer the old name, > or do you think the old name misleading? IIUC, this variable holds a second virtual RTP packet which is embedded after the first virtual RTP packet in the RED packet. The previous name is actually more descriptive, so I changed it back. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:11: #include "webrtc/base/basictypes.h" On 2016/07/04 15:35:04, philipel wrote: > Should be with the rest of the webrtc includes, in alphabetic order. Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:14: #include "webrtc/modules/rtp_rtcp/source/producer_fec.h" On 2016/07/04 15:35:04, philipel wrote: > Should be on top, before basictypes.h Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:21: constexpr size_t kREDForFECHeaderLength = 1; On 2016/07/04 15:35:04, philipel wrote: > kRedForFecHeaderLength Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:47: RedPacket::~RedPacket() = default; On 2016/07/04 15:35:04, philipel wrote: > Remove destructor. Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:99: ProducerFec::~ProducerFec() = default; On 2016/07/04 15:35:04, philipel wrote: > Remove destructor. Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:118: // Number of important packets (in unequal protection parlance) cannot On 2016/07/04 15:35:04, philipel wrote: > Wut? Is parlence some established term when it comes to FEC? Rephrased :) https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:215: RTC_DCHECK_GT(media_packets_.size(), static_cast<size_t>(0)); On 2016/07/04 15:35:04, philipel wrote: > RTC_DCHECK(!media_packets_.empty()) Ugh, this unsightly thing was my bad. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.h:89: int num_frames_protected_; On 2016/07/04 15:35:04, philipel wrote: > num_protected_frames_ Done. https://codereview.webrtc.org/2110763002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.h:91: int minimum_num_media_packets_; On 2016/07/04 15:35:04, philipel wrote: > min_num_media_packets_ Done.
https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:187: float average_num_packets_frame_per_frame = num_packets/num_protected_frames_; Should |average_num_packets_frame_per_frame| be |average_num_packets_per_frame|? https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:188: if (average_num_packets_frame_per_frame < 2.0f) { What is 2.0 in this case? Replace with a 'constexpr float kDescriptiveNameForThisRatio = 2.0'. https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.h:68: bool MinimumMediaPacketsReached(); bool MinimumMediaPacketsReached() const;
Rebased on top of master, to reduce unnecessary dependencies between semi-unrelated CLs. Addressed comments. https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:187: float average_num_packets_frame_per_frame = num_packets/num_protected_frames_; On 2016/07/05 13:05:51, philipel wrote: > Should |average_num_packets_frame_per_frame| be |average_num_packets_per_frame|? Good catch, thanks. https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:188: if (average_num_packets_frame_per_frame < 2.0f) { On 2016/07/05 13:05:51, philipel wrote: > What is 2.0 in this case? Replace with a 'constexpr float > kDescriptiveNameForThisRatio = 2.0'. See added comment at top of file. I have no clue why 2.0 is the magic number here. https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2110763002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.h:68: bool MinimumMediaPacketsReached(); On 2016/07/05 13:05:51, philipel wrote: > bool MinimumMediaPacketsReached() const; Done.
https://codereview.webrtc.org/2110763002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:200: kNumPacketsPerFrameThresholdForMinNumMediaPacketsAdditiveAdaptation) { Sorry, to long :) Maybe kPacketRatioAdaptation? A sensible name + a comment (which there is) is much better.
https://codereview.webrtc.org/2110763002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:200: kNumPacketsPerFrameThresholdForMinNumMediaPacketsAdditiveAdaptation) { On 2016/07/07 15:19:42, philipel wrote: > Sorry, to long :) > > Maybe kPacketRatioAdaptation? A sensible name + a comment (which there is) is > much better. Haha, ok :) What about "kMinMediaPacketsAdaptationThreshold" ?
lgtm
danilchap@webrtc.org changed reviewers: + danilchap@webrtc.org
https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:140: received_packet->pkt->data[1] &= 0x80; // reset RED payload type fix comment here and next line: Uppercase 1st letter and add a '.' at the end. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:184: received_packet->pkt->data[1] &= 0x80; // reset RED payload type fix this 2 comments too. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:177: // TODO(brandtr): The fact above means that the value of set a goal or remove TODO() prefix in the comment. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:197: float average_num_packets_per_frame = num_media_packets/num_protected_frames_; add spaces around operator / (e.g. by running git cl format) https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:199: return (num_media_packets >= min_num_media_packets_); instead of return (something); write return something; comparing float vs int here instead comparing two ints here? that might return incorrect results when media_packets_.size() == min_num_media_packets_; https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:203: return (num_media_packets >= min_num_media_packets_ + 1); same here. (both about () and float vs int) https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:50: std::unique_ptr<ForwardErrorCorrection> fec_; may be use classes directly instead of smart pointers to them since they are initialized once. (just need to remove SetUp and use initialization in constructor instead): class ProdcuerFecTest : public ::testing::Test { protected: ProducerFecTest() : producer_(&fec_) {} ForwardErrorCorrection fec_; ProducerFec producer_; FrameGenerator generator_; } https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:166: fec_packets.clear(); needed? (fec_packets is a local variable and this is end of the function) https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:245: // TODO(brandtr): Note that we currently do not use unequal protection in TODO hints that something should be done in the future. Comment doesn't describe nor plan nor when that should be done. Either rewrite comment with clear goal or turn it into normal comment, without TODO. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:248: const int kNumImportantPackets = 0; might be constexpr. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/test/fuzzers/rtcp... File webrtc/test/fuzzers/rtcp_receiver_fuzzer.cc (left): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/test/fuzzers/rtcp... webrtc/test/fuzzers/rtcp_receiver_fuzzer.cc:29: changing this file doesn't look relevant to this CL
lgtm as soon as danil's comments are addressed. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:113: block_length += incoming_rtp_packet[header.headerLength + 3]; remove extra space after +=
Patchset #7 (id:220001) has been deleted
Patchset #8 (id:260001) has been deleted
Thanks for the feedback; comments have been addressed. Android and libfuzzer failures should be independent of this CL. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:113: block_length += incoming_rtp_packet[header.headerLength + 3]; On 2016/07/20 09:29:19, stefan-webrtc (holmer) wrote: > remove extra space after += Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:140: received_packet->pkt->data[1] &= 0x80; // reset RED payload type On 2016/07/19 14:55:32, danilchap wrote: > fix comment here and next line: Uppercase 1st letter and add a '.' at the end. Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:184: received_packet->pkt->data[1] &= 0x80; // reset RED payload type On 2016/07/19 14:55:32, danilchap wrote: > fix this 2 comments too. Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:177: // TODO(brandtr): The fact above means that the value of On 2016/07/19 14:55:32, danilchap wrote: > set a goal or remove TODO() prefix in the comment. Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:197: float average_num_packets_per_frame = num_media_packets/num_protected_frames_; On 2016/07/19 14:55:32, danilchap wrote: > add spaces around operator / > (e.g. by running git cl format) Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:199: return (num_media_packets >= min_num_media_packets_); On 2016/07/19 14:55:32, danilchap wrote: > instead of > return (something); > write > return something; > > comparing float vs int here instead comparing two ints here? > that might return incorrect results when media_packets_.size() == > min_num_media_packets_; Fixed! https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:203: return (num_media_packets >= min_num_media_packets_ + 1); On 2016/07/19 14:55:32, danilchap wrote: > same here. (both about () and float vs int) Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:50: std::unique_ptr<ForwardErrorCorrection> fec_; On 2016/07/19 14:55:32, danilchap wrote: > may be use classes directly instead of smart pointers to them since they are > initialized once. (just need to remove SetUp and use initialization in > constructor instead): > > class ProdcuerFecTest : public ::testing::Test { > protected: > ProducerFecTest() : producer_(&fec_) {} > > ForwardErrorCorrection fec_; > ProducerFec producer_; > FrameGenerator generator_; > } Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:166: fec_packets.clear(); On 2016/07/19 14:55:32, danilchap wrote: > needed? (fec_packets is a local variable and this is end of the function) Good point :) https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:245: // TODO(brandtr): Note that we currently do not use unequal protection in On 2016/07/19 14:55:32, danilchap wrote: > TODO hints that something should be done in the future. Comment doesn't describe > nor plan nor when that should be done. > Either rewrite comment with clear goal or turn it into normal comment, without > TODO. Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:248: const int kNumImportantPackets = 0; On 2016/07/19 14:55:33, danilchap wrote: > might be constexpr. Done. https://codereview.webrtc.org/2110763002/diff/200001/webrtc/test/fuzzers/rtcp... File webrtc/test/fuzzers/rtcp_receiver_fuzzer.cc (left): https://codereview.webrtc.org/2110763002/diff/200001/webrtc/test/fuzzers/rtcp... webrtc/test/fuzzers/rtcp_receiver_fuzzer.cc:29: On 2016/07/19 14:55:33, danilchap wrote: > changing this file doesn't look relevant to this CL This snuck in by mistake. Fixed.
lgtm % nit https://codereview.webrtc.org/2110763002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2110763002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:202: bool ret; why introduce 'ret' variable when you have no code between assigning and returning it?
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...)
Patchset #10 (id:320001) has been deleted
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, stefan@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2110763002/#ps300001 (title: "Final nit.")
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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/15460) android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/15331)
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/2110763002/#ps340001 (title: "Rebase + 'git cl format'.")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by brandtr@webrtc.org
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 ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. BUG=webrtc:5654 ========== to ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. BUG=webrtc:5654 ========== to ========== Style updates to ProducerFec/FecReceiver. - Make more use of std::unique_ptr. - Auto type deduction for iterator type names. - More extensive comments. - Variable renaming. - Make ProducerFec::BuildRedPacket() static. - Avoid dynamic allocation of ProducerFec::fec_. BUG=webrtc:5654 Committed: https://crrev.com/74811e5fa36b25f7c2c9e567223c1366fa1f52b9 Cr-Commit-Position: refs/heads/master@{#13700} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/74811e5fa36b25f7c2c9e567223c1366fa1f52b9 Cr-Commit-Position: refs/heads/master@{#13700} |
