Reland of Use RtxReceiveStream.

Reland of Use RtxReceiveStream. (patchset #1 id:1 of https://codereview.webrtc.org/3010983002/ )

Reason for revert:
Intend to fix perf failures and reland.

Original issue's description:
> Revert of Use RtxReceiveStream. (patchset #5 id:80001 of https://codereview.webrtc.org/3008773002/ )
> 
> Reason for revert:
> A few perf tests broken, including
> 
> RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx
> RampUpTest.UpDownUpTransportSequenceNumberRtx
> RampUpTest.UpDownUpTransportSequenceNumberPacketLoss
> 
> 
> Original issue's description:
> > Use RtxReceiveStream.
> > 
> > This also has the beneficial side-effect that when a media stream
> > which is protected by FlexFEC receives an RTX retransmission, the
> > retransmitted media packet is passed into the FlexFEC machinery,
> > which should improve its ability to recover packets via FEC.
> >  
> > BUG=webrtc:7135
> > 
> > Review-Url: https://codereview.webrtc.org/3008773002
> > Cr-Commit-Position: refs/heads/master@{#19649}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152961af27033475f7
> 
> TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7135
> 
> Review-Url: https://codereview.webrtc.org/3010983002
> Cr-Commit-Position: refs/heads/master@{#19653}
> Committed: https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150b50304b38a2847b

TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/3006063002
Cr-Commit-Position: refs/heads/master@{#19715}
Committed: https://chromium.googlesource.com/external/webrtc/+/35713eaf565c0fef07c8afc158d7b8fdf7ec3d78

[nisse-webrtc, 2017-09-04 12:26:32]

Created Reland of Use RtxReceiveStream.

[nisse-webrtc, 2017-09-05 14:44:15]

I've done some debugging, with help from Björn. It seems that we don't get any
RTCP feedback for the RtxStream. On master, 

RTCPReceiver::HandleReportBlock

is called sometimes with report_block.source_ssrc() == 200 (the rtx ssrc), and
sometimes with 100 (the media ssrc).

With this cl, it's only called with report_block.source_ssrc() == 100.

I don't have much clue on how that got broken; as far as I understand,
generation of feedback is driven from Call::NotifyBweOfReceivedPacket, which is
unchanged.

[danilchap, 2017-09-05 15:04:59]

On 2017/09/05 14:44:15, nisse-webrtc wrote:
> I've done some debugging, with help from Björn. It seems that we don't get any
> RTCP feedback for the RtxStream. On master, 
> 
> RTCPReceiver::HandleReportBlock
> 
> is called sometimes with report_block.source_ssrc() == 200 (the rtx ssrc), and
> sometimes with 100 (the media ssrc).
> 
> With this cl, it's only called with report_block.source_ssrc() == 100.
> 
> I don't have much clue on how that got broken; as far as I understand,
> generation of feedback is driven from Call::NotifyBweOfReceivedPacket, which
is
> unchanged.

NotifyBweOfReceivedPacket handles rtcp::TransportFeedback

ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
ReportBlocks.
Those are generated by ReceiveStatistics class - try to find where you miss
passing incoming packets to same ReceiveStatistics object you pass packets
before.

[brandtr, 2017-09-05 15:37:03]

On 2017/09/05 15:04:59, danilchap wrote:
> On 2017/09/05 14:44:15, nisse-webrtc wrote:
> > I've done some debugging, with help from Björn. It seems that we don't get
any
> > RTCP feedback for the RtxStream. On master, 
> > 
> > RTCPReceiver::HandleReportBlock
> > 
> > is called sometimes with report_block.source_ssrc() == 200 (the rtx ssrc),
and
> > sometimes with 100 (the media ssrc).
> > 
> > With this cl, it's only called with report_block.source_ssrc() == 100.
> > 
> > I don't have much clue on how that got broken; as far as I understand,
> > generation of feedback is driven from Call::NotifyBweOfReceivedPacket, which
> is
> > unchanged.
> 
> NotifyBweOfReceivedPacket handles rtcp::TransportFeedback
> 
> ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
> ReportBlocks.
> Those are generated by ReceiveStatistics class - try to find where you miss
> passing incoming packets to same ReceiveStatistics object you pass packets
> before.

I think that Danil is right on the money.

Before this CL, incoming RTX packets would hit this line:
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...,
and RTCP feedback would be generated by the correspondingly connected RTP
module.

After this CL, the RTX packets go to the RtxReceiveStream directly, which does
not have a ReceiveStatistics object and no RTP module.

I guess the rampup tests broke because RTX is used instead of padding in the
rampup phases?

As a reference, this is how RTCP RRs are generated by the FlexfecReceiveStream:
https://cs.chromium.org/chromium/src/third_party/webrtc/call/flexfec_receive_...
Note that the FlexfecReceiveStream does not receive any RTCP SRs, so the RTT
cannot be calculated for the FlexFEC SSRC. This should probably be wired up.

[nisse-webrtc, 2017-09-06 07:32:59]

On 2017/09/05 15:37:03, brandtr wrote:
> On 2017/09/05 15:04:59, danilchap wrote:
> > On 2017/09/05 14:44:15, nisse-webrtc wrote:
> > > I've done some debugging, with help from Björn. It seems that we don't get
> any
> > > RTCP feedback for the RtxStream. On master, 
> > > 
> > > RTCPReceiver::HandleReportBlock
> > > 
> > > is called sometimes with report_block.source_ssrc() == 200 (the rtx ssrc),
> and
> > > sometimes with 100 (the media ssrc).
> > > 
> > > With this cl, it's only called with report_block.source_ssrc() == 100.
> > > 
> > > I don't have much clue on how that got broken; as far as I understand,
> > > generation of feedback is driven from Call::NotifyBweOfReceivedPacket,
which
> > is
> > > unchanged.
> > 
> > NotifyBweOfReceivedPacket handles rtcp::TransportFeedback
> > 
> > ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
> > ReportBlocks.
> > Those are generated by ReceiveStatistics class - try to find where you miss
> > passing incoming packets to same ReceiveStatistics object you pass packets
> > before.
> 
> I think that Danil is right on the money.
> 
> Before this CL, incoming RTX packets would hit this line:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...,
> and RTCP feedback would be generated by the correspondingly connected RTP
> module.
> 
> After this CL, the RTX packets go to the RtxReceiveStream directly, which does
> not have a ReceiveStatistics object and no RTP module.
> 
> I guess the rampup tests broke because RTX is used instead of padding in the
> rampup phases?
> 
> As a reference, this is how RTCP RRs are generated by the
FlexfecReceiveStream:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/flexfec_receive_...
> Note that the FlexfecReceiveStream does not receive any RTCP SRs, so the RTT
> cannot be calculated for the FlexFEC SSRC. This should probably be wired up.

Thanks for the explanation. One version of the cl did send the rtx packets to
the ReceiveStatistics object, but *after* decapsulation, meaning it was
bookkeeped with the wrong ssrc, and broke flexfec tests.

For an easy fix, can we share a ReceiveStatistics instance with the
corresponding media stream (that's how it worked before)? The alternative would
be to let the RTX receiver create it's own instance and its own RtpRtcpModule,
which seems like a detour.

Longer term, could we move ReceiveStatistics to RtpStreamReceiverController? It
gets complete packet headers, so maybe we don't need one instance per receive
stream?

[danilchap, 2017-09-06 07:41:20]

On 2017/09/06 07:32:59, nisse-webrtc wrote:
> On 2017/09/05 15:37:03, brandtr wrote:
> > On 2017/09/05 15:04:59, danilchap wrote:
> > > On 2017/09/05 14:44:15, nisse-webrtc wrote:
> > > > I've done some debugging, with help from Björn. It seems that we don't
get
> > any
> > > > RTCP feedback for the RtxStream. On master, 
> > > > 
> > > > RTCPReceiver::HandleReportBlock
> > > > 
> > > > is called sometimes with report_block.source_ssrc() == 200 (the rtx
ssrc),
> > and
> > > > sometimes with 100 (the media ssrc).
> > > > 
> > > > With this cl, it's only called with report_block.source_ssrc() == 100.
> > > > 
> > > > I don't have much clue on how that got broken; as far as I understand,
> > > > generation of feedback is driven from Call::NotifyBweOfReceivedPacket,
> which
> > > is
> > > > unchanged.
> > > 
> > > NotifyBweOfReceivedPacket handles rtcp::TransportFeedback
> > > 
> > > ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
> > > ReportBlocks.
> > > Those are generated by ReceiveStatistics class - try to find where you
miss
> > > passing incoming packets to same ReceiveStatistics object you pass packets
> > > before.
> > 
> > I think that Danil is right on the money.
> > 
> > Before this CL, incoming RTX packets would hit this line:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...,
> > and RTCP feedback would be generated by the correspondingly connected RTP
> > module.
> > 
> > After this CL, the RTX packets go to the RtxReceiveStream directly, which
does
> > not have a ReceiveStatistics object and no RTP module.
> > 
> > I guess the rampup tests broke because RTX is used instead of padding in the
> > rampup phases?
> > 
> > As a reference, this is how RTCP RRs are generated by the
> FlexfecReceiveStream:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/flexfec_receive_...
> > Note that the FlexfecReceiveStream does not receive any RTCP SRs, so the RTT
> > cannot be calculated for the FlexFEC SSRC. This should probably be wired up.
> 
> Thanks for the explanation. One version of the cl did send the rtx packets to
> the ReceiveStatistics object, but *after* decapsulation, meaning it was
> bookkeeped with the wrong ssrc, and broke flexfec tests.
> 
> For an easy fix, can we share a ReceiveStatistics instance with the
> corresponding media stream (that's how it worked before)? The alternative
would
> be to let the RTX receiver create it's own instance and its own RtpRtcpModule,
> which seems like a detour.
> 
> Longer term, could we move ReceiveStatistics to RtpStreamReceiverController?
It
> gets complete packet headers, so maybe we don't need one instance per receive
> stream?

easy fix - looks good.

creating own RtpRtcpModule for rtx likely will create new unexpected problems.

longer term solution - yes, once there will be single rtcp module, there also
should be single receive statistics that can handle packets even before
demuxing.
But for longer solution to work need to review how packets are passed to it -
that could be a good next step to figure out (specially how 'retransmitted' flag
is calculated)

[brandtr, 2017-09-06 07:55:23]

On 2017/09/06 07:41:20, danilchap wrote:
> On 2017/09/06 07:32:59, nisse-webrtc wrote:
> > On 2017/09/05 15:37:03, brandtr wrote:
> > > On 2017/09/05 15:04:59, danilchap wrote:
> > > > On 2017/09/05 14:44:15, nisse-webrtc wrote:
> > > > > I've done some debugging, with help from Björn. It seems that we don't
> get
> > > any
> > > > > RTCP feedback for the RtxStream. On master, 
> > > > > 
> > > > > RTCPReceiver::HandleReportBlock
> > > > > 
> > > > > is called sometimes with report_block.source_ssrc() == 200 (the rtx
> ssrc),
> > > and
> > > > > sometimes with 100 (the media ssrc).
> > > > > 
> > > > > With this cl, it's only called with report_block.source_ssrc() == 100.
> > > > > 
> > > > > I don't have much clue on how that got broken; as far as I understand,
> > > > > generation of feedback is driven from Call::NotifyBweOfReceivedPacket,
> > which
> > > > is
> > > > > unchanged.
> > > > 
> > > > NotifyBweOfReceivedPacket handles rtcp::TransportFeedback
> > > > 
> > > > ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
> > > > ReportBlocks.
> > > > Those are generated by ReceiveStatistics class - try to find where you
> miss
> > > > passing incoming packets to same ReceiveStatistics object you pass
packets
> > > > before.
> > > 
> > > I think that Danil is right on the money.
> > > 
> > > Before this CL, incoming RTX packets would hit this line:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...,
> > > and RTCP feedback would be generated by the correspondingly connected RTP
> > > module.
> > > 
> > > After this CL, the RTX packets go to the RtxReceiveStream directly, which
> does
> > > not have a ReceiveStatistics object and no RTP module.
> > > 
> > > I guess the rampup tests broke because RTX is used instead of padding in
the
> > > rampup phases?
> > > 
> > > As a reference, this is how RTCP RRs are generated by the
> > FlexfecReceiveStream:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/flexfec_receive_...
> > > Note that the FlexfecReceiveStream does not receive any RTCP SRs, so the
RTT
> > > cannot be calculated for the FlexFEC SSRC. This should probably be wired
up.
> > 
> > Thanks for the explanation. One version of the cl did send the rtx packets
to
> > the ReceiveStatistics object, but *after* decapsulation, meaning it was
> > bookkeeped with the wrong ssrc, and broke flexfec tests.
> > 
> > For an easy fix, can we share a ReceiveStatistics instance with the
> > corresponding media stream (that's how it worked before)? The alternative
> would
> > be to let the RTX receiver create it's own instance and its own
RtpRtcpModule,
> > which seems like a detour.
> > 
> > Longer term, could we move ReceiveStatistics to RtpStreamReceiverController?
> It
> > gets complete packet headers, so maybe we don't need one instance per
receive
> > stream?
> 
> easy fix - looks good.
> 
> creating own RtpRtcpModule for rtx likely will create new unexpected problems.
> 
> longer term solution - yes, once there will be single rtcp module, there also
> should be single receive statistics that can handle packets even before
> demuxing.
> But for longer solution to work need to review how packets are passed to it -
> that could be a good next step to figure out (specially how 'retransmitted'
flag
> is calculated)

Yep, easy fix sounds good.

Long-term solution sounds good too. Then we could remove the ReceiveStatistics
and RTCP module from FlexfecReceiveStream, and maybe remove that whole class.

[nisse-webrtc, 2017-09-06 08:50:27]

PTAL.

webrtc_perf_tests --gtest_filter='RampUpTest.*'

now work when I run it locally.

[danilchap, 2017-09-06 10:29:52]

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
File webrtc/call/rtx_receive_stream.cc (right):

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
webrtc/call/rtx_receive_stream.cc:42: const bool kNotRetransmitted = false;
to avoid confusion about double negation (not retransmitted is false, can it
mean it is re-transmitted?)

suggest use inline comment to document the parameter:
->IncomingPacket(header, rtx_packet.size(), /*retransmitted=*/false);

or, if you prefer constant, negate later:
const bool kRetransmitted = true;
// RTX packets are never retransmitted.
->IncomingPacket(header, rtx_packet.size(), !kRetransmitted);

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
File webrtc/call/rtx_receive_stream.h (right):

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
webrtc/call/rtx_receive_stream.h:29: // TODO(nisse): Should we make this
argument
no, would rather see TODO to remove it once receive statistic will be common for
all streams:

it is not really job of the Rtx Stream to report incoming incoming packets to
statistic, it just simplest place to insert it for now.

[nisse-webrtc, 2017-09-06 11:25:10]

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
File webrtc/call/rtx_receive_stream.cc (right):

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
webrtc/call/rtx_receive_stream.cc:42: const bool kNotRetransmitted = false;
On 2017/09/06 10:29:52, danilchap wrote:
> to avoid confusion about double negation (not retransmitted is false, can it
> mean it is re-transmitted?)
> 
> suggest use inline comment to document the parameter:
> ->IncomingPacket(header, rtx_packet.size(), /*retransmitted=*/false);
> 
> or, if you prefer constant, negate later:
> const bool kRetransmitted = true;
> // RTX packets are never retransmitted.
> ->IncomingPacket(header, rtx_packet.size(), !kRetransmitted);

I'm following the first suggestion. (The kNotRetransmitted thing was copied from
the corresponding FlexFEC code).

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
File webrtc/call/rtx_receive_stream.h (right):

https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
webrtc/call/rtx_receive_stream.h:29: // TODO(nisse): Should we make this
argument
On 2017/09/06 10:29:52, danilchap wrote:
> no, would rather see TODO to remove it once receive statistic will be common
for
> all streams:
> 
> it is not really job of the Rtx Stream to report incoming incoming packets to
> statistic, it just simplest place to insert it for now.

Done.

[nisse-webrtc, 2017-09-06 11:32:47]

On 2017/09/06 11:25:10, nisse-webrtc wrote:
>
https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
> File webrtc/call/rtx_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
> webrtc/call/rtx_receive_stream.cc:42: const bool kNotRetransmitted = false;
> On 2017/09/06 10:29:52, danilchap wrote:
> > to avoid confusion about double negation (not retransmitted is false, can it
> > mean it is re-transmitted?)
> > 
> > suggest use inline comment to document the parameter:
> > ->IncomingPacket(header, rtx_packet.size(), /*retransmitted=*/false);
> > 
> > or, if you prefer constant, negate later:
> > const bool kRetransmitted = true;
> > // RTX packets are never retransmitted.
> > ->IncomingPacket(header, rtx_packet.size(), !kRetransmitted);
> 
> I'm following the first suggestion. (The kNotRetransmitted thing was copied
from
> the corresponding FlexFEC code).
> 
>
https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
> File webrtc/call/rtx_receive_stream.h (right):
> 
>
https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_...
> webrtc/call/rtx_receive_stream.h:29: // TODO(nisse): Should we make this
> argument
> On 2017/09/06 10:29:52, danilchap wrote:
> > no, would rather see TODO to remove it once receive statistic will be common
> for
> > all streams:
> > 
> > it is not really job of the Rtx Stream to report incoming incoming packets
to
> > statistic, it just simplest place to insert it for now.
> 
> Done.

Ooops, forgot to upload the patchset, coming in a moment.

[nisse-webrtc, 2017-09-06 11:33:37]

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

[commit-bot: I haz the power, 2017-09-06 11:33:50]

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-09-06 11:33:52]

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

[commit-bot: I haz the power, 2017-09-06 11:33:53]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-09-06 11:35:58]

Description was changed from

==========
Reland of Use RtxReceiveStream. (patchset #1 id:1 of
https://codereview.webrtc.org/3010983002/ )

Reason for revert:
Intend to fix perf failures and reland.

Original issue's description:
> Revert of Use RtxReceiveStream. (patchset #5 id:80001 of
https://codereview.webrtc.org/3008773002/ )
> 
> Reason for revert:
> A few perf tests broken, including
> 
> RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx
> RampUpTest.UpDownUpTransportSequenceNumberRtx
> RampUpTest.UpDownUpTransportSequenceNumberPacketLoss
> 
> 
> Original issue's description:
> > Use RtxReceiveStream.
> > 
> > This also has the beneficial side-effect that when a media stream
> > which is protected by FlexFEC receives an RTX retransmission, the
> > retransmitted media packet is passed into the FlexFEC machinery,
> > which should improve its ability to recover packets via FEC.
> >  
> > BUG=webrtc:7135
> > 
> > Review-Url: https://codereview.webrtc.org/3008773002
> > Cr-Commit-Position: refs/heads/master@{#19649}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152...
> 
>
TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7135
> 
> Review-Url: https://codereview.webrtc.org/3010983002
> Cr-Commit-Position: refs/heads/master@{#19653}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150...

TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:7135
==========

to

==========
Reland of Use RtxReceiveStream. (patchset #1 id:1 of
https://codereview.webrtc.org/3010983002/ )

Reason for revert:
Intend to fix perf failures and reland.

Original issue's description:
> Revert of Use RtxReceiveStream. (patchset #5 id:80001 of
https://codereview.webrtc.org/3008773002/ )
> 
> Reason for revert:
> A few perf tests broken, including
> 
> RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx
> RampUpTest.UpDownUpTransportSequenceNumberRtx
> RampUpTest.UpDownUpTransportSequenceNumberPacketLoss
> 
> 
> Original issue's description:
> > Use RtxReceiveStream.
> > 
> > This also has the beneficial side-effect that when a media stream
> > which is protected by FlexFEC receives an RTX retransmission, the
> > retransmitted media packet is passed into the FlexFEC machinery,
> > which should improve its ability to recover packets via FEC.
> >  
> > BUG=webrtc:7135
> > 
> > Review-Url: https://codereview.webrtc.org/3008773002
> > Cr-Commit-Position: refs/heads/master@{#19649}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152...
> 
>
TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7135
> 
> Review-Url: https://codereview.webrtc.org/3010983002
> Cr-Commit-Position: refs/heads/master@{#19653}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150...

TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
BUG=webrtc:7135
==========

[nisse-webrtc, 2017-09-06 11:36:05]

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

[commit-bot: I haz the power, 2017-09-06 11:36:09]

Dry run: CQ is trying da patch.

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

[danilchap, 2017-09-06 11:51:45]

lgtm

[commit-bot: I haz the power, 2017-09-06 12:16:41]

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

[commit-bot: I haz the power, 2017-09-06 12:16:42]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-09-06 12:20:37]

On 2017/09/06 07:41:20, danilchap wrote:
> On 2017/09/06 07:32:59, nisse-webrtc wrote:
> > On 2017/09/05 15:37:03, brandtr wrote:
> > > On 2017/09/05 15:04:59, danilchap wrote:
> > > > On 2017/09/05 14:44:15, nisse-webrtc wrote:
> > > > > I've done some debugging, with help from Björn. It seems that we don't
> get
> > > any
> > > > > RTCP feedback for the RtxStream. On master, 
> > > > > 
> > > > > RTCPReceiver::HandleReportBlock
> > > > > 
> > > > > is called sometimes with report_block.source_ssrc() == 200 (the rtx
> ssrc),
> > > and
> > > > > sometimes with 100 (the media ssrc).
> > > > > 
> > > > > With this cl, it's only called with report_block.source_ssrc() == 100.
> > > > > 
> > > > > I don't have much clue on how that got broken; as far as I understand,
> > > > > generation of feedback is driven from Call::NotifyBweOfReceivedPacket,
> > which
> > > > is
> > > > > unchanged.
> > > > 
> > > > NotifyBweOfReceivedPacket handles rtcp::TransportFeedback
> > > > 
> > > > ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
> > > > ReportBlocks.
> > > > Those are generated by ReceiveStatistics class - try to find where you
> miss
> > > > passing incoming packets to same ReceiveStatistics object you pass
packets
> > > > before.
> > > 
> > > I think that Danil is right on the money.
> > > 
> > > Before this CL, incoming RTX packets would hit this line:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...,
> > > and RTCP feedback would be generated by the correspondingly connected RTP
> > > module.
> > > 
> > > After this CL, the RTX packets go to the RtxReceiveStream directly, which
> does
> > > not have a ReceiveStatistics object and no RTP module.
> > > 
> > > I guess the rampup tests broke because RTX is used instead of padding in
the
> > > rampup phases?
> > > 
> > > As a reference, this is how RTCP RRs are generated by the
> > FlexfecReceiveStream:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/flexfec_receive_...
> > > Note that the FlexfecReceiveStream does not receive any RTCP SRs, so the
RTT
> > > cannot be calculated for the FlexFEC SSRC. This should probably be wired
up.
> > 
> > Thanks for the explanation. One version of the cl did send the rtx packets
to
> > the ReceiveStatistics object, but *after* decapsulation, meaning it was
> > bookkeeped with the wrong ssrc, and broke flexfec tests.
> > 
> > For an easy fix, can we share a ReceiveStatistics instance with the
> > corresponding media stream (that's how it worked before)? The alternative
> would
> > be to let the RTX receiver create it's own instance and its own
RtpRtcpModule,
> > which seems like a detour.
> > 
> > Longer term, could we move ReceiveStatistics to RtpStreamReceiverController?
> It
> > gets complete packet headers, so maybe we don't need one instance per
receive
> > stream?
> 
> easy fix - looks good.
> 
> creating own RtpRtcpModule for rtx likely will create new unexpected problems.

Yes, there's a big risk with this since we'll get separate RTCPs for rtx and
regular video, and the rtx report will be based on fewer packets. Currently
there's no weighting them together based on number of packets if they are
reported in separate RTCPs, so the BWE may take bad decisions based on an RTCP
based on few packets. When they're reported within the same RTCP (two report
blocks) they are correctly weighted.

> 
> longer term solution - yes, once there will be single rtcp module, there also
> should be single receive statistics that can handle packets even before
> demuxing.
> But for longer solution to work need to review how packets are passed to it -
> that could be a good next step to figure out (specially how 'retransmitted'
flag
> is calculated)

[stefan-webrtc, 2017-09-06 12:23:53]

lgtm

[brandtr, 2017-09-06 13:34:10]

On 2017/09/06 12:20:37, stefan-webrtc wrote:
> On 2017/09/06 07:41:20, danilchap wrote:
> > On 2017/09/06 07:32:59, nisse-webrtc wrote:
> > > On 2017/09/05 15:37:03, brandtr wrote:
> > > > On 2017/09/05 15:04:59, danilchap wrote:
> > > > > On 2017/09/05 14:44:15, nisse-webrtc wrote:
> > > > > > I've done some debugging, with help from Björn. It seems that we
don't
> > get
> > > > any
> > > > > > RTCP feedback for the RtxStream. On master, 
> > > > > > 
> > > > > > RTCPReceiver::HandleReportBlock
> > > > > > 
> > > > > > is called sometimes with report_block.source_ssrc() == 200 (the rtx
> > ssrc),
> > > > and
> > > > > > sometimes with 100 (the media ssrc).
> > > > > > 
> > > > > > With this cl, it's only called with report_block.source_ssrc() ==
100.
> > > > > > 
> > > > > > I don't have much clue on how that got broken; as far as I
understand,
> > > > > > generation of feedback is driven from
Call::NotifyBweOfReceivedPacket,
> > > which
> > > > > is
> > > > > > unchanged.
> > > > > 
> > > > > NotifyBweOfReceivedPacket handles rtcp::TransportFeedback
> > > > > 
> > > > > ReportBlocks comes from classic feedback: rtcp::ReceiverReport with
> > > > > ReportBlocks.
> > > > > Those are generated by ReceiveStatistics class - try to find where you
> > miss
> > > > > passing incoming packets to same ReceiveStatistics object you pass
> packets
> > > > > before.
> > > > 
> > > > I think that Danil is right on the money.
> > > > 
> > > > Before this CL, incoming RTX packets would hit this line:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea...,
> > > > and RTCP feedback would be generated by the correspondingly connected
RTP
> > > > module.
> > > > 
> > > > After this CL, the RTX packets go to the RtxReceiveStream directly,
which
> > does
> > > > not have a ReceiveStatistics object and no RTP module.
> > > > 
> > > > I guess the rampup tests broke because RTX is used instead of padding in
> the
> > > > rampup phases?
> > > > 
> > > > As a reference, this is how RTCP RRs are generated by the
> > > FlexfecReceiveStream:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/call/flexfec_receive_...
> > > > Note that the FlexfecReceiveStream does not receive any RTCP SRs, so the
> RTT
> > > > cannot be calculated for the FlexFEC SSRC. This should probably be wired
> up.
> > > 
> > > Thanks for the explanation. One version of the cl did send the rtx packets
> to
> > > the ReceiveStatistics object, but *after* decapsulation, meaning it was
> > > bookkeeped with the wrong ssrc, and broke flexfec tests.
> > > 
> > > For an easy fix, can we share a ReceiveStatistics instance with the
> > > corresponding media stream (that's how it worked before)? The alternative
> > would
> > > be to let the RTX receiver create it's own instance and its own
> RtpRtcpModule,
> > > which seems like a detour.
> > > 
> > > Longer term, could we move ReceiveStatistics to
RtpStreamReceiverController?
> > It
> > > gets complete packet headers, so maybe we don't need one instance per
> receive
> > > stream?
> > 
> > easy fix - looks good.
> > 
> > creating own RtpRtcpModule for rtx likely will create new unexpected
problems.
> 
> Yes, there's a big risk with this since we'll get separate RTCPs for rtx and
> regular video, and the rtx report will be based on fewer packets. Currently
> there's no weighting them together based on number of packets if they are
> reported in separate RTCPs, so the BWE may take bad decisions based on an RTCP
> based on few packets. When they're reported within the same RTCP (two report
> blocks) they are correctly weighted.

Another place where FlexFEC would benefit from sharing the media
ReceiveStatistics.

> 
> > 
> > longer term solution - yes, once there will be single rtcp module, there
also
> > should be single receive statistics that can handle packets even before
> > demuxing.
> > But for longer solution to work need to review how packets are passed to it
-
> > that could be a good next step to figure out (specially how 'retransmitted'
> flag
> > is calculated)

[brandtr, 2017-09-06 13:35:50]

lgtm

[nisse-webrtc, 2017-09-06 14:03:05]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-09-06 14:03:11]

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-09-06 14:03:17]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1504706585133030,
"parent_rev": "3c6abd200cb9865402c96466fafe6867730cb2be", "commit_rev":
"35713eaf565c0fef07c8afc158d7b8fdf7ec3d78"}

[commit-bot: I haz the power, 2017-09-06 14:03:23]

Description was changed from

==========
Reland of Use RtxReceiveStream. (patchset #1 id:1 of
https://codereview.webrtc.org/3010983002/ )

Reason for revert:
Intend to fix perf failures and reland.

Original issue's description:
> Revert of Use RtxReceiveStream. (patchset #5 id:80001 of
https://codereview.webrtc.org/3008773002/ )
> 
> Reason for revert:
> A few perf tests broken, including
> 
> RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx
> RampUpTest.UpDownUpTransportSequenceNumberRtx
> RampUpTest.UpDownUpTransportSequenceNumberPacketLoss
> 
> 
> Original issue's description:
> > Use RtxReceiveStream.
> > 
> > This also has the beneficial side-effect that when a media stream
> > which is protected by FlexFEC receives an RTX retransmission, the
> > retransmitted media packet is passed into the FlexFEC machinery,
> > which should improve its ability to recover packets via FEC.
> >  
> > BUG=webrtc:7135
> > 
> > Review-Url: https://codereview.webrtc.org/3008773002
> > Cr-Commit-Position: refs/heads/master@{#19649}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152...
> 
>
TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7135
> 
> Review-Url: https://codereview.webrtc.org/3010983002
> Cr-Commit-Position: refs/heads/master@{#19653}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150...

TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
BUG=webrtc:7135
==========

to

==========
Reland of Use RtxReceiveStream. (patchset #1 id:1 of
https://codereview.webrtc.org/3010983002/ )

Reason for revert:
Intend to fix perf failures and reland.

Original issue's description:
> Revert of Use RtxReceiveStream. (patchset #5 id:80001 of
https://codereview.webrtc.org/3008773002/ )
> 
> Reason for revert:
> A few perf tests broken, including
> 
> RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx
> RampUpTest.UpDownUpTransportSequenceNumberRtx
> RampUpTest.UpDownUpTransportSequenceNumberPacketLoss
> 
> 
> Original issue's description:
> > Use RtxReceiveStream.
> > 
> > This also has the beneficial side-effect that when a media stream
> > which is protected by FlexFEC receives an RTX retransmission, the
> > retransmitted media packet is passed into the FlexFEC machinery,
> > which should improve its ability to recover packets via FEC.
> >  
> > BUG=webrtc:7135
> > 
> > Review-Url: https://codereview.webrtc.org/3008773002
> > Cr-Commit-Position: refs/heads/master@{#19649}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152...
> 
>
TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7135
> 
> Review-Url: https://codereview.webrtc.org/3010983002
> Cr-Commit-Position: refs/heads/master@{#19653}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150...

TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/3006063002
Cr-Commit-Position: refs/heads/master@{#19715}
Committed:
https://chromium.googlesource.com/external/webrtc/+/35713eaf565c0fef07c8afc15...
==========

[commit-bot: I haz the power, 2017-09-06 14:03:24]

Committed patchset #5 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/35713eaf565c0fef07c8afc15...

[nisse-webrtc, 2017-09-08 12:24:36]

A revert of this CL (patchset #5 id:320001) has been created in
https://codereview.webrtc.org/3007303002/ by nisse@webrtc.org.

The reason for reverting is: This change appears to break ulpfec, with severe
regressions, e.g., for webrtc_perf_test FullStackTest.ForemanCifPlr5Ulpfec.