Inform jitter buffer about FlexFEC protection.

Inform jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer adaptation logic.

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

[brandtr, 2017-01-24 12:29:14]

Description was changed from

==========
Inform jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer delay adaptation logic.

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

to

==========
Inform video receiver and jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer delay adaptation logic.

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

[brandtr, 2017-01-24 12:31:13]

Description was changed from

==========
Inform video receiver and jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer delay adaptation logic.

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

to

==========
Inform jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer adaptation logic.

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

[brandtr, 2017-01-24 12:46:27]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-01-24 13:02:49]

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

[brandtr, 2017-01-24 13:02:49]

Please take a look.

https://codereview.webrtc.org/2649973005/diff/20001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2649973005/diff/20001/webrtc/call/call.h#newcod...
webrtc/call/call.h:124: // the former is started.
An alternative design would be to make VideoReceiveStream implement an interface
"FlexfecProtectionObserver" that would be called by Call whenever a new
FlexfecReceiveStream is created that protects that particular
VideoReceiveStream.

That design would however require either recreating the receiver objects (e.g.,
by calling Stop() and Start()), or creating a new "method path" down to the JB
that propagates the information that FEC has been dynamically enabled. Neither
seem like great choices, so I went with this sort of static design instead. The
downside of this is that we must always create the FlexfecReceiveStream before
Starting the VideoReceiveStream. There aren't too many places where the
VideoReceiveStream is started(*), so the chance of making mistakes is not big
however.

(*) VideoReceiveStreams are started in
WebRtcVideoChannel2::WebRtcVideoReceiveStream::RecreateWebRtcStream (production
code), CallTest (E2E and VSS tests) and VideoQualityTest (perf tests).

https://codereview.webrtc.org/2649973005/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2649973005/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:111: 
The methods below are not strictly needed, but I thought it made sense to
declare these here to clarify the assumptions around their usage.

[philipel, 2017-01-25 13:17:36]

If the flexfec mode can only be set in the ctor of the VideoReceiveStream, why
don't we just propagate a bool down instead of a mediator?

Also, this makes it clear wherever a VideoReceiveStream is instantiated that we
need to know beforehand if it is protected by flexfec or not.

[brandtr, 2017-01-26 09:17:56]

Rebase.

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

Patchset #3 (id:60001) has been deleted

[brandtr, 2017-01-26 15:44:51]

On 2017/01/25 13:17:36, philipel wrote:
> If the flexfec mode can only be set in the ctor of the VideoReceiveStream, why
> don't we just propagate a bool down instead of a mediator?
> 
> Also, this makes it clear wherever a VideoReceiveStream is instantiated that
we
> need to know beforehand if it is protected by flexfec or not.

It's set in Start(), so the API contract would allow for the FEC protection mode
to change between stream creation and stream start. I've added a new PS that
simplifies this to be set on stream creation only, since at least in the
production code, we start the stream right after creating it.

The benefit of this change is that it is simpler, but I don't know if it is any
more clear that the previous approach: users create VideoReceiveStreams by
calling Call::CreateVideoReceiveStream, and it's on that level that FlexFEC
streams are created too. Since Call is no longer a public API, I don't think it
matters that much.

[philipel, 2017-01-27 11:40:52]

On 2017/01/26 15:44:51, brandtr wrote:
> On 2017/01/25 13:17:36, philipel wrote:
> > If the flexfec mode can only be set in the ctor of the VideoReceiveStream,
why
> > don't we just propagate a bool down instead of a mediator?
> > 
> > Also, this makes it clear wherever a VideoReceiveStream is instantiated that
> we
> > need to know beforehand if it is protected by flexfec or not.
> 
> It's set in Start(), so the API contract would allow for the FEC protection
mode
> to change between stream creation and stream start. I've added a new PS that
> simplifies this to be set on stream creation only, since at least in the
> production code, we start the stream right after creating it.

Aha, didn't realize that. In that case I think your first approach is better
actually. :)

[philipel, 2017-01-27 13:07:34]

lgtm

[stefan-webrtc, 2017-01-27 13:26:03]

lgtm % nit

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

https://codereview.webrtc.org/2649973005/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2282:
call_->DestroyVideoReceiveStream(stream_);
Should we set stream_ to nullptr here too just to be on the safe side?

[brandtr, 2017-01-27 13:37:31]

Rebase + git cl format.

[brandtr, 2017-01-27 13:38:21]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-01-27 13:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2649973005/#ps120001 (title:
"Rebase + git cl format.")

[commit-bot: I haz the power, 2017-01-27 13:38:31]

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

[brandtr, 2017-01-27 14:10:46]

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

https://codereview.webrtc.org/2649973005/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2282:
call_->DestroyVideoReceiveStream(stream_);
On 2017/01/27 13:26:02, stefan-webrtc wrote:
> Should we set stream_ to nullptr here too just to be on the safe side?

Might as well. Done.

[commit-bot: I haz the power, 2017-01-27 14:47:55]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485524301522420,
"parent_rev": "5a2c506e8e8fb4941ab428e355bf8f68954af348", "commit_rev":
"fb45c6c103eb5fe594a54f4291726e8242f0ec38"}

[commit-bot: I haz the power, 2017-01-27 14:47:57]

Description was changed from

==========
Inform jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer adaptation logic.

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

to

==========
Inform jitter buffer about FlexFEC protection.

This CL introduces a way for the VideoReceiveStreams to check whether
they are protected by any FlexfecReceiveStreams. This is done in the
VideoReceiveStream::Start() method, which then propagates this information
down to the jitter buffer adaptation logic.

BUG=webrtc:5654

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

[commit-bot: I haz the power, 2017-01-27 14:47:58]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/fb45c6c103eb5fe594a54f429...