Cleanup RtcpReceiver::TMMBRReceived function

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 bfcc1bdfde02d4b822d367f4f3404d335f761d05..4ad1ddeb7616e6205d6a2d0a6168725ca55fe36f 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc
@@ -159,30 +159,23 @@ void RTCPReceiveInformation::InsertTMMBRItem(
   _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;
-  }
-  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;
+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;
   }
-  candidateSet->SetEntry(targetIdx,
-                         TmmbrSet.Tmmbr(sourceIdx),
-                         TmmbrSet.PacketOH(sourceIdx),
-                         TmmbrSet.Ssrc(sourceIdx));
-  return 0;
 }
 
 void RTCPReceiveInformation::VerifyAndAllocateBoundingSet(