Only compare sequence numbers from the same SSRC in ForwardErrorCorrection.

Only compare sequence numbers from the same SSRC in ForwardErrorCorrection.

Prior to this CL, the ForwardErrorCorrection state would be reset whenever
the difference in sequence numbers of the last recovered media packet
and the new packet (media or FEC) was too large. This comparison did not
take into account that FlexFEC uses a different SSRC for the FEC packets,
meaning that the the state would be reset very frequently when FlexFEC
is used. This should not have led to any major problems, except for a
decreased decoding efficiency.

This CL verifies that whenever we compare sequence numbers in
ForwardErrorCorrection, they do indeed belong to the same SSRC.

BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2893293003
Cr-Commit-Position: refs/heads/master@{#18399}
Committed: https://chromium.googlesource.com/external/webrtc/+/1476a9d789db03457595cf7dbea7e362972f2a4d

[brandtr, 2017-05-22 13:55:40]

brandtr@webrtc.org changed reviewers:
+ stefan@webrtc.org

[brandtr, 2017-05-22 13:55:41]

Stefan, would you mind taking a look at this fix? You may have been involved in
writing these sequence number checks in the first place, based on nearby TODOs.

https://codereview.webrtc.org/2893293003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (left):

https://codereview.webrtc.org/2893293003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:694: if (seq_num_diff
> max_media_packets) {
This condition is very often true when using FlexFEC, since the sequence numbers
for media and FEC belong to different spaces.

[brandtr, 2017-05-22 13:58:23]

Add support for FlexFEC in FecTest.

[holmer, 2017-05-23 16:47:30]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-05-23 16:47:32]

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:493: // RED+ULPFEC is
used.
So there can't be a wraparound between the second packet in
received_fec_packets_ and received_packet, because they're sorted, right?

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:500:
received_fec_packets_.pop_front();
It's not clear to me that it's necessarily enough to pop one packet here?

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:722:
ResetState(recovered_packets);
Is it worth adding some logging here?

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (left):

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:964:
FecRecoveryWithLossAndDifferentMediaAndFlexfecSsrcs) {
This is now covered by tests above?

[brandtr, 2017-05-30 12:04:10]

Patchset #3 (id:40001) has been deleted

[brandtr, 2017-05-30 13:12:49]

brandtr@webrtc.org changed reviewers:
- holmer@google.com

[brandtr, 2017-05-30 13:12:50]

Please take another look.

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:493: // RED+ULPFEC is
used.
On 2017/05/23 16:47:31, holmer wrote:
> So there can't be a wraparound between the second packet in
> received_fec_packets_ and received_packet, because they're sorted, right?

I think there could be? For example, let

received_fec_packets_ = {65533, 65534}
received_packet = 0.

Then there wrap is between |received_fec_packets_[0]| and |received_packet|, but
also between |received_fec_packets_[1]| and |received_packet|.

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:500:
received_fec_packets_.pop_front();
On 2017/05/23 16:47:31, holmer wrote:
> It's not clear to me that it's necessarily enough to pop one packet here?

I guess it's a trade-off? We will be able to handle the same number of outlier
packets as the number of packets that we pop here. Assuming that we don't have
any outlier packets, I believe the current code will cap the number of packets
in |received_fec_packets_| to 1/4 of the sequence number space, by construction.
(0x3fff = 2^14 - 1).

If we were to replace this with a loop, we would discard all existing packets
whenever we hit a wrap-around situation. That would happen in the example above,
for instance.

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:722:
ResetState(recovered_packets);
On 2017/05/23 16:47:31, holmer wrote:
> Is it worth adding some logging here?

Yep.

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (left):

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:964:
FecRecoveryWithLossAndDifferentMediaAndFlexfecSsrcs) {
On 2017/05/23 16:47:31, holmer wrote:
> This is now covered by tests above?

Yes. Before this CL, this test was the only one that tested having different
SSRCs on FEC and media. After this CL, this is the case for all the tests. (See
line 130.) That means, specifically, that this test is made redundant by this
one:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...

[brandtr, 2017-05-31 09:19:40]

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2893293003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:500:
received_fec_packets_.pop_front();
On 2017/05/30 13:12:49, brandtr wrote:
> On 2017/05/23 16:47:31, holmer wrote:
> > It's not clear to me that it's necessarily enough to pop one packet here?
> 
> I guess it's a trade-off? We will be able to handle the same number of outlier
> packets as the number of packets that we pop here. Assuming that we don't have
> any outlier packets, I believe the current code will cap the number of packets
> in |received_fec_packets_| to 1/4 of the sequence number space, by
construction.
> (0x3fff = 2^14 - 1).
> 
> If we were to replace this with a loop, we would discard all existing packets
> whenever we hit a wrap-around situation. That would happen in the example
above,
> for instance.

Never mind, I didn't consider the cast happening when assigning to
|seq_num_diff|, invalidating my statement above.

[brandtr, 2017-05-31 11:10:13]

Patchset #4 (id:80001) has been deleted

[brandtr, 2017-05-31 11:17:31]

Added loop.

[brandtr, 2017-06-01 12:07:01]

Typo fix.

[holmer, 2017-06-01 14:08:42]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-06-01 14:08:43]

lgtm

[brandtr, 2017-06-01 14:23:05]

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

[commit-bot: I haz the power, 2017-06-01 14:23:10]

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-06-01 15:01:50]

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

[commit-bot: I haz the power, 2017-06-01 15:01:51]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-06-02 07:51:08]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-06-02 07:51:18]

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-06-02 07:51:19]

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

[commit-bot: I haz the power, 2017-06-02 07:51:20]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[stefan-webrtc, 2017-06-02 07:54:49]

lgtm

[brandtr, 2017-06-02 07:56:11]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-06-02 07:56:19]

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-06-02 07:58:12]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496390171144680,
"parent_rev": "cbd3ee3e7793b600059f724b7a30b79b20ec56fd", "commit_rev":
"1476a9d789db03457595cf7dbea7e362972f2a4d"}

[commit-bot: I haz the power, 2017-06-02 07:58:16]

Description was changed from

==========
Only compare sequence numbers from the same SSRC in ForwardErrorCorrection.

Prior to this CL, the ForwardErrorCorrection state would be reset whenever
the difference in sequence numbers of the last recovered media packet
and the new packet (media or FEC) was too large. This comparison did not
take into account that FlexFEC uses a different SSRC for the FEC packets,
meaning that the the state would be reset very frequently when FlexFEC
is used. This should not have led to any major problems, except for a
decreased decoding efficiency.

This CL verifies that whenever we compare sequence numbers in
ForwardErrorCorrection, they do indeed belong to the same SSRC.

BUG=webrtc:5654
==========

to

==========
Only compare sequence numbers from the same SSRC in ForwardErrorCorrection.

Prior to this CL, the ForwardErrorCorrection state would be reset whenever
the difference in sequence numbers of the last recovered media packet
and the new packet (media or FEC) was too large. This comparison did not
take into account that FlexFEC uses a different SSRC for the FEC packets,
meaning that the the state would be reset very frequently when FlexFEC
is used. This should not have led to any major problems, except for a
decreased decoding efficiency.

This CL verifies that whenever we compare sequence numbers in
ForwardErrorCorrection, they do indeed belong to the same SSRC.

BUG=webrtc:5654

Review-Url: https://codereview.webrtc.org/2893293003
Cr-Commit-Position: refs/heads/master@{#18399}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1476a9d789db03457595cf7db...
==========

[commit-bot: I haz the power, 2017-06-02 07:58:17]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/1476a9d789db03457595cf7db...

[brandtr, 2017-06-05 13:58:19]

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

The reason for reverting is: Breaks fuzzer..

[Taylor Brandstetter, 2017-06-05 22:19:08]

On 2017/06/05 13:58:19, brandtr wrote:
> A revert of this CL (patchset #5 id:120001) has been created in
> https://codereview.webrtc.org/2919313005/ by mailto:brandtr@webrtc.org.
> 
> The reason for reverting is: Breaks fuzzer..

Note that this also broke a couple buildbots (or at least it's my best guess for
the culprit).

https://chromium-swarm.appspot.com/task?id=3691553c47211510&refresh=10&show_r...
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fclient.webrtc%2FAndroid3...

[brandtr, 2017-06-07 09:13:57]

On 2017/06/05 22:19:08, Taylor Brandstetter wrote:
> On 2017/06/05 13:58:19, brandtr wrote:
> > A revert of this CL (patchset #5 id:120001) has been created in
> > https://codereview.webrtc.org/2919313005/ by mailto:brandtr@webrtc.org.
> > 
> > The reason for reverting is: Breaks fuzzer..
> 
> Note that this also broke a couple buildbots (or at least it's my best guess
for
> the culprit).
> 
>
https://chromium-swarm.appspot.com/task?id=3691553c47211510&refresh=10&show_r...
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fclient.webrtc%2FAndroid3...

Yes, I saw something similar on a bot too. Turns out that receive SSRCs are not
always well-defined in the RTP receiver, which may hit the DCHECK that I added.