Cleanup RtcpReceiver::TMMBRReceived function

webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  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.
  */
 
@@ skipping 141 matching lines @@
       return;
     }
   }
   VerifyAndAllocateTMMBRSet(TmmbrSet.lengthOfSet() + 1);
   TmmbrSet.AddEntry(TMMBRItem.MaxTotalMediaBitRate,
                     TMMBRItem.MeasuredOverhead,
                     senderSSRC);
   _tmmbrSetTimeouts.push_back(currentTimeMS);
 }
 
-int32_t RTCPReceiveInformation::GetTMMBRSet(
-    const uint32_t sourceIdx,
-    const uint32_t targetIdx,
-    TMMBRSet* candidateSet,
-    const int64_t currentTimeMS) {
-  if (sourceIdx >= TmmbrSet.lengthOfSet()) {
-    return -1;
+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] >

[philipel, 2016/08/16 15:12:16]

Is there a reason the timeout is stored in a separate vector and not in the
TmmbItem itself?

[danilchap, 2016/08/16 15:35:15]

TmmbItem match network structure and doesn't have place for timeout.
Timeout (or actually the time when item was added) not part of the item itself.
E.g. the result vector for this function doesn't expect items with creation
time.

On the other side, it is refactorable: It make sense to add a struct
TmmbItemWithTimeout : public TmmbItem { int64_t created; }
and keep vector of those in the RTCPReceiveInformation.

You think it make sense to do it in same CL or prefer a follow-up one? (with the
goal to remove TMMBRSet class it is inevitable either way)

[philipel, 2016/08/16 15:40:06]

Do it in a follow-up.

+        5 * RTCP_INTERVAL_AUDIO_MS) {
+      // Value timed out.
+      TmmbrSet.erase(TmmbrSet.begin() + source_idx);

[philipel, 2016/08/16 15:12:16]

Maybe save the indexes of the elements you want to erase and then erase them
after you have iterated over the vectors. Also makes the for-loop cleaner since
you can move the ++soruce_idx to the for-loop statement. 

[danilchap, 2016/08/16 15:35:15]

Not sure separating extracting and cleaning the array would make loop(s)
cleaner, but it will make it less efficient (need an extra dynamic array and a
careful second loop where need to remember go through indexes in reverse order).
I'm not convinced that worse it.

[philipel, 2016/08/16 15:40:06]

Acknowledged.

+      _tmmbrSetTimeouts.erase(_tmmbrSetTimeouts.begin() + source_idx);
+      continue;
+    }
+    candidates->push_back(TmmbrSet[source_idx]);
+    ++source_idx;
   }
-  if (targetIdx >= candidateSet->sizeOfSet()) {
-    return -1;
-  }
-  // use audio define since we don't know what interval the remote peer is using
-  if (currentTimeMS - _tmmbrSetTimeouts[sourceIdx] >
-      5 * RTCP_INTERVAL_AUDIO_MS) {
-    // value timed out
-    TmmbrSet.RemoveEntry(sourceIdx);
-    _tmmbrSetTimeouts.erase(_tmmbrSetTimeouts.begin() + sourceIdx);
-    return -1;
-  }
-  candidateSet->SetEntry(targetIdx,
-                         TmmbrSet.Tmmbr(sourceIdx),
-                         TmmbrSet.PacketOH(sourceIdx),
-                         TmmbrSet.Ssrc(sourceIdx));
-  return 0;
 }
 
 void RTCPReceiveInformation::VerifyAndAllocateBoundingSet(
     const uint32_t minimumSize) {
   TmmbnBoundingSet.VerifyAndAllocateSet(minimumSize);
 }
 }  // namespace RTCPHelp
 }  // namespace webrtc