Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver

Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
move all logic from that class into RTCPReceiver too,
Simplify and fix style on the way.

BUG=webrtc:5565
Committed: https://crrev.com/7851bda9bc1df4f1efbaf8eba66bce4abe43005d
Cr-Commit-Position: refs/heads/master@{#14442}

[danilchap, 2016-09-27 16:52:19]

Description was changed from

==========
Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
and all logic from that class into RTCPReceiver,
simplifying and fixing style on the way.

BUG=webrtc:5565
==========

to

==========
Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
move all logic from that class into RTCPReceiver too,
Simplify and fix style on the way.

BUG=webrtc:5565
==========

[danilchap, 2016-09-28 08:52:54]

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

[philipel, 2016-09-29 11:39:21]

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

https://codereview.webrtc.org/2373053002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:150: int64_t last_received_rr =
-1;
Maybe make |last_received_rr| a member variable and update it in
CreateReceiveInformation?

https://codereview.webrtc.org/2373053002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:669: } else if
(receive_info->ready_for_delete) {
Should a ReceiveInfo that is ready for deletion ever not be deleted? Is it
correct to insert DCHECK(!receive_info->ready_for_delete) at line 660?

If so I think you should check this condition first, and then erase and continue
if it's true, and perform the rest of the logic after that check. This way you
don't have to repeat the ++receice_info_it statement.

[danilchap, 2016-09-29 13:00:58]

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

https://codereview.webrtc.org/2373053002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:150: int64_t last_received_rr =
-1;
On 2016/09/29 11:39:21, philipel wrote:
> Maybe make |last_received_rr| a member variable and update it in
> CreateReceiveInformation?

It totaly make sense, thank you, but it turn out to be bigger thing than it
seems:
Such variable already exists (_lastReceivedRrMs).
It behave slightly different: set on report block, not on a sender/receiver
report, might be reset.
That different behavior make more sense how LastReceivedReceiverReport is used
but not how it is named.
It make even more sense to remove LastReceivedReceiverReport method altogether
and provide a different, single function for calculating rtt.

Would prefer to address those issues in a dedicated CL.

https://codereview.webrtc.org/2373053002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:669: } else if
(receive_info->ready_for_delete) {
On 2016/09/29 11:39:21, philipel wrote:
> Should a ReceiveInfo that is ready for deletion ever not be deleted? Is it
> correct to insert DCHECK(!receive_info->ready_for_delete) at line 660?
> 
> If so I think you should check this condition first, and then erase and
continue
> if it's true, and perform the rest of the logic after that check. This way you
> don't have to repeat the ++receice_info_it statement.

Comment (not mine, I just moved it) explicitly say it should be deleted when
ready_for_delete AND dont have a last_time_received_ms (i.e. timeouted).
Before timeouted still used for tmmbr/tmmbn.

[philipel, 2016-09-29 14:43:38]

lgtm

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

https://codereview.webrtc.org/2373053002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:150: int64_t last_received_rr =
-1;
On 2016/09/29 13:00:58, danilchap wrote:
> On 2016/09/29 11:39:21, philipel wrote:
> > Maybe make |last_received_rr| a member variable and update it in
> > CreateReceiveInformation?
> 
> It totaly make sense, thank you, but it turn out to be bigger thing than it
> seems:
> Such variable already exists (_lastReceivedRrMs).
> It behave slightly different: set on report block, not on a sender/receiver
> report, might be reset.
> That different behavior make more sense how LastReceivedReceiverReport is used
> but not how it is named.
> It make even more sense to remove LastReceivedReceiverReport method altogether
> and provide a different, single function for calculating rtt.
> 
> Would prefer to address those issues in a dedicated CL.

Lets do it in another CL then.

https://codereview.webrtc.org/2373053002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:669: } else if
(receive_info->ready_for_delete) {
On 2016/09/29 13:00:58, danilchap wrote:
> On 2016/09/29 11:39:21, philipel wrote:
> > Should a ReceiveInfo that is ready for deletion ever not be deleted? Is it
> > correct to insert DCHECK(!receive_info->ready_for_delete) at line 660?
> > 
> > If so I think you should check this condition first, and then erase and
> continue
> > if it's true, and perform the rest of the logic after that check. This way
you
> > don't have to repeat the ++receice_info_it statement.
> 
> Comment (not mine, I just moved it) explicitly say it should be deleted when
> ready_for_delete AND dont have a last_time_received_ms (i.e. timeouted).
> Before timeouted still used for tmmbr/tmmbn.

Oops, missed that comment :)

[danilchap, 2016-09-29 14:51:19]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-09-29 14:51:33]

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-09-29 16:52:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 16:52:07]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[danilchap, 2016-09-29 22:19:46]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-09-29 22:19:50]

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-09-29 22:28:09]

Description was changed from

==========
Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
move all logic from that class into RTCPReceiver too,
Simplify and fix style on the way.

BUG=webrtc:5565
==========

to

==========
Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
move all logic from that class into RTCPReceiver too,
Simplify and fix style on the way.

BUG=webrtc:5565
==========

[commit-bot: I haz the power, 2016-09-29 22:28:10]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-29 22:28:15]

Description was changed from

==========
Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
move all logic from that class into RTCPReceiver too,
Simplify and fix style on the way.

BUG=webrtc:5565
==========

to

==========
Move RTCPHelp::RTCPReceiveInformation inside RTCPReceiver
move all logic from that class into RTCPReceiver too,
Simplify and fix style on the way.

BUG=webrtc:5565

Committed: https://crrev.com/7851bda9bc1df4f1efbaf8eba66bce4abe43005d
Cr-Commit-Position: refs/heads/master@{#14442}
==========

[commit-bot: I haz the power, 2016-09-29 22:28:16]

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