Add RTCConfiguration getter and setter methods.

Add RTCConfiguration getter and setter methods. The immediate plan is to move some flags into an embedded MediaConfig (https://codereview.webrtc.org/1818033002/), which will be possible after Chrome is updated to use these new setter methods.

BUG=webrtc:4906
R=hbos@google.com, hbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/71bdda0ededda74cdd73a8309419cf626499627f
Cr-Commit-Position: refs/heads/master@{#12177}

[nisse-webrtc, 2016-03-29 12:58:05]

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

[nisse-webrtc, 2016-03-29 12:58:06]

This cl adds getter and setter methods for the RTCConfiguration flags which
really belong in a struct MediaConfig, and which are set by chrome.

Since this is intended to be public interfaces (in preference over accessing
attributes directly), naming is particularly important. Feedback appreciated.

I'm not sure if it should be get_enable_foo or simply enable_foo(). The latter
may collide, since this is still a struct and conventions of no underscore
suffix on members. Also enable_foo may be read as a verb rather than a getter,
which might be confusing.

I also thing consistency is important, hence everything is called
[gs]et_enable_*, and I'm tempted to rename suspend_below_min_bitrate ->
enable_suspend_below_min_bitrate and disable_prerenderer_smoothing ->
enable_prerenderer_smoothing in MediaConfig in a later cl.

[perkj_webrtc, 2016-03-29 15:25:12]

https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:232: return
cpu_overuse_detection.value_or(true);
Use in this cl? ie- Stop using the to be private structs.

Name cpu_overuse()
set_cpu_overuse(bool enabled)
?

......

https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:239: return
suspend_below_min_bitrate.value_or(false);
dito

https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:245: bool
get_enable_prerenderer_smoothing(bool flag) {
|flag| is unused

[nisse-webrtc, 2016-03-30 08:50:23]

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

[nisse-webrtc, 2016-03-30 08:50:24]

New version. I renamed the setter methods as simply set_<name of attribute>,
which seems to be what the coding standard prescribes.

I deleted the getter methods, they are not immediately needed, and proper names
collides with the attributes themselves. Getter methods can be added in cl  
https://codereview.webrtc.org/1818033002/, if desired.

https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:232: return
cpu_overuse_detection.value_or(true);
On 2016/03/29 15:25:11, perkj_webrtc wrote:
> Use in this cl? ie- Stop using the to be private structs.

(Some important words seem to be missing). My intention is to land this, update
chrome, and then rebase and land https://codereview.webrtc.org/1818033002/. At
that point, webrtc won't really do any read accesses to these individual flags
in RTCConfiguration, just pass on a pointer/reference to the MediaConfig where
they are stored.

And for write accesses, it's the (deprecated) code converting constraints to
config values, which will be using FindConstraint or (before my changes)
ConstraintToOptionalBool. Going via the setter function there is just making
things unnecessarily complicated.

But there are a few uses in the test cases, and there it makes sense to update
to the getter and setter methods.

> Name cpu_overuse()
> set_cpu_overuse(bool enabled)
> ?
> 
> ......

I think just cpu_overuse is confusing. "cpu_adaptation", maybe?

I would also like (in a later cl) to make the name of the MediaConfig fields a
bit more consistent. There, we have an enable_ prefix on two flags, disable_ on
one flag, and one flag with no prefix. So really inconsistent. Is it reasonable
to have "enable_" prefixes on the variables, but not on the methods?

[pthatcher1, 2016-03-30 16:59:36]

lgtm 

On 2016/03/30 08:50:24, nisse-webrtc wrote:
> New version. I renamed the setter methods as simply set_<name of attribute>,
> which seems to be what the coding standard prescribes.
> 
> I deleted the getter methods, they are not immediately needed, and proper
names
> collides with the attributes themselves. Getter methods can be added in cl  
> https://codereview.webrtc.org/1818033002/, if desired.
> 
>
https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
> File webrtc/api/peerconnectioninterface.h (right):
> 
>
https://codereview.webrtc.org/1836083003/diff/1/webrtc/api/peerconnectioninte...
> webrtc/api/peerconnectioninterface.h:232: return
> cpu_overuse_detection.value_or(true);
> On 2016/03/29 15:25:11, perkj_webrtc wrote:
> > Use in this cl? ie- Stop using the to be private structs.
> 
> (Some important words seem to be missing). My intention is to land this,
update
> chrome, and then rebase and land https://codereview.webrtc.org/1818033002/. At
> that point, webrtc won't really do any read accesses to these individual flags
> in RTCConfiguration, just pass on a pointer/reference to the MediaConfig where
> they are stored.
> 
> And for write accesses, it's the (deprecated) code converting constraints to
> config values, which will be using FindConstraint or (before my changes)
> ConstraintToOptionalBool. Going via the setter function there is just making
> things unnecessarily complicated.
> 
> But there are a few uses in the test cases, and there it makes sense to update
> to the getter and setter methods.
> 
> > Name cpu_overuse()
> > set_cpu_overuse(bool enabled)
> > ?
> > 
> > ......
> 
> I think just cpu_overuse is confusing. "cpu_adaptation", maybe?

cpu_adaptation sounds nice to me.

> 
> I would also like (in a later cl) to make the name of the MediaConfig fields a
> bit more consistent. There, we have an enable_ prefix on two flags, disable_
on
> one flag, and one flag with no prefix. So really inconsistent. Is it
reasonable
> to have "enable_" prefixes on the variables, but not on the methods?

I agree with being consistent.  It looks like we either have nouns (servers,
bundle_policy) or verbs (prioritize_most_likely_ice_candidate_pairs,
disable_ipv6).  I think we should fix that for cpu_overuse_detection and
combined_audio_video_bwe.  Those should probably be enable_cpu_adaptation and
enable_combined_audio_video_bwe.

As for the two "disable_"s.  If we change it to enable_, we just need to be make
it very clear that the default is that they are enabled.

[nisse-webrtc, 2016-03-31 07:19:34]

New version. I think it is ready.

[perkj_webrtc, 2016-03-31 07:35:21]

https://codereview.webrtc.org/1836083003/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1836083003/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:232: void set_enable_dscp(bool flag) {
enable_dscp = rtc::Optional<bool>(flag); }
set_dscp(bool enable)

https://codereview.webrtc.org/1836083003/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:250: void
set_disable_prerenderer_smoothing(bool disabled) {
set_prerender_smoo.... (bool enable)

[nisse-webrtc, 2016-03-31 07:41:40]

Some further renames, after discussion with Per.

[hbos_google, 2016-03-31 08:10:21]

hbos@google.com changed reviewers:
+ hbos@google.com

[hbos_google, 2016-03-31 08:10:22]

lgtm.

Can you clarify in the CL description why you're doing this, such as what you
wrote in comment #5.

https://codereview.webrtc.org/1836083003/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1836083003/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:227: // by applications, chrome in
particular.
super-nit: chrome -> Chrome.
Also, this comment makes me ask "why?" since its a struct and all members are
directly accessible. If it's because of the transition to use MediaConfig so
that we don't break Chrome in the meantime, or due to replacing Optional<bool>
with bool, clarify that this is why. Or perhaps this comment isn't necessary?

[hbos, 2016-03-31 08:12:05]

Oops, was signed in to google account. lgtm from webrtc account.

[nisse-webrtc, 2016-03-31 08:43:10]

Description was changed from

==========
Add RTCConfiguration getter and setter methods.

BUG=webrtc:4906
==========

to

==========
Add RTCConfiguration getter and setter methods. The immediate plan is to move
some flags into an embedded MediaConfig
(https://codereview.webrtc.org/1818033002/), which will be possible after Chrome
is updated to use these new setter methods.

BUG=webrtc:4906
==========

[perkj_webrtc, 2016-03-31 08:50:04]

lgtm

[nisse-webrtc, 2016-03-31 08:53:25]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-03-31 08:53:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, hbos@webrtc.org, hbos@google.com
Link to the patchset: https://codereview.webrtc.org/1836083003/#ps80001 (title:
"Comment improvement.")

[commit-bot: I haz the power, 2016-03-31 08:53:28]

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

[commit-bot: I haz the power, 2016-03-31 10:54:19]

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

[commit-bot: I haz the power, 2016-03-31 10:54:20]

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

[nisse-webrtc, 2016-03-31 11:00:13]

Description was changed from

==========
Add RTCConfiguration getter and setter methods. The immediate plan is to move
some flags into an embedded MediaConfig
(https://codereview.webrtc.org/1818033002/), which will be possible after Chrome
is updated to use these new setter methods.

BUG=webrtc:4906
==========

to

==========
Add RTCConfiguration getter and setter methods. The immediate plan is to move
some flags into an embedded MediaConfig
(https://codereview.webrtc.org/1818033002/), which will be possible after Chrome
is updated to use these new setter methods.

BUG=webrtc:4906
R=hbos@google.com, hbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/71bdda0ededda74cdd73a8309...
==========

[commit-bot: I haz the power, 2016-03-31 11:00:14]

Description was changed from

==========
Add RTCConfiguration getter and setter methods. The immediate plan is to move
some flags into an embedded MediaConfig
(https://codereview.webrtc.org/1818033002/), which will be possible after Chrome
is updated to use these new setter methods.

BUG=webrtc:4906
R=hbos@google.com, hbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/71bdda0ededda74cdd73a8309...
==========

to

==========
Add RTCConfiguration getter and setter methods. The immediate plan is to move
some flags into an embedded MediaConfig
(https://codereview.webrtc.org/1818033002/), which will be possible after Chrome
is updated to use these new setter methods.

BUG=webrtc:4906
R=hbos@google.com, hbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/71bdda0ededda74cdd73a8309419cf626499627f
Cr-Commit-Position: refs/heads/master@{#12177}
==========

[nisse-webrtc, 2016-03-31 11:00:15]

Committed patchset #5 (id:80001) manually as
71bdda0ededda74cdd73a8309419cf626499627f (presubmit successful).

[commit-bot: I haz the power, 2016-03-31 11:00:15]

Patchset 5 (id:??) landed as
https://crrev.com/71bdda0ededda74cdd73a8309419cf626499627f
Cr-Commit-Position: refs/heads/master@{#12177}