Recreate FlexfecReceiveStream separately from VideoReceiveStream.

Recreate FlexfecReceiveStream separately from VideoReceiveStream.

After this CL, reconfiguring the FlexFEC payload type at the
WebRtcVideoChannel2 level will no longer lead to the recreation of
the VideoReceiveStream. This means that the jitter buffer will
not be destroyed and a smoother video playback is achieved during
SDP renegotiation.

BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2911913002
Cr-Commit-Position: refs/heads/master@{#18318}
Committed: https://chromium.googlesource.com/external/webrtc/+/11fb472ae40de4e03e5b12aa0bb3f99d7e9c1a69

[brandtr, 2017-05-29 12:20:02]

Unit test.

[brandtr, 2017-05-29 12:20:11]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-05-29 12:26:37]

Change.

[brandtr, 2017-05-29 14:05:19]

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

[brandtr, 2017-05-29 14:05:20]

Hey Per,

Would you mind taking a look at this change? It's basically a sibling CL to
https://codereview.webrtc.org/2882433003/, but here I'm focusing on the receive
side. The goal is to reduce the number of VideoReceiveStream recreations during
SDP renegotiation. Recall that the previous CL focused on reducing the number of
VideoSendStream recreations during SDP renegotiation.

Before this CL, the VideoReceiveStream and the FlexfecReceiveStream recreations
were coupled -- one could not be recreated without the other one also being
recreated. After this CL, the recreations have been decoupled. This means, in
particular, that adding "flexfec-03" to a second SDP offer (*) will not mean
that the existing VideoReceiveStream is recreated.

(*) I had a look at the JSEP RFC again, and it turns out that adding a new codec
is indeed allowed, as long as the new SDP offer is created by the callee.

If it's unclear what I'm trying to do here, I'm happy to discuss in the office! 

-- Rasmus

[perkj_webrtc, 2017-05-30 05:49:54]

lgtm

In general it seems like webrtcvideoengine2.cc needs some love. It looks like
way to much logic is pushed there instead of kept in call.

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

https://codereview.webrtc.org/2911913002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2415: stream_ =
call_->CreateVideoReceiveStream(config.Copy());
why config.copy? The config supports move.

[brandtr, 2017-05-30 07:46:50]

On 2017/05/30 05:49:54, perkj_webrtc wrote:
> lgtm
> 
> In general it seems like webrtcvideoengine2.cc needs some love. It looks like
> way to much logic is pushed there instead of kept in call.
> 
>
https://codereview.webrtc.org/2911913002/diff/40001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/2911913002/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2.cc:2415: stream_ =
> call_->CreateVideoReceiveStream(config.Copy());
> why config.copy? The config supports move.

Thanks for quick review.

Agree that webrtcvideoengine2.cc is a bit of a mess. At least when it comes to
the RTP header extensions, maybe the work on the RtpTransportController will
lead to some simplifications there?

[brandtr, 2017-05-30 07:57:25]

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

[commit-bot: I haz the power, 2017-05-30 07:57:34]

Dry run: CQ is trying da patch.

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

[brandtr, 2017-05-30 07:57:34]

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

https://codereview.webrtc.org/2911913002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2415: stream_ =
call_->CreateVideoReceiveStream(config.Copy());
On 2017/05/30 05:49:54, perkj_webrtc wrote:
> why config.copy? The config supports move.

Done.

[commit-bot: I haz the power, 2017-05-30 08:15:38]

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

[commit-bot: I haz the power, 2017-05-30 08:15:38]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-05-30 08:29:33]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-05-30 08:29:33]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2911913002/#ps60001 (title:
"perkj comments 1.")

[commit-bot: I haz the power, 2017-05-30 08:30:01]

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-05-30 08:31:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496132973519840,
"parent_rev": "eaf4a1e10372e1a5a09f8669fca6ae61b156837f", "commit_rev":
"11fb472ae40de4e03e5b12aa0bb3f99d7e9c1a69"}

[commit-bot: I haz the power, 2017-05-30 08:31:40]

Description was changed from

==========
Recreate FlexfecReceiveStream separately from VideoReceiveStream.

After this CL, reconfiguring the FlexFEC payload type at the
WebRtcVideoChannel2 level will no longer lead to the recreation of
the VideoReceiveStream. This means that the jitter buffer will
not be destroyed and a smoother video playback is achieved during
SDP renegotiation.

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

to

==========
Recreate FlexfecReceiveStream separately from VideoReceiveStream.

After this CL, reconfiguring the FlexFEC payload type at the
WebRtcVideoChannel2 level will no longer lead to the recreation of
the VideoReceiveStream. This means that the jitter buffer will
not be destroyed and a smoother video playback is achieved during
SDP renegotiation.

BUG=webrtc:5654

Review-Url: https://codereview.webrtc.org/2911913002
Cr-Commit-Position: refs/heads/master@{#18318}
Committed:
https://chromium.googlesource.com/external/webrtc/+/11fb472ae40de4e03e5b12aa0...
==========

[commit-bot: I haz the power, 2017-05-30 08:31:41]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/11fb472ae40de4e03e5b12aa0...