First-order-FEC recoverability calculation

webrtc/voice_engine/transport_feedback_packet_loss_tracker.h

Index: webrtc/voice_engine/transport_feedback_packet_loss_tracker.h
diff --git a/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h b/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h
index e5626eb9b22a44d7961809f7f9a32f2f74938602..63230795e15f39460007e197d1b50e07ac7f6f28 100644
--- a/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h
+++ b/webrtc/voice_engine/transport_feedback_packet_loss_tracker.h
@@ -26,31 +26,35 @@ class TransportFeedbackPacketLossTracker final {
  public:
   // Up to |max_window_size| latest packet statuses wil be used for calculating
   // the packet loss metrics. When less than |min_window_size| samples are
-  // available for making a reliable estimation, GetPacketLossRates() will
-  // return false to indicate packet loss metrics are not ready.
+  // available for making a reliable estimation, neither PLR nor RPLR would
+  // be considered validly computable.
+  // When |min_pairs_num_for_rplr| pairs or more are available, RPLR is reliably
+  // computable. This is independent of PLR being computable.
   TransportFeedbackPacketLossTracker(size_t min_window_size,

[minyue-webrtc, 2017/01/25 09:38:45]

rename min_window_size to make it specific to plr, maybe plr_min_num_packets_

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

Please see subsequent CL, which renames to min/max-acked.

[minyue-webrtc, 2017/01/25 20:00:59]

You see I did not ask you to rename max_window_size, since that belongs to the
next CL.

However, renaming "min" and reordering it with max_window_size would make this
CL more solid on itself, since the introduction of rplr_min_num_pairs_ makes
min_window_size harder to understand.

-                                     size_t max_window_size);
+                                     size_t max_window_size,

[minyue-webrtc, 2017/01/25 09:38:45]

change the order of min_window_size (and rename it) and max_window_size.

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

Min-then-max looks fine to me. That's how I'm used to seeing it everywhere.
Max-then-min would be confusing.

+                                     size_t min_pairs_num_for_rplr);

[minyue-webrtc, 2017/01/25 09:38:45]

rename to rplr_min_num_pairs_

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

Done.

 
   void OnReceivedTransportFeedback(const rtcp::TransportFeedback& feedback);
 
-  // Returns true if packet loss rate and packet loss episode duration are ready
-  // and assigns respective values to |*packet_loss_rate| and
-  // |*consecutive_packet_loss_rate|. Continuous packet loss rate is defined as
-  // the probability of losing two adjacent packets.
-  bool GetPacketLossRates(float* packet_loss_rate,
-                          float* consecutive_packet_loss_rate) const;
+  // Returns the packet loss rate, if the window has enough data to
+  // reliably compute it.
+  rtc::Optional<float> GetPacketLossRate() const;
+
+  // Returns the first-order-FEC recoverable packet loss rate, if the window
+  // has enough data to reliably compute it.
+  rtc::Optional<float> GetRecoverablePacketLossRate() const;
 
   // Verifies that the internal states are correct. Only used for tests.
   void Validate() const;
 
  private:
-  // PacketStatus is a map from sequence number to its reception status. The
-  // status is true if the corresponding packet is received, and false if it is
-  // lost. Unknown statuses are not present in the map.
+  // PacketStatus is a map from sequence number to a boolean. The boolean is

[minyue-webrtc, 2017/01/25 09:38:45]

use the old comment.

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

Done. (Edited needlessly when doing some rebasing work for the second CL, which
does intentionally change this comment due to a change of the bool to a
tri-state enum.)

[minyue-webrtc, 2017/01/25 20:00:59]

Acknowledged.

+  // true if it is received.
   typedef std::map<uint16_t, bool> PacketStatus;
   typedef PacketStatus::const_iterator PacketStatusIterator;
 
   void Reset();
+
   // ReferenceSequenceNumber() provides a sequence number that defines the
   // order of packet reception info stored in |packet_status_window_|. In
   // particular, given any sequence number |x|,
@@ -60,22 +64,51 @@ class TransportFeedbackPacketLossTracker final {
   bool IsOldSequenceNumber(uint16_t seq_num) const;
   void InsertPacketStatus(uint16_t seq_num, bool received);
   void RemoveOldestPacketStatus();
-  void ApplyPacketStatus(PacketStatusIterator it);
-  void UndoPacketStatus(PacketStatusIterator it);
+
+  void UpdateMetrics(PacketStatusIterator it, bool apply /* false = undo */);

[minyue-webrtc, 2017/01/25 09:38:45]

I find it would be easier for me to understand

void UpdateMetrics(PacketStatusIterator it, bool undo = false);

because I want UpdateMetrics to apply a new input by default, and only does undo
when it is specially told to do so.

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

1. I think it would be confusing for a reader to see UpdateMetrics(it, true)
somehow do an undo operation. One normally associates negative indicators with
undoing. When I see UpdateMetrics(it, false), even if I haven't seen the
function's signature yet, I'm already mentally prepared for the possibility that
it would do an inverse operation.
2. Assuming we've reached agreement on #1, I think it would be less error-prone
to force an explicit passing of the second (bool) parameter for both doing and
undoing.

[minyue-webrtc, 2017/01/25 20:00:59]

Sure, anyway, either way should not be a problem for a programmer to read.

+  void UpdatePlr(PacketStatusIterator it, bool apply /* false = undo */);
+  void UpdateRplr(PacketStatusIterator it, bool apply /* false = undo */);
+
   PacketStatusIterator PreviousPacketStatus(PacketStatusIterator it);
   PacketStatusIterator NextPacketStatus(PacketStatusIterator it);
 
-  const size_t min_window_size_;
   const size_t max_window_size_;
 
   PacketStatus packet_status_window_;
   // |ref_packet_status_| points to the oldest item in |packet_status_window_|.
   PacketStatusIterator ref_packet_status_;
 
-  size_t num_received_packets_;
-  size_t num_lost_packets_;
-  size_t num_consecutive_losses_;
-  size_t num_consecutive_old_reports_;
+  struct PlrState {
+    PlrState(size_t min_window_size)
+        : min_window_size_(min_window_size) {
+      Reset();
+    }
+    void Reset() {
+      num_received_packets_ = 0;
+      num_lost_packets_ = 0;
+    }
+    rtc::Optional<float> GetMetric() const;
+    const size_t min_window_size_;

[minyue-webrtc, 2017/01/25 09:38:45]

rename min_window_size -> min_num_packets_

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

In this CL, it is still the window. In the subsequent CL, the exact meaning has
changed, and a new name is already used there.

[minyue-webrtc, 2017/01/25 20:00:59]

True, but doing it now will make it clearer, particularly if you agree to do the
renaming I suggest on the var in ctor.

+    size_t num_received_packets_;
+    size_t num_lost_packets_;
+  } plr_state_;
+
+  struct RplrState {
+    RplrState(size_t min_pairs)
+        : min_pairs_(min_pairs) {
+      Reset();
+    }
+    void Reset() {
+      num_known_status_pairs_ = 0;
+      num_loss_followed_by_reception_pairs_ = 0;
+    }
+    rtc::Optional<float> GetMetric() const;
+    const size_t min_pairs_;
+    size_t num_known_status_pairs_;
+    size_t num_loss_followed_by_reception_pairs_;

[minyue-webrtc, 2017/01/25 09:38:45]

let's consider making the var names more precise and concise

how about 

"min_num_pairs_"
"num_pairs_"
"num_lost_received_pairs_"

and add a comment under struct RplrState 

RPLR is the probability that a lost packet is followed by a received packet. We
count the fraction of such pairs of packets out of all observed pairs in the
transport feedbacks. 

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

min_num_pair - OK
num_pairs - I prefer num_known_pairs; I think it's more informative, and not yet
overly long.
num_lost_received_pairs - sounds too paradoxical to me. A reader might not
immediately understand it's a pair of X followed by Y, thinking instead it's a
pair that's somehow both X and Y at the same time.

[minyue-webrtc, 2017/01/25 20:00:59]

known is not a clearly defined word. In the next step, we introduce SENT, does
it belong to known or unknown? So I think a half vague name could be ok here.
And please note that I have suggested adding a comment after the struct. 
Then how about num_recoverable_lost_packets

+  } rplr_state_;
+
+  size_t num_consecutive_old_reports_;  // TODO(eladalon): Upcoming CL removes.

[minyue-webrtc, 2017/01/25 09:38:46]

elad.alon

[elad.alon_webrtc.org, 2017/01/25 12:47:55]

Done.

 };
 
 }  // namespace webrtc