Reland of Only compare sequence numbers from the same SSRC in ForwardErrorCorrection.

Reland of Only compare sequence numbers from the same SSRC in ForwardErrorCorrection. (patchset #1 id:1 of https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Fix RtpStreamReceiver to not recover RTX packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in ForwardErrorCorrection. (patchset #5 id:120001 of https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed: https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75ecc5e34c7ff6274f

R=stefan@webrtc.org
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2918333002
Cr-Commit-Position: refs/heads/master@{#18827}
Committed: https://chromium.googlesource.com/external/webrtc/+/d726a3f48745a01a74509022bd2e2f6a7e548ea7

[brandtr, 2017-06-05 14:27:41]

Created Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection.

[brandtr, 2017-06-06 06:47:51]

Patchset #2 (id:170001) has been deleted

[brandtr, 2017-06-06 06:48:14]

Description was changed from

==========
Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #1 id:1 of
https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Do not accept packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #5 id:120001 of
https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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...
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75e...

TBR=stefan@webrtc.org,holmer@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5654
==========

to

==========
Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #1 id:1 of
https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Do not accept packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #5 id:120001 of
https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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...
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75e...

R=stefan@webrtc.org
BUG=webrtc:5654
==========

[brandtr, 2017-06-09 11:49:21]

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

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

Dry run: CQ is trying da patch.

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

[brandtr, 2017-06-09 11:49:32]

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

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

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

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

Dry run: Try jobs failed on following builders:
  ios_api_framework on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/25304)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/22149)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/26379)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17951)

[brandtr, 2017-06-09 11:53:09]

Patchset #2 (id:190001) has been deleted

[brandtr, 2017-06-09 11:53:11]

Rebase.

[brandtr, 2017-06-09 11:54:13]

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

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

Dry run: CQ is trying da patch.

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

[brandtr, 2017-06-09 11:59:14]

Description was changed from

==========
Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #1 id:1 of
https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Do not accept packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #5 id:120001 of
https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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...
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75e...

R=stefan@webrtc.org
BUG=webrtc:5654
==========

to

==========
Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #1 id:1 of
https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Fix RtpStreamReceiver to not recover RTX packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #5 id:120001 of
https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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...
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75e...

R=stefan@webrtc.org
BUG=webrtc:5654
==========

[commit-bot: I haz the power, 2017-06-09 12:31:48]

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

[commit-bot: I haz the power, 2017-06-09 12:31:48]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-06-09 13:02:38]

Patchset #3 (id:230001) has been deleted

[stefan-webrtc, 2017-06-16 13:48:35]

lgtm

[brandtr, 2017-06-20 11:39:25]

Rebase.

[brandtr, 2017-06-21 13:41:07]

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

[commit-bot: I haz the power, 2017-06-21 13:41:18]

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-21 14:21:28]

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

[commit-bot: I haz the power, 2017-06-21 14:21:29]

Dry run: Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/25202)

[brandtr, 2017-06-22 12:29:33]

Patchset #4 (id:270001) has been deleted

[brandtr, 2017-06-22 12:29:37]

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

[commit-bot: I haz the power, 2017-06-22 12:29:49]

Dry run: CQ is trying da patch.

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

[brandtr, 2017-06-22 12:41:11]

Patchset #4 (id:290001) has been deleted

[brandtr, 2017-06-22 12:45:50]

Patchset #4 (id:310001) has been deleted

[brandtr, 2017-06-22 12:54:49]

Patchset #4 (id:330001) has been deleted

[brandtr, 2017-06-22 13:38:20]

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

[commit-bot: I haz the power, 2017-06-22 13:38:26]

Dry run: CQ is trying da patch.

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

[brandtr, 2017-06-22 14:28:36]

Added SSRC knowledge to the erasure code; please take another look. This should
be refactored sometime, making the erasure code oblivious to RTP stuff like
SSRCs and FEC headers.

[commit-bot: I haz the power, 2017-06-22 15:30:21]

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

[commit-bot: I haz the power, 2017-06-22 15:30:23]

Dry run: This issue passed the CQ dry run.

[holmer, 2017-06-28 13:53:05]

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

[holmer, 2017-06-28 13:53:06]

lgtm

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right):

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:79: LOG(LS_INFO)
Should this be a warning?

[brandtr_google, 2017-06-28 14:01:55]

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

[brandtr_google, 2017-06-28 14:01:56]

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right):

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:79: LOG(LS_INFO)
On 2017/06/28 13:53:06, holmer wrote:
> Should this be a warning?

I was thinking that could be a bit spammy. If we receive one of these packets,
we might receive several.

[brandtr, 2017-06-29 08:58:47]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-06-29 08:58:48]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2918333002/#ps350001 (title:
"Let ForwardErrorCorrection be aware of its SSRCs.")

[commit-bot: I haz the power, 2017-06-29 08:58:59]

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-29 09:07:40]

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

[commit-bot: I haz the power, 2017-06-29 09:07:42]

Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/13464)

[brandtr, 2017-06-29 09:31:54]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-06-29 09:32:02]

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-29 09:45:36]

CQ is committing da patch.

Bot data: {"patchset_id": 350001, "attempt_start_ts": 1498728714270640,
"parent_rev": "e4350197ecd85bf1bd47e5f3e205f76239631e00", "commit_rev":
"d726a3f48745a01a74509022bd2e2f6a7e548ea7"}

[commit-bot: I haz the power, 2017-06-29 09:45:43]

Description was changed from

==========
Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #1 id:1 of
https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Fix RtpStreamReceiver to not recover RTX packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #5 id:120001 of
https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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...
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75e...

R=stefan@webrtc.org
BUG=webrtc:5654
==========

to

==========
Reland of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #1 id:1 of
https://codereview.webrtc.org/2919313005/ )

Reason for revert:
Fix RtpStreamReceiver to not recover RTX packets with incorrect SSRC.

Original issue's description:
> Revert of Only compare sequence numbers from the same SSRC in
ForwardErrorCorrection. (patchset #5 id:120001 of
https://codereview.webrtc.org/2893293003/ )
> 
> Reason for revert:
> Breaks fuzzer.
> 
> Original issue's description:
> > 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...
> 
> TBR=stefan@webrtc.org,holmer@google.com
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:5654
> 
> Review-Url: https://codereview.webrtc.org/2919313005
> Cr-Commit-Position: refs/heads/master@{#18446}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/92732ecc5ccc81e17c1dea75e...

R=stefan@webrtc.org
BUG=webrtc:5654

Review-Url: https://codereview.webrtc.org/2918333002
Cr-Commit-Position: refs/heads/master@{#18827}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d726a3f48745a01a74509022b...
==========

[commit-bot: I haz the power, 2017-06-29 09:45:44]

Committed patchset #4 (id:350001) as
https://chromium.googlesource.com/external/webrtc/+/d726a3f48745a01a74509022b...

[holmer, 2017-06-29 10:50:12]

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right):

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:79: LOG(LS_INFO)
On 2017/06/28 14:01:56, brandtr_google wrote:
> On 2017/06/28 13:53:06, holmer wrote:
> > Should this be a warning?
> 
> I was thinking that could be a bit spammy. If we receive one of these packets,
> we might receive several.

But we don't expect to ever do that, right? I'd want to know if my sender sends
broken packets.

You could throttle them if you are worried about frequency.

[brandtr, 2017-06-30 08:24:40]

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right):

https://codereview.webrtc.org/2918333002/diff/350001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:79: LOG(LS_INFO)
On 2017/06/29 10:50:11, holmer wrote:
> On 2017/06/28 14:01:56, brandtr_google wrote:
> > On 2017/06/28 13:53:06, holmer wrote:
> > > Should this be a warning?
> > 
> > I was thinking that could be a bit spammy. If we receive one of these
packets,
> > we might receive several.
> 
> But we don't expect to ever do that, right? I'd want to know if my sender
sends
> broken packets.
> 
> You could throttle them if you are worried about frequency.

Right, this should never happen. Updating the logging severity in
https://codereview.webrtc.org/2965533003/.