Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629883003:
First-order-FEC recoverability calculation (Closed)
|
Created:
3 years, 11 months ago by elad.alon Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, henrika_webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReplace consecutive-losses count by a calculation of first-order-FEC recoverability.
Note:
* PLR is calculated over all of the known packets.
* RPLR is calculated over all of the known packet *pairs*. That is, only over sets of subsequent packets where the reception status is known for both.
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2629883003
Cr-Commit-Position: refs/heads/master@{#16401}
Committed: https://chromium.googlesource.com/external/webrtc/+/d83b9670a67c1445a81a3cf20984185c99faa3dc
Patch Set 1 #Patch Set 2 : UT updated #Patch Set 3 : nit #
Total comments: 1
Patch Set 4 : Unused variables in Validate() #Patch Set 5 : Rebased #Patch Set 6 : Fuzzer modified accordingly #Patch Set 7 : Fuzzer mistake fixed. #Patch Set 8 : nit #
Total comments: 1
Patch Set 9 : Some refactoring #Patch Set 10 : Undo unintended changes (from rebasing the diff) #
Total comments: 72
Patch Set 11 : CR response #
Total comments: 2
Patch Set 12 : . #
Total comments: 4
Patch Set 13 : . #Patch Set 14 : . #
Total comments: 20
Patch Set 15 : CR #
Total comments: 4
Patch Set 16 : CR #
Total comments: 25
Patch Set 17 : . #
Messages
Total messages: 61 (14 generated)
https://codereview.webrtc.org/2629883003/diff/40001/webrtc/voice_engine/trans... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (left): https://codereview.webrtc.org/2629883003/diff/40001/webrtc/voice_engine/trans... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:146: void TransportFeedbackPacketLossTracker::ApplyPacketStatus( Style question: ApplyPacketStatus and UndoPacketStatus are pretty much the same, except for one incrementing while the other decrementing. It looks like merging them would be the way to go, but the function which would do both - ModifyPacketStatus(PacketStatusIterator it, bool addition) - would need to add -1/+1 to an unsigned type (size_t), meaning unwanted conversions, etc. The way out of that is to also add a convenience function, Update(size_t& val, bool increment), which would ++ or -- as needed. However, that already gets a bit unsightly, and I am not sure if I'll have created a bigger mess than I had set out to clean. Thoughts?
Description was changed from ========== RPLR BUG= ========== to ========== Replace consecutive-losses count by a calculation of first-order-FEC recoverability. Note: * PLR is calculated over all of the known packets. * RPLR is calculated over all of the known packet *pairs*. That is, only over sets of subsequent packets where the reception status is known for both. ==========
elad.alon@webrtc.org changed reviewers: + michaelt@webrtc.org, minyue@google.com, sprang@webrtc.org, stefan@webrtc.org
elad.alon@webrtc.org changed reviewers: + minyue@webrtc.org - minyue@google.com
The calculation looks good. I may review some details like naming etc later. First, please consider my question. PLR and RPLR are calculated quite separately, while PLR will not be used. I think we can simplify the code a lot by removing PLR. Regarding your question on merging the apply and undo function, yes, good to do it. Better in a separate CL. https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:112: TransportFeedbackPacketLossTracker::GetPacketLossRate() const { Marginal packet loss rate will not be used, will it? Maybe good to remove the calculation and API.
On 2017/01/23 14:04:54, minyue-webrtc wrote: > The calculation looks good. I may review some details like naming etc later. > First, please consider my question. > > PLR and RPLR are calculated quite separately, while PLR will not be used. I > think we can simplify the code a lot by removing PLR. > > Regarding your question on merging the apply and undo function, yes, good to do > it. Better in a separate CL. > > https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:112: > TransportFeedbackPacketLossTracker::GetPacketLossRate() const { > Marginal packet loss rate will not be used, will it? > > Maybe good to remove the calculation and API. I think it would be good to keep PLR until experimentation has proven that RPLR is indeed superior. I have in the past seen empirical data contradict learned intuition. We might end up using either RTCP-PLR, TWCC-PLR or TWCC-RPLR, or even a function of all together. (Please also bear in mind that very little code would be saved by getting rid of PLR. I don't think it's worth to simplify it away just yet.)
On 2017/01/23 14:09:40, elad.alon wrote: > On 2017/01/23 14:04:54, minyue-webrtc wrote: > > The calculation looks good. I may review some details like naming etc later. > > First, please consider my question. > > > > PLR and RPLR are calculated quite separately, while PLR will not be used. I > > think we can simplify the code a lot by removing PLR. > > > > Regarding your question on merging the apply and undo function, yes, good to > do > > it. Better in a separate CL. > > > > > https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... > > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > > > > https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... > > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:112: > > TransportFeedbackPacketLossTracker::GetPacketLossRate() const { > > Marginal packet loss rate will not be used, will it? > > > > Maybe good to remove the calculation and API. > > I think it would be good to keep PLR until experimentation has proven that RPLR > is indeed superior. I have in the past seen empirical data contradict learned > intuition. We might end up using either RTCP-PLR, TWCC-PLR or TWCC-RPLR, or even > a function of all together. > (Please also bear in mind that very little code would be saved by getting rid of > PLR. I don't think it's worth to simplify it away just yet.) Ok. but since they are quite separated (they use different counters). Would it be possible to separate them by introducing the following nested classes. class RateCalculator { public: virtual void ApplyPacketStatus(...) = 0; virtual void UndoPacketStatus(...) = 0; virtual rtc::Optional<float> GetResult() const override; virtual void Verify(...) = 0; } class PacketLossRate : public RateCalculator { public: void ApplyPacketStatus(...) override; void UndoPacketStatus(...) override; rtc::Optional<float> GetResult() const override; void Verify(...) override; private: const size_t min_num_samples_; size_t num_received_packets_; size_t num_lost_packets_; } class RecoverablePacketLossRate : public RateCalculator { public: void ApplyPacketStatus(...) override; void UndoPacketStatus(...) override; rtc::Optional<float> GetResult() const override; void Verify(...) override; private: const size_t min_num_samples_; size_t num_lost_lost; size_t num_lost_received; }
On 2017/01/23 14:58:11, minyue-webrtc wrote: > On 2017/01/23 14:09:40, elad.alon wrote: > > On 2017/01/23 14:04:54, minyue-webrtc wrote: > > > The calculation looks good. I may review some details like naming etc later. > > > First, please consider my question. > > > > > > PLR and RPLR are calculated quite separately, while PLR will not be used. I > > > think we can simplify the code a lot by removing PLR. > > > > > > Regarding your question on merging the apply and undo function, yes, good to > > do > > > it. Better in a separate CL. > > > > > > > > > https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... > > > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > > > > > > > > https://codereview.webrtc.org/2629883003/diff/140001/webrtc/voice_engine/tran... > > > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:112: > > > TransportFeedbackPacketLossTracker::GetPacketLossRate() const { > > > Marginal packet loss rate will not be used, will it? > > > > > > Maybe good to remove the calculation and API. > > > > I think it would be good to keep PLR until experimentation has proven that > RPLR > > is indeed superior. I have in the past seen empirical data contradict learned > > intuition. We might end up using either RTCP-PLR, TWCC-PLR or TWCC-RPLR, or > even > > a function of all together. > > (Please also bear in mind that very little code would be saved by getting rid > of > > PLR. I don't think it's worth to simplify it away just yet.) > > Ok. but since they are quite separated (they use different counters). Would it > be possible to separate them by introducing the following nested classes. > > class RateCalculator { > public: > virtual void ApplyPacketStatus(...) = 0; > virtual void UndoPacketStatus(...) = 0; > virtual rtc::Optional<float> GetResult() const override; should be "= 0" rather than override. > virtual void Verify(...) = 0; > } > > class PacketLossRate : public RateCalculator { > public: > void ApplyPacketStatus(...) override; > void UndoPacketStatus(...) override; > rtc::Optional<float> GetResult() const override; > void Verify(...) override; > private: > const size_t min_num_samples_; > size_t num_received_packets_; > size_t num_lost_packets_; > } > > class RecoverablePacketLossRate : public RateCalculator { > public: > void ApplyPacketStatus(...) override; > void UndoPacketStatus(...) override; > rtc::Optional<float> GetResult() const override; > void Verify(...) override; > private: > const size_t min_num_samples_; > size_t num_lost_lost; > size_t num_lost_received; > }
Would you be okay with me introducing this proposed change in its own CL? Rationale: 1. Would give better visibility to the changes in this CL and in your proposed change. 2. Would minimize rebasing work for the two CLs which are currently based on top of this current one. - By the way, I'm not sure it would be advisable to do this change. I think introducing virtual-functions, etc., for something this simple would both incur overhead as well as not really make the code simpler. But it would be easier to tell if I show you the CL it would require in isolation.
On 2017/01/23 15:17:15, elad.alon wrote: > Would you be okay with me introducing this proposed change in its own CL? > Rationale: > 1. Would give better visibility to the changes in this CL and in your proposed > change. > 2. Would minimize rebasing work for the two CLs which are currently based on top > of this current one. > - > By the way, I'm not sure it would be advisable to do this change. I think > introducing virtual-functions, etc., for something this simple would both incur > overhead as well as not really make the code simpler. But it would be easier to > tell if I show you the CL it would require in isolation. True that it does not make the code simpler but I would argue that it makes it simpler (for understanding and maintenance) What if you allow me to add another CL after https://codereview.webrtc.org/2579613003/, in which, I introduce the nested class. I remove CPLR in that one. Then this CL becomes as easy as introducing a new RateCalculator. That is easy to rebase and review.
On 2017/01/23 15:27:56, minyue-webrtc wrote: > On 2017/01/23 15:17:15, elad.alon wrote: > > Would you be okay with me introducing this proposed change in its own CL? > > Rationale: > > 1. Would give better visibility to the changes in this CL and in your proposed > > change. > > 2. Would minimize rebasing work for the two CLs which are currently based on > top > > of this current one. > > - > > By the way, I'm not sure it would be advisable to do this change. I think > > introducing virtual-functions, etc., for something this simple would both > incur > > overhead as well as not really make the code simpler. But it would be easier > to > > tell if I show you the CL it would require in isolation. > > True that it does not make the code simpler but I would argue that it makes it > simpler (for understanding and maintenance) > > What if you allow me to add another CL after > https://codereview.webrtc.org/2579613003/, in which, I introduce the nested > class. I remove CPLR in that one. Then this CL becomes as easy as introducing a > new RateCalculator. That is easy to rebase and review. Thing is, I've already written, tested, rebased and retested this diff (which removes CPLR and introduced RPLR) and the subsequent CL, which does RPLR and PLR for separated streams. I think it would be best to introduce the CL that uses RateCalculator (maybe MetricCalculator would be a better name, btw?) on top of those, rather than below, to minimize redundant effort.
Good work! Please see my comments. Most of them are cosmetic suggestions. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T FuzzInput(const uint8_t*& data, size_t& size) { const uint8_t*& -> uint8_t const** size_t& -> size_t* https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:105: static_cast<size_t>(std::max(static_cast<uint16_t>(2), use std::min<size_t> and std::max<uint16_t> to avoid static cast https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:115: static_cast<size_t>(std::max(static_cast<uint16_t>(1), use max<uint16_t> and min<size_t> https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:25: inline void UpdateCounter(size_t& counter, bool increment) { & always goes with const, so use size_t* counter https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:25: inline void UpdateCounter(size_t& counter, bool increment) { remove "inline", it happens automatically. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:41: RTC_DCHECK_GT(min_window_size, 0u); remove "u" after 0 https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:120: // Returns the first-order-FEC recoverable packet loss rate, if the window merge this comment to the header file https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:166: // Record or undo recetion status of currently handled packet. reception But I think the code is clear enough to remove the whole comment. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:177: // Previous packet and current packet might compose a pair. preferably, move the comment after 180 and write // Previous packet and current packet compose a pair. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:188: // Current packet and next packet might compose a pair. preferably, move the comment after 190 and write // Current packet and next packet compose a pair. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:250: size_t known_status_pairs = 0; if you accept to simplify the private var to num_pairs_, this can be simplified to "pairs" https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:251: size_t loss_followed_by_reception_pairs = 0; lost_received_pairs https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:267: static_cast<uint16_t>(it->first + 1) == next->first) { next->first == static_cast<uint16_t>(it->first + 1) https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:290: if (total < min_window_size_) have to add {} when there are multiple lines in if. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:299: if (num_known_status_pairs_ < min_pairs_) add {} around if and else https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:33: TransportFeedbackPacketLossTracker(size_t min_window_size, rename min_window_size to make it specific to plr, maybe plr_min_num_packets_ https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:34: size_t max_window_size, change the order of min_window_size (and rename it) and max_window_size. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:35: size_t min_pairs_num_for_rplr); rename to rplr_min_num_pairs_ https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:51: // PacketStatus is a map from sequence number to a boolean. The boolean is use the old comment. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:68: void UpdateMetrics(PacketStatusIterator it, bool apply /* false = undo */); I find it would be easier for me to understand void UpdateMetrics(PacketStatusIterator it, bool undo = false); because I want UpdateMetrics to apply a new input by default, and only does undo when it is specially told to do so. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:91: const size_t min_window_size_; rename min_window_size -> min_num_packets_ https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:108: size_t num_loss_followed_by_reception_pairs_; let's consider making the var names more precise and concise how about "min_num_pairs_" "num_pairs_" "num_lost_received_pairs_" and add a comment under struct RplrState RPLR is the probability that a lost packet is followed by a received packet. We count the fraction of such pairs of packets out of all observed pairs in the transport feedbacks. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:111: size_t num_consecutive_old_reports_; // TODO(eladalon): Upcoming CL removes. elad.alon https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:53: RTC_CHECK_EQ(base_sequence_num, test_feedback.GetBaseSequence()); DCHECK https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: RTC_CHECK_EQ(reception_status_vec.size(), vec.size()); write a bool ReceptionStatusVectorMatches(const vector<bool>&, const vector<StatusSymbol>&) and change 55 ~ 59 into RTC_DCHECK(ReceptionStatusVectorMatches(xxx, xxx)); https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:67: void ValidatePacketReceptionStatistics( PacketReceptionStatistics -> PacketLossStatistics https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:74: EXPECT_EQ(bool(expected_plr), bool(plr)); change 74~77 into EXPECT_EQ(expected_plr, plr) https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:79: rtc::Optional<float> rplr = tracker.GetRecoverablePacketLossRate(); same here https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:88: void ValidatePacketReceptionStatistics( PacketReceptionStatistics -> PacketLossStatistics https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:433: // [1110-101-GAP-101] -> [11101-01-GAP-10001-GAP-101] I don't know what "-" means, it can be confusing with GAP
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T FuzzInput(const uint8_t*& data, size_t& size) { On 2017/01/25 09:38:45, minyue-webrtc wrote: > const uint8_t*& -> uint8_t const** > > size_t& -> size_t* For the future instances - is the guideline "const reference for input, pointer for output"? Something else, perhaps? https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:105: static_cast<size_t>(std::max(static_cast<uint16_t>(2), On 2017/01/25 09:38:45, minyue-webrtc wrote: > use std::min<size_t> and std::max<uint16_t> to avoid static cast Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:115: static_cast<size_t>(std::max(static_cast<uint16_t>(1), On 2017/01/25 09:38:45, minyue-webrtc wrote: > use max<uint16_t> and min<size_t> Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:41: RTC_DCHECK_GT(min_window_size, 0u); On 2017/01/25 09:38:45, minyue-webrtc wrote: > remove "u" after 0 Rebase relic. Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:120: // Returns the first-order-FEC recoverable packet loss rate, if the window On 2017/01/25 09:38:45, minyue-webrtc wrote: > merge this comment to the header file Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:166: // Record or undo recetion status of currently handled packet. On 2017/01/25 09:38:45, minyue-webrtc wrote: > reception > > But I think the code is clear enough to remove the whole comment. I want this comment to indicate the "currently handled packet" part, as opposed to the RPLR calculation, which handles pairs. Can't hurt, imho. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:177: // Previous packet and current packet might compose a pair. On 2017/01/25 09:38:45, minyue-webrtc wrote: > preferably, move the comment after 180 and write > > // Previous packet and current packet compose a pair. IMHO, clearer to have it over the entire block, which makes it clearer what the entire block does. If inside the block, someone who reads this quickly could think it only illuminates some particular part only. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:188: // Current packet and next packet might compose a pair. On 2017/01/25 09:38:45, minyue-webrtc wrote: > preferably, move the comment after 190 and write > > // Current packet and next packet compose a pair. Similarly. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:250: size_t known_status_pairs = 0; On 2017/01/25 09:38:45, minyue-webrtc wrote: > if you accept to simplify the private var to num_pairs_, this can be simplified > to "pairs" I prefer known_status_pairs, because the next CL introduces the possibility of pairs of type (SENT, LOST), where SENT is unacked, and therefore not "a real pair", or more accurately not a fully known-status pair. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:251: size_t loss_followed_by_reception_pairs = 0; On 2017/01/25 09:38:45, minyue-webrtc wrote: > lost_received_pairs Discussed in .h file comments. More feedback welcome. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:267: static_cast<uint16_t>(it->first + 1) == next->first) { On 2017/01/25 09:38:45, minyue-webrtc wrote: > next->first == static_cast<uint16_t>(it->first + 1) Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:290: if (total < min_window_size_) On 2017/01/25 09:38:45, minyue-webrtc wrote: > have to add {} when there are multiple lines in if. There aren't multiple lines here. Or do you mean visual lines? Coding-style-wise? If so - done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:299: if (num_known_status_pairs_ < min_pairs_) On 2017/01/25 09:38:45, minyue-webrtc wrote: > add {} around if and else Similarly. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:33: TransportFeedbackPacketLossTracker(size_t min_window_size, On 2017/01/25 09:38:45, minyue-webrtc wrote: > rename min_window_size to make it specific to plr, maybe plr_min_num_packets_ Please see subsequent CL, which renames to min/max-acked. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:34: size_t max_window_size, On 2017/01/25 09:38:45, minyue-webrtc wrote: > change the order of min_window_size (and rename it) and max_window_size. Min-then-max looks fine to me. That's how I'm used to seeing it everywhere. Max-then-min would be confusing. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:35: size_t min_pairs_num_for_rplr); On 2017/01/25 09:38:45, minyue-webrtc wrote: > rename to rplr_min_num_pairs_ Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:51: // PacketStatus is a map from sequence number to a boolean. The boolean is On 2017/01/25 09:38:45, minyue-webrtc wrote: > use the old comment. Done. (Edited needlessly when doing some rebasing work for the second CL, which does intentionally change this comment due to a change of the bool to a tri-state enum.) https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:68: void UpdateMetrics(PacketStatusIterator it, bool apply /* false = undo */); On 2017/01/25 09:38:45, minyue-webrtc wrote: > I find it would be easier for me to understand > > void UpdateMetrics(PacketStatusIterator it, bool undo = false); > > because I want UpdateMetrics to apply a new input by default, and only does undo > when it is specially told to do so. 1. I think it would be confusing for a reader to see UpdateMetrics(it, true) somehow do an undo operation. One normally associates negative indicators with undoing. When I see UpdateMetrics(it, false), even if I haven't seen the function's signature yet, I'm already mentally prepared for the possibility that it would do an inverse operation. 2. Assuming we've reached agreement on #1, I think it would be less error-prone to force an explicit passing of the second (bool) parameter for both doing and undoing. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:91: const size_t min_window_size_; On 2017/01/25 09:38:45, minyue-webrtc wrote: > rename min_window_size -> min_num_packets_ In this CL, it is still the window. In the subsequent CL, the exact meaning has changed, and a new name is already used there. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:108: size_t num_loss_followed_by_reception_pairs_; On 2017/01/25 09:38:45, minyue-webrtc wrote: > let's consider making the var names more precise and concise > > how about > > "min_num_pairs_" > "num_pairs_" > "num_lost_received_pairs_" > > and add a comment under struct RplrState > > RPLR is the probability that a lost packet is followed by a received packet. We > count the fraction of such pairs of packets out of all observed pairs in the > transport feedbacks. min_num_pair - OK num_pairs - I prefer num_known_pairs; I think it's more informative, and not yet overly long. num_lost_received_pairs - sounds too paradoxical to me. A reader might not immediately understand it's a pair of X followed by Y, thinking instead it's a pair that's somehow both X and Y at the same time. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:111: size_t num_consecutive_old_reports_; // TODO(eladalon): Upcoming CL removes. On 2017/01/25 09:38:46, minyue-webrtc wrote: > elad.alon Done. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:53: RTC_CHECK_EQ(base_sequence_num, test_feedback.GetBaseSequence()); On 2017/01/25 09:38:46, minyue-webrtc wrote: > DCHECK I've used CHECK because, as I understand it, this would be a problem in the UT itself, and not in the module, if this condition doesn't hold. It means someone has written a UT with an illegal feedback. Shouldn't it be CHECK in that case? https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: RTC_CHECK_EQ(reception_status_vec.size(), vec.size()); On 2017/01/25 09:38:46, minyue-webrtc wrote: > write a > > bool ReceptionStatusVectorMatches(const vector<bool>&, const > vector<StatusSymbol>&) > > and change 55 ~ 59 into > > RTC_DCHECK(ReceptionStatusVectorMatches(xxx, xxx)); 1. IMHO, this wouldn't make things clearer. Not sure what to call the arguments of the proposed function, for a start. Since this is a simple loop that's only used once, I'd prefer to keep it as it is. 2. Same question about CHECK vs. DCHECK as above. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:67: void ValidatePacketReceptionStatistics( On 2017/01/25 09:38:46, minyue-webrtc wrote: > PacketReceptionStatistics -> PacketLossStatistics Acknowledged. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:74: EXPECT_EQ(bool(expected_plr), bool(plr)); On 2017/01/25 09:38:46, minyue-webrtc wrote: > change 74~77 into > > EXPECT_EQ(expected_plr, plr) Please see the comment above, which explains why this is preferable. Here's an example: 1.1. My code with different values: Value of: *plr Actual: 0.4 Expected: *expected_plr Which is: 0.6 1.2. Intentionally avoided simpler code with different values: Value of: plr Actual: 8-byte object <01-A6 00-00 CD-CC CC-3E> Expected: expected_plr Which is: 8-byte object <01-00 00-00 9A-99 19-3F> 2.1. My code with different known/unknown state: Value of: bool(rplr) Actual: false Expected: bool(expected_rplr) Which is: true 2.2. Intentionally avoided simpler code with different known/unknown state: Value of: rplr Actual: 8-byte object <00-DC BF-5F 00-7F 00-00> Expected: expected_rplr Which is: 8-byte object <01-3E 00-00 00-00 00-3F> As you can see, the simpler code yields less informative error messages. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:79: rtc::Optional<float> rplr = tracker.GetRecoverablePacketLossRate(); On 2017/01/25 09:38:46, minyue-webrtc wrote: > same here Same answer. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:88: void ValidatePacketReceptionStatistics( On 2017/01/25 09:38:46, minyue-webrtc wrote: > PacketReceptionStatistics -> PacketLossStatistics Acknowledged.
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:433: // [1110-101-GAP-101] -> [11101-01-GAP-10001-GAP-101] On 2017/01/25 09:38:46, minyue-webrtc wrote: > I don't know what "-" means, it can be confusing with GAP Missed that comment the first time around. "-" is just to separate long sequences into sequences of length 5, like "," in "100,000,000.00 dollars".
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T FuzzInput(const uint8_t*& data, size_t& size) { On 2017/01/25 12:47:54, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > const uint8_t*& -> uint8_t const** > > > > size_t& -> size_t* > > For the future instances - is the guideline "const reference for input, pointer > for output"? Something else, perhaps? Rather simple: see https://google.github.io/styleguide/cppguide.html#Reference_Arguments https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:433: // [1110-101-GAP-101] -> [11101-01-GAP-10001-GAP-101] On 2017/01/25 13:14:33, elad.alon wrote: > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > I don't know what "-" means, it can be confusing with GAP > > Missed that comment the first time around. "-" is just to separate long > sequences into sequences of length 5, like "," in "100,000,000.00 dollars". I thought the same. But with GAP, since you always put - on both sides of GAP, it becomes unclearer. I suggest remove "{0.1}-{0,1}"s, leaving only -GAP-
On 2017/01/25 13:54:57, minyue-webrtc wrote: > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... > File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc > (right): > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/test/fuzzers/tran... > webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T > FuzzInput(const uint8_t*& data, size_t& size) { > On 2017/01/25 12:47:54, elad.alon wrote: > > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > > const uint8_t*& -> uint8_t const** > > > > > > size_t& -> size_t* > > > > For the future instances - is the guideline "const reference for input, > pointer > > for output"? Something else, perhaps? > > Rather simple: see > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc > (right): > > https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:433: // > [1110-101-GAP-101] -> [11101-01-GAP-10001-GAP-101] > On 2017/01/25 13:14:33, elad.alon wrote: > > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > > I don't know what "-" means, it can be confusing with GAP > > > > Missed that comment the first time around. "-" is just to separate long > > sequences into sequences of length 5, like "," in "100,000,000.00 dollars". > > I thought the same. But with GAP, since you always put - on both sides of GAP, > it becomes unclearer. I suggest remove "{0.1}-{0,1}"s, leaving only -GAP- * "GAP" is a sequence of at least one unknown-status packets. * "-" is purely cosmetic.
https://codereview.webrtc.org/2629883003/diff/220001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/220001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:35: size_t rplr_min_num_pairs); May want to define rplr above. https://codereview.webrtc.org/2629883003/diff/220001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:87: void Reset() { Instead of using a Reset() method, how about creating a new instance instead? plr_state_ = Plrstate(min_window_size);
https://codereview.webrtc.org/2629883003/diff/220001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/220001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:35: size_t rplr_min_num_pairs); On 2017/01/25 16:27:48, stefan-webrtc wrote: > May want to define rplr above. Please explain? You mean to make the order min-x, min-y, max-y? https://codereview.webrtc.org/2629883003/diff/220001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:87: void Reset() { On 2017/01/25 16:27:48, stefan-webrtc wrote: > Instead of using a Reset() method, how about creating a new instance instead? > > plr_state_ = Plrstate(min_window_size); That would rely on bitwise copy being appropriate here. Are we sure we want to add this assumption? It does hold at the moment. But I am not sure it saves CPU or on binary size, since machine code for a non-trivial ctor - one which sets the values to zero - would still need to be produced. Alternatively, we could hold a std::unique_ptr<PlrState> instead, but that seems to me like overkill. Thoughts?
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:290: if (total < min_window_size_) On 2017/01/25 12:47:54, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > have to add {} when there are multiple lines in if. > > There aren't multiple lines here. Or do you mean visual lines? > Coding-style-wise? If so - done. when else is present, it is considered multiple line :) https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:33: TransportFeedbackPacketLossTracker(size_t min_window_size, On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > rename min_window_size to make it specific to plr, maybe plr_min_num_packets_ > > Please see subsequent CL, which renames to min/max-acked. You see I did not ask you to rename max_window_size, since that belongs to the next CL. However, renaming "min" and reordering it with max_window_size would make this CL more solid on itself, since the introduction of rplr_min_num_pairs_ makes min_window_size harder to understand. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:51: // PacketStatus is a map from sequence number to a boolean. The boolean is On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > use the old comment. > > Done. (Edited needlessly when doing some rebasing work for the second CL, which > does intentionally change this comment due to a change of the bool to a > tri-state enum.) Acknowledged. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:68: void UpdateMetrics(PacketStatusIterator it, bool apply /* false = undo */); On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > I find it would be easier for me to understand > > > > void UpdateMetrics(PacketStatusIterator it, bool undo = false); > > > > because I want UpdateMetrics to apply a new input by default, and only does > undo > > when it is specially told to do so. > > 1. I think it would be confusing for a reader to see UpdateMetrics(it, true) > somehow do an undo operation. One normally associates negative indicators with > undoing. When I see UpdateMetrics(it, false), even if I haven't seen the > function's signature yet, I'm already mentally prepared for the possibility that > it would do an inverse operation. > 2. Assuming we've reached agreement on #1, I think it would be less error-prone > to force an explicit passing of the second (bool) parameter for both doing and > undoing. Sure, anyway, either way should not be a problem for a programmer to read. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:91: const size_t min_window_size_; On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > rename min_window_size -> min_num_packets_ > > In this CL, it is still the window. In the subsequent CL, the exact meaning has > changed, and a new name is already used there. True, but doing it now will make it clearer, particularly if you agree to do the renaming I suggest on the var in ctor. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:108: size_t num_loss_followed_by_reception_pairs_; On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:45, minyue-webrtc wrote: > > let's consider making the var names more precise and concise > > > > how about > > > > "min_num_pairs_" > > "num_pairs_" > > "num_lost_received_pairs_" > > > > and add a comment under struct RplrState > > > > RPLR is the probability that a lost packet is followed by a received packet. > We > > count the fraction of such pairs of packets out of all observed pairs in the > > transport feedbacks. > > min_num_pair - OK > num_pairs - I prefer num_known_pairs; I think it's more informative, and not yet > overly long. known is not a clearly defined word. In the next step, we introduce SENT, does it belong to known or unknown? So I think a half vague name could be ok here. And please note that I have suggested adding a comment after the struct. > num_lost_received_pairs - sounds too paradoxical to me. A reader might not > immediately understand it's a pair of X followed by Y, thinking instead it's a > pair that's somehow both X and Y at the same time. Then how about num_recoverable_lost_packets https://codereview.webrtc.org/2629883003/diff/200001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2629883003/diff/200001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:257: size_t known_status_pairs = 0; I am still a bit concerned on "known" https://codereview.webrtc.org/2629883003/diff/200001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:258: size_t loss_followed_by_reception_pairs = 0; how about recoverable_lost_packets
Description was changed from ========== Replace consecutive-losses count by a calculation of first-order-FEC recoverability. Note: * PLR is calculated over all of the known packets. * RPLR is calculated over all of the known packet *pairs*. That is, only over sets of subsequent packets where the reception status is known for both. ========== to ========== Replace consecutive-losses count by a calculation of first-order-FEC recoverability. Note: * PLR is calculated over all of the known packets. * RPLR is calculated over all of the known packet *pairs*. That is, only over sets of subsequent packets where the reception status is known for both. BUG=webrtc:7058 ==========
hi Elad, I see that you have uploaded new patch. Next time, please send an email by "publish all my drafts" after uploading a new reviewable patch. Thanks! I will give a final look at the CL. It should generally look well. You may rebase second CL on top of the latest patch set, if you like.
On 2017/01/27 08:52:34, minyue-webrtc wrote: > hi Elad, > > I see that you have uploaded new patch. Next time, please send an email by > "publish all my drafts" after uploading a new reviewable patch. Thanks! > > I will give a final look at the CL. It should generally look well. You may > rebase second CL on top of the latest patch set, if you like. Okay, will do. Second CL is already rebased on top of latest patch and waiting your review. :-)
Some wording suggestion + DCHECK discussion https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:53: RTC_CHECK_EQ(base_sequence_num, test_feedback.GetBaseSequence()); On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > DCHECK > > I've used CHECK because, as I understand it, this would be a problem in the UT > itself, and not in the module, if this condition doesn't hold. It means someone > has written a UT with an illegal feedback. Shouldn't it be CHECK in that case? To me, DCHECK is what you can find in debug mode. and CHECK is something you can only find in production code. So I prefer DCHECK here and also line 55-59 https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: RTC_CHECK_EQ(reception_status_vec.size(), vec.size()); On 2017/01/25 12:47:55, elad.alon wrote: > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > write a > > > > bool ReceptionStatusVectorMatches(const vector<bool>&, const > > vector<StatusSymbol>&) > > > > and change 55 ~ 59 into > > > > RTC_DCHECK(ReceptionStatusVectorMatches(xxx, xxx)); > > 1. IMHO, this wouldn't make things clearer. Not sure what to call the arguments > of the proposed function, for a start. Since this is a simple loop that's only > used once, I'd prefer to keep it as it is. > 2. Same question about CHECK vs. DCHECK as above. same here https://codereview.webrtc.org/2629883003/diff/260001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/260001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:12: #include <array> no need if you take my suggestion https://codereview.webrtc.org/2629883003/diff/260001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:101: // (The distribution isn't uniform, but it's enough; more would be overkill.) looks a bit cumbersome. You sort the values, but sorting the values does not give you a readily feasible set of parameters. I would do rplr_min_num_pairs = std::min<size_t>(0x7999, std::max<uint_16>(1, FuzzInput<uint_16>(&data, &size))); plr_min_num_packets = std::min<size_t>(0x8000, rplr_min_num_pairs + std::max<uint_16>(1, FuzzInput<uint_16>(&data, &size))); max_window_size = std::min<size_t>(0x8000, plr_min_num_packets + FuzzInput<uint_16>(&data, &size)) https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:27: // * Up to |max_window_size| latest packet statuses wil be used for wil -> will https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:29: // * PLR (packet-loss-rate) is reliably computable once the status of status -> statuses https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:30: // |plr_min_num_packets| packets is known. is -> are https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:32: // status of |rplr_min_num_pairs| pairs is known. statuses, are https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:39: // Returns the packet loss rate, if the window has enough data to ... if there are enough feedback statuses to reliably compute it. Otherwise, return empty. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: // has enough data to reliably compute it. ... if there are enough feedback status pairs to reliably compute it. Otherwise, return empty. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:105: num_known_pairs_ = 0; ok if you change "known" to "ack" in the following CL, https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:111: // the data from lost packet that preceded it could be recovered. lost packet that preceded it -> the former lost packet https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:113: // of all consecutive packet pairs, where the status (lost/received) of where the status (lost/received) of both packets is known. -> of which both statuses are known.
https://codereview.webrtc.org/2629883003/diff/260001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/260001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:101: // (The distribution isn't uniform, but it's enough; more would be overkill.) On 2017/01/30 09:30:23, minyue-webrtc wrote: > looks a bit cumbersome. You sort the values, but sorting the values does not > give you a readily feasible set of parameters. > > I would do > > rplr_min_num_pairs = std::min<size_t>(0x7999, std::max<uint_16>(1, > FuzzInput<uint_16>(&data, &size))); > > plr_min_num_packets = std::min<size_t>(0x8000, rplr_min_num_pairs + > std::max<uint_16>(1, FuzzInput<uint_16>(&data, &size))); > > max_window_size = std::min<size_t>(0x8000, plr_min_num_packets + > FuzzInput<uint_16>(&data, &size)) nit: I think you meant 0x7fff, not 0x7999. All of these (simple) methods skew the distribution away from completely uniform, and picking something else would be overkill. * Randomly picking three, then sorting, gives a higher likelihood to higher maximums and lower minimums. * Doing it one by one, but with capping, means the first value is uniformly distributed, but subsequent values get ever worse biases. Thinking it again, yes, I do prefer the second approach. I'll do away with the sorting and fuzz them one by one. To reduce the skew, I'll disregard those bits in subsequent fuzzed values which are known to be irrelevant before using std::max, meaning I'll get a slightly less severe bias.
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:53: RTC_CHECK_EQ(base_sequence_num, test_feedback.GetBaseSequence()); On 2017/01/30 09:30:23, minyue-webrtc wrote: > On 2017/01/25 12:47:55, elad.alon wrote: > > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > > DCHECK > > > > I've used CHECK because, as I understand it, this would be a problem in the UT > > itself, and not in the module, if this condition doesn't hold. It means > someone > > has written a UT with an illegal feedback. Shouldn't it be CHECK in that case? > > To me, DCHECK is what you can find in debug mode. and CHECK is something you can > only find in production code. So I prefer DCHECK here and also line 55-59 For other reviewers - after discussing this offline, we've decided to always use CHECK in UT and fuzzers, to make sure that people who run in Release would still nevertheless fail sooner. https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:55: RTC_CHECK_EQ(reception_status_vec.size(), vec.size()); On 2017/01/30 09:30:23, minyue-webrtc wrote: > On 2017/01/25 12:47:55, elad.alon wrote: > > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > > write a > > > > > > bool ReceptionStatusVectorMatches(const vector<bool>&, const > > > vector<StatusSymbol>&) > > > > > > and change 55 ~ 59 into > > > > > > RTC_DCHECK(ReceptionStatusVectorMatches(xxx, xxx)); > > > > 1. IMHO, this wouldn't make things clearer. Not sure what to call the > arguments > > of the proposed function, for a start. Since this is a simple loop that's only > > used once, I'd prefer to keep it as it is. > > 2. Same question about CHECK vs. DCHECK as above. > > same here For other reviewers - please see my comment summarizing the conclusions of my offline discussion with Minyue on the fuzzer file. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:27: // * Up to |max_window_size| latest packet statuses wil be used for On 2017/01/30 09:30:23, minyue-webrtc wrote: > wil -> will Thanks. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:29: // * PLR (packet-loss-rate) is reliably computable once the status of On 2017/01/30 09:30:23, minyue-webrtc wrote: > status -> statuses Done. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:39: // Returns the packet loss rate, if the window has enough data to On 2017/01/30 09:30:23, minyue-webrtc wrote: > ... if there are enough feedback statuses to reliably compute it. Otherwise, > return empty. "Feedback statuses" is confusing to me. We get *feedback messages* which teach us the *loss/reception status* of the packet (or packet status); there are no "feedback statuses". Except for that, implemented your suggestion. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: // has enough data to reliably compute it. On 2017/01/30 09:30:24, minyue-webrtc wrote: > ... if there are enough feedback status pairs to reliably compute it. Otherwise, > return empty. Similarly. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:105: num_known_pairs_ = 0; On 2017/01/30 09:30:23, minyue-webrtc wrote: > ok if you change "known" to "ack" in the following CL, Sure, will do. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:111: // the data from lost packet that preceded it could be recovered. On 2017/01/30 09:30:23, minyue-webrtc wrote: > lost packet that preceded it -> the former lost packet Done. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:113: // of all consecutive packet pairs, where the status (lost/received) of On 2017/01/30 09:30:23, minyue-webrtc wrote: > where the status (lost/received) of both packets is known. -> of which both > statuses are known. I've employed a compromise phrasing which I hope would satisfy us both.
good, just a suggestion on the uniform distribution and a word in the comment. https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/260001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:113: // of all consecutive packet pairs, where the status (lost/received) of On 2017/01/30 16:37:13, elad.alon wrote: > On 2017/01/30 09:30:23, minyue-webrtc wrote: > > where the status (lost/received) of both packets is known. -> of which both > > statuses are known. > > I've employed a compromise phrasing which I hope would satisfy us both. yes, that is good. https://codereview.webrtc.org/2629883003/diff/280001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/280001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:31: size_t FuzzInRange(const uint8_t** data, I see that you want a uniformly distributed value between lower and upper then you'd better do static_cast<float>(FuzzInput<uint16_t>(data, size)) / 0xffff * range; https://codereview.webrtc.org/2629883003/diff/280001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/280001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: // enough packet statuses to reliably compute it. Otherwise, returns empty. I think packet "status pairs" would be better, to reflect the ctor arg.
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:53: RTC_CHECK_EQ(base_sequence_num, test_feedback.GetBaseSequence()); On 2017/01/30 16:37:13, elad.alon wrote: > On 2017/01/30 09:30:23, minyue-webrtc wrote: > > On 2017/01/25 12:47:55, elad.alon wrote: > > > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > > > DCHECK > > > > > > I've used CHECK because, as I understand it, this would be a problem in the > UT > > > itself, and not in the module, if this condition doesn't hold. It means > > someone > > > has written a UT with an illegal feedback. Shouldn't it be CHECK in that > case? > > > > To me, DCHECK is what you can find in debug mode. and CHECK is something you > can > > only find in production code. So I prefer DCHECK here and also line 55-59 > > For other reviewers - after discussing this offline, we've decided to always use > CHECK in UT and fuzzers, to make sure that people who run in Release would still > nevertheless fail sooner. ASSERT_EQ is maybe a better option as it won't crash the test executable. I think it should work as long as it's a void function.
https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:53: RTC_CHECK_EQ(base_sequence_num, test_feedback.GetBaseSequence()); On 2017/01/31 11:45:19, stefan-webrtc wrote: > On 2017/01/30 16:37:13, elad.alon wrote: > > On 2017/01/30 09:30:23, minyue-webrtc wrote: > > > On 2017/01/25 12:47:55, elad.alon wrote: > > > > On 2017/01/25 09:38:46, minyue-webrtc wrote: > > > > > DCHECK > > > > > > > > I've used CHECK because, as I understand it, this would be a problem in > the > > UT > > > > itself, and not in the module, if this condition doesn't hold. It means > > > someone > > > > has written a UT with an illegal feedback. Shouldn't it be CHECK in that > > case? > > > > > > To me, DCHECK is what you can find in debug mode. and CHECK is something you > > can > > > only find in production code. So I prefer DCHECK here and also line 55-59 > > > > For other reviewers - after discussing this offline, we've decided to always > use > > CHECK in UT and fuzzers, to make sure that people who run in Release would > still > > nevertheless fail sooner. > > ASSERT_EQ is maybe a better option as it won't crash the test executable. I > think it should work as long as it's a void function. Maybe I am wrong, but I suggested using CHECK/DCHECK from beginning, since this is not error in the implementation code but unintended error made in the test code.
https://codereview.webrtc.org/2629883003/diff/280001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/280001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:31: size_t FuzzInRange(const uint8_t** data, On 2017/01/31 08:28:11, minyue-webrtc wrote: > I see that you want a uniformly distributed value between lower and upper > > then you'd better do > > static_cast<float>(FuzzInput<uint16_t>(data, size)) / 0xffff * range; Good suggestion, thanks. I'll just modify to 0x10000 instead, to avoid the highest possible value having a much lower probability than the others. Then (range + 1) too, to extend the range back to include the highest value. https://codereview.webrtc.org/2629883003/diff/280001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/280001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: // enough packet statuses to reliably compute it. Otherwise, returns empty. On 2017/01/31 08:28:11, minyue-webrtc wrote: > I think packet "status pairs" would be better, to reflect the ctor arg. True.
lgtm good work + a small note https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:40: const size_t offset = (static_cast<float>(fuzzed) / 0x10000) * (range + 1); nit: you probably need an explicit static_cast<size_t> here to pass some bot, or maybe even better a std::floor
https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:41: RTC_CHECK_LE(offset, range); // (fuzzed <= 0xffff) -> (offset < range + 1) BTW, this CHECK is not necessary. Aren't you sure about the calculation in line 40?
The CQ bit was checked by elad.alon@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/...
https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:40: const size_t offset = (static_cast<float>(fuzzed) / 0x10000) * (range + 1); On 2017/01/31 12:50:19, minyue-webrtc wrote: > nit: you probably need an explicit static_cast<size_t> here to pass some bot, or > maybe even better a std::floor Do you think I should do that even if all bots pass, or only if one complains? (I wouldn't go with std::floor, as that would mask my logic being wrong, if it ever indeed has an effect.) https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:41: RTC_CHECK_LE(offset, range); // (fuzzed <= 0xffff) -> (offset < range + 1) On 2017/01/31 12:53:27, minyue-webrtc wrote: > BTW, this CHECK is not necessary. Aren't you sure about the calculation in line > 40? This check explains why the calculation above was correct, and shows it with a CHECK. I think it's helpful for code readers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:40: const size_t offset = (static_cast<float>(fuzzed) / 0x10000) * (range + 1); On 2017/01/31 13:43:54, elad.alon wrote: > On 2017/01/31 12:50:19, minyue-webrtc wrote: > > nit: you probably need an explicit static_cast<size_t> here to pass some bot, > or > > maybe even better a std::floor > > Do you think I should do that even if all bots pass, or only if one complains? > (I wouldn't go with std::floor, as that would mask my logic being wrong, if it > ever indeed has an effect.) Forget about my saying of std::floor, it returns a float number which also requires casting. Since bots don't complain, this is fine. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:41: RTC_CHECK_LE(offset, range); // (fuzzed <= 0xffff) -> (offset < range + 1) On 2017/01/31 13:43:54, elad.alon wrote: > On 2017/01/31 12:53:27, minyue-webrtc wrote: > > BTW, this CHECK is not necessary. Aren't you sure about the calculation in > line > > 40? > > This check explains why the calculation above was correct, and shows it with a > CHECK. I think it's helpful for code readers. It looks redundant. 40 can speaks for itself.
And I call other reviewers to take a look.
lgtm
The CQ bit was checked by elad.alon@webrtc.org
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12865)
https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T FuzzInput(const uint8_t** data, size_t* size) { Seems like over complicating things to me that we make this a template. It's only used with uint16_t?
Looks good, just some minor comments. I haven't looked at all the test cases in detail (I'll trust you/minyue), but I wonder if we should have at rudimentary test case with the maximum number of elements allowed. I didn't see one like that in there, unless we trust the fuzzer for that? https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T FuzzInput(const uint8_t** data, size_t* size) { Only used in one place, seems like you don't need (to templetize) this? https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:24: RTC_CHECK(*size >= sizeof(T)); nit: RTC_CHECK_GE https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { Why final? https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: PlrState(size_t min_num_packets) explicit Would prefer to initialize everything in the initializer list. If you need to do a reset you can just create a new instance and assign it? https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:100: RplrState(size_t min_num_pairs) dito https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:51: // status vector. For instance, the vector it cannot terminate in a lost nit: remove "it" https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:74: EXPECT_EQ(bool(expected_plr), bool(plr)); prefer static_cast to c-style
https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:23: T FuzzInput(const uint8_t** data, size_t* size) { On 2017/02/01 14:27:19, språng wrote: > Only used in one place, seems like you don't need (to templetize) this? Please see the subsequent CL in the series, where this is: 1. used for uint8_t and for uint16_t. 2. is used in the already-templatized ReadData(). https://codereview.webrtc.org/2629883003/diff/300001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:24: RTC_CHECK(*size >= sizeof(T)); On 2017/02/01 14:27:19, språng wrote: > nit: RTC_CHECK_GE Done. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { On 2017/02/01 14:27:19, språng wrote: > Why final? Are there arguments against marking classes as "final" when nobody expects to be sub-classing them? I've checked https://google.github.io/styleguide/cppguide.html for guidelines on this, and could only find: "Explicitly annotate overrides of virtual functions or virtual destructors with an override or (less frequently) final specifier." Admittedly, this only refers to virtual functions, not to classes. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: PlrState(size_t min_num_packets) On 2017/02/01 14:27:19, språng wrote: > explicit > > Would prefer to initialize everything in the initializer list. > If you need to do a reset you can just create a new instance and assign it? * Will add "explicit". * Assigning a new object has its own associated problems. We either use a std::unique_ptr to hold the object, which has a (small) additional cost, or we use bitwise-assignment, which introduces the restriction on the struct that it must remain bitwise-assignable. Stefan has suggested this before; me and Minyue seem to prefer Reset(). I am not sure if Stefan was convinced by my arguments against, at the time. Any thoughts? https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:51: // status vector. For instance, the vector it cannot terminate in a lost On 2017/02/01 14:27:20, språng wrote: > nit: remove "it" Done. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:74: EXPECT_EQ(bool(expected_plr), bool(plr)); On 2017/02/01 14:27:19, språng wrote: > prefer static_cast to c-style Done.
lgtm for webrtc/test
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2629883003/#ps320001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/02/01 15:27:45, stefan-webrtc wrote: > lgtm for webrtc/test Committed. Erik, I've assumed my handling of your comments would be to your satisfaction. If not, please let me know, and I'll upload a CL with whatever additional changes might be required.
On 2017/02/01 15:31:17, elad.alon wrote: > On 2017/02/01 15:27:45, stefan-webrtc wrote: > > lgtm for webrtc/test > > Committed. > Erik, I've assumed my handling of your comments would be to your satisfaction. > If not, please let me know, and I'll upload a CL with whatever additional > changes might be required. Going over things again, I realized I've not responded to your non-inline comment: TransportFeedbackPacketLossTrackerTest.MaxWindowSize covers the case of reaching the maximum window size.
lgtm for now, but maybe keep comments in mind for future updates? https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { On 2017/02/01 15:25:50, elad.alon wrote: > On 2017/02/01 14:27:19, språng wrote: > > Why final? > > Are there arguments against marking classes as "final" when nobody expects to be > sub-classing them? > I've checked https://google.github.io/styleguide/cppguide.html for guidelines on > this, and could only find: > > "Explicitly annotate overrides of virtual functions or virtual destructors with > an override or (less frequently) final specifier." > > Admittedly, this only refers to virtual functions, not to classes. I think final is only rarely used, to indicate that it strictly forbidden to subclass something, perhaps for security reasons or that something can subtly break because components have made dangerous assumptions. It's not at all uncommon that classes are inherited from at some point or other, most commonly for testing purposes. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: PlrState(size_t min_num_packets) On 2017/02/01 15:25:50, elad.alon wrote: > On 2017/02/01 14:27:19, språng wrote: > > explicit > > > > Would prefer to initialize everything in the initializer list. > > If you need to do a reset you can just create a new instance and assign it? > > * Will add "explicit". > * Assigning a new object has its own associated problems. We either use a > std::unique_ptr to hold the object, which has a (small) additional cost, or we > use bitwise-assignment, which introduces the restriction on the struct that it > must remain bitwise-assignable. Stefan has suggested this before; me and Minyue > seem to prefer Reset(). I am not sure if Stefan was convinced by my arguments > against, at the time. Any thoughts? I generally dislike reset methods, unless it's a very specific case like containers. The usually seems to be only used by tests, and in that case I don't think it's worth worrying over the minor performance aspects. It also makes it harder for static analysis tools to reason about initialization and if we have const elements (like here), we split up initialization into two places which makes it harder to read and keep in sync.
Will do; thanks for the input. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { On 2017/02/01 15:52:22, språng wrote: > On 2017/02/01 15:25:50, elad.alon wrote: > > On 2017/02/01 14:27:19, språng wrote: > > > Why final? > > > > Are there arguments against marking classes as "final" when nobody expects to > be > > sub-classing them? > > I've checked https://google.github.io/styleguide/cppguide.html for guidelines > on > > this, and could only find: > > > > "Explicitly annotate overrides of virtual functions or virtual destructors > with > > an override or (less frequently) final specifier." > > > > Admittedly, this only refers to virtual functions, not to classes. > > I think final is only rarely used, to indicate that it strictly forbidden to > subclass something, perhaps for security reasons or that something can subtly > break because components have made dangerous assumptions. > It's not at all uncommon that classes are inherited from at some point or other, > most commonly for testing purposes. If Minyue doesn't object, I could remove the "final" keyword in the next CL. But for my two cents, I think it would be good to have it here until someone does really need to inherit from the class, since knowing if there aren any sub-classes is something that a modifier of the class would be interested in.
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1485962985184800,
"parent_rev": "14245cc9397284cabab5057c0c661953e2d0cdec", "commit_rev":
"d83b9670a67c1445a81a3cf20984185c99faa3dc"}
Message was sent while issue was closed.
Description was changed from ========== Replace consecutive-losses count by a calculation of first-order-FEC recoverability. Note: * PLR is calculated over all of the known packets. * RPLR is calculated over all of the known packet *pairs*. That is, only over sets of subsequent packets where the reception status is known for both. BUG=webrtc:7058 ========== to ========== Replace consecutive-losses count by a calculation of first-order-FEC recoverability. Note: * PLR is calculated over all of the known packets. * RPLR is calculated over all of the known packet *pairs*. That is, only over sets of subsequent packets where the reception status is known for both. BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2629883003 Cr-Commit-Position: refs/heads/master@{#16401} Committed: https://chromium.googlesource.com/external/webrtc/+/d83b9670a67c1445a81a3cf20... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/d83b9670a67c1445a81a3cf20...
Message was sent while issue was closed.
some post-submit comment (only comments, no action need from my side.) https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { On 2017/02/01 15:58:56, elad.alon wrote: > On 2017/02/01 15:52:22, språng wrote: > > On 2017/02/01 15:25:50, elad.alon wrote: > > > On 2017/02/01 14:27:19, språng wrote: > > > > Why final? > > > > > > Are there arguments against marking classes as "final" when nobody expects > to > > be > > > sub-classing them? > > > I've checked https://google.github.io/styleguide/cppguide.html for > guidelines > > on > > > this, and could only find: > > > > > > "Explicitly annotate overrides of virtual functions or virtual destructors > > with > > > an override or (less frequently) final specifier." > > > > > > Admittedly, this only refers to virtual functions, not to classes. > > > > I think final is only rarely used, to indicate that it strictly forbidden to > > subclass something, perhaps for security reasons or that something can subtly > > break because components have made dangerous assumptions. > > It's not at all uncommon that classes are inherited from at some point or > other, > > most commonly for testing purposes. > > If Minyue doesn't object, I could remove the "final" keyword in the next CL. But > for my two cents, I think it would be good to have it here until someone does > really need to inherit from the class, since knowing if there aren any > sub-classes is something that a modifier of the class would be interested in. it looks like that audio team prefers using "final" on all class implementation, leaving only non-virtual class non-virtual. I have been convinced that this is a good practice. But I am open-minded. https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: PlrState(size_t min_num_packets) On 2017/02/01 15:52:22, språng wrote: > On 2017/02/01 15:25:50, elad.alon wrote: > > On 2017/02/01 14:27:19, språng wrote: > > > explicit > > > > > > Would prefer to initialize everything in the initializer list. > > > If you need to do a reset you can just create a new instance and assign it? > > > > * Will add "explicit". > > * Assigning a new object has its own associated problems. We either use a > > std::unique_ptr to hold the object, which has a (small) additional cost, or we > > use bitwise-assignment, which introduces the restriction on the struct that it > > must remain bitwise-assignable. Stefan has suggested this before; me and > Minyue > > seem to prefer Reset(). I am not sure if Stefan was convinced by my arguments > > against, at the time. Any thoughts? > > I generally dislike reset methods, unless it's a very specific case like > containers. The usually seems to be only used by tests, and in that case I don't > think it's worth worrying over the minor performance aspects. It also makes it > harder for static analysis tools to reason about initialization and if we have > const elements (like here), we split up initialization into two places which > makes it harder to read and keep in sync. > It feels natural to use Reset() here since the TransportFeedbackPacketLossTracker has a Reset() method. What does not get reset in this class is min_num_packets_ which would otherwise need to be stored in TransportFeedbackPacketLossTracker to be able to do a Reset(). So I think, we either find a way to remove all Resets, or we keep all of them.
Message was sent while issue was closed.
On 2017/02/01 16:39:25, minyue-webrtc wrote: > some post-submit comment (only comments, no action need from my side.) > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class > TransportFeedbackPacketLossTracker final { > On 2017/02/01 15:58:56, elad.alon wrote: > > On 2017/02/01 15:52:22, språng wrote: > > > On 2017/02/01 15:25:50, elad.alon wrote: > > > > On 2017/02/01 14:27:19, språng wrote: > > > > > Why final? > > > > > > > > Are there arguments against marking classes as "final" when nobody expects > > to > > > be > > > > sub-classing them? > > > > I've checked https://google.github.io/styleguide/cppguide.html for > > guidelines > > > on > > > > this, and could only find: > > > > > > > > "Explicitly annotate overrides of virtual functions or virtual destructors > > > with > > > > an override or (less frequently) final specifier." > > > > > > > > Admittedly, this only refers to virtual functions, not to classes. > > > > > > I think final is only rarely used, to indicate that it strictly forbidden to > > > subclass something, perhaps for security reasons or that something can > subtly > > > break because components have made dangerous assumptions. > > > It's not at all uncommon that classes are inherited from at some point or > > other, > > > most commonly for testing purposes. > > > > If Minyue doesn't object, I could remove the "final" keyword in the next CL. > But > > for my two cents, I think it would be good to have it here until someone does > > really need to inherit from the class, since knowing if there aren any > > sub-classes is something that a modifier of the class would be interested in. > > it looks like that audio team prefers using "final" on all class implementation, > leaving only non-virtual class non-virtual. I have been convinced that this is a > good practice. But I am open-minded. > Sorry, what did I write here! I meant "leaving only virtual class non-final" > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: PlrState(size_t > min_num_packets) > On 2017/02/01 15:52:22, språng wrote: > > On 2017/02/01 15:25:50, elad.alon wrote: > > > On 2017/02/01 14:27:19, språng wrote: > > > > explicit > > > > > > > > Would prefer to initialize everything in the initializer list. > > > > If you need to do a reset you can just create a new instance and assign > it? > > > > > > * Will add "explicit". > > > * Assigning a new object has its own associated problems. We either use a > > > std::unique_ptr to hold the object, which has a (small) additional cost, or > we > > > use bitwise-assignment, which introduces the restriction on the struct that > it > > > must remain bitwise-assignable. Stefan has suggested this before; me and > > Minyue > > > seem to prefer Reset(). I am not sure if Stefan was convinced by my > arguments > > > against, at the time. Any thoughts? > > > > I generally dislike reset methods, unless it's a very specific case like > > containers. The usually seems to be only used by tests, and in that case I > don't > > think it's worth worrying over the minor performance aspects. It also makes it > > harder for static analysis tools to reason about initialization and if we have > > const elements (like here), we split up initialization into two places which > > makes it harder to read and keep in sync. > > > > It feels natural to use Reset() here since the > TransportFeedbackPacketLossTracker has a Reset() method. > What does not get reset in this class is min_num_packets_ which would otherwise > need to be stored in TransportFeedbackPacketLossTracker to be able to do a > Reset(). So I think, we either find a way to remove all Resets, or we keep all > of them.
Message was sent while issue was closed.
On 2017/02/01 16:39:25, minyue-webrtc wrote: > some post-submit comment (only comments, no action need from my side.) > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class > TransportFeedbackPacketLossTracker final { > On 2017/02/01 15:58:56, elad.alon wrote: > > On 2017/02/01 15:52:22, språng wrote: > > > On 2017/02/01 15:25:50, elad.alon wrote: > > > > On 2017/02/01 14:27:19, språng wrote: > > > > > Why final? > > > > > > > > Are there arguments against marking classes as "final" when nobody expects > > to > > > be > > > > sub-classing them? > > > > I've checked https://google.github.io/styleguide/cppguide.html for > > guidelines > > > on > > > > this, and could only find: > > > > > > > > "Explicitly annotate overrides of virtual functions or virtual destructors > > > with > > > > an override or (less frequently) final specifier." > > > > > > > > Admittedly, this only refers to virtual functions, not to classes. > > > > > > I think final is only rarely used, to indicate that it strictly forbidden to > > > subclass something, perhaps for security reasons or that something can > subtly > > > break because components have made dangerous assumptions. > > > It's not at all uncommon that classes are inherited from at some point or > > other, > > > most commonly for testing purposes. > > > > If Minyue doesn't object, I could remove the "final" keyword in the next CL. > But > > for my two cents, I think it would be good to have it here until someone does > > really need to inherit from the class, since knowing if there aren any > > sub-classes is something that a modifier of the class would be interested in. > > it looks like that audio team prefers using "final" on all class implementation, > leaving only non-virtual class non-virtual. I have been convinced that this is a > good practice. But I am open-minded. That's the first I've heard about that, and I'm not sure I agree. For some classes, such as RplrState, I would agree. For more complex classes that are designed to interact in a specific way with other components, I'd rather have it mandatory to make it virtual and provide a mock for testing. For this class I think it would also make sense to expose some things as protected so that we could move the Validate() method to the tests. Our code base suffers a lot from poor testability because we have nested complex classes and so end up with more like loose integration tests than unit tests in many places. But that's perhaps a topic for discussion outside of this cl :) > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: PlrState(size_t > min_num_packets) > On 2017/02/01 15:52:22, språng wrote: > > On 2017/02/01 15:25:50, elad.alon wrote: > > > On 2017/02/01 14:27:19, språng wrote: > > > > explicit > > > > > > > > Would prefer to initialize everything in the initializer list. > > > > If you need to do a reset you can just create a new instance and assign > it? > > > > > > * Will add "explicit". > > > * Assigning a new object has its own associated problems. We either use a > > > std::unique_ptr to hold the object, which has a (small) additional cost, or > we > > > use bitwise-assignment, which introduces the restriction on the struct that > it > > > must remain bitwise-assignable. Stefan has suggested this before; me and > > Minyue > > > seem to prefer Reset(). I am not sure if Stefan was convinced by my > arguments > > > against, at the time. Any thoughts? > > > > I generally dislike reset methods, unless it's a very specific case like > > containers. The usually seems to be only used by tests, and in that case I > don't > > think it's worth worrying over the minor performance aspects. It also makes it > > harder for static analysis tools to reason about initialization and if we have > > const elements (like here), we split up initialization into two places which > > makes it harder to read and keep in sync. > > > > It feels natural to use Reset() here since the > TransportFeedbackPacketLossTracker has a Reset() method. > What does not get reset in this class is min_num_packets_ which would otherwise > need to be stored in TransportFeedbackPacketLossTracker to be able to do a > Reset(). So I think, we either find a way to remove all Resets, or we keep all > of them. You don't need to store it externally since it's a public member of the struct. plr_state_ = PlrState(plr_state_.min_num_packets_); I'd lean toward removing all resets.
Message was sent while issue was closed.
On 2017/02/02 09:48:40, språng wrote: > On 2017/02/01 16:39:25, minyue-webrtc wrote: > > some post-submit comment (only comments, no action need from my side.) > > > > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): > > > > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > > webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class > > TransportFeedbackPacketLossTracker final { > > On 2017/02/01 15:58:56, elad.alon wrote: > > > On 2017/02/01 15:52:22, språng wrote: > > > > On 2017/02/01 15:25:50, elad.alon wrote: > > > > > On 2017/02/01 14:27:19, språng wrote: > > > > > > Why final? > > > > > > > > > > Are there arguments against marking classes as "final" when nobody > expects > > > to > > > > be > > > > > sub-classing them? > > > > > I've checked https://google.github.io/styleguide/cppguide.html for > > > guidelines > > > > on > > > > > this, and could only find: > > > > > > > > > > "Explicitly annotate overrides of virtual functions or virtual > destructors > > > > with > > > > > an override or (less frequently) final specifier." > > > > > > > > > > Admittedly, this only refers to virtual functions, not to classes. > > > > > > > > I think final is only rarely used, to indicate that it strictly forbidden > to > > > > subclass something, perhaps for security reasons or that something can > > subtly > > > > break because components have made dangerous assumptions. > > > > It's not at all uncommon that classes are inherited from at some point or > > > other, > > > > most commonly for testing purposes. > > > > > > If Minyue doesn't object, I could remove the "final" keyword in the next CL. > > But > > > for my two cents, I think it would be good to have it here until someone > does > > > really need to inherit from the class, since knowing if there aren any > > > sub-classes is something that a modifier of the class would be interested > in. > > > > it looks like that audio team prefers using "final" on all class > implementation, > > leaving only non-virtual class non-virtual. I have been convinced that this is > a > > good practice. But I am open-minded. > > That's the first I've heard about that, and I'm not sure I agree. > For some classes, such as RplrState, I would agree. For more complex classes > that > are designed to interact in a specific way with other components, I'd rather > have > it mandatory to make it virtual and provide a mock for testing. For this class > I think it would also make sense to expose some things as protected so that we > could move the Validate() method to the tests. > > Our code base suffers a lot from poor testability because we have nested complex > classes and so end up with more like loose integration tests than unit tests in > many places. But that's perhaps a topic for discussion outside of this cl :) > > > > https://codereview.webrtc.org/2629883003/diff/300001/webrtc/voice_engine/tran... > > webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:84: > PlrState(size_t > > min_num_packets) > > On 2017/02/01 15:52:22, språng wrote: > > > On 2017/02/01 15:25:50, elad.alon wrote: > > > > On 2017/02/01 14:27:19, språng wrote: > > > > > explicit > > > > > > > > > > Would prefer to initialize everything in the initializer list. > > > > > If you need to do a reset you can just create a new instance and assign > > it? > > > > > > > > * Will add "explicit". > > > > * Assigning a new object has its own associated problems. We either use a > > > > std::unique_ptr to hold the object, which has a (small) additional cost, > or > > we > > > > use bitwise-assignment, which introduces the restriction on the struct > that > > it > > > > must remain bitwise-assignable. Stefan has suggested this before; me and > > > Minyue > > > > seem to prefer Reset(). I am not sure if Stefan was convinced by my > > arguments > > > > against, at the time. Any thoughts? > > > > > > I generally dislike reset methods, unless it's a very specific case like > > > containers. The usually seems to be only used by tests, and in that case I > > don't > > > think it's worth worrying over the minor performance aspects. It also makes > it > > > harder for static analysis tools to reason about initialization and if we > have > > > const elements (like here), we split up initialization into two places which > > > makes it harder to read and keep in sync. > > > > > > > It feels natural to use Reset() here since the > > TransportFeedbackPacketLossTracker has a Reset() method. > > What does not get reset in this class is min_num_packets_ which would > otherwise > > need to be stored in TransportFeedbackPacketLossTracker to be able to do a > > Reset(). So I think, we either find a way to remove all Resets, or we keep all > > of them. > > You don't need to store it externally since it's a public member of the struct. > plr_state_ = PlrState(plr_state_.min_num_packets_); > > I'd lean toward removing all resets. +1 to removing resets. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
