Apply VideoOptions per stream.

Apply VideoOptions per stream.

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

[nisse-webrtc, 2016-01-19 12:18:24]

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

[commit-bot: I haz the power, 2016-01-19 12:18:26]

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

[commit-bot: I haz the power, 2016-01-19 12:23:39]

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

[commit-bot: I haz the power, 2016-01-19 12:23:39]

Dry run: Try jobs failed on following builders:
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/12167)

[nisse-webrtc, 2016-01-19 12:24:41]

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

[nisse-webrtc, 2016-01-19 12:24:41]

This is a first attempt at fixing the propagation of VideoOptions. Some unclear
issues:

1. The disable_prerenderer_smoothing setting. Now set to always false, when
calling the WebRtcVideoReceiveStream constructor. Maybe extract the value into a
member variable in some method where the options are available?

2. Some options are global to the media channel (cpu adaptation, dscp), handled
in the new method SetSharedOptions. Seems generally messy to me, no idea if it
is right.

[nisse-webrtc, 2016-01-19 12:46:14]

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

[commit-bot: I haz the power, 2016-01-19 12:46:25]

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

[commit-bot: I haz the power, 2016-01-19 12:54:45]

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

[commit-bot: I haz the power, 2016-01-19 12:54:46]

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

[nisse-webrtc, 2016-01-22 13:50:45]

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

[commit-bot: I haz the power, 2016-01-22 13:50:49]

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

[nisse-webrtc, 2016-01-22 13:56:12]

On 2016/01/19 12:54:46, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_asan on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/11940)

Seems to pass my local tests now (waiting for cq dry run).

Known issues: the WebRtcVideoChannel2 doesn't save the passed in options. Should
perhaps copy it to send_params_.options, and call SetSharedOptions.

The disable_prerenderer_smoothing option is ignored. Is there any test for that?
I think the right way is to copy it from the options to a member variable, used
by WebRtcVideoChannel2::AddRecvStream. It could be set based on the options
passed in to the WebRtcVideoChannel2 constructor, or in SetSharedOptions. The
latter has the unintuitive effect that it will be modified also by
SetSendParameters.

[commit-bot: I haz the power, 2016-01-22 16:06:49]

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

[commit-bot: I haz the power, 2016-01-22 16:06:50]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-01-28 13:52:28]

Following pbos' suggestion, I'm trying to eliminate the
SetSharedOptions method. I'm collecting creation time options into
struct WebRtcVideoChannel2::Config, containing

    bool signal_cpu_adaptation;
    bool dscp;
    bool disable_prerenderer_smoothing;

with the intention to ignore corresponding values in VideoOptions
until they are deleted there. But it doesn't work... Problem is that
WebRtcSession doesn't create WebRtcVideoChannel2 directly. The call
chain on creation seems to be:

WebRtcSession::CreateVideoChannel calls
ChannelManager::CreateVideoChannel. ChannelManager::CreateVideoChannel
first creates a VideoMediaChannel by calling
MediaEngineInterface::CreateVideoChannel which calls
WebRtcVideoEngine2::CreateChannel, and then it creates and returns a
VideoChannel using that VideoMediaChannel.

One part of the mess is that VideoOptions are used both for
configuring a VideoMediaChannel, with settings originating from the
WebRtcSession, and configure a VideoChannel, with settings originating
from the VideoSource and passed to the SetOption method.

I see two possible directions: Separate VideoOptions into two structs,
one intended for VideoChannel and one for VideoMediaChannel. Or merge
VideoChannel and VideoMediaChannel.

And I'm still looking for the minimal amount of reorganization to make
it possible to fix hte immediate problem, which means passing along
the ssrc with the SetOptions call.

[nisse-webrtc, 2016-01-28 14:23:43]

New version, handling disable_prerenderer_smoothing, and storing the
creation-time options.

I'm keeping SetSharedOptions for now, because test cases, in particular, 
WebRtcVideoChannel2Test.DoesNotAdaptOnOveruseWhenDisabled expect that options
set with SetSendParameters overrides creation-time settings.

[nisse-webrtc, 2016-01-28 14:23:49]

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

[commit-bot: I haz the power, 2016-01-28 14:23:57]

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

[commit-bot: I haz the power, 2016-01-28 14:24:58]

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

[commit-bot: I haz the power, 2016-01-28 14:24:59]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/4820)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/4817)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7140)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7062)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12312)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/10970)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12089)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/8774)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6721)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12395)

[pbos-webrtc, 2016-01-28 14:45:15]

On 2016/01/28 14:24:59, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/4820)
>   ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/4817)
>   ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7140)
>   ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7062)
>   ios_dbg on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12312)
>   ios_rel on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/10970)
>   mac_asan on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12089)
>   mac_baremetal on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/8774)
>   mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
>   mac_gn_rel on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6721)
>   mac_rel on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12395)

I'd rather see the struct split, VideoOptions that are partially runtime and
partially construction time are confusing to me.

[pbos-webrtc, 2016-01-28 14:55:45]

https://codereview.webrtc.org/1608793004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1608793004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1105: // TODO(nisse): Used to pass
This should be const on construction, this needs to propagate from
RTCConfiguration. If this isn't broken right now we're missing test coverage.

[pthatcher1, 2016-01-28 19:13:29]

Mostly style things, although I do think we don't need to call SetSharedOptions
when SetOptions is called for a specific SSRC.

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:59: const bool
kDefault_suspend_below_min_bitrate = false;
Please make the style like kDefaultSuspendBelowMinBitrate on both of these.

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1483: SetSharedOptions(options);
Is this really necessary?  I don't think it is.

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1489:
send_streams_[ssrc]->SetOptions(options);
You can avoid a double look up by doing this:

auto kv = send_streams_.find(ssrc);
if (kv == send_streams_.end()) {
  return false;
}
kv.second->SetOptions(options);
return true;

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1952: : webrtc::RtcpMode::kCompound;
I'd call the variable "rtcp_mode".

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1956: parameters_.config.rtp.rtcp_mode =
mode;
This might be more clear as:

bool need_recreate = false;
webrtc::RtcpMode rtcp_mode = ...;
if (parameters_.config.rtp.rtcp_mode != rtcp_mode) {
  parameters_.config.rtp.rtcp_mode = rtcp_mode;
  need_recreate = true;
}

[pbos-webrtc, 2016-01-28 22:47:36]

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1956: parameters_.config.rtp.rtcp_mode =
mode;
On 2016/01/28 19:13:28, pthatcher1 wrote:
> This might be more clear as:
> 
> bool need_recreate = false;
> webrtc::RtcpMode rtcp_mode = ...;
> if (parameters_.config.rtp.rtcp_mode != rtcp_mode) {
>   parameters_.config.rtp.rtcp_mode = rtcp_mode;
>   need_recreate = true;
> }
> 

I think this code looks different at HEAD, I touched SetSendParameters.

[nisse-webrtc, 2016-01-29 10:33:12]

Thanks for the feedback.

On 2016/01/28 19:13:29, pthatcher1 wrote:
> Mostly style things, although I do think we don't need to call
SetSharedOptions
> when SetOptions is called for a specific SSRC.

I agree, but the current tests don't...

I've made a crude attempt to rebase this on top of pbos recent changes, in cl
https://codereview.webrtc.org/1647103002/.

But I think I will leave these changes for a while, and try to first split out
the VideoOptions which are applied at construction time to their own struct, in
a spearate cl. This will require some changes to
WebRtcVideoChannel2Test.DoesNotAdaptOnOveruseWhenDisabled
WebRtcVideoChannel2Test.TestSetDscpOptions to recreate then channel when these
settings are changed.

Which will also eliminate the need for SetSharedOptions. 

So I will get back to the stylistic fixes later.

[pbos-webrtc, 2016-01-29 10:35:09]

On 2016/01/29 10:33:12, nisse-webrtc wrote:
> Thanks for the feedback.
> 
> On 2016/01/28 19:13:29, pthatcher1 wrote:
> > Mostly style things, although I do think we don't need to call
> SetSharedOptions
> > when SetOptions is called for a specific SSRC.
> 
> I agree, but the current tests don't...
> 
> I've made a crude attempt to rebase this on top of pbos recent changes, in cl
> https://codereview.webrtc.org/1647103002/.
> 
> But I think I will leave these changes for a while, and try to first split out
> the VideoOptions which are applied at construction time to their own struct,
in
> a spearate cl. This will require some changes to
> WebRtcVideoChannel2Test.DoesNotAdaptOnOveruseWhenDisabled
> WebRtcVideoChannel2Test.TestSetDscpOptions to recreate then channel when these
> settings are changed.
> 
> Which will also eliminate the need for SetSharedOptions. 
> 
> So I will get back to the stylistic fixes later.

The tests don't agree because these were implemented to be reconfigurable on the
fly, so this was the easiest way to test that they got enabled at all. The tests
are wrong in this case, since the only real way to set these are on
construction, so the tests should be updated to reflect that. That they test
their properties through reconfiguring options is just because that was the
easiest way to test it before (when we permitted that).

[nisse-webrtc, 2016-02-12 11:17:01]

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

[commit-bot: I haz the power, 2016-02-12 11:17:09]

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

[nisse-webrtc, 2016-02-12 11:22:05]

After the MediaConfig change was landed, this change is back on track. It looks
reasonable to me, but I don't really understand what SetSend, SetSendParameters
and SetOptions are intended to do and how they should fit together.

A related change might be to remove VideoOptions from VideoSendParameters, but I
don't know if that's easy or not to do now.

[nisse-webrtc, 2016-02-12 12:40:47]

Discussed this with pbos. We think this is the way forward:

1. VideoOptions are always per stream. So drop them from VideoSendParameters.

2. Add a VideoOptions to AddStream, to let RtpSender or PeerConnection pass
appropriate defaults. That code gets the responsibility to apply a
peerconnection-wide constraint to all streams.

3. pbos then wants to keep the VideoOptions argument to SetVideoSend, but rename
the method, unclear what the new name should be. That's the path we'll pass the
is_screencast flag from source to send stream (unless we figure out something
better).

4. Delete SetOption methods, per-stream options set with AddStream and
SetVideoSend.

5. Change VideoOptions to use plain bools, not rtc::Optional.

I think I'll start with a cl doing (1), to figure out how difficult that is.

/Niels

[commit-bot: I haz the power, 2016-02-12 13:17:14]

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

[commit-bot: I haz the power, 2016-02-12 13:17:15]

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

[nisse-webrtc, 2016-02-15 08:14:20]

It seems that sorting out VideoSendParameters vs VideoOptions vs StreamOptions
is non-trivial...

If you think this cl (which is now simpler than it used to be) is a step in the
right direction, I think it would be good to get it landed son.

Then the refactoring aiming at eliminating cricket::VideoCapturer can add
is_screencast to VideoOptions and make progress in parallel with further
refactoring of VideoSendParameters/VideoOptions.

I've fixed the double lookups pthatcher pointed out. I think the other stylistic
comments on patchset 5 have been mooted by the recent changes by pbos and perkj.

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1608793004/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1489:
send_streams_[ssrc]->SetOptions(options);
On 2016/01/28 19:13:28, pthatcher1 wrote:
> You can avoid a double look up by doing this:
> 
> auto kv = send_streams_.find(ssrc);
> if (kv == send_streams_.end()) {
>   return false;
> }
> kv.second->SetOptions(options);
> return true;

Done. Changed all similar patterns in the file.

[pthatcher1, 2016-02-17 07:20:35]

lgtm

[pbos-webrtc, 2016-02-17 12:09:47]

lgtm

https://codereview.webrtc.org/1608793004/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1608793004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1271: auto kv =
send_streams_.find(ssrc);
auto& kv

https://codereview.webrtc.org/1608793004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1370: auto kv =
send_streams_.find(ssrc);
auto& kv

https://codereview.webrtc.org/1608793004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1386: auto kv =
send_streams_.find(ssrc);
auto& kv

[nisse-webrtc, 2016-02-17 12:27:49]

https://codereview.webrtc.org/1608793004/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1608793004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1370: auto kv =
send_streams_.find(ssrc);
On 2016/02/17 12:09:47, pbos-webrtc wrote:
> auto& kv

Done. const auto& was needed to make the compiler happy. Also found an unrelated
find call in WebRtcVideoChannel2::AddRecvStream, which I fixed similarly.

[pbos-webrtc, 2016-02-17 12:31:17]

lgtm

[nisse-webrtc, 2016-02-17 12:38:26]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-02-17 12:38:26]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1608793004/#ps160001 (title:
"Use const auto& when assigning find() return value.")

[commit-bot: I haz the power, 2016-02-17 12:38:43]

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

[commit-bot: I haz the power, 2016-02-17 14:39:15]

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

[commit-bot: I haz the power, 2016-02-17 14:39:16]

Try jobs failed on following builders:
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-02-17 15:02:09]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-02-17 15:03:28]

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

[commit-bot: I haz the power, 2016-02-17 15:25:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-17 15:25:02]

Description was changed from

==========
Apply VideoOptions per stream.

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

to

==========
Apply VideoOptions per stream.

BUG=webrtc:5438

Committed: https://crrev.com/a293ef0618ea78f063140c9214302c11c30c6283
Cr-Commit-Position: refs/heads/master@{#11656}
==========

[commit-bot: I haz the power, 2016-02-17 15:25:03]

Patchset 9 (id:??) landed as
https://crrev.com/a293ef0618ea78f063140c9214302c11c30c6283
Cr-Commit-Position: refs/heads/master@{#11656}