Make RTX pt/apt reconfigurable by calling WebRtcVideoChannel2::SetRecvParameters.

Make RTX pt/apt reconfigurable by calling WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed.
We remove the possibility for each video payload type to have an associated
specific RTX SSRC. Although the config previously allowed for this, all payload
types always had the same RTX SSRC set, and the underlying RtpPayloadRegistry
did not support multiple SSRCs. This change to the config struct should thus not
have any functional impact. The change does however affect the RtcEventLog, since
that is used for storing the VideoReceiveStream::Configs. For simplicity,
this CL does not change the event log proto definitions, instead duplicating
the serialized RTX SSRCs such that they fit in the existing proto definition.

BUG=webrtc:7011
Review-Url: https://codereview.webrtc.org/2646073004
Cr-Commit-Position: refs/heads/master@{#16302}
Committed: https://chromium.googlesource.com/external/webrtc/+/fe2bef39cd2a5c891a49f7320514fb04324dc66c

[brandtr, 2017-01-20 12:35:09]

Description was changed from

==========
Make RTX pt/apt reconfigurable by calling
WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

This CL fixes the above problem by changing the VideoReceiveStream::Config::Rtp
struct. Notably, we remove the possibility for each video payload type to
have a specific RTX SSRC. This possibility was never used in the code, and
was not supported by the receiving RtpPayloadRegistry.

BUG=webrtc:7011
==========

to

==========
Make RTX pt/apt reconfigurable by calling
WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed in
this CL. We remove the possiblity for each video payload type to have a
specific RTX SSRC. Although the config previously allowed for this, all payload
types always had the same RTX SSRC, and the underlying RtpPayloadRegistry
did not support multiple SSRCs. This change to the config struct should thus not
have any functional impact. It does however affect the RtcEventLog. For
simplicity, we have not made changes to the event log protos in this CL, instead
duplicating the RTX SSRCs that are logged.

BUG=webrtc:7011
==========

[brandtr, 2017-01-20 14:00:10]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-01-20 14:02:55]

Description was changed from

==========
Make RTX pt/apt reconfigurable by calling
WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed in
this CL. We remove the possiblity for each video payload type to have a
specific RTX SSRC. Although the config previously allowed for this, all payload
types always had the same RTX SSRC, and the underlying RtpPayloadRegistry
did not support multiple SSRCs. This change to the config struct should thus not
have any functional impact. It does however affect the RtcEventLog. For
simplicity, we have not made changes to the event log protos in this CL, instead
duplicating the RTX SSRCs that are logged.

BUG=webrtc:7011
==========

to

==========
Make RTX pt/apt reconfigurable by calling
WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed.
We remove the possibility for each video payload type to have an associated
specific RTX SSRC. Although the config previously allowed for this, all payload
types always had the same RTX SSRC set, and the underlying RtpPayloadRegistry
did not support multiple SSRCs. This change to the config struct should thus not
have any functional impact. The change does however affect the RtcEventLog,
since
that is used for storing the VideoReceiveStream::Configs. For simplicity,
this CL does not change the event log proto definitions, instead duplicating
the serialized RTX SSRCs such that they fit in the existing proto definition.

BUG=webrtc:7011
==========

[brandtr, 2017-01-20 14:38:09]

Patchset #1 (id:20001) has been deleted

[brandtr, 2017-01-20 14:49:23]

Please take a look.

holmer: General.
magjed: VideoEngine2 and interaction to dynamic payload types.
terelius: RtcEventLog.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.cc:246:
rtx->mutable_config()->set_rtx_ssrc(config.rtp.rtx_ssrc);
Always writing the same SSRC here, which is effectively what was done before as
well.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1248: sp.GetFidSsrc(ssrc, &rtx_ssrc))
{
Since |ssrc| is invariant in this loop, sp.GetFidSsrc will always write the same
value to |rtx_ssrc|. Cf. line 1237.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1251: rtx.ssrc = rtx_ssrc;
... and therefore all key-value pairs in |rtx| have the same SSRC set.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2462: const int
kUnusedPayloadType1 = 126;
Prior to this CL, this test already passes. I.e., the reconfiguring on the send
side is not a problem.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2829: const int
kUnusedPayloadType1 = 126;
Prior to this CL, this test fails.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:163: if (config_.rtp.rtx_ssrc) {
Both an SSRC and the pt/apt mapping is needed to decode received RTX packets.

[brandtr, 2017-01-20 14:50:10]

brandtr@webrtc.org changed reviewers:
+ magjed@webrtc.org, stefan@webrtc.org, terelius@webrtc.org

[brandtr, 2017-01-20 14:50:11]

Forgot to add reviewers...
==
Please take a look.

holmer: General.
magjed: VideoEngine2 and interaction to dynamic payload types.
terelius: RtcEventLog.

[magjed_webrtc, 2017-01-23 10:19:17]

VideoEngine2 changes lgtm

[brandtr, 2017-01-23 12:15:28]

Rebase.

[stefan-webrtc, 2017-01-23 12:45:11]

lgtm

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2495:
EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx.ssrcs[0]);
Is it possible to break this code out into a helper function that can be used
both here and in SetRecvCodecsWithChangedRtxPayloadType? Looks like it could be
possible, maybe with a template function.

May not be worth it, I'll leave it to you to decide. :)

[terelius, 2017-01-23 12:52:01]

lgtm with one question/comment:

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log_parser.cc:322: config->rtp.rtx_ssrc =
map.config().rtx_ssrc();
Check that all RTX-SSRCs are identical?

[brandtr, 2017-01-24 10:04:41]

Answers to holmer and terelius.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log_parser.cc:322: config->rtp.rtx_ssrc =
map.config().rtx_ssrc();
On 2017/01/23 12:52:01, terelius wrote:
> Check that all RTX-SSRCs are identical?

WDYT about this approach?

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2495:
EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx.ssrcs[0]);
On 2017/01/23 12:45:11, stefan-webrtc wrote:
> Is it possible to break this code out into a helper function that can be used
> both here and in SetRecvCodecsWithChangedRtxPayloadType? Looks like it could
be
> possible, maybe with a template function.
> 
> May not be worth it, I'll leave it to you to decide. :)

It would reduce the verbosity a bit, but I think it would reduce clarity for new
readers of the tests.

https://codereview.webrtc.org/2646073004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2868:
EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx_ssrc);
... Also, the EXPECTs are slightly difference between the receive and send
stream objects, since the receive stream object only has a single RTX SSRC,
compared to the multiple RTX SSRCs of the send stream object.

[brandtr, 2017-01-24 13:27:58]

Rebase.

[brandtr, 2017-01-26 09:18:14]

Rebase.

[brandtr, 2017-01-26 14:08:57]

Rebase.

[brandtr, 2017-01-26 14:24:17]

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

[commit-bot: I haz the power, 2017-01-26 14:24:20]

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-01-26 14:38:05]

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

[commit-bot: I haz the power, 2017-01-26 14:38:06]

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

[brandtr, 2017-01-26 15:06:10]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[brandtr, 2017-01-26 15:37:30]

The CQ bit was unchecked by brandtr@webrtc.org

[brandtr, 2017-01-26 15:37:36]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-01-26 15:37:37]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
terelius@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2646073004/#ps160001 (title:
"Fixes.")

[commit-bot: I haz the power, 2017-01-26 15:37:45]

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-01-26 16:04:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485445056460860,
"parent_rev": "4703741e49919313ac335ffa332ee8ebb288e192", "commit_rev":
"fe2bef39cd2a5c891a49f7320514fb04324dc66c"}

[commit-bot: I haz the power, 2017-01-26 16:04:06]

Description was changed from

==========
Make RTX pt/apt reconfigurable by calling
WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed.
We remove the possibility for each video payload type to have an associated
specific RTX SSRC. Although the config previously allowed for this, all payload
types always had the same RTX SSRC set, and the underlying RtpPayloadRegistry
did not support multiple SSRCs. This change to the config struct should thus not
have any functional impact. The change does however affect the RtcEventLog,
since
that is used for storing the VideoReceiveStream::Configs. For simplicity,
this CL does not change the event log proto definitions, instead duplicating
the serialized RTX SSRCs such that they fit in the existing proto definition.

BUG=webrtc:7011
==========

to

==========
Make RTX pt/apt reconfigurable by calling
WebRtcVideoChannel2::SetRecvParameters.

Prior to this CL, received RTX (associated) payload types were only configured
when WebRtcVideoChannel2::AddRecvStream was called. In the same method, the RTX
SSRC was set up.

After this CL, the RTX (associated) payload types are set in
WebRtcVideoChannel2::SetRecvParameters, which is the appropriate place to set
them. The RTX SSRC is still set in WebRtcVideoChannel2::AddRecvStream, since
that is the code path that sets other SSRCs.

As part of this fix, the VideoReceiveStream::Config::Rtp struct is changed.
We remove the possibility for each video payload type to have an associated
specific RTX SSRC. Although the config previously allowed for this, all payload
types always had the same RTX SSRC set, and the underlying RtpPayloadRegistry
did not support multiple SSRCs. This change to the config struct should thus not
have any functional impact. The change does however affect the RtcEventLog,
since
that is used for storing the VideoReceiveStream::Configs. For simplicity,
this CL does not change the event log proto definitions, instead duplicating
the serialized RTX SSRCs such that they fit in the existing proto definition.

BUG=webrtc:7011

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

[commit-bot: I haz the power, 2017-01-26 16:04:08]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/fe2bef39cd2a5c891a49f7320...

[kjellander (google.com), 2017-01-26 21:20:55]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.webrtc.org/2649323010/ by kjellander@google.com.

The reason for reverting is: Breaks internal downstream project..