Drop VideoOptions from VideoSendParameters.

Drop VideoOptions from VideoSendParameters.

BUG=webrtc:5438
Committed: https://crrev.com/05103314e52e1b21e17dd4f0b28d038ed8942e2f
Cr-Commit-Position: refs/heads/master@{#12011}

[nisse-webrtc, 2016-02-12 13:45:46]

nisse@webrtc.org changed reviewers:
+ pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2016-02-12 13:45:46]

Comments appreciated. I think the changes to webrtcvideoengine2.cc are natural.
The stream's SetOptions method is now called only from SetVideoSend (and only if
enabled, not sure if that's right).

But there's code, both in tests and, more importantly, in channel.cc, which
expects to change options by calling SetSendParameters. I'd need some advice on
how to reorganize that.

[pthatcher1, 2016-02-12 20:45:38]

I think you could move all video options that apply to all streams in
VideoParameters, such as conference mode and the bitrate things (although how
you'll get those form the PeerConnection configuration, I'm not sure).

And then make VideoOptions only be per stream.  But really, there are only two
things I'm aware of that are per-stream: video denoise and maybe simulcast.  

So maybe what you should do is the opposite: make VideoOptions be only across
all streams, and make the per-streaam options some other struct, perhaps a
VideoTrackOptions, since both video denoise and simulcast come from the track.

https://codereview.webrtc.org/1695663003/diff/1/webrtc/media/base/videoengine...
File webrtc/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1695663003/diff/1/webrtc/media/base/videoengine...
webrtc/media/base/videoengine_unittest.h:735: parameters.options.conference_mode
= rtc::Optional<bool>(true);
You could just put it in VideoParameters directly.  Or figure out how to get rid
of it entirely (which we've always wanted to do).

https://codereview.webrtc.org/1695663003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1695663003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1023:
params.options.video_noise_reduction = rtc::Optional<bool>(enabled);
Probably with channel_->SetOptions or channel_->SetVideoSend.

https://codereview.webrtc.org/1695663003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1470:
rtc::Optional<int>(kScreenshareMinBitrateKbps);
Again, could go on VideoParameters directly.

https://codereview.webrtc.org/1695663003/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1695663003/diff/1/webrtc/pc/channel.cc#newcode1803
webrtc/pc/channel.cc:1803: // conference_mode is per stream, we'd need the ssrc.
Conference mode is not per-ssrc.

[nisse-webrtc, 2016-03-04 13:46:26]

nisse@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[nisse-webrtc, 2016-03-04 13:46:27]

I've now rebased this on top of other recent changes related to VideoOptions. So
it's now a quite small change.

In the testcode, use SetVideoSend, rather than SetSendParameters, to configure
the video options.

Do you think this makes sense? 

It would be nice if we could go one step further, and drop the options argument
from the WebRtcVideoChannel2 contructor, and drop the use of rtc::Optional.
Maybe that's as easy as moving screencast_min_bitrate_kbps to Mediaconfig, it's
a construction time setting?

PS. I'll be on vacation the coming week.

[nisse-webrtc, 2016-03-04 14:41:12]

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

[commit-bot: I haz the power, 2016-03-04 14:41:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695663003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695663003/40001

[commit-bot: I haz the power, 2016-03-04 16:42:03]

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

[commit-bot: I haz the power, 2016-03-04 16:42:04]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2016-03-05 01:49:14]

lgtm

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

https://codereview.webrtc.org/1695663003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:519: VideoOptions
send_default_options_;
default_send_options_

[nisse-webrtc, 2016-03-15 15:37:40]

Hi, a minor update (rebase + renaming suggested by pthatcher). I'd like to land
this soon. 

taylor, pbos: If you have feedback you want me to wait for, please speak up
asap.

[Taylor Brandstetter, 2016-03-15 16:57:26]

lgtm

[pbos-webrtc, 2016-03-15 18:40:16]

lgtm

https://codereview.webrtc.org/1695663003/diff/80001/webrtc/media/base/fakemed...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1695663003/diff/80001/webrtc/media/base/fakemed...
webrtc/media/base/fakemediaengine.h:545: class DataOptions {};
Is this necessary? If so put a comment.

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

https://codereview.webrtc.org/1695663003/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1535: VideoOptions
mbr_options;
mbr?

[nisse-webrtc, 2016-03-16 08:17:03]

https://codereview.webrtc.org/1695663003/diff/80001/webrtc/media/base/fakemed...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1695663003/diff/80001/webrtc/media/base/fakemed...
webrtc/media/base/fakemediaengine.h:545: class DataOptions {};
On 2016/03/15 18:40:15, pbos-webrtc wrote:
> Is this necessary? If so put a comment.

The DataTraits templatery in channel_unittest.cc wants it. I'm adding a comment.

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

https://codereview.webrtc.org/1695663003/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1535: VideoOptions
mbr_options;
On 2016/03/15 18:40:15, pbos-webrtc wrote:
> mbr?

Intended meaning: VideoOptions instance used for setting the
screencast_min_bitrate_kbps option (compare to the screencast_options further
down). I'll rename to the less concise min_bitrate_options, is that better?

[nisse-webrtc, 2016-03-16 08:19:42]

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

[commit-bot: I haz the power, 2016-03-16 08:19:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695663003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695663003/100001

[commit-bot: I haz the power, 2016-03-16 09:19:18]

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

[commit-bot: I haz the power, 2016-03-16 09:19:19]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-03-16 09:21:11]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-03-16 09:21:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pbos@webrtc.org, deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1695663003/#ps100001 (title:
"Addressed pbos' comments.")

[commit-bot: I haz the power, 2016-03-16 09:21:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695663003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695663003/100001

[commit-bot: I haz the power, 2016-03-16 09:22:56]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-16 09:23:01]

Description was changed from

==========
Drop VideoOptions from VideoSendParameters.

BUG=webrtc:5438
==========

to

==========
Drop VideoOptions from VideoSendParameters.

BUG=webrtc:5438

Committed: https://crrev.com/05103314e52e1b21e17dd4f0b28d038ed8942e2f
Cr-Commit-Position: refs/heads/master@{#12011}
==========

[commit-bot: I haz the power, 2016-03-16 09:23:02]

Patchset 6 (id:??) landed as
https://crrev.com/05103314e52e1b21e17dd4f0b28d038ed8942e2f
Cr-Commit-Position: refs/heads/master@{#12011}