Implement the NackModule as part of the new jitter buffer.

webrtc/modules/video_coding/nack_module.h

+/*
+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.

[stefan-webrtc, 2016/02/23 12:29:01]

2016

+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_MODULES_VIDEO_CODING_NACK_MODULE_H_
+#define WEBRTC_MODULES_VIDEO_CODING_NACK_MODULE_H_
+
+#include <map>
+#include <vector>
+
+#include "webrtc/base/thread_annotations.h"
+#include "webrtc/modules/include/module.h"
+#include "webrtc/modules/video_coding/include/video_coding_defines.h"
+#include "webrtc/modules/video_coding/packet.h"
+#include "webrtc/system_wrappers/include/clock.h"
+#include "webrtc/system_wrappers/include/critical_section_wrapper.h"
+
+namespace webrtc {
+
+class NackModule : public Module {

[stefan-webrtc, 2016/02/23 12:29:01]

It's difficult to see why this needs to be a Module without seeing how it's
going to be used.

[philipel, 2016/02/25 15:09:32]

The reason this is a module is because it needs to frequently check if it is
time to resend nacks.

[stefan-webrtc, 2016/02/26 09:26:54]

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?

[philipel, 2016/02/26 15:12:06]

It is included in the NackModule::Process function, line 133 in the .cc file, or
line 137 in patch set 2.

[stefan-webrtc, 2016/03/01 08:52:09]

I'm talking about the code that registers the NackModule to a process thread,
and which creates the actual NackModule instance.

+ public:
+  NackModule(CriticalSectionWrapper* critical_section,
+             Clock* clock,
+             VCMNackSender* nack_sender,
+             VCMKeyFrameRequestSender* keyframe_request_sender);
+
+  ~NackModule();
+
+  void Notify(const VCMPacket& packet);

[stefan-webrtc, 2016/02/23 12:29:02]

OnReceivedPacket, possibly?

[philipel, 2016/02/25 15:09:32]

Done.

+  void UpdateRtt(int64_t rtt_ms);
+  void Stop();
+
+  // Module implementation
+  int64_t TimeUntilNextProcess() override;
+  int32_t Process() override;
+
+ private:
+  static const uint16_t kMaxNackPackets = 1000;
+  static const uint16_t kMaxReorderedPackets = 128;
+  static const uint16_t kNumReorderingBuckets = 5;
+  static const uint16_t kDefaultRttMs = 100;
+  static const uint8_t kMaxNackRetries = 10;
+  static const uint8_t kProcessFrequency = 50;
+  static const uint8_t kProcessIntervalMs = 1000 / kProcessFrequency;

[stefan-webrtc, 2016/02/23 12:29:02]

Any reason we're mixing static constants defined in the header file and
constants defined in the .cc file?

[philipel, 2016/02/25 15:09:32]

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.

[stefan-webrtc, 2016/02/26 09:26:54]

I guess my question is, can we move more constants to the .cc file? Most don't
seem to be used in this file.

[philipel, 2016/02/26 15:12:06]

Sure!

+
+  struct NackInfo {

[stefan-webrtc, 2016/02/23 12:29:02]

Could you add a comment which shortly describes what this is? Or improve the
name of the class.

[philipel, 2016/02/25 15:09:32]

Done.

+    uint16_t seq_num;
+    uint16_t send_at_seq_num;
+    int64_t send_at_timestamp;

[stefan-webrtc, 2016/02/23 12:29:01]

time_last_sent_ms?

[philipel, 2016/02/25 15:09:32]

This is the timestamp at which this nack should be sent again, not when it was
last sent.

+    uint8_t retries;

[stefan-webrtc, 2016/02/23 12:29:02]

int

[philipel, 2016/02/25 15:09:32]

Done.

+
+    NackInfo();
+    NackInfo(uint16_t seq_num, uint16_t send_at_seq_num);

[stefan-webrtc, 2016/02/23 12:29:02]

Constructors at the top.

[philipel, 2016/02/25 15:09:32]

Done.

+  };
+
+  void AddPacketsToNack(uint16_t seq_num_start, uint16_t seq_num_end)
+      EXCLUSIVE_LOCKS_REQUIRED(critical_section_);
+  void RemovePacketFromNack(uint16_t seq_num)
+      EXCLUSIVE_LOCKS_REQUIRED(critical_section_);
+  std::vector<uint16_t> GetNackBatch(bool consider_seq_num,
+                                     bool consider_timestamp)
+      EXCLUSIVE_LOCKS_REQUIRED(critical_section_);
+
+  // Update the reordering distribution.
+  void UpdateReorderingStatistics(uint16_t seq_num)
+      EXCLUSIVE_LOCKS_REQUIRED(critical_section_);
+
+  // Returns how many packets we have to wait in order to receive
+  // the packet with probability |probabilty| or higher.
+  uint8_t WaitNumberOfPackets(float probability) const
+      EXCLUSIVE_LOCKS_REQUIRED(critical_section_);
+
+  CriticalSectionWrapper* critical_section_;

[stefan-webrtc, 2016/02/23 12:29:01]

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.

[philipel, 2016/02/25 15:09:32]

NackModule now has its own rtc::CriticalSection crit_.

+  Clock* clock_;

[stefan-webrtc, 2016/02/23 12:29:01]

Clock* const

[philipel, 2016/02/25 15:09:32]

Done.

+  VCMNackSender* nack_sender_;
+  VCMKeyFrameRequestSender* keyframe_request_sender_;

[stefan-webrtc, 2016/02/23 12:29:02]

Can these two be const too?

[philipel, 2016/02/25 15:09:32]

Done.

+
+  typedef std::map<uint16_t, NackInfo> NackList;

[stefan-webrtc, 2016/02/23 12:29:01]

I think we can live without this typedef now that we have c++11 and can use
auto.

[philipel, 2016/02/25 15:09:32]

Done.

+  NackList nack_list_ GUARDED_BY(critical_section_);
+
+  bool running_ GUARDED_BY(critical_section_);
+  uint16_t last_seq_num_ GUARDED_BY(critical_section_);

[stefan-webrtc, 2016/02/23 12:29:01]

Move this down to the other last_ members.

[philipel, 2016/02/25 15:09:32]

Done.

+  bool initialized_ GUARDED_BY(critical_section_);
+  int64_t rtt_ms_ GUARDED_BY(critical_section_);
+  int64_t last_process_timestamp_ms_ GUARDED_BY(critical_section_);
+  uint16_t last_keyframe_seq_num_ GUARDED_BY(critical_section_);
+
+  uint8_t reordering_index_ GUARDED_BY(critical_section_);
+  uint8_t reordering_count_ GUARDED_BY(critical_section_);
+  int8_t reordering_occurences_[kMaxReorderedPackets] GUARDED_BY(
+      critical_section_);
+  uint8_t reordering_buckets_[kNumReorderingBuckets] GUARDED_BY(
+      critical_section_);

[stefan-webrtc, 2016/02/23 12:29:02]

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.

[philipel, 2016/02/25 15:09:32]

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.

[stefan-webrtc, 2016/02/26 09:26:55]

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.

[philipel, 2016/02/26 15:12:06]

Sure, I will add it to the next patch set.

+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_VIDEO_CODING_NACK_MODULE_H_