Remove TMMBRSet class

webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc
index 4ad1ddeb7616e6205d6a2d0a6168725ca55fe36f..2a1fc8c6a836727f472ac353257116e79700a787 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc
@@ -115,72 +115,47 @@ RTCPReportBlockInformation::~RTCPReportBlockInformation()
 {
 }
 
-RTCPReceiveInformation::RTCPReceiveInformation()
-    : lastTimeReceived(0),
-      lastFIRSequenceNumber(-1),
-      lastFIRRequest(0),
-      readyForDelete(false) {
-}
-
-RTCPReceiveInformation::~RTCPReceiveInformation() {
-}
-
-// Increase size of TMMBRSet if needed, and also take care of
-// the _tmmbrSetTimeouts vector.
-void RTCPReceiveInformation::VerifyAndAllocateTMMBRSet(
-    const uint32_t minimumSize) {
-  if (minimumSize > TmmbrSet.sizeOfSet()) {
-    TmmbrSet.VerifyAndAllocateSetKeepingData(minimumSize);
-    // make sure that our buffers are big enough
-    _tmmbrSetTimeouts.reserve(minimumSize);
-  }
-}
-
-void RTCPReceiveInformation::InsertTMMBRItem(
-    const uint32_t senderSSRC,
-    const RTCPUtility::RTCPPacketRTPFBTMMBRItem& TMMBRItem,
-    const int64_t currentTimeMS) {
-  // serach to see if we have it in our list
-  for (uint32_t i = 0; i < TmmbrSet.lengthOfSet(); i++)  {
-    if (TmmbrSet.Ssrc(i) == senderSSRC) {
-      // we already have this SSRC in our list update it
-      TmmbrSet.SetEntry(i,
-                        TMMBRItem.MaxTotalMediaBitRate,
-                        TMMBRItem.MeasuredOverhead,
-                        senderSSRC);
-      _tmmbrSetTimeouts[i] = currentTimeMS;
+RTCPReceiveInformation::RTCPReceiveInformation() = default;
+RTCPReceiveInformation::~RTCPReceiveInformation() = default;

[philipel, 2016/08/17 13:53:06]

Any particular reason for not using a ctor to initialize values?

[danilchap, 2016/08/17 14:56:05]

I find member initialization cleaner: harder to miss initializing a new member,
easier to change member order or name.

[philipel, 2016/08/17 15:30:06]

In that case remove the default ctor/dtor.

[danilchap, 2016/08/17 16:15:33]

Ack

They are default, but complex. This kind of constructor/destuctor better be in
.cc file.
When line "configs -= [ "//build/config/clang:find_bad_constructs" ]" in
BUILD.gn file will be removed, absent or declaring this kind of constructor in
.h file will be an error.

+
+void RTCPReceiveInformation::InsertTmmbrItem(uint32_t sender_ssrc,
+                                             const rtcp::TmmbItem& tmmbr_item,
+                                             int64_t current_time_ms) {
+  // Serach to see if we have it in our list.
+  for (auto& entry : tmmbr_) {

[philipel, 2016/08/17 13:53:06]

How large do we expect |tmmbr_| to be? Should we maybe use a map instead?

[danilchap, 2016/08/17 14:56:05]

Not large. One per one per participant that actively use tmmbr.
Most of the time - zero.
Using map though can make code cleaner.
(e.g. makes disagreement in next comment irrelevant)

[philipel, 2016/08/17 15:30:06]

Acknowledged.

+    if (entry.tmmbr_item.ssrc() == sender_ssrc) {
+      // We already have this SSRC in our list. Update it.
+      entry.tmmbr_item.set_bitrate_bps(tmmbr_item.bitrate_bps());
+      entry.tmmbr_item.set_packet_overhead(tmmbr_item.packet_overhead());
+      entry.last_updated_ms = current_time_ms;
       return;
     }
   }
-  VerifyAndAllocateTMMBRSet(TmmbrSet.lengthOfSet() + 1);
-  TmmbrSet.AddEntry(TMMBRItem.MaxTotalMediaBitRate,
-                    TMMBRItem.MeasuredOverhead,
-                    senderSSRC);
-  _tmmbrSetTimeouts.push_back(currentTimeMS);
+  TimedTmmbrItem entry = {
+      {sender_ssrc, tmmbr_item.bitrate_bps(), tmmbr_item.packet_overhead()},

[philipel, 2016/08/17 13:53:06]

Implement TimedTmmbrItem(int64_t last, TmmbItem item) and use that instead.

[danilchap, 2016/08/17 14:56:05]

Why?
Those strait-forward constructors were helpful when braced initialization was in
banned features.

[philipel, 2016/08/17 15:30:06]

If someone change TimedTmmbrItem or the TmmbrItem class itself then this will
still compile and the values will be incorrectly assigned, Very easy to break.

[danilchap, 2016/08/17 16:15:33]

I guess you right. But this constructor doesn't look needed now.
Unless you think replacing vector with a map was a bad idea.

+      current_time_ms};
+  tmmbr_.push_back(entry);
 }
 
-void RTCPReceiveInformation::GetTMMBRSet(
+void RTCPReceiveInformation::GetTmmbrSet(
     int64_t current_time_ms,
     std::vector<rtcp::TmmbItem>* candidates) {
-  // Erase timeout entries.
-  for (size_t source_idx = 0; source_idx < TmmbrSet.size();) {
-    // Use audio define since we don't know what interval the remote peer is
-    // using.
-    if (current_time_ms - _tmmbrSetTimeouts[source_idx] >
-        5 * RTCP_INTERVAL_AUDIO_MS) {
-      // Value timed out.
-      TmmbrSet.erase(TmmbrSet.begin() + source_idx);
-      _tmmbrSetTimeouts.erase(_tmmbrSetTimeouts.begin() + source_idx);
-      continue;
+  // Use audio define since we don't know what interval the remote peer use.
+  int64_t timeouted_ms = current_time_ms - 5 * RTCP_INTERVAL_AUDIO_MS;
+  for (auto it = tmmbr_.begin(); it != tmmbr_.end();) {
+    if (it->last_updated_ms < timeouted_ms) {
+      // Erase timeout entries.
+      it = tmmbr_.erase(it);
+    } else {
+      candidates->push_back(it->tmmbr_item);
+      ++it;
     }
-    candidates->push_back(TmmbrSet[source_idx]);
-    ++source_idx;
   }
 }
 
-void RTCPReceiveInformation::VerifyAndAllocateBoundingSet(
-    const uint32_t minimumSize) {
-  TmmbnBoundingSet.VerifyAndAllocateSet(minimumSize);
+void RTCPReceiveInformation::ClearTmmbr() {
+  tmmbr_.clear();
 }
+
 }  // namespace RTCPHelp
 }  // namespace webrtc