Don't send FEC for H.264 with NACK enabled.

Don't send FEC for H.264 with NACK enabled.

The H.264 does not contain picture IDs and are not sufficient to
determine that a packet may be skipped. This causes retransmission
requests for FEC that are currently dropped by the sender (since they
should be redundant).

The receiver is then unable to continue without having the packet gap
filled (unlike VP8/VP9 which moves on since it has a consecutive stream
of picture IDs).

Even if FEC retransmission did work it's a huge waste of bandwidth,
since it just adds additional overhead that has to be unconditionally
transmitted. This bandwidth is better used to send higher-quality
frames.

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

Committed: https://crrev.com/25558ad819b4df41ba51537e26a77480ace1e525
Cr-Commit-Position: refs/heads/master@{#11601}

[pbos-webrtc, 2016-02-11 11:53:50]

PTAL, are you fine with me moving this test to end_to_end_tests.cc as is or
should I change something else with it? I have the test remaining in
video_send_stream_tests.cc now so you can see the diff.

[pbos-webrtc, 2016-02-11 11:53:59]

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

[commit-bot: I haz the power, 2016-02-11 11:54:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1687303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1687303002/1

[commit-bot: I haz the power, 2016-02-11 11:54:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1687303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1687303002/1

[pbos-webrtc, 2016-02-11 11:54:55]

remove redundant code

[pbos-webrtc, 2016-02-11 11:58:14]

add log warning

[stefan-webrtc, 2016-02-12 10:46:27]

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:795: // Configure encoders and decoding with
VP8, generic packetization doesn't
"encoder and decoder"?

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:429:
VideoSendStreamTest::kUlpfecPayloadType;
I personally prefer to configure the sender first and the receivers last, which
I think is what is typically done in this file. Feel free to change to be
aligned.

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:466: // since we'll still have to
re-request FEC packets, efficiently wasting
"effectively wasting"?

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:468: // requests for FEC packets are
dropped.
I think the main issue is that we have to wait for the FEC packets to arrive
before we can decode anything since we can't determine if we were missing
something valuable.

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:427: if (enable_fec || payload_type_red != -1 ||
payload_type_fec != -1) {
Shouldn't this be:
enable_fec && payload_type_red != -1 && payload_type_fec != -1

[pbos-webrtc, 2016-02-12 12:00:27]

PTAL

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:795: // Configure encoders and decoding with
VP8, generic packetization doesn't
On 2016/02/12 10:46:27, stefan-webrtc (holmer) wrote:
> "encoder and decoder"?

encoding and decoding, done.

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:429:
VideoSendStreamTest::kUlpfecPayloadType;
On 2016/02/12 10:46:27, stefan-webrtc (holmer) wrote:
> I personally prefer to configure the sender first and the receivers last,
which
> I think is what is typically done in this file. Feel free to change to be
> aligned.

Done.

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:466: // since we'll still have to
re-request FEC packets, efficiently wasting
On 2016/02/12 10:46:27, stefan-webrtc (holmer) wrote:
> "effectively wasting"?

Done.

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:468: // requests for FEC packets are
dropped.
On 2016/02/12 10:46:27, stefan-webrtc (holmer) wrote:
> I think the main issue is that we have to wait for the FEC packets to arrive
> before we can decode anything since we can't determine if we were missing
> something valuable.

Done.

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1687303002/diff/40001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:427: if (enable_fec || payload_type_red != -1 ||
payload_type_fec != -1) {
On 2016/02/12 10:46:27, stefan-webrtc (holmer) wrote:
> Shouldn't this be:
> enable_fec && payload_type_red != -1 && payload_type_fec != -1

Comment updated.

[pbos-webrtc, 2016-02-12 12:00:33]

feedback

[stefan-webrtc, 2016-02-12 12:03:19]

lgtm given nit fixed.

https://codereview.webrtc.org/1687303002/diff/60001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1687303002/diff/60001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:417: // Validate payload types. If either RED of FEC
payload types are set then
red or fec

[pbos-webrtc, 2016-02-12 12:34:44]

RED or FEC

[pbos-webrtc, 2016-02-12 12:34:48]

https://codereview.webrtc.org/1687303002/diff/60001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1687303002/diff/60001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:417: // Validate payload types. If either RED of FEC
payload types are set then
On 2016/02/12 12:03:19, stefan-webrtc (holmer) wrote:
> red or fec

Done.

[pbos-webrtc, 2016-02-12 12:35:27]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2016-02-12 12:35:27]

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/1687303002/#ps80001 (title:
"RED or FEC")

[commit-bot: I haz the power, 2016-02-12 12:35:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1687303002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1687303002/80001

[pbos-webrtc, 2016-02-12 14:08:43]

Description was changed from

==========
Don't send FEC for H.264 with NACK enabled.

The H.264 does not contain picture IDs and are not sufficient to
determine that a packet may be skipped. This causes retransmission
requests for FEC that are currently dropped by the sender (since they
should be redundant).

The receiver is then unable to continue without having the packet gap
filled (unlike VP8/VP9 which moves on since it has a consecutive stream
of picture IDs).

Even if FEC retransmission did work it's a huge waste of bandwidth,
since it just adds additional overhead that has to be unconditionally
transmitted. This bandwidth is better used to send higher-quality
frames.

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

to

==========
Don't send FEC for H.264 with NACK enabled.

The H.264 does not contain picture IDs and are not sufficient to
determine that a packet may be skipped. This causes retransmission
requests for FEC that are currently dropped by the sender (since they
should be redundant).

The receiver is then unable to continue without having the packet gap
filled (unlike VP8/VP9 which moves on since it has a consecutive stream
of picture IDs).

Even if FEC retransmission did work it's a huge waste of bandwidth,
since it just adds additional overhead that has to be unconditionally
transmitted. This bandwidth is better used to send higher-quality
frames.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/25558ad819b4df41ba51537e2...
==========

[pbos-webrtc, 2016-02-12 14:08:44]

Committed patchset #5 (id:80001) manually as
25558ad819b4df41ba51537e26a77480ace1e525 (presubmit successful).

[commit-bot: I haz the power, 2016-02-12 14:08:48]

Description was changed from

==========
Don't send FEC for H.264 with NACK enabled.

The H.264 does not contain picture IDs and are not sufficient to
determine that a packet may be skipped. This causes retransmission
requests for FEC that are currently dropped by the sender (since they
should be redundant).

The receiver is then unable to continue without having the packet gap
filled (unlike VP8/VP9 which moves on since it has a consecutive stream
of picture IDs).

Even if FEC retransmission did work it's a huge waste of bandwidth,
since it just adds additional overhead that has to be unconditionally
transmitted. This bandwidth is better used to send higher-quality
frames.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/25558ad819b4df41ba51537e2...
==========

to

==========
Don't send FEC for H.264 with NACK enabled.

The H.264 does not contain picture IDs and are not sufficient to
determine that a packet may be skipped. This causes retransmission
requests for FEC that are currently dropped by the sender (since they
should be redundant).

The receiver is then unable to continue without having the packet gap
filled (unlike VP8/VP9 which moves on since it has a consecutive stream
of picture IDs).

Even if FEC retransmission did work it's a huge waste of bandwidth,
since it just adds additional overhead that has to be unconditionally
transmitted. This bandwidth is better used to send higher-quality
frames.

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

Committed: https://crrev.com/25558ad819b4df41ba51537e26a77480ace1e525
Cr-Commit-Position: refs/heads/master@{#11601}
==========

[commit-bot: I haz the power, 2016-02-12 14:08:49]

Patchset 5 (id:??) landed as
https://crrev.com/25558ad819b4df41ba51537e26a77480ace1e525
Cr-Commit-Position: refs/heads/master@{#11601}

[Taylor Brandstetter, 2016-02-12 20:00:23]

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

The reason for reverting is: Broke the VerifyHistogramStatsWithRed test on the
Windows DrMemory Full bot and Linux Memcheck bot. Please fix the test and
reland..