Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in WebRtcVideoEngine2.
- Add a IsCompleteAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.
- Simplify setup for receiving FlexFEC in VideoQualityTest.

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

[brandtr, 2016-12-19 13:43:06]

Patchset #1 (id:1) has been deleted

[brandtr, 2016-12-19 13:59:40]

Patchset #1 (id:20001) has been deleted

[brandtr, 2016-12-19 13:59:59]

Description was changed from

==========
Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from
FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in
WebRtcVideoEngine2.
- Add a IsCompletelAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.

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

to

==========
Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from
FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in
WebRtcVideoEngine2.
- Add a IsCompleteAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.

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

[brandtr, 2016-12-19 14:08:20]

Patchset #1 (id:40001) has been deleted

[brandtr, 2016-12-19 14:32:58]

Patchset #1 (id:60001) has been deleted

[brandtr, 2016-12-19 15:31:45]

Description was changed from

==========
Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from
FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in
WebRtcVideoEngine2.
- Add a IsCompleteAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.

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

to

==========
Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from
FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in
WebRtcVideoEngine2.
- Add a IsCompleteAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.
- Simplify setup for receiving FlexFEC in VideoQualityTest.

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

[brandtr, 2016-12-19 15:31:45]

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

[brandtr, 2016-12-19 15:32:18]

Please have a look :)

[brandtr, 2016-12-19 15:32:20]

Please have a look :)

[brandtr, 2016-12-20 09:53:01]

Rebase.

[brandtr, 2016-12-20 11:53:32]

Rebase.

[philipel, 2016-12-20 12:09:18]

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.h:32: Config() = delete;
Default ctors are automatically removed if you implement any ctor.

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:224:
webrtc::FlexfecReceiveStream::Config* flexfec_config,
Why don't we remove webrtc::FlexfecConfig?

[brandtr, 2016-12-20 12:22:11]

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.h:32: Config() = delete;
On 2016/12/20 12:09:18, philipel wrote:
> Default ctors are automatically removed if you implement any ctor.

Removed.

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/2589713003/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:224:
webrtc::FlexfecReceiveStream::Config* flexfec_config,
On 2016/12/20 12:09:18, philipel wrote:
> Why don't we remove webrtc::FlexfecConfig?

I might do that in the future.

It is still used by the send side however:
https://cs.chromium.org/chromium/src/third_party/webrtc/video_send_stream.h?q...

[philipel, 2016-12-20 12:40:41]

lgtm

[brandtr, 2016-12-21 08:48:21]

Rebase onto master after revert of https://codereview.webrtc.org/2553863003/

[brandtr, 2016-12-21 11:29:10]

Rebase.

[brandtr, 2016-12-21 12:29:56]

Matching FlexfecReceiveStream configs are only created by RunBaseTest, not by
CallTest...

[brandtr, 2016-12-21 14:40:50]

Rebase onto master.

[brandtr, 2016-12-21 14:44:45]

Clarify config setup in VideoQualityTest a bit.

[stefan-webrtc, 2016-12-22 09:14:54]

lg

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.h:39: bool IsCompleteAndEnabled() const;
Is this used for anything but tests? If not, I'd prefer to have it part of the
test code.

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3891: FlexfecReceiveStream::Config
default_receive_config(nullptr);
I guess it's questionable if it should be OK to pass nullptr as the transport if
you've made it required? Maybe you should DCHECK for it in the constructor of
the config?

[brandtr, 2017-01-03 09:43:35]

Rebase.

[brandtr, 2017-01-03 10:01:05]

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream.h (right):

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream.h:39: bool IsCompleteAndEnabled() const;
On 2016/12/22 09:14:54, stefan-webrtc_OOO_Jan9 wrote:
> Is this used for anything but tests? If not, I'd prefer to have it part of the
> test code.

Yes, it's used here:
https://codereview.webrtc.org/2589713003/diff/280001/webrtc/media/engine/webr...

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2589713003/diff/260001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3891: FlexfecReceiveStream::Config
default_receive_config(nullptr);
On 2016/12/22 09:14:54, stefan-webrtc_OOO_Jan9 wrote:
> I guess it's questionable if it should be OK to pass nullptr as the transport
if
> you've made it required? Maybe you should DCHECK for it in the constructor of
> the config?

Done.

[brandtr, 2017-01-09 07:58:36]

Rebase.

[stefan-webrtc, 2017-01-12 14:36:19]

lgtm % nit

https://codereview.webrtc.org/2589713003/diff/320001/webrtc/call/flexfec_rece...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2589713003/diff/320001/webrtc/call/flexfec_rece...
webrtc/call/flexfec_receive_stream_impl.cc:54: if (remote_ssrc == 0)
Is 0 an invalid ssrc? :)

[brandtr, 2017-01-12 14:44:07]

On 2017/01/12 14:36:19, stefan-webrtc wrote:
> lgtm % nit
> 
>
https://codereview.webrtc.org/2589713003/diff/320001/webrtc/call/flexfec_rece...
> File webrtc/call/flexfec_receive_stream_impl.cc (right):
> 
>
https://codereview.webrtc.org/2589713003/diff/320001/webrtc/call/flexfec_rece...
> webrtc/call/flexfec_receive_stream_impl.cc:54: if (remote_ssrc == 0)
> Is 0 an invalid ssrc? :)

No idea! But we don't support it being 0 at least :)

See e.g.:
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_stream_rece...
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

[brandtr, 2017-01-12 14:51:03]

Rebase.

[brandtr, 2017-01-12 14:58:22]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-01-12 14:58: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/2589713003/#ps340001 (title:
"Rebase.")

[commit-bot: I haz the power, 2017-01-12 14:58:32]

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-12 16:07:41]

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

[commit-bot: I haz the power, 2017-01-12 16:07:42]

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/20994)

[brandtr, 2017-01-13 12:13:02]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-01-13 12:13:10]

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-13 13:24:16]

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

[commit-bot: I haz the power, 2017-01-13 13:24:17]

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/21032)

[brandtr, 2017-01-13 15:19:13]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-01-13 15:19:20]

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-13 15:41:20]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1484320753508330,
"parent_rev": "44b3ef65ed562aec042c0f1d29a983207228e40e", "commit_rev":
"8313a6fa8f78d18e0f5e2bff31086da01d0db4bf"}

[commit-bot: I haz the power, 2017-01-13 15:41:23]

Description was changed from

==========
Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from
FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in
WebRtcVideoEngine2.
- Add a IsCompleteAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.
- Simplify setup for receiving FlexFEC in VideoQualityTest.

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

to

==========
Make |rtcp_send_transport| mandatory in FlexfecReceiveStream::Config.

That object will be used when we enable RTCP reporting from
FlexfecReceiveStream.

Other related changes:
- Stop using FlexfecConfig (from config.h) at receive side in
WebRtcVideoEngine2.
- Add a IsCompleteAndEnabled() method to FlexfecReceiveStream::Config, to be
  used in WebRtcVideoEngine2.
- Centralize the construction of the FlexfecReceiveStream::Config in unit tests.
  This will make future additions to the unit tests cleaner.
- Simplify setup for receiving FlexFEC in VideoQualityTest.

BUG=webrtc:5654

Review-Url: https://codereview.webrtc.org/2589713003
Cr-Commit-Position: refs/heads/master@{#16059}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8313a6fa8f78d18e0f5e2bff3...
==========

[commit-bot: I haz the power, 2017-01-13 15:41:25]

Committed patchset #14 (id:340001) as
https://chromium.googlesource.com/external/webrtc/+/8313a6fa8f78d18e0f5e2bff3...