Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1715673002:
Implement the NackModule as part of the new jitter buffer. (Closed)
|
Created:
4 years, 10 months ago by philipel Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement the NackModule as part of the new jitter buffer.
Things done/implemented in this CL:
- An interface that can send Nack (VCMNackSender).
- An interface that can request KeyFrames (VCMKeyFrameRequestSender).
- The nack module (NackModule).
- A set of convenience functions for modular numbers (mod_ops.h).
BUG=webrtc:5514
Committed: https://crrev.com/f472c5b6722dfb221f929fc4d3a2b4ca54647701
Cr-Commit-Position: refs/heads/master@{#11882}
Patch Set 1 #
Total comments: 96
Patch Set 2 : Feedback fixes. #
Total comments: 26
Patch Set 3 : Feedback fixes. #
Total comments: 3
Patch Set 4 : Implemented reordering statistics in separate class. #
Total comments: 62
Patch Set 5 : ModOps fixes. #Patch Set 6 : Feedback fixes. #
Total comments: 6
Patch Set 7 : Removed commented code. #
Total comments: 21
Patch Set 8 : Fixing the last comments™. #
Total comments: 1
Patch Set 9 : ModOps Add optimization. #
Total comments: 13
Patch Set 10 : Comment fixes. #
Total comments: 8
Patch Set 11 : ModOps fixes and unittests. #Patch Set 12 : std::max type fix. #Patch Set 13 : SeqNumComparator operator marked const. #Patch Set 14 : Include cstddef for size_t #Patch Set 15 : Added missing comma in base.gyp. #Patch Set 16 : Testing stuff for MSVC #Patch Set 17 : Testing stuff for MSVC 2 #Patch Set 18 : MSVC Fix. #Patch Set 19 : Format #
Total comments: 1
Messages
Total messages: 73 (27 generated)
Description was changed from ========== Implement the NackModule as part of the new jitter buffer. Things done/implemented in this CL: - An interface that can send Nack (VCMNackSender). - An interface that can request KeyFrames (VCMKeyFrameRequestSender). - The nack module (NackModule). - A set of convenience functions for modular numbers (mod_ops.h). BUG=webrtc:5514 ========== to ========== Implement the NackModule as part of the new jitter buffer. Things done/implemented in this CL: - An interface that can send Nack (VCMNackSender). - An interface that can request KeyFrames (VCMKeyFrameRequestSender). - The nack module (NackModule). - A set of convenience functions for modular numbers (mod_ops.h). BUG=webrtc:5514 ==========
philipel@webrtc.org changed reviewers: + sprang@webrtc.org
philipel@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:16: #include "webrtc/modules/video_coding/nack_module.h" I tend to prefer short names. Should we call this nack.h/.cc instead? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:25: // =========================================================== I prefer not having comments like this. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:45: last_process_timestamp_ms_(0), Initialize to -1 instead. 0 is a bad initial value for time. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:53: RTC_DCHECK(keyframe_request_sender_ != nullptr); You don't need != nullptr. Just do RTC_DCHECK(keyframe_request_sender_); https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:57: CriticalSectionScoped cs(critical_section_); Seems odd that we would have to take the critical section in the destructor. Why is that? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:62: CriticalSectionScoped cs(critical_section_); Name this lock instead of cs, and use rtc::CritScoped https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:65: uint16_t seq_num(packet.seqNum); uint16_t seq_num = packet.seqNum; or use packet.seqNum directly? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:88: std::vector<uint16_t> nack_batch = GetNackBatch(true, false); I think this call is a bit hard to read since I need to know what true and false means. Could we use enums or helper methods with a more descriptive name? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:89: if (!nack_batch.empty() && nack_sender_ != nullptr) Can we make sure nack_sender_ is always set so that we don't have to check this? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:112: clock_->TimeInMilliseconds(); Maybe use max(0, expr) to ensure it doesn't get negative? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:125: if (last_process_timestamp_ms_ == 0) s/timestamp/time Use brackets {} for multi-line if/else-statements. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:131: kProcessIntervalMs * kProcessIntervalMs; All of this looks really strange to me. Why not simply set last_process_timestamp_ms_ = now_ms? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:145: uint16_t num_new_nacks = PositiveDiff(seq_num_start, seq_num_end); Are these sequence numbers guaranteed not to already be in the nack list? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:147: NackList::iterator nack_it = nack_list_.lower_bound(last_keyframe_seq_num_); auto instead of NackList::iterator, and just call it "it". How do we notify the jitter buffer that it should skip ahead to the last key frame when this happens? And is the last key frame correct, or should we pick the oldest key frame? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:147: NackList::iterator nack_it = nack_list_.lower_bound(last_keyframe_seq_num_); What if last_keyframe_seq_num_ is 0xffff and the last incoming packet had sequence number 0? Don't we end up erasing 0 in that case? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:151: if (nack_list_.size() + num_new_nacks >= kMaxNackPackets) { Are the new sequence numbers guaranteed to be later than last_keyframe_seq_num_, or is there a chance they should have been removed too? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:153: keyframe_request_sender_->RequestKeyFrame(); What do you think of instead returning kRequestKeyFrame if the nack module needs a key frame to avoid another callback? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:177: nack_it->second.retries++; ++...retries; https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:179: if (nack_it->second.retries >= kMaxNackRetries) Brackets https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:180: nack_it = nack_list_.erase(nack_it); If we decide to stop trying, should we then make sure there is a key frame to skip ahead to? Or should we allow nacking indefinitely and rely on the jitter buffer to let us know when to stop retrying? Maybe by having an API which it can call to notify the nack module that it's no longer interested in packets with timestamp < X (or seq num < Y)? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:191: if (nack_it->second.retries >= kMaxNackRetries) See above https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:202: void NackModule::UpdateReorderingStatistics(uint16_t seq_num) { I think I would recommend breaking out the histogram handling and probability computations into a separate histogram class that can be tested in isolation. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:205: int8_t remove_from_bucket = reordering_occurences_[reordering_index_]; int https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:208: DCHECK_GE(remove_from_bucket, 0); and maybe also DCHECK_LT(remove_from_bucket, reordering_buckets_.size())? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:209: uint16_t diff = NegativeDiff(last_seq_num_, seq_num); Is this the same as PositiveDiff(seq_num, last_seq_num)? I think it's a bit hard to interpret what a negative diff is. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:220: uint8_t NackModule::WaitNumberOfPackets(float probability) const { return int https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:230: float accumelated_probability = 0; accumulated https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:244: // =========================================================== Remove https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:252: retries(0) {} Maybe put these methods at the top instead. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:26: class NackModule : public Module { It's difficult to see why this needs to be a Module without seeing how it's going to be used. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:35: void Notify(const VCMPacket& packet); OnReceivedPacket, possibly? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:50: static const uint8_t kProcessIntervalMs = 1000 / kProcessFrequency; Any reason we're mixing static constants defined in the header file and constants defined in the .cc file? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:52: struct NackInfo { Could you add a comment which shortly describes what this is? Or improve the name of the class. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:55: int64_t send_at_timestamp; time_last_sent_ms? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:56: uint8_t retries; int https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:59: NackInfo(uint16_t seq_num, uint16_t send_at_seq_num); Constructors at the top. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:79: CriticalSectionWrapper* critical_section_; Prefer keeping this shorter, e.g., crit_ which is a name typically used in the video codebase. Also use rtc::CriticalSection instead, and make it a "rtc::CriticalSection* const". Btw, why can't this class own its own critical section? Seems a bit error prone, and it's a pattern I haven't seen before. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:80: Clock* clock_; Clock* const https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:82: VCMKeyFrameRequestSender* keyframe_request_sender_; Can these two be const too? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:84: typedef std::map<uint16_t, NackInfo> NackList; I think we can live without this typedef now that we have c++11 and can use auto. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:88: uint16_t last_seq_num_ GUARDED_BY(critical_section_); Move this down to the other last_ members. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:99: critical_section_); Most members are protected by critical_section_. Can we add an rtc::ThreadChecker (or several) to document the threads accessing the methods of this class so that it's easier to understand why we need to lock everything? See for instance packet_router.cc for an example. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module_unittest.cc (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module_unittest.cc:153: I think it would be good to add more tests where we have sequence number overflows and reordering. For instance the case when we've had a sequence number overflow after the last received key frame and the nack list becomes too large.
https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:16: #include "webrtc/modules/video_coding/nack_module.h" On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > I tend to prefer short names. Should we call this nack.h/.cc instead? Acknowledged. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:25: // =========================================================== On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > I prefer not having comments like this. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:45: last_process_timestamp_ms_(0), On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Initialize to -1 instead. 0 is a bad initial value for time. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:53: RTC_DCHECK(keyframe_request_sender_ != nullptr); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > You don't need != nullptr. > > Just do RTC_DCHECK(keyframe_request_sender_); Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:57: CriticalSectionScoped cs(critical_section_); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Seems odd that we would have to take the critical section in the destructor. Why > is that? If some thread is inside another function then the object wont be destroyed until that thread is done. But if the nack module is destroyed when some other thread is using it then something is very wrong. Removed the destructor. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:62: CriticalSectionScoped cs(critical_section_); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Name this lock instead of cs, and use rtc::CritScoped Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:65: uint16_t seq_num(packet.seqNum); On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > uint16_t seq_num = packet.seqNum; > > or use packet.seqNum directly? If we change packet class then we only have to change here. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:88: std::vector<uint16_t> nack_batch = GetNackBatch(true, false); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > I think this call is a bit hard to read since I need to know what true and false > means. Could we use enums or helper methods with a more descriptive name? Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:89: if (!nack_batch.empty() && nack_sender_ != nullptr) On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Can we make sure nack_sender_ is always set so that we don't have to check this? Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:112: clock_->TimeInMilliseconds(); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Maybe use max(0, expr) to ensure it doesn't get negative? Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:125: if (last_process_timestamp_ms_ == 0) On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > s/timestamp/time > > Use brackets {} for multi-line if/else-statements. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:131: kProcessIntervalMs * kProcessIntervalMs; On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > All of this looks really strange to me. Why not simply set > last_process_timestamp_ms_ = now_ms? The idea was that to make sure we send at a certain rate over time. If process is called on average lets say 5 ms late then the interval would become 25 ms instead of 20, or 40 times per second instead of 50. In short, it is a way to compensate for the worker thread to always call process a bit late (usually somewhere between 2-8 ms). https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:145: uint16_t num_new_nacks = PositiveDiff(seq_num_start, seq_num_end); On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > Are these sequence numbers guaranteed not to already be in the nack list? This function is called from Notify (now OnReceivePacket) and the logic for detecting new packets to nack is in the Notify function, so yes, they SHOULD not already be in the nack list. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:147: NackList::iterator nack_it = nack_list_.lower_bound(last_keyframe_seq_num_); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > auto instead of NackList::iterator, and just call it "it". > > How do we notify the jitter buffer that it should skip ahead to the last key > frame when this happens? And is the last key frame correct, or should we pick > the oldest key frame? Changed the implementation completely, now we clear until the next keyframe and continue to do so until there is enough space. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:153: keyframe_request_sender_->RequestKeyFrame(); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > What do you think of instead returning kRequestKeyFrame if the nack module needs > a key frame to avoid another callback? Don't we need to do the callback anyway, only in the jitter buffer instead of in the nack module? I would like to keep the nack module as contained as possible. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:177: nack_it->second.retries++; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > ++...retries; Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:180: nack_it = nack_list_.erase(nack_it); On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > If we decide to stop trying, should we then make sure there is a key frame to > skip ahead to? Or should we allow nacking indefinitely and rely on the jitter > buffer to let us know when to stop retrying? Maybe by having an API which it can > call to notify the nack module that it's no longer interested in packets with > timestamp < X (or seq num < Y)? I think a ClearUpToSequenceNumber(uint16_t seq_num) function would be very useful to have. But I still think we should avoid signaling to the jitter buffer that we have stopped nacking a packet. The general idea is that the nack module will try hard to get every packet, and if that is not possible then the jitter buffer has to recover on its own. Of course requesting a keyframe from the nack module should be very rare. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:191: if (nack_it->second.retries >= kMaxNackRetries) On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > See above Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:202: void NackModule::UpdateReorderingStatistics(uint16_t seq_num) { On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > I think I would recommend breaking out the histogram handling and probability > computations into a separate histogram class that can be tested in isolation. I agree, will do it for the next upload. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:205: int8_t remove_from_bucket = reordering_occurences_[reordering_index_]; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > int Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:209: uint16_t diff = NegativeDiff(last_seq_num_, seq_num); On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > Is this the same as PositiveDiff(seq_num, last_seq_num)? I think it's a bit hard > to interpret what a negative diff is. NegativeDiff(a,b) == PositiveDiff(b,a) Added comments in mod_ops.h which describes what it does. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:220: uint8_t NackModule::WaitNumberOfPackets(float probability) const { On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > return int Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:230: float accumelated_probability = 0; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > accumulated Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:244: // =========================================================== On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Remove Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:252: retries(0) {} On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > Maybe put these methods at the top instead. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:26: class NackModule : public Module { On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > It's difficult to see why this needs to be a Module without seeing how it's > going to be used. The reason this is a module is because it needs to frequently check if it is time to resend nacks. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:35: void Notify(const VCMPacket& packet); On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > OnReceivedPacket, possibly? Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:50: static const uint8_t kProcessIntervalMs = 1000 / kProcessFrequency; On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > Any reason we're mixing static constants defined in the header file and > constants defined in the .cc file? Earlier I had logic which used these constants as arguments in std::min/max, and since they are passed by reference they had to be declare in the .cc file as well. Not needed anymore, so I removed them from the .cc file. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:52: struct NackInfo { On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > Could you add a comment which shortly describes what this is? Or improve the > name of the class. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:55: int64_t send_at_timestamp; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > time_last_sent_ms? This is the timestamp at which this nack should be sent again, not when it was last sent. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:56: uint8_t retries; On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > int Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:59: NackInfo(uint16_t seq_num, uint16_t send_at_seq_num); On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > Constructors at the top. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:79: CriticalSectionWrapper* critical_section_; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Prefer keeping this shorter, e.g., crit_ which is a name typically used in the > video codebase. Also use rtc::CriticalSection instead, and make it a > "rtc::CriticalSection* const". > > Btw, why can't this class own its own critical section? Seems a bit error prone, > and it's a pattern I haven't seen before. NackModule now has its own rtc::CriticalSection crit_. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:80: Clock* clock_; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Clock* const Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:82: VCMKeyFrameRequestSender* keyframe_request_sender_; On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > Can these two be const too? Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:84: typedef std::map<uint16_t, NackInfo> NackList; On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > I think we can live without this typedef now that we have c++11 and can use > auto. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:88: uint16_t last_seq_num_ GUARDED_BY(critical_section_); On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > Move this down to the other last_ members. Done. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:99: critical_section_); On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > Most members are protected by critical_section_. Can we add an > rtc::ThreadChecker (or several) to document the threads accessing the methods of > this class so that it's easier to understand why we need to lock everything? See > for instance packet_router.cc for an example. My understanding of most classes is that almost every member can only be safely modified when holding a lock. I just added these constraints so the compiler can check that it holds true.
https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:131: kProcessIntervalMs * kProcessIntervalMs; On 2016/02/25 15:09:32, philipel wrote: > On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > > All of this looks really strange to me. Why not simply set > > last_process_timestamp_ms_ = now_ms? > > The idea was that to make sure we send at a certain rate over time. If process > is called on average lets say 5 ms late then the interval would become 25 ms > instead of 20, or 40 times per second instead of 50. > > In short, it is a way to compensate for the worker thread to always call process > a bit late (usually somewhere between 2-8 ms). Ok, but isn't it weird to not set last_process_time_ms_ = now_ms on the first call to Process(). Why now_ms + kProcessIntervalMs? Sounds more like next_process_time_ms_ to me. I'd like to have unittests which verifies this is correct too. How much we want to compensate for this? We don't want to burst out 5 nacks in 10 ms just because the worker thread was busy for some time, right? Does this guarantee this won't happen? Maybe add a unittest for that case too. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:145: uint16_t num_new_nacks = PositiveDiff(seq_num_start, seq_num_end); On 2016/02/25 15:09:30, philipel wrote: > On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > > Are these sequence numbers guaranteed not to already be in the nack list? > > This function is called from Notify (now OnReceivePacket) and the logic for > detecting new packets to nack is in the Notify function, so yes, they SHOULD not > already be in the nack list. > Would it make sense to dcheck on that somewhere in this method? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:153: keyframe_request_sender_->RequestKeyFrame(); On 2016/02/25 15:09:31, philipel wrote: > On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > > What do you think of instead returning kRequestKeyFrame if the nack module > needs > > a key frame to avoid another callback? > > Don't we need to do the callback anyway, only in the jitter buffer instead of in > the nack module? I would like to keep the nack module as contained as possible. Ok! At some point we will have to call back, yes. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:180: nack_it = nack_list_.erase(nack_it); On 2016/02/25 15:09:31, philipel wrote: > On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > > If we decide to stop trying, should we then make sure there is a key frame to > > skip ahead to? Or should we allow nacking indefinitely and rely on the jitter > > buffer to let us know when to stop retrying? Maybe by having an API which it > can > > call to notify the nack module that it's no longer interested in packets with > > timestamp < X (or seq num < Y)? > > I think a ClearUpToSequenceNumber(uint16_t seq_num) function would be very > useful to have. But I still think we should avoid signaling to the jitter buffer > that we have stopped nacking a packet. The general idea is that the nack module > will try hard to get every packet, and if that is not possible then the jitter > buffer has to recover on its own. Of course requesting a keyframe from the nack > module should be very rare. So in that case we will be waiting for the jitter buffer to time out and request a key frame on its own? The question is how much extra latency we will get for having to wait for the jitter buffer timeout. The trade off is that the nack module may be requesting a key frame for a packet it has stopped nacking which wasn't really needed to continue decoding. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:26: class NackModule : public Module { On 2016/02/25 15:09:32, philipel wrote: > On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > > It's difficult to see why this needs to be a Module without seeing how it's > > going to be used. > > The reason this is a module is because it needs to frequently check if it is > time to resend nacks. Maybe you should include the code which uses it in this CL to make it easier to review? Or is that a lot of code? https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:50: static const uint8_t kProcessIntervalMs = 1000 / kProcessFrequency; On 2016/02/25 15:09:32, philipel wrote: > On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > > Any reason we're mixing static constants defined in the header file and > > constants defined in the .cc file? > > Earlier I had logic which used these constants as arguments in std::min/max, and > since they are passed by reference they had to be declare in the .cc file as > well. Not needed anymore, so I removed them from the .cc file. I guess my question is, can we move more constants to the .cc file? Most don't seem to be used in this file. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:99: critical_section_); On 2016/02/25 15:09:32, philipel wrote: > On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > > Most members are protected by critical_section_. Can we add an > > rtc::ThreadChecker (or several) to document the threads accessing the methods > of > > this class so that it's easier to understand why we need to lock everything? > See > > for instance packet_router.cc for an example. > > My understanding of most classes is that almost every member can only be safely > modified when holding a lock. I just added these constraints so the compiler can > check that it holds true. Right, but I think it would be good to go over what threads we expect will be calling which methods and document that using a ThreadChecker. Maybe we then can figure out that some of these are only accessed from a single thread. In any case, at least we will make sure the thread model is known. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:41: // Calculates the forward differance between two numbers. forward difference https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:63: if (is_keyframe) seq_num_keyframes_.emplace(unwrapper_.Unwrap(seq_num)); Maybe we should be unwrapping all sequence numbers and timestamps instead of mixing unwrapping with modular comparisons? Either use unwrapping everywhere, or use a map with a specialized comparison operator? I think I would prefer the latter. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:90: nack_sender_->SendNack(nack_batch.data(), nack_batch.size()); Is there any risk that we end up deadlocking here? Packets are being received and parsed in the rtp module on the same thread which calls SendNack which also goes to the rtp module. I suspect we may end up taking the same locks in rtp module, so that we at least get recursive locking? https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:96: } move the else to line 70 and make an early return: if (AheadOf(last_seq_num_, seq_num)) { RemovePacketFromNack(seq_num); if (!is_retransmitted) UpdateReorderingStatistics(seq_num); return; } AddPacketsToNack(...); ... https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:115: 0l); 0ll is what you want, I think? Or just do 0 as I think that works equally well. You could also do std::max<int64_t>(...); https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:28: public: I think we need a method which can be used to inform the nack module that the stream is decodable beyond a certain timestamp (or sequence number, maybe), so that the nack module can drop all nacks prior to that timestamp. Otherwise it may keep nacking things that won't be used. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:59: int64_t send_at_time; Would it be more logical to have "sent_at_time" and sent_at_seq_num? That way the time to resend the packet can be dynamic, which is good since the rtt may vary. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:77: kSeqNumAndTime}; Place type definitions directly after "private:" https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:97: std::set<int64_t> seq_num_keyframes_ GUARDED_BY(crit_); std::set<uint16_t>, right? https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:105: uint8_t reordering_count_ GUARDED_BY(crit_); int https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:106: int8_t reordering_occurences_[kMaxReorderedPackets] GUARDED_BY(crit_); int https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:107: uint8_t reordering_buckets_[kNumReorderingBuckets] GUARDED_BY(crit_); int
Fixed most of the feedback, will separate the reordering statistics into a separate class in a later patch. Will also take a look at the ThreadChecker. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:131: kProcessIntervalMs * kProcessIntervalMs; On 2016/02/26 09:26:54, stefan-webrtc (holmer) wrote: > On 2016/02/25 15:09:32, philipel wrote: > > On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > > > All of this looks really strange to me. Why not simply set > > > last_process_timestamp_ms_ = now_ms? > > > > The idea was that to make sure we send at a certain rate over time. If process > > is called on average lets say 5 ms late then the interval would become 25 ms > > instead of 20, or 40 times per second instead of 50. > > > > In short, it is a way to compensate for the worker thread to always call > process > > a bit late (usually somewhere between 2-8 ms). > > Ok, but isn't it weird to not set last_process_time_ms_ = now_ms on the first > call to Process(). Why now_ms + kProcessIntervalMs? Sounds more like > next_process_time_ms_ to me. > > I'd like to have unittests which verifies this is correct too. > > How much we want to compensate for this? We don't want to burst out 5 nacks in > 10 ms just because the worker thread was busy for some time, right? Does this > guarantee this won't happen? Maybe add a unittest for that case too. The name last_process_timestamp_ms_ is not perfect since it doesn't actually tells us when it was last processed, it is the adjusted timestamp to achieve a certain frequency. The last two lines of the calculation is used to adjust the time so we don't burst if the worker thread has been busy. Maybe I should change my comment if it wasn't clear :) I will absolutely implement a unittest for this. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:145: uint16_t num_new_nacks = PositiveDiff(seq_num_start, seq_num_end); On 2016/02/26 09:26:54, stefan-webrtc (holmer) wrote: > On 2016/02/25 15:09:30, philipel wrote: > > On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > > > Are these sequence numbers guaranteed not to already be in the nack list? > > > > This function is called from Notify (now OnReceivePacket) and the logic for > > detecting new packets to nack is in the Notify function, so yes, they SHOULD > not > > already be in the nack list. > > > > Would it make sense to dcheck on that somewhere in this method? It would, will add it for the next patch. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.cc:180: nack_it = nack_list_.erase(nack_it); On 2016/02/26 09:26:54, stefan-webrtc (holmer) wrote: > On 2016/02/25 15:09:31, philipel wrote: > > On 2016/02/23 12:29:00, stefan-webrtc (holmer) wrote: > > > If we decide to stop trying, should we then make sure there is a key frame > to > > > skip ahead to? Or should we allow nacking indefinitely and rely on the > jitter > > > buffer to let us know when to stop retrying? Maybe by having an API which it > > can > > > call to notify the nack module that it's no longer interested in packets > with > > > timestamp < X (or seq num < Y)? > > > > I think a ClearUpToSequenceNumber(uint16_t seq_num) function would be very > > useful to have. But I still think we should avoid signaling to the jitter > buffer > > that we have stopped nacking a packet. The general idea is that the nack > module > > will try hard to get every packet, and if that is not possible then the jitter > > buffer has to recover on its own. Of course requesting a keyframe from the > nack > > module should be very rare. > > So in that case we will be waiting for the jitter buffer to time out and request > a key frame on its own? The question is how much extra latency we will get for > having to wait for the jitter buffer timeout. The trade off is that the nack > module may be requesting a key frame for a packet it has stopped nacking which > wasn't really needed to continue decoding. The idea with this nack module is that it should never stop nacking a packet until we have tried really really hard to get that packet and it seems extremely unlikely to ever get those packets. If this condition is met then the jitter buffer should have tried to recover before that. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:26: class NackModule : public Module { On 2016/02/26 09:26:54, stefan-webrtc (holmer) wrote: > On 2016/02/25 15:09:32, philipel wrote: > > On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > > > It's difficult to see why this needs to be a Module without seeing how it's > > > going to be used. > > > > The reason this is a module is because it needs to frequently check if it is > > time to resend nacks. > > Maybe you should include the code which uses it in this CL to make it easier to > review? Or is that a lot of code? It is included in the NackModule::Process function, line 133 in the .cc file, or line 137 in patch set 2. https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:50: static const uint8_t kProcessIntervalMs = 1000 / kProcessFrequency; On 2016/02/26 09:26:54, stefan-webrtc (holmer) wrote: > On 2016/02/25 15:09:32, philipel wrote: > > On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > > > Any reason we're mixing static constants defined in the header file and > > > constants defined in the .cc file? > > > > Earlier I had logic which used these constants as arguments in std::min/max, > and > > since they are passed by reference they had to be declare in the .cc file as > > well. Not needed anymore, so I removed them from the .cc file. > > I guess my question is, can we move more constants to the .cc file? Most don't > seem to be used in this file. Sure! https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:99: critical_section_); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > On 2016/02/25 15:09:32, philipel wrote: > > On 2016/02/23 12:29:02, stefan-webrtc (holmer) wrote: > > > Most members are protected by critical_section_. Can we add an > > > rtc::ThreadChecker (or several) to document the threads accessing the > methods > > of > > > this class so that it's easier to understand why we need to lock everything? > > See > > > for instance packet_router.cc for an example. > > > > My understanding of most classes is that almost every member can only be > safely > > modified when holding a lock. I just added these constraints so the compiler > can > > check that it holds true. > > Right, but I think it would be good to go over what threads we expect will be > calling which methods and document that using a ThreadChecker. Maybe we then can > figure out that some of these are only accessed from a single thread. > > In any case, at least we will make sure the thread model is known. Sure, I will add it to the next patch set. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:41: // Calculates the forward differance between two numbers. On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > forward difference Done. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:63: if (is_keyframe) seq_num_keyframes_.emplace(unwrapper_.Unwrap(seq_num)); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > Maybe we should be unwrapping all sequence numbers and timestamps instead of > mixing unwrapping with modular comparisons? > > Either use unwrapping everywhere, or use a map with a specialized comparison > operator? I think I would prefer the latter. Implemented custom comparator. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:90: nack_sender_->SendNack(nack_batch.data(), nack_batch.size()); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > Is there any risk that we end up deadlocking here? Packets are being received > and parsed in the rtp module on the same thread which calls SendNack which also > goes to the rtp module. I suspect we may end up taking the same locks in rtp > module, so that we at least get recursive locking? I have not experienced any deadlocks when testing it and by looking at the RTCPSender::SendCompoundRTCP function I can see that the lock critical_section_rtcp_sender_ is obtained when sending nacks. I guess we don't use the RTCPSender while parsing/inserting packets, so I think we should be fine. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:96: } On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > move the else to line 70 and make an early return: > > if (AheadOf(last_seq_num_, seq_num)) { > RemovePacketFromNack(seq_num); > if (!is_retransmitted) > UpdateReorderingStatistics(seq_num); > return; > } > > AddPacketsToNack(...); > ... Done. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:115: 0l); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > 0ll is what you want, I think? Or just do 0 as I think that works equally well. > > You could also do > std::max<int64_t>(...); Compiler complains if I use 0ll :) https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:28: public: On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > I think we need a method which can be used to inform the nack module that the > stream is decodable beyond a certain timestamp (or sequence number, maybe), so > that the nack module can drop all nacks prior to that timestamp. Otherwise it > may keep nacking things that won't be used. Agree, implemented. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:59: int64_t send_at_time; On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > Would it be more logical to have "sent_at_time" and sent_at_seq_num? That way > the time to resend the packet can be dynamic, which is good since the rtt may > vary. For the timestamp I think this is a good idea. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:77: kSeqNumAndTime}; On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > Place type definitions directly after "private:" Done. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:97: std::set<int64_t> seq_num_keyframes_ GUARDED_BY(crit_); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > std::set<uint16_t>, right? I use the unrolled sequence number as the key, maybe rename it to unwrapped_keyframes_? https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:105: uint8_t reordering_count_ GUARDED_BY(crit_); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > int Done. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:106: int8_t reordering_occurences_[kMaxReorderedPackets] GUARDED_BY(crit_); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > int Done. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:107: uint8_t reordering_buckets_[kNumReorderingBuckets] GUARDED_BY(crit_); On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > int Done.
philipel@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/1715673002/diff/40001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/40001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:21: !std::numeric_limits<T>::is_signed, \ Would static_assert(std::is_unsigned<T>::value, ...) be better? https://codereview.webrtc.org/1715673002/diff/40001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:26: template <size_t M> inline size_t Add(size_t a , size_t b) { What's the reason for using size_t here when the functions below operate on a custom type T?
I choose not to implement the ThreadChecker in this patch as I have no way of testing it. I will implement it in the experiment CL instead. https://codereview.webrtc.org/1715673002/diff/40001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/40001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:26: template <size_t M> inline size_t Add(size_t a , size_t b) { On 2016/02/29 10:11:25, terelius wrote: > What's the reason for using size_t here when the functions below operate on a > custom type T? Below the length of the number is implied by the type T. Here we use Add and Subtract because we don't want to wrap at any particular T.
torbjorng@webrtc.org changed reviewers: + torbjorng@webrtc.org
You need to decide whether to require the arguments to be principal residues
(mod M) for well-defined results or if the can be any value allowed by size_t
(or equivalently the subtype T if you follow my suggestion about using T instead
of size_t). Now you allow b to be any value but require a to be principal.
I think it makes a lot of sense to require principal residues.
If this is to be used for time-critical code, avoid "% M" even with the current
(typically) compile-time constants. Each such op require two dependent
multiplications.
If you require principal residues, you can do a %-free addition like this:
t = M - b
res = a - t
if (t > a)
res += M
return res;
This will compile to multiplication free and branch free code.
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h
File webrtc/base/mod_ops.h (right):
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new...
webrtc/base/mod_ops.h:26: template <size_t M> inline size_t Add(size_t a ,
size_t b) {
Why use size_t and not T? Using T seems cleaner, but also is more general as it
would allow correct operation for T = 'unsigned long long" on a 32-bit machine
where size_t is just 32 bits.
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new...
webrtc/base/mod_ops.h:29: return M - a + add;
This looks incorrect, a should not be subtracted as (a) and (-a) are not in
general the same residue class (mod M). Some expression like (add + a - M) is
required. You may rearrange that to your taste; intermediate subtype overflow
will not matter here.
Note that if (a) is not a principal residue (mod M) this return statement will
also not return principal residues.
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new...
webrtc/base/mod_ops.h:37: return a - sub;
Note that if a is not a principal residue (mod M) this return statement will not
necessarily yield principal residues.
https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/1/webrtc/modules/video_coding/n... webrtc/modules/video_coding/nack_module.h:26: class NackModule : public Module { On 2016/02/26 15:12:06, philipel wrote: > On 2016/02/26 09:26:54, stefan-webrtc (holmer) wrote: > > On 2016/02/25 15:09:32, philipel wrote: > > > On 2016/02/23 12:29:01, stefan-webrtc (holmer) wrote: > > > > It's difficult to see why this needs to be a Module without seeing how > it's > > > > going to be used. > > > > > > The reason this is a module is because it needs to frequently check if it is > > > time to resend nacks. > > > > Maybe you should include the code which uses it in this CL to make it easier > to > > review? Or is that a lot of code? > > It is included in the NackModule::Process function, line 133 in the .cc file, or > line 137 in patch set 2. I'm talking about the code that registers the NackModule to a process thread, and which creates the actual NackModule instance. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:90: nack_sender_->SendNack(nack_batch.data(), nack_batch.size()); On 2016/02/26 15:12:07, philipel wrote: > On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > > Is there any risk that we end up deadlocking here? Packets are being received > > and parsed in the rtp module on the same thread which calls SendNack which > also > > goes to the rtp module. I suspect we may end up taking the same locks in rtp > > module, so that we at least get recursive locking? > > I have not experienced any deadlocks when testing it and by looking at the > RTCPSender::SendCompoundRTCP function I can see that the lock > critical_section_rtcp_sender_ is obtained when sending nacks. I guess we don't > use the RTCPSender while parsing/inserting packets, so I think we should be > fine. Acknowledged. https://codereview.webrtc.org/1715673002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:115: 0l); On 2016/02/26 15:12:07, philipel wrote: > On 2016/02/26 09:26:55, stefan-webrtc (holmer) wrote: > > 0ll is what you want, I think? Or just do 0 as I think that works equally > well. > > > > You could also do > > std::max<int64_t>(...); > > Compiler complains if I use 0ll :) Acknowledged. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/distribution.cc (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:13: #include "webrtc/modules/video_coding/distribution.h" this one should be first https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:29: buckets_[values_[index_]]--; --buckets_...; https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:35: buckets_[value]++; ++buckets_...; https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:47: bucket < buckets_.size()) { git cl format https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/distribution.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:17: class Distribution { Prefer calling this Histogram. To me, a distribution is the underlying model we're trying to estimate with the histogram. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:24: void AddValue(int value); Maybe just Add(int value)? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:30: // How many values that makes up this distribution. make up https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:34: std::vector<int> values_; Comment on that this is a circular buffer of values? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/include/video_coding_defines.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/include/video_coding_defines.h:174: class VCMNackSender { Call it NackSender instead. I think we should get rid of the VCM prefixes. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/include/video_coding_defines.h:183: class VCMKeyFrameRequestSender { Same here. Maybe we can even merge them into one class called FeedbackSender? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:73: if (is_keyframe) keyframe_list_.emplace(seq_num); Is there a reason for doing emplace rather than insert? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:161: // packets from the list, remove if from the list of "remove *it* from the list" https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:169: // atleast one packet in the nack list. at least https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:171: return true; I'd prefer to change the structure of this loop a bit, so that the special case is to return true and the default is to continue, e.g.: while (...) { auto it = lower_bound(...); if (it != nack_list_.begin()) { DCHECK(it != nack_list.end()); nack_list_.erase(...); return true; } key_frame_list_.erase(...); } https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:179: auto it = nack_list_.lower_bound(seq_num_end - kMaxPacketAge); We've been talking about adding support for a time limit on NACK lists as negotiated with rtx (see rtx-time here https://tools.ietf.org/html/rfc4588#section-8.1). Is that something we should take into account here already? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:187: while (RemovePacketsUntilKeyframe() && Search and replace Keyframe -> KeyFrame https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:204: void NackModule::RemovePacketFromNack(uint16_t seq_num) { Maybe remove this method now that it's trivial? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:19: #include "webrtc/base/thread_checker.h" Currently not used https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:25: #include "webrtc/system_wrappers/include/critical_section_wrapper.h" Use webrtc/base/criticalsection.h instead https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:46: // which packet to nack in GetNackBatch. Format comment so that it uses the full line length. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:47: enum NackFilterOptions {kSeqNumOnly, kTimeOnly, kSeqNumAndTime}; kSeqNumAndTime doesn't seem to be used? https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:66: // keyframe. Returns true if packets were removed. Reformat https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:79: // the packet with probability |probabilty| or higher. Reformat https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:83: rtc::CriticalSection crit_; Maybe move the crit down to the group of members that are protected by it. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:97: SequenceNumberUnwrapper unwrapper_; I'd prefer to get rid of the state and instead build the comparator using IsNewerSequenceNumber() from module_common_types.h https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:100: std::map<int16_t, NackInfo, SeqNumComparator> nack_list_ GUARDED_BY(crit_); Comment on what the int16_t represent here and below. Should uint16_t right?
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:26: template <size_t M> inline size_t Add(size_t a , size_t b) { On 2016/02/29 17:23:46, torbjorng (webrtc) wrote: > Why use size_t and not T? Using T seems cleaner, but also is more general as it > would allow correct operation for T = 'unsigned long long" on a 32-bit machine > where size_t is just 32 bits. Having to specify the type makes it less practical to use, for example Add<1000>(a, b) would become Add<uint16_t, 1000>(a, b). You are right about this being incorrect for uint64_t on a 32 bit machine, but I would rather just change the type to uint64_t to keep it as easy to use as possible. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:29: return M - a + add; On 2016/02/29 17:23:46, torbjorng (webrtc) wrote: > This looks incorrect, a should not be subtracted as (a) and (-a) are not in > general the same residue class (mod M). Some expression like (add + a - M) is > required. You may rearrange that to your taste; intermediate subtype overflow > will not matter here. > > Note that if (a) is not a principal residue (mod M) this return statement will > also not return principal residues. Yepp, this implementation is clearly wrong. A was assumed to be a principal residue in the ModNum class so this detail is a residue (huehuehue) of that implementation. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:37: return a - sub; On 2016/02/29 17:23:46, torbjorng (webrtc) wrote: > Note that if a is not a principal residue (mod M) this return statement will not > necessarily yield principal residues. True.
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/distribution.cc (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:13: #include "webrtc/modules/video_coding/distribution.h" On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > this one should be first Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:29: buckets_[values_[index_]]--; On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > --buckets_...; Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:35: buckets_[value]++; On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > ++buckets_...; Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.cc:47: bucket < buckets_.size()) { On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > git cl format Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/distribution.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:17: class Distribution { On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Prefer calling this Histogram. To me, a distribution is the underlying model > we're trying to estimate with the histogram. Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:24: void AddValue(int value); On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Maybe just Add(int value)? Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:30: // How many values that makes up this distribution. On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > make up Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/distribution.h:34: std::vector<int> values_; On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Comment on that this is a circular buffer of values? Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/include/video_coding_defines.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/include/video_coding_defines.h:174: class VCMNackSender { On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Call it NackSender instead. I think we should get rid of the VCM prefixes. Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/include/video_coding_defines.h:183: class VCMKeyFrameRequestSender { On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Same here. Maybe we can even merge them into one class called FeedbackSender? Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:73: if (is_keyframe) keyframe_list_.emplace(seq_num); On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Is there a reason for doing emplace rather than insert? Good habit since emplace does the same thing as insert only it might do it more efficient for certain data types. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:161: // packets from the list, remove if from the list of On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > "remove *it* from the list" Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:169: // atleast one packet in the nack list. On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > at least Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:171: return true; On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > I'd prefer to change the structure of this loop a bit, so that the special case > is to return true and the default is to continue, e.g.: > > while (...) { > auto it = lower_bound(...); > if (it != nack_list_.begin()) { > DCHECK(it != nack_list.end()); > nack_list_.erase(...); > return true; > } > key_frame_list_.erase(...); > } Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:179: auto it = nack_list_.lower_bound(seq_num_end - kMaxPacketAge); On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > We've been talking about adding support for a time limit on NACK lists as > negotiated with rtx (see rtx-time here > https://tools.ietf.org/html/rfc4588#section-8.1). Is that something we should > take into account here already? It is something we can implement later on, I don't want to add more to this CL than necessary. When we add it I think it's easier to add it in the GetNackBatch functions which can remove all old packets according to some timestamp we can add in the NackInfo struct. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:187: while (RemovePacketsUntilKeyframe() && On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Search and replace Keyframe -> KeyFrame Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:204: void NackModule::RemovePacketFromNack(uint16_t seq_num) { On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Maybe remove this method now that it's trivial? Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:19: #include "webrtc/base/thread_checker.h" On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Currently not used Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:25: #include "webrtc/system_wrappers/include/critical_section_wrapper.h" On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Use webrtc/base/criticalsection.h instead Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:46: // which packet to nack in GetNackBatch. On 2016/03/01 08:52:11, stefan-webrtc (holmer) wrote: > Format comment so that it uses the full line length. Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:66: // keyframe. Returns true if packets were removed. On 2016/03/01 08:52:11, stefan-webrtc (holmer) wrote: > Reformat Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:79: // the packet with probability |probabilty| or higher. On 2016/03/01 08:52:11, stefan-webrtc (holmer) wrote: > Reformat Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:83: rtc::CriticalSection crit_; On 2016/03/01 08:52:11, stefan-webrtc (holmer) wrote: > Maybe move the crit down to the group of members that are protected by it. Acknowledged. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:97: SequenceNumberUnwrapper unwrapper_; On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > I'd prefer to get rid of the state and instead build the comparator using > IsNewerSequenceNumber() from module_common_types.h Done. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.h:100: std::map<int16_t, NackInfo, SeqNumComparator> nack_list_ GUARDED_BY(crit_); On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > Comment on what the int16_t represent here and below. > > Should uint16_t right? Yes, should be uint16_t.
This starts looking good I think. A few more comments from me. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:73: if (is_keyframe) keyframe_list_.emplace(seq_num); On 2016/03/01 10:27:06, philipel wrote: > On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > > Is there a reason for doing emplace rather than insert? > > Good habit since emplace does the same thing as insert only it might do it more > efficient for certain data types. Acknowledged. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/nack_module.cc:179: auto it = nack_list_.lower_bound(seq_num_end - kMaxPacketAge); On 2016/03/01 10:27:06, philipel wrote: > On 2016/03/01 08:52:10, stefan-webrtc (holmer) wrote: > > We've been talking about adding support for a time limit on NACK lists as > > negotiated with rtx (see rtx-time here > > https://tools.ietf.org/html/rfc4588#section-8.1). Is that something we should > > take into account here already? > > It is something we can implement later on, I don't want to add more to this CL > than necessary. When we add it I think it's easier to add it in the GetNackBatch > functions which can remove all old packets according to some timestamp we can > add in the NackInfo struct. Sounds good. https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:168: continue; You no longer need continue here. https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:27: remove https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:94: // SequenceNumberUnwrapper unwrapper_; Remove commented code. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.h:18: class Histogram { Please add unittests for this class. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.h:28: size_t InverseCDF(float probability) const; InverseCdf https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/include/video_coding_defines.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/include/video_coding_defines.h:177: uint16_t length) = 0; length should probably be size_t. Maybe we could even pass in a const vector<uint16_t>& instead? https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/include/video_coding_defines.h:185: virtual int32_t RequestKeyFrame() = 0; Return bool instead, or maybe even void? Same for SendNack. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:104: nack_sender_->SendNack(nack_batch.data(), nack_batch.size()); Not checking return value https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:184: while (RemovePacketsUntilKeyFrame() && I'd prefer the following as it's slightly easier to read: while (nack_list_.size() + num_new_nacks > kMaxNackPackets) { if (!RemovePacketsUntilKeyFrame()) break; } https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:190: keyframe_request_sender_->RequestKeyFrame(); Not checking return value. Can probably return void instead? https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:89: }; Prefer moving this up to the other types, e.g., NackInfo https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:93: video_coding::Histogram reordering_histogram_; Does this not have to be guarded by crit_? In that case move it from this group of members.
https://codereview.webrtc.org/1715673002/diff/120001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:28: RTC_DCHECK_LT(a, M); Do you really want to enforce a<=M? I though the point of not making it a class was to ensure that people can use it just like a normal integer.
https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:168: continue; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > You no longer need continue here. Done. https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:27: On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > remove Done. https://codereview.webrtc.org/1715673002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:94: // SequenceNumberUnwrapper unwrapper_; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Remove commented code. Done. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:28: RTC_DCHECK_LT(a, M); On 2016/03/01 12:27:01, terelius wrote: > Do you really want to enforce a<=M? I though the point of not making it a class > was to ensure that people can use it just like a normal integer. The idea with these functions is that one can more easily do modular operations on numbers that are suppose to be modular. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.h:18: class Histogram { On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Please add unittests for this class. Done. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.h:28: size_t InverseCDF(float probability) const; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > InverseCdf Done. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/include/video_coding_defines.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/include/video_coding_defines.h:177: uint16_t length) = 0; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > length should probably be size_t. Maybe we could even pass in a const > vector<uint16_t>& instead? Agree, the vector is much cleaner, fixed. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/include/video_coding_defines.h:185: virtual int32_t RequestKeyFrame() = 0; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Return bool instead, or maybe even void? Same for SendNack. Done. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:104: nack_sender_->SendNack(nack_batch.data(), nack_batch.size()); On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Not checking return value SendNack now returns void. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:184: while (RemovePacketsUntilKeyFrame() && On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > I'd prefer the following as it's slightly easier to read: > while (nack_list_.size() + num_new_nacks > kMaxNackPackets) { > if (!RemovePacketsUntilKeyFrame()) > break; > } We only want to remove packets until we have enough space, so we need to check both conditions in the loop. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:190: keyframe_request_sender_->RequestKeyFrame(); On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Not checking return value. Can probably return void instead? Returns void now. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.h (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:89: }; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Prefer moving this up to the other types, e.g., NackInfo Done. https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.h:93: video_coding::Histogram reordering_histogram_; On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > Does this not have to be guarded by crit_? In that case move it from this group > of members. Should be guarded by crit_, fixed.
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:26: template <size_t M> inline size_t Add(size_t a , size_t b) { On 2016/03/01 09:29:34, philipel wrote: > On 2016/02/29 17:23:46, torbjorng (webrtc) wrote: > > Why use size_t and not T? Using T seems cleaner, but also is more general as > it > > would allow correct operation for T = 'unsigned long long" on a 32-bit machine > > where size_t is just 32 bits. > > Having to specify the type makes it less practical to use, for example > Add<1000>(a, b) would become Add<uint16_t, 1000>(a, b). > > You are right about this being incorrect for uint64_t on a 32 bit machine, but I > would rather just change the type to uint64_t to keep it as easy to use as > possible. On 32-bit machines, uint64_t is a quite poor choice, generating a complex set of branches for most arithmetic and comparisons. https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:29: return M - a + add; On 2016/03/01 09:29:34, philipel wrote: > On 2016/02/29 17:23:46, torbjorng (webrtc) wrote: > > This looks incorrect, a should not be subtracted as (a) and (-a) are not in > > general the same residue class (mod M). Some expression like (add + a - M) is > > required. You may rearrange that to your taste; intermediate subtype overflow > > will not matter here. > > > > Note that if (a) is not a principal residue (mod M) this return statement will > > also not return principal residues. > > Yepp, this implementation is clearly wrong. A was assumed to be a principal > residue in the ModNum class so this detail is a residue (huehuehue) of that > implementation. Whether a principal residue or not, that code was incorrect. https://codereview.webrtc.org/1715673002/diff/140001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/140001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:27: inline uint64_t Add(uint64_t a, uint64_t b) { Subtract() is now several times faster than Add() for typical usages. Consider doing e.g.: T t = M - b % M T res1 = a - t T res2 = res1 + M if (t > a) return res2 else return res1 This might look convoluted, but it is pretty efficient. It will compile into branch free and multiplication free code. Please check my logic... I suppose one could also insert if ((M & (M - 1)) == 0) // Is M a power of 2? return (a + b) % M; first in these functions.
https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/60001/webrtc/base/mod_ops.h#new... webrtc/base/mod_ops.h:29: return M - a + add; On 2016/03/01 14:13:35, torbjorng (webrtc) wrote: > On 2016/03/01 09:29:34, philipel wrote: > > On 2016/02/29 17:23:46, torbjorng (webrtc) wrote: > > > This looks incorrect, a should not be subtracted as (a) and (-a) are not in > > > general the same residue class (mod M). Some expression like (add + a - M) > is > > > required. You may rearrange that to your taste; intermediate subtype > overflow > > > will not matter here. > > > > > > Note that if (a) is not a principal residue (mod M) this return statement > will > > > also not return principal residues. > > > > Yepp, this implementation is clearly wrong. A was assumed to be a principal > > residue in the ModNum class so this detail is a residue (huehuehue) of that > > implementation. > > Whether a principal residue or not, that code was incorrect. Reading my own comment in hindsight I realize I was not very clear. Yes, the calculation was wrong regardless of 'a' being a principal residue or not.
Looks good. I don't have any other comments than these. I'd like for torbjorng or terelius to approve mod_ops.h https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:184: while (RemovePacketsUntilKeyFrame() && On 2016/03/01 13:31:09, philipel wrote: > On 2016/03/01 12:16:34, stefan-webrtc (holmer) wrote: > > I'd prefer the following as it's slightly easier to read: > > while (nack_list_.size() + num_new_nacks > kMaxNackPackets) { > > if (!RemovePacketsUntilKeyFrame()) > > break; > > } > > We only want to remove packets until we have enough space, so we need to check > both conditions in the loop. Isn't my suggestion equivalent to what you have already written? It breaks if removing more packets failed, and the while condition is to remove packets until we have enough space? https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram.cc (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.cc:19: Histogram::Histogram(int num_buckets, int max_num_values) { Should these be size_t? https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.cc:29: --buckets_[values_[index_]]; DCHECK that values_[index_] < buckets_.size() https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.cc:41: RTC_DCHECK_GE(1.f, probability); This is easier to read in my opinion. Feel free to change. RTC_DCHECK_GE(probabiliy, 0.0f); RTC_DCHECK_LE(probabiliy, 1.0f); https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram.h (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.h:20: Histogram(int num_buckets, int max_num_values); Maybe add a comment that the histogram is limited to one buckets of interval size 1? https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram_unittest.cc (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram_unittest.cc:20: Histogram histogram_; Indentation https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:189: keyframe_request_sender_->RequestKeyFrame(); Log a warning if this happens. https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:226: it = nack_list_.erase(it); Maybe log a warning at the end of this method if this has happened? Same if the erase on line 214 has happened.
https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram.cc (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.cc:19: Histogram::Histogram(int num_buckets, int max_num_values) { On 2016/03/02 14:32:05, stefan-webrtc (holmer) wrote: > Should these be size_t? Done. https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.cc:29: --buckets_[values_[index_]]; On 2016/03/02 14:32:05, stefan-webrtc (holmer) wrote: > DCHECK that values_[index_] < buckets_.size() Done. https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.cc:41: RTC_DCHECK_GE(1.f, probability); On 2016/03/02 14:32:05, stefan-webrtc (holmer) wrote: > This is easier to read in my opinion. Feel free to change. > RTC_DCHECK_GE(probabiliy, 0.0f); > RTC_DCHECK_LE(probabiliy, 1.0f); Done. https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/histogram.h (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/histogram.h:20: Histogram(int num_buckets, int max_num_values); On 2016/03/02 14:32:05, stefan-webrtc (holmer) wrote: > Maybe add a comment that the histogram is limited to one buckets of interval > size 1? Done. https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/nack_module.cc (right): https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:189: keyframe_request_sender_->RequestKeyFrame(); On 2016/03/02 14:32:05, stefan-webrtc (holmer) wrote: > Log a warning if this happens. Done. https://codereview.webrtc.org/1715673002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/nack_module.cc:226: it = nack_list_.erase(it); On 2016/03/02 14:32:05, stefan-webrtc (holmer) wrote: > Maybe log a warning at the end of this method if this has happened? Same if the > erase on line 214 has happened. Done.
https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:38: RTC_DCHECK_LT(a, M); // NOLINT Nit: this NOLINT seems unneeded. https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:68: inline T ForwardDiff(T a, T b) { See comment about ReverseDiff. https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:95: template <typename T> What's the intended range here? Presumably 0..255 for uint8_t. Then, if a=255, b=0 we get a-b=255, sensibly. Now let e.g. increment both, getting a=0 and b=1. Now we get 255-(1-0) = 254. That cannot be right, stupid. I suggest that you make the test code catch that error first, then fix the code. https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:101: template <typename T> The criteria for "ahead of" is a bit arbitrary. Please document it.
Oh, that was not intended tone. I wrote the pejorative word with the author behind my desk as a joke, than deleted it. I apologize!
https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h File webrtc/base/mod_ops.h (right): https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:38: RTC_DCHECK_LT(a, M); // NOLINT On 2016/03/02 16:20:50, torbjorng (webrtc) wrote: > Nit: this NOLINT seems unneeded. Done. https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:68: inline T ForwardDiff(T a, T b) { On 2016/03/02 16:20:50, torbjorng (webrtc) wrote: > See comment about ReverseDiff. Done. https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:95: template <typename T> On 2016/03/02 16:20:50, torbjorng (webrtc) wrote: > What's the intended range here? Presumably 0..255 for uint8_t. > > Then, if a=255, b=0 we get a-b=255, sensibly. Now let e.g. increment both, > getting a=0 and b=1. Now we get 255-(1-0) = 254. > > That cannot be right, stupid. > > I suggest that you make the test code catch that error first, then fix the code. I have now unstupidified the code and implemented unittests. :) https://codereview.webrtc.org/1715673002/diff/180001/webrtc/base/mod_ops.h#ne... webrtc/base/mod_ops.h:101: template <typename T> On 2016/03/02 16:20:50, torbjorng (webrtc) wrote: > The criteria for "ahead of" is a bit arbitrary. Please document it. Done.
lgtm wrt webrtc/base/mod_ops.h I truly appreciate the unstupification. :-)
lgtm
Feel free to proceed without me. Unless there is anything in particular you want me to look at?
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5649)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1715673002/#ps220001 (title: "std::max type fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5651) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7987) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13177) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11813) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13209)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1715673002/#ps240001 (title: "SeqNumComparator operator marked const.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1715673002/#ps260001 (title: "Include cstddef for size_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_clang_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12924)
Added missing comma in base.gyp.
Testing stuff for MSVC 2
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1715673002/#ps340001 (title: "MSVC Fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3889)
mflodman@ ptal, I need owners approval for mod_ops.h and mod_ops_unittest.cc.
lgtm wrt mod_ops_unittest.cc. https://codereview.webrtc.org/1715673002/diff/360001/webrtc/base/mod_ops_unit... File webrtc/base/mod_ops_unittest.cc (right): https://codereview.webrtc.org/1715673002/diff/360001/webrtc/base/mod_ops_unit... webrtc/base/mod_ops_unittest.cc:40: const unsigned long D = ulmax - 10ul; // NOLINT Nit: We don't usually put 'ul' suffixes where they are not needed. More examples below. (But in your EXPECT_* they are sometimes needed to avoid comparisons between different signedness.)
torbjorng@webrtc.org changed reviewers: + perkj@webrtc.org
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
rs lgtm
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1715673002/#ps360001 (title: "Format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715673002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715673002/360001
Message was sent while issue was closed.
Description was changed from ========== Implement the NackModule as part of the new jitter buffer. Things done/implemented in this CL: - An interface that can send Nack (VCMNackSender). - An interface that can request KeyFrames (VCMKeyFrameRequestSender). - The nack module (NackModule). - A set of convenience functions for modular numbers (mod_ops.h). BUG=webrtc:5514 ========== to ========== Implement the NackModule as part of the new jitter buffer. Things done/implemented in this CL: - An interface that can send Nack (VCMNackSender). - An interface that can request KeyFrames (VCMKeyFrameRequestSender). - The nack module (NackModule). - A set of convenience functions for modular numbers (mod_ops.h). BUG=webrtc:5514 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Implement the NackModule as part of the new jitter buffer. Things done/implemented in this CL: - An interface that can send Nack (VCMNackSender). - An interface that can request KeyFrames (VCMKeyFrameRequestSender). - The nack module (NackModule). - A set of convenience functions for modular numbers (mod_ops.h). BUG=webrtc:5514 ========== to ========== Implement the NackModule as part of the new jitter buffer. Things done/implemented in this CL: - An interface that can send Nack (VCMNackSender). - An interface that can request KeyFrames (VCMKeyFrameRequestSender). - The nack module (NackModule). - A set of convenience functions for modular numbers (mod_ops.h). BUG=webrtc:5514 Committed: https://crrev.com/f472c5b6722dfb221f929fc4d3a2b4ca54647701 Cr-Commit-Position: refs/heads/master@{#11882} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/f472c5b6722dfb221f929fc4d3a2b4ca54647701 Cr-Commit-Position: refs/heads/master@{#11882}
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:360001) has been created in https://codereview.webrtc.org/1771883002/ by kjellander@webrtc.org. The reason for reverting is: Unfortunately this breaks in the main waterfall: https://build.chromium.org/p/client.webrtc/builders/Android32%20Builder/build... I think it's related to dcheck_always_on=1 which is set in GYP_DEFINES only on the trybots, but not on the bots in the main waterfall.. |
