Cleanup RtcpReceiver::TMMBRReceived function

Cleanup RtcpReceiver::TMMBRReceived function

BUG=webrtc:951
Committed: https://crrev.com/287e54820b30764ea95efd46208ea932efdbef69
Cr-Commit-Position: refs/heads/master@{#13786}

[danilchap, 2016-08-16 10:54:55]

danilchap@webrtc.org changed reviewers:
+ philipel@webrtc.org

[danilchap, 2016-08-16 10:54:55]

Next small step clearing out TMMBRSet

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

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc (right):

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc:169: if (current_time_ms -
_tmmbrSetTimeouts[source_idx] >
Is there a reason the timeout is stored in a separate vector and not in the
TmmbItem itself?

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc:172:
TmmbrSet.erase(TmmbrSet.begin() + source_idx);
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]

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc (right):

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc:169: if (current_time_ms -
_tmmbrSetTimeouts[source_idx] >
On 2016/08/16 15:12:16, philipel wrote:
> Is there a reason the timeout is stored in a separate vector and not in the
> TmmbItem itself?

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)

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc:172:
TmmbrSet.erase(TmmbrSet.begin() + source_idx);
On 2016/08/16 15:12:16, philipel wrote:
> 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. 

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]

lgtm

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc (right):

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc:169: if (current_time_ms -
_tmmbrSetTimeouts[source_idx] >
On 2016/08/16 15:35:15, danilchap wrote:
> On 2016/08/16 15:12:16, philipel wrote:
> > Is there a reason the timeout is stored in a separate vector and not in the
> > TmmbItem itself?
> 
> 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)

Do it in a follow-up.

https://codereview.webrtc.org/2250633002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.cc:172:
TmmbrSet.erase(TmmbrSet.begin() + source_idx);
On 2016/08/16 15:35:15, danilchap wrote:
> On 2016/08/16 15:12:16, philipel wrote:
> > 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. 
> 
> 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.

Acknowledged.

[danilchap, 2016-08-16 19:56:53]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-08-16 19:56:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-16 22:15:44]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-16 22:15:52]

Description was changed from

==========
Cleanup RtcpReceiver::TMMBRReceived function

BUG=webrtc:951
==========

to

==========
Cleanup RtcpReceiver::TMMBRReceived function

BUG=webrtc:951

Committed: https://crrev.com/287e54820b30764ea95efd46208ea932efdbef69
Cr-Commit-Position: refs/heads/master@{#13786}
==========

[commit-bot: I haz the power, 2016-08-16 22:15:53]

Patchset 1 (id:??) landed as
https://crrev.com/287e54820b30764ea95efd46208ea932efdbef69
Cr-Commit-Position: refs/heads/master@{#13786}