Adjust RtcpReceiver to be testable with callbacks:

Adjust RtcpReceiver to be testable with callbacks:
Instead of full RtpRtcpImpl takes interface of all functions it needs from it.
Added single function for parsing packets and sending feedback, moving that
logic from RtpRtcpImpl to RtcpReceiver.

BUG=webrtc:5260
Committed: https://crrev.com/59cb2bd20ee76c83deb85979a3d9a255dbc55197
Cr-Commit-Position: refs/heads/master@{#13960}

[danilchap, 2016-08-23 18:05:31]

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

[danilchap, 2016-08-23 18:05:31]

This is slice of the https://codereview.webrtc.org/1527003002/
that you review half a year ago, but I never landed.

After fixing a TODO there, that CL grew wider, so now I'm splitting it. This CL
hold all the changes to non-test code needed to change test code.

[philipel, 2016-08-24 13:50:26]

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

https://codereview.webrtc.org/2274573002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:33: class SenderCallbacks {
I think this name is a bit ambiguous. Maybe call it RTCPReceiver::Callbacks or
RTCPReceiver::RtcpReceivedCallbacks or something like that.

https://codereview.webrtc.org/2274573002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:37: virtual void OnReceivedNACK(
OnReceiverNack

https://codereview.webrtc.org/2274573002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:38: const std::list<uint16_t>&
nack_sequence_numbers) = 0;
If it only requires a small change, use vector instead.

[danilchap, 2016-08-26 11:47:51]

The mentioned separate CL: https://codereview.webrtc.org/2276833003/

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

https://codereview.webrtc.org/2274573002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:33: class SenderCallbacks {
On 2016/08/24 13:50:26, philipel wrote:
> I think this name is a bit ambiguous. Maybe call it RTCPReceiver::Callbacks or
> RTCPReceiver::RtcpReceivedCallbacks or something like that.

Can agree name is ambigous, SenderCallbacks sounds like it is callbacks from
sender not for sender.
Just Callbacks feel too generic, it like having no name.
RtcpReceiver::RtcpReceivedCallbacks - both redundant and generic.

What about just 'Sender'. Feels ok, or should I rethink?
(this 4 function are either for rtp_sender or for rtcp_sender).

https://codereview.webrtc.org/2274573002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:37: virtual void OnReceivedNACK(
On 2016/08/24 13:50:26, philipel wrote:
> OnReceiverNack

done in separate CL since it is unrelated to this one.

https://codereview.webrtc.org/2274573002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:38: const std::list<uint16_t>&
nack_sequence_numbers) = 0;
On 2016/08/24 13:50:26, philipel wrote:
> If it only requires a small change, use vector instead.

Not that small, done in separate CL since it is unrelated to this one.

[philipel, 2016-08-29 08:19:08]

https://codereview.webrtc.org/2274573002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.h (right):

https://codereview.webrtc.org/2274573002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:32: class Sender {
This class implements functions that are called when we receive certain rtcp
messages right?

I don't understand how "Sender" tries to convey that.

Maybe move this class outside of RTCPReceiver and call it RtcpReceivedCallbacks?

[danilchap, 2016-08-29 08:46:45]

On 2016/08/29 08:19:08, philipel wrote:
>
https://codereview.webrtc.org/2274573002/diff/60001/webrtc/modules/rtp_rtcp/s...
> File webrtc/modules/rtp_rtcp/source/rtcp_receiver.h (right):
> 
>
https://codereview.webrtc.org/2274573002/diff/60001/webrtc/modules/rtp_rtcp/s...
> webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:32: class Sender {
> This class implements functions that are called when we receive certain rtcp
> messages right?
> 
> I don't understand how "Sender" tries to convey that.
> 
> Maybe move this class outside of RTCPReceiver and call it
RtcpReceivedCallbacks?

These are not the only callbacks (there are several observers that are callbacks
too)
And these are not just callbacks (SetTmmbn might be called on timeout, not just
as a response to an rtcp message).
Will think more about name that can catch that.

[philipel, 2016-08-29 08:55:02]

RtcpEventCallbacks?

[danilchap, 2016-08-29 08:55:44]

On 2016/08/29 08:46:45, danilchap wrote:
> On 2016/08/29 08:19:08, philipel wrote:
> >
>
https://codereview.webrtc.org/2274573002/diff/60001/webrtc/modules/rtp_rtcp/s...
> > File webrtc/modules/rtp_rtcp/source/rtcp_receiver.h (right):
> > 
> >
>
https://codereview.webrtc.org/2274573002/diff/60001/webrtc/modules/rtp_rtcp/s...
> > webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:32: class Sender {
> > This class implements functions that are called when we receive certain rtcp
> > messages right?
> > 
> > I don't understand how "Sender" tries to convey that.
> > 
> > Maybe move this class outside of RTCPReceiver and call it
> RtcpReceivedCallbacks?
> 
> These are not the only callbacks (there are several observers that are
callbacks
> too)
> And these are not just callbacks (SetTmmbn might be called on timeout, not
just
> as a response to an rtcp message).
> Will think more about name that can catch that.

May be I can leave old name 'ModuleRtpRtcp'
and keep it nested in RTCPReceiver to stress it is ModuleRtpRtcp as seen by
RTCPReceiver?

[philipel, 2016-08-29 11:39:35]

lgtm

[danilchap, 2016-08-29 12:28:06]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-08-29 12:28:13]

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-29 13:57:33]

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

[commit-bot: I haz the power, 2016-08-29 13:57:34]

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

[danilchap, 2016-08-29 18:04:04]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-08-29 18:04:21]

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-29 18:08:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-29 18:09:00]

Description was changed from

==========
Adjust RtcpReceiver to be testable with callbacks:
Instead of full RtpRtcpImpl takes interface of all functions it needs from it.
Added single function for parsing packets and sending feedback, moving that
logic from RtpRtcpImpl to RtcpReceiver.

BUG=webrtc:5260
==========

to

==========
Adjust RtcpReceiver to be testable with callbacks:
Instead of full RtpRtcpImpl takes interface of all functions it needs from it.
Added single function for parsing packets and sending feedback, moving that
logic from RtpRtcpImpl to RtcpReceiver.

BUG=webrtc:5260

Committed: https://crrev.com/59cb2bd20ee76c83deb85979a3d9a255dbc55197
Cr-Commit-Position: refs/heads/master@{#13960}
==========

[commit-bot: I haz the power, 2016-08-29 18:09:01]

Patchset 6 (id:??) landed as
https://crrev.com/59cb2bd20ee76c83deb85979a3d9a255dbc55197
Cr-Commit-Position: refs/heads/master@{#13960}