cast: Split Rtcp into two for sender and receiver

cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843
Committed: https://crrev.com/7c78ac257eb380c35b27c34c26ab4764f9d4fee7
Cr-Commit-Position: refs/heads/master@{#367143}

[Irfan, 2015-12-11 00:19:43]

Description was changed from

==========
cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843
==========

to

==========
cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843
==========

[Irfan, 2015-12-11 00:19:43]

isheriff@chromium.org changed reviewers:
+ miu@chromium.org, xjz@chromium.org

[Irfan, 2015-12-11 00:20:33]

PTAL. Primary goal was to split first and make more improvement iteratively.
Needs more testing on my end.

[xjz, 2015-12-11 18:02:47]

https://codereview.chromium.org/1520613004/diff/1/media/cast/net/rtcp/receive...
File media/cast/net/rtcp/receiver_rtcp_session.h (right):

https://codereview.chromium.org/1520613004/diff/1/media/cast/net/rtcp/receive...
media/cast/net/rtcp/receiver_rtcp_session.h:20: class ReceiverRtcpSession :
public RtcpSession {
Where is RtcpSession defined? Did you forget "rtcp_session.h"?

[Irfan, 2015-12-11 18:42:57]

https://codereview.chromium.org/1520613004/diff/1/media/cast/net/rtcp/receive...
File media/cast/net/rtcp/receiver_rtcp_session.h (right):

https://codereview.chromium.org/1520613004/diff/1/media/cast/net/rtcp/receive...
media/cast/net/rtcp/receiver_rtcp_session.h:20: class ReceiverRtcpSession :
public RtcpSession {
On 2015/12/11 18:02:47, xjz wrote:
> Where is RtcpSession defined? Did you forget "rtcp_session.h"?

Thanks for catching. Added now.

[miu, 2015-12-12 00:53:25]

Excellent clean-up!  :)  lgtm % nits:

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:476: rtcp.SendRtcpReport(time_data,
cast_message, target_delay, rtcp_events,
Huh.  Interesting that this was in the CastTransportSENDERImpl.  ;-)  Perhaps
add a TODO and file a crbug (with Cr=Internals-Cast label) to resolve this code
factoring issue?

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/rtcp/rec...
File media/cast/net/rtcp/receiver_rtcp_session.h (right):

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/rtcp/rec...
media/cast/net/rtcp/receiver_rtcp_session.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
Can you try re-uploading the patch with the similarity setting set low (e.g.,
git cl upload --similarity=10)?  This would help preserve code change history
and make it easy to determine what code changed as part of the refactoring.

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/rtcp/sen...
File media/cast/net/rtcp/sender_rtcp_session.cc (right):

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/rtcp/sen...
media/cast/net/rtcp/sender_rtcp_session.cc:29: kOutOfOrderMaxAgeMs = 500,
Can you put a TODO (and crbug) here too?  It feels like this is a very arbitrary
and fragile way to prevent the problem it claims to solve.  Seems like our
implementation should just re-order and/or drop packets that are delivered
out-of-sequence; or, at the least, not be thrown into a bad state by them.

[Irfan, 2015-12-12 01:07:37]

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:476: rtcp.SendRtcpReport(time_data,
cast_message, target_delay, rtcp_events,
On 2015/12/12 00:53:25, miu wrote:
> Huh.  Interesting that this was in the CastTransportSENDERImpl.  ;-)  Perhaps
> add a TODO and file a crbug (with Cr=Internals-Cast label) to resolve this
code
> factoring issue?

will do because this is indeed confusing code, but the "sender" in transport and
"receiver" on RTCP are different entities (The RTP receiver still uses cast
transport sender IPC essentially)

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/rtcp/sen...
File media/cast/net/rtcp/sender_rtcp_session.cc (right):

https://codereview.chromium.org/1520613004/diff/20001/media/cast/net/rtcp/sen...
media/cast/net/rtcp/sender_rtcp_session.cc:29: kOutOfOrderMaxAgeMs = 500,
On 2015/12/12 00:53:25, miu wrote:
> Can you put a TODO (and crbug) here too?  It feels like this is a very
arbitrary
> and fragile way to prevent the problem it claims to solve.  Seems like our
> implementation should just re-order and/or drop packets that are delivered
> out-of-sequence; or, at the least, not be thrown into a bad state by them.

Done.

[Irfan, 2015-12-12 01:22:13]

PTAL

[miu, 2015-12-12 01:45:44]

lgtm

[xjz, 2015-12-14 18:06:05]

lgtm

[Irfan, 2015-12-30 00:17:46]

Patchset #5 (id:80001) has been deleted

[Irfan, 2015-12-30 00:19:16]

PTAL

[miu, 2015-12-30 05:29:42]

lgtm

[Irfan, 2015-12-30 06:33:26]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2015-12-30 06:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org
Link to the patchset: https://codereview.chromium.org/1520613004/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2015-12-30 06:33:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1520613004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1520613004/100001

[commit-bot: I haz the power, 2015-12-30 06:38:06]

Description was changed from

==========
cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843
==========

to

==========
cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843
==========

[commit-bot: I haz the power, 2015-12-30 06:38:07]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-12-30 06:38:59]

Description was changed from

==========
cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843
==========

to

==========
cast: Split Rtcp into two for sender and receiver

Splits the Rtcp class into RTP sender and receiver specific
files. The result is that each class now maintains state that
is only appropriate as a RTP sender or a RTP receiver.

For example, the receiver now removes:

- Callbacks that were not being invoked
- RTT calculations that never get used (since it is only needed for
  congestion control on sender)
- Log handling and deduplication that is only useful at sender

The sender removes:

- Lip sync info tracking (from a sender) that is done at a receiver

The current CL does not modify any of the rest of existing functionality
which make sense as follow ups.

BUG=530843

Committed: https://crrev.com/7c78ac257eb380c35b27c34c26ab4764f9d4fee7
Cr-Commit-Position: refs/heads/master@{#367143}
==========

[commit-bot: I haz the power, 2015-12-30 06:39:01]

Patchset 5 (id:??) landed as
https://crrev.com/7c78ac257eb380c35b27c34c26ab4764f9d4fee7
Cr-Commit-Position: refs/heads/master@{#367143}