Let Call::OnRecoveredPacket parse RTP header extensions.

Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

TESTED=Using Android app with FlexFEC field trial enabled.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/3002023002
Cr-Commit-Position: refs/heads/master@{#19460}
Committed: https://chromium.googlesource.com/external/webrtc/+/caea68f31e33a1bdf8adcdacaa53bc2f3c29ab88

[brandtr, 2017-08-21 08:58:19]

Description was changed from

==========
Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC or RTX enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

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

to

==========
Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

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

[brandtr, 2017-08-21 09:00:45]

Failing test.

[brandtr, 2017-08-21 09:00:58]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-08-21 09:35:04]

Description was changed from

==========
Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

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

to

==========
Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

TESTED=Using Android app with FlexFEC field trial enabled.
BUG=webrtc:5654
==========

[brandtr, 2017-08-21 11:05:31]

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

[brandtr, 2017-08-21 11:05:32]

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:1397: if (it == receive_rtp_config_.end()) {
This logic is now duplicated between Call::DeliverRtp and
Call::OnRecoveredPacket. Perhaps it's better to move it back to ParseRtpPacket?

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

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1174:
frame_generator_capturer->SetFakeRotation(kVideoRotation_90);
Added this to catch similar regressions affecting RTX in the future.

[nisse-webrtc, 2017-08-21 11:54:24]

lgtm

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:1397: if (it == receive_rtp_config_.end()) {
On 2017/08/21 11:05:31, brandtr wrote:
> This logic is now duplicated between Call::DeliverRtp and
> Call::OnRecoveredPacket. Perhaps it's better to move it back to
ParseRtpPacket? 

If that makes things simpler, sure. Longer term, I'd like to move identification
of extensions to the RtpStreamReceiverController, with extensions configured per
transport rather than per stream, which as far as I understand is also closer to
how they are defined in the spec.

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

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:1174:
frame_generator_capturer->SetFakeRotation(kVideoRotation_90);
On 2017/08/21 11:05:32, brandtr wrote:
> Added this to catch similar regressions affecting RTX in the future.

Nice!

[brandtr, 2017-08-22 13:14:23]

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/3002023002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:1397: if (it == receive_rtp_config_.end()) {
On 2017/08/21 11:54:24, nisse-webrtc wrote:
> On 2017/08/21 11:05:31, brandtr wrote:
> > This logic is now duplicated between Call::DeliverRtp and
> > Call::OnRecoveredPacket. Perhaps it's better to move it back to
> ParseRtpPacket? 
> 
> If that makes things simpler, sure. Longer term, I'd like to move
identification
> of extensions to the RtpStreamReceiverController, with extensions configured
per
> transport rather than per stream, which as far as I understand is also closer
to
> how they are defined in the spec.

I'll leave it as is, because it will not be simpler if I move this code to
Call::ParseRtpPacket. The reason is that Call::DeliverRtp returns different
error codes for different parsing errors.

[brandtr, 2017-08-22 13:15:24]

ping holmer due to recent discussion :)

[stefan-webrtc, 2017-08-22 13:31:37]

lgtm

[brandtr, 2017-08-22 13:40:29]

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

[commit-bot: I haz the power, 2017-08-22 13:40:36]

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-08-22 15:26:25]

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

[commit-bot: I haz the power, 2017-08-22 15:26:27]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-08-23 07:52:37]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-08-23 07:52:46]

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-08-23 07:55:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1503474757479990,
"parent_rev": "41cadbcb0a3535353a4b431b67c96798f8eed06d", "commit_rev":
"caea68f31e33a1bdf8adcdacaa53bc2f3c29ab88"}

[commit-bot: I haz the power, 2017-08-23 07:55:25]

Description was changed from

==========
Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

TESTED=Using Android app with FlexFEC field trial enabled.
BUG=webrtc:5654
==========

to

==========
Let Call::OnRecoveredPacket parse RTP header extensions.

Packets recovered by ULPFEC enter through
RtpVideoStreamReceiver::OnRecoveredPacket, which does RTP
header extension parsing. Packets recovered by FlexFEC, however,
enter through Call::OnRecoveredPacket, which prior to this
CL did not do RTP header extension parsing.

The lack of RTP header extension parsing for FlexFEC packets is a
regression since https://codereview.webrtc.org/2886993005/.

TESTED=Using Android app with FlexFEC field trial enabled.
BUG=webrtc:5654

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

[commit-bot: I haz the power, 2017-08-23 07:55:26]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/caea68f31e33a1bdf8adcdaca...