Don't allocate any RTPSender object for a receive only RtpRtcp module

Don't allocate any RTPSender object for a receive only RtpRtcp module.

This is one step towards separation of send-side and receive-side
processing.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2740163002
Cr-Commit-Position: refs/heads/master@{#17306}
Committed: https://chromium.googlesource.com/external/webrtc/+/14adba77ecdf6b8ec604363c3717def1e60d7b73

[nisse-webrtc, 2017-03-10 10:23:47]

Description was changed from

==========
Don't allocate any RTP sender for a receiver_only RTP module.

BUG=webrtc:7135
==========

to

==========
Don't allocate any RTPSender object for a receive only RtpRtcp module.

BUG=webrtc:7135
==========

[nisse-webrtc, 2017-03-10 12:56:33]

nisse@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-03-10 12:56:33]

PTAL.

This is a first step towards separating send and receive objects.

[danilchap, 2017-03-10 15:48:13]

Can you add motivation to CL description.
Is it a step towards splitting RtpRtcpModule into RtpRtcpSender and
RtpRtcpReceiver?

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:321: // TODO(nisse): This is
currently called also when receiver_only is true. Hence
this called when rtcp should be send, it collects information for various rtcp
messages. Some of the information is not need for receiver_only module, so not a
problem not filling it.

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:337: state.packets_sent =
rtp_stats.transmitted.packets +
this and next field doesn't make sense if rtp_sender is absent - note that
rtp_stats/rtx_stats are filled inside if(rtp_sender_) block.
(they are used for SenderReport)

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:814: void
ModuleRtpRtcpImpl::OnReceivedNack(
if receive-only client get an rtcp nack message, it will call this function.
So to protect client against malicious sender, you want to check if rtp_sender_
is not null: e.g. add at the start of the function
if (!rtp_sender_)
  return;

You might want to run fuzzer on ModuleRtpRtcpImpl to find more cases like this
one.

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:833: if (rtp_sender_) {
in rtp module it is more common to omit {} around single-line statement.

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (left):

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:60: int8_t SendPayloadType()
const;
may be delete this method in another, tiny CL.

[nisse-webrtc, 2017-03-13 08:16:21]

On 2017/03/10 15:48:13, danilchap wrote:
> Can you add motivation to CL description.
> Is it a step towards splitting RtpRtcpModule into RtpRtcpSender and
> RtpRtcpReceiver?

I can try expanding it. But I'm afraid I'm not sure what the next step is...

I think ultimately we should maybe classes like

  RtpSender
  RtpSender::RtcpSender
  RtpSender::RtcpReceiver

  RtpReceiver
  RtpReceiver::RtcpSender
  RtpReceiver::RtcpReceiver

i.e. four different RTCP classes, depending on the direction of the RTCP
packets, and the direction of the RTP stream the packets describe. But at the
moment, I'm happy if we can just clarify what's needed for the send and receive
cases. 

Hmm, and just now I notice that ModuleRtpRtcpImpl does *not* have any
RtpReceiver, in the receiver_only case it's handing RTCP only, and it's not on
the RTP packet path. I have to figure out how these two classes are associated
for a receive stream.

[nisse-webrtc, 2017-03-13 08:20:10]

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:321: // TODO(nisse): This is
currently called also when receiver_only is true. Hence
On 2017/03/10 15:48:12, danilchap wrote:
> this called when rtcp should be send, it collects information for various rtcp
> messages. Some of the information is not need for receiver_only module, so not
a
> problem not filling it.

What info is relevant in the receive receiver_only case? I guess only
LastReceivedNTP and LastReceivedXrReferenceTimeInfo?

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:337: state.packets_sent =
rtp_stats.transmitted.packets +
On 2017/03/10 15:48:12, danilchap wrote:
> this and next field doesn't make sense if rtp_sender is absent - note that
> rtp_stats/rtx_stats are filled inside if(rtp_sender_) block.
> (they are used for SenderReport)

Fixed.

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:814: void
ModuleRtpRtcpImpl::OnReceivedNack(
On 2017/03/10 15:48:12, danilchap wrote:
> if receive-only client get an rtcp nack message, it will call this function.
> So to protect client against malicious sender, you want to check if
rtp_sender_
> is not null: e.g. add at the start of the function
> if (!rtp_sender_)
>   return;
> 
> You might want to run fuzzer on ModuleRtpRtcpImpl to find more cases like this
> one.

Done.

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:833: if (rtp_sender_) {
On 2017/03/10 15:48:12, danilchap wrote:
> in rtp module it is more common to omit {} around single-line statement.

Done.

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (left):

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:60: int8_t SendPayloadType()
const;
On 2017/03/10 15:48:12, danilchap wrote:
> may be delete this method in another, tiny CL.

I can do that, to reduce the risk of downstream breakage from this cl.

[danilchap, 2017-03-13 12:40:09]

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:321: // TODO(nisse): This is
currently called also when receiver_only is true. Hence
On 2017/03/13 08:20:10, nisse-webrtc wrote:
> On 2017/03/10 15:48:12, danilchap wrote:
> > this called when rtcp should be send, it collects information for various
rtcp
> > messages. Some of the information is not need for receiver_only module, so
not
> a
> > problem not filling it.
> 
> What info is relevant in the receive receiver_only case? I guess only
> LastReceivedNTP and LastReceivedXrReferenceTimeInfo?

yes, those are used on recieve side.
state.module is used too, I think.

https://codereview.webrtc.org/2740163002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:338: state.send_payload_type =
static_cast<uint8_t>(-1);
separation receiver and sender reveals this is broken:
send_payload_type seems only used by media receiver to send RPSI message.
but payload_type only set when media sender is present and active.
Which mean sending RPSI is broken - even when it is possible to send an rpsi,
incorrect payload type will be used.

Stefan, do you know if we still want to support RPSI?

May it is better for now to remove sending rpsi (and thus
state.send_payload_type) and then restore it correctly if we want to use it.

[nisse-webrtc, 2017-03-13 12:52:29]

On 2017/03/13 12:40:09, danilchap wrote:
>
https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
> File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):
> 
>
https://codereview.webrtc.org/2740163002/diff/80001/webrtc/modules/rtp_rtcp/s...
> webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:321: // TODO(nisse): This is
> currently called also when receiver_only is true. Hence
> On 2017/03/13 08:20:10, nisse-webrtc wrote:
> > On 2017/03/10 15:48:12, danilchap wrote:
> > > this called when rtcp should be send, it collects information for various
> rtcp
> > > messages. Some of the information is not need for receiver_only module, so
> not
> > a
> > > problem not filling it.
> > 
> > What info is relevant in the receive receiver_only case? I guess only
> > LastReceivedNTP and LastReceivedXrReferenceTimeInfo?
> 
> yes, those are used on recieve side.
> state.module is used too, I think.
> 
>
https://codereview.webrtc.org/2740163002/diff/100001/webrtc/modules/rtp_rtcp/...
> File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):
> 
>
https://codereview.webrtc.org/2740163002/diff/100001/webrtc/modules/rtp_rtcp/...
> webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:338: state.send_payload_type =
> static_cast<uint8_t>(-1);
> separation receiver and sender reveals this is broken:
> send_payload_type seems only used by media receiver to send RPSI message.
> but payload_type only set when media sender is present and active.
> Which mean sending RPSI is broken - even when it is possible to send an rpsi,
> incorrect payload type will be used.
> 
> Stefan, do you know if we still want to support RPSI?
> 
> May it is better for now to remove sending rpsi (and thus
> state.send_payload_type) and then restore it correctly if we want to use it.

And it seems SendRTCPReferencePictureSelection is never called from anywhere?
Last call deleted in mflodman's cl https://codereview.webrtc.org/1929313002.

[danilchap, 2017-03-16 15:08:43]

https://codereview.webrtc.org/2740163002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:377:
rtp_sender_->SetSendingMediaStatus(sending);
add if (rtp_sender_) check to protect against receiver only modules that call
SetSendingMediaStatus(false) to be safe.
may with an RTC_CHECK(!sending || rtp_sender_);

[nisse-webrtc, 2017-03-16 15:14:08]

https://codereview.webrtc.org/2740163002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:377:
rtp_sender_->SetSendingMediaStatus(sending);
On 2017/03/16 15:08:42, danilchap wrote:
> add if (rtp_sender_) check to protect against receiver only modules that call
> SetSendingMediaStatus(false) to be safe.
> may with an RTC_CHECK(!sending || rtp_sender_);

Done.

[danilchap, 2017-03-16 15:41:13]

looks scary, looks good

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:100: uint32_t SSRC() const;
Ssrc()
(no need to introduce functions that break naming style guide)

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:123:
SetMaxRtpPacketSize(IP_PACKET_SIZE - kTcpOverIpv4HeaderSize);
rtcp_sender also need to know max packet size.

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:338: state.send_payload_type =
static_cast<uint8_t>(-1);
send_payload_type is now unused (thanks to rpsi removal) - no need to set it.
send_bitrate is set to 0 in the constructor, so else block can be removed.

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:438:
rtp_sender_->SetMaxRtpPacketSize(rtp_packet_size);
since SetMaxRtpPacketSize make sense to call for receiver-only module too,
protect this line

[nisse-webrtc, 2017-03-17 09:29:45]

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:100: uint32_t SSRC() const;
On 2017/03/16 15:41:12, danilchap wrote:
> Ssrc()
> (no need to introduce functions that break naming style guide)

In this case, I think consistency with existing methods like SetSSRC and
SetRemoteSSRC is desirable. So I'd prefer all uppercase. 

But I'll change if you insist.

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:123:
SetMaxRtpPacketSize(IP_PACKET_SIZE - kTcpOverIpv4HeaderSize);
On 2017/03/16 15:41:12, danilchap wrote:
> rtcp_sender also need to know max packet size.

Done.

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:338: state.send_payload_type =
static_cast<uint8_t>(-1);
On 2017/03/16 15:41:12, danilchap wrote:
> send_payload_type is now unused (thanks to rpsi removal) - no need to set it.
> send_bitrate is set to 0 in the constructor, so else block can be removed.

Ok, I'm deleting the else clause and setting of send_payload_type. And then I'll
make a separate small cl to delete send_payload_type.

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:438:
rtp_sender_->SetMaxRtpPacketSize(rtp_packet_size);
On 2017/03/16 15:41:13, danilchap wrote:
> since SetMaxRtpPacketSize make sense to call for receiver-only module too,
> protect this line

Done.

[danilchap, 2017-03-17 09:49:39]

lgtm

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/2740163002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:100: uint32_t SSRC() const;
On 2017/03/17 09:29:44, nisse-webrtc wrote:
> On 2017/03/16 15:41:12, danilchap wrote:
> > Ssrc()
> > (no need to introduce functions that break naming style guide)
> 
> In this case, I think consistency with existing methods like SetSSRC and
> SetRemoteSSRC is desirable. So I'd prefer all uppercase. 
> 
> But I'll change if you insist.

I don't insist: local consistency is a good argument.

[nisse-webrtc, 2017-03-17 12:23:09]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 12:23:13]

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

[stefan-webrtc, 2017-03-17 12:31:29]

https://codereview.webrtc.org/2740163002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:349: // this method for a
receive-only stream, which makes little sense. If that is
I think this was because a receiver had to be sending for it to be able to send
RTCP and REMB. Is that no longer the case?

[commit-bot: I haz the power, 2017-03-17 12:45:33]

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

[commit-bot: I haz the power, 2017-03-17 12:45:33]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-17 12:49:35]

https://codereview.webrtc.org/2740163002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2740163002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:349: // this method for a
receive-only stream, which makes little sense. If that is
On 2017/03/17 12:31:29, stefan-webrtc wrote:
> I think this was because a receiver had to be sending for it to be able to
send
> RTCP and REMB. Is that no longer the case?

It looks like this is the only call to rtcp_sender_.SetSendingStatus, and
RTCPSender::sending_ is off by default. So my guess is that nothing interesting
happens for a receive-only stream.

Then I'm assuming that SetSendingStatus(true) should never be called on a
receive-only stream. But the ModifyVideoConfigs method in
video/end_to_end_tests.cc does precisely that.

[danilchap, 2017-03-17 12:54:36]

On 2017/03/17 12:31:29, stefan-webrtc wrote:
>
https://codereview.webrtc.org/2740163002/diff/160001/webrtc/modules/rtp_rtcp/...
> File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):
> 
>
https://codereview.webrtc.org/2740163002/diff/160001/webrtc/modules/rtp_rtcp/...
> webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:349: // this method for a
> receive-only stream, which makes little sense. If that is
> I think this was because a receiver had to be sending for it to be able to
send
> RTCP and REMB. Is that no longer the case?

remb is independent from sending_ flag
rtcp in general can be turned off with rtcp mode kOff, but not with sending_
flag.
sending_ flag usually used to indicate if module is media sender or media
receiver (e.g. to decided if SR or RR should be generated).

[stefan-webrtc, 2017-03-20 08:08:51]

lgtm after offline discussion

[nisse-webrtc, 2017-03-20 08:51:31]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 08:51:38]

Dry run: 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, 2017-03-20 09:19:53]

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

[commit-bot: I haz the power, 2017-03-20 09:19:54]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-20 10:49:08]

Description was changed from

==========
Don't allocate any RTPSender object for a receive only RtpRtcp module.

BUG=webrtc:7135
==========

to

==========
Don't allocate any RTPSender object for a receive only RtpRtcp module.

This is one step towards separation of send-side and receive-side
processing.

BUG=webrtc:7135
==========

[nisse-webrtc, 2017-03-20 10:50:19]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-20 10:50:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2740163002/#ps200001 (title:
"Rebased.")

[commit-bot: I haz the power, 2017-03-20 10:50:23]

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, 2017-03-20 10:52:39]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490007019269130,
"parent_rev": "7c3da271419ceeef6601c99dcb5cb5400b7157c4", "commit_rev":
"14adba77ecdf6b8ec604363c3717def1e60d7b73"}

[commit-bot: I haz the power, 2017-03-20 10:52:42]

Description was changed from

==========
Don't allocate any RTPSender object for a receive only RtpRtcp module.

This is one step towards separation of send-side and receive-side
processing.

BUG=webrtc:7135
==========

to

==========
Don't allocate any RTPSender object for a receive only RtpRtcp module.

This is one step towards separation of send-side and receive-side
processing.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2740163002
Cr-Commit-Position: refs/heads/master@{#17306}
Committed:
https://chromium.googlesource.com/external/webrtc/+/14adba77ecdf6b8ec604363c3...
==========

[commit-bot: I haz the power, 2017-03-20 10:52:43]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/external/webrtc/+/14adba77ecdf6b8ec604363c3...