Introduce struct MediaConfig, with construction-time settings.

Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

TBR=pthatcher@webrtc.org
BUG=webrtc:5438
Committed: https://crrev.com/51542be8ce35503c3cec4755ded415ecb1da2d37
Cr-Commit-Position: refs/heads/master@{#11595}

[nisse-webrtc, 2016-02-05 14:27:52]

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

[nisse-webrtc, 2016-02-05 14:27:53]

Added a MediaConfig struct now. Tests need more work. A few stylistic questions:


In which file and in which namespace does struct MediaConfig belong?

Should MediaControllerInterface::config() return a pointer or a reference?

[pthatcher1, 2016-02-05 17:11:30]

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/fakemediacont...
File talk/app/webrtc/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/fakemediacont...
talk/app/webrtc/fakemediacontroller.h:49: const webrtc::MediaConfig* config()
const override {
I'd say make it a reference.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:41: // Construction-time settings, passedon
when creating MediaChannels.
passedon => passed on

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:43: // Set DSCP value for packet sent from
media channel. This flag
DSCP value for packet => DSCP on packets 
from media channel => (deleted)

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:49: // Enable WebRTC Cpu Overuse Detection.
This flag comes from the
Cpu => CPU

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:53: bool cpu_overuse_detection = true;
Please provide a blank line here

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:53: bool cpu_overuse_detection = true;
To be consistent, shouldn't this be enable_cpu_overuse_detection?

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:78: const MediaConfig& config,
I think it should be the first argument.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:84: // TODO(nisse): Reference or pointer?
Reference (or copy; it's so small it doesn't matter)

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession.cc
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession.cc:681: &audio_options_.combined_audio_video_bwe);
Shouldn't this also be part of MediaConfig?

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession_unittest.cc:4081: #if 0
Are you going to fix this in this CL?

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:49: #include
"talk/app/webrtc/mediacontroller.h"
We could put MediaConfig here in mediachannel.h or in its own mediaconfig.h. 
Putting it here seems wrong.

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:360: SetDscp(enable_dscp_ ?
MediaTypeDscpValue() : rtc::DSCP_DEFAULT);
Can you call this something like preferred_dscp() instead of
MediaTypeDscpValue()?

[nisse-webrtc, 2016-02-08 09:02:32]

I've addressed the simpler issues now. Next, I'll update tests.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:41: // Construction-time settings, passedon
when creating MediaChannels.
On 2016/02/05 17:11:30, pthatcher1 wrote:
> passedon => passed on

Done.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:43: // Set DSCP value for packet sent from
media channel. This flag
On 2016/02/05 17:11:30, pthatcher1 wrote:
> DSCP value for packet => DSCP on packets 
> from media channel => (deleted)

Done.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:49: // Enable WebRTC Cpu Overuse Detection.
This flag comes from the
On 2016/02/05 17:11:30, pthatcher1 wrote:
> Cpu => CPU

Done.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:53: bool cpu_overuse_detection = true;
On 2016/02/05 17:11:30, pthatcher1 wrote:
> To be consistent, shouldn't this be enable_cpu_overuse_detection?

Makes sense. (I just moved it over from VideoOptions without edits to name or
comments).

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:78: const MediaConfig& config,
On 2016/02/05 17:11:30, pthatcher1 wrote:
> I think it should be the first argument.

Done.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/mediacontroll...
talk/app/webrtc/mediacontroller.h:84: // TODO(nisse): Reference or pointer?
On 2016/02/05 17:11:30, pthatcher1 wrote:
> Reference (or copy; it's so small it doesn't matter)

Done.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession.cc
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession.cc:681: &audio_options_.combined_audio_video_bwe);
On 2016/02/05 17:11:30, pthatcher1 wrote:
> Shouldn't this also be part of MediaConfig?

Maybe. Can you explain briefly what it's intended to do?

Actually, it appears it is set here, from a constraint named
"googCombinedAudioVideoBwe", but then it's never used for anything.

We might need a CL deleting unused members of AudioOptions?

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession_unittest.cc:4081: #if 0
On 2016/02/05 17:11:30, pthatcher1 wrote:
> Are you going to fix this in this CL?

Yes. I think the previous cl (https://codereview.webrtc.org/1646253004/)
includes the needed changes, patchset 4, I just have to move it over.

The plan is to use FakeNetworkInterface and check the dscp property there, and
not use the options() method. 

(I'd actually prefer to delete the options() method, SetOptions should be able
to apply options in any way it needs without keeping a literal copy, but that's
unrelated to this cl).

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:49: #include
"talk/app/webrtc/mediacontroller.h"
On 2016/02/05 17:11:30, pthatcher1 wrote:
> We could put MediaConfig here in mediachannel.h or in its own mediaconfig.h. 
> Putting it here seems wrong.

mediachannel.h sounds reasonable to me. And then also in the cricket namespace?

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:360: SetDscp(enable_dscp_ ?
MediaTypeDscpValue() : rtc::DSCP_DEFAULT);
On 2016/02/05 17:11:30, pthatcher1 wrote:
> Can you call this something like preferred_dscp() instead of
> MediaTypeDscpValue()?

Sure. But for consistency, shouldn't the method name use CamelCase (ugly as it
is)? I.e., PreferredDscp()?

[pbos-webrtc, 2016-02-08 10:38:48]

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:360: SetDscp(enable_dscp_ ?
MediaTypeDscpValue() : rtc::DSCP_DEFAULT);
On 2016/02/08 09:02:32, nisse-webrtc wrote:
> On 2016/02/05 17:11:30, pthatcher1 wrote:
> > Can you call this something like preferred_dscp() instead of
> > MediaTypeDscpValue()?
> 
> Sure. But for consistency, shouldn't the method name use CamelCase (ugly as it
> is)? I.e., PreferredDscp()?

https://google.github.io/styleguide/cppguide.html#Function_Names

Regular functions have mixed case; "cheap" functions may use lower case with
underscores.

I think this qualifies as a cheap method, so I think you can choose.

[nisse-webrtc, 2016-02-08 12:22:31]

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession_unittest.cc:4081: #if 0
I'm getting into trouble with the webrtcsession unittests.

Problem is, the MediaController is created by PeerConnection::Initialize. So
populating MediaConfig, and looking for the corresponding constraints, is moved
to this method. After creating the MediaController, it creates a WebRtcSession
and passes the MediaController to it.

But in WebRtcSessionTest.TestDscpConstraint, there's no PeerConnection involved,
and hence the code to handle the constraint, in PeerConnection::Initialize,
simply has no place in the test. I guess the sanest solution is to somehow move
this test to the peerconnection unittests?

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1670153003/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:360: SetDscp(enable_dscp_ ?
MediaTypeDscpValue() : rtc::DSCP_DEFAULT);
On 2016/02/08 10:38:48, pbos-webrtc wrote:
> On 2016/02/08 09:02:32, nisse-webrtc wrote:
> > On 2016/02/05 17:11:30, pthatcher1 wrote:
> > > Can you call this something like preferred_dscp() instead of
> > > MediaTypeDscpValue()?
> > 
> > Sure. But for consistency, shouldn't the method name use CamelCase (ugly as
it
> > is)? I.e., PreferredDscp()?
> 
> https://google.github.io/styleguide/cppguide.html#Function_Names
> 
> Regular functions have mixed case; "cheap" functions may use lower case with
> underscores.
> 
> I think this qualifies as a cheap method, so I think you can choose.

Ok. I used CamelCase for now, but I can change it if anyone wants, I don't have
a strong opinion here.

[nisse-webrtc, 2016-02-08 14:34:08]

On 2016/02/08 12:22:31, nisse-webrtc wrote:
> But in WebRtcSessionTest.TestDscpConstraint, there's no PeerConnection
involved,
> and hence the code to handle the constraint, in PeerConnection::Initialize,
> simply has no place in the test. I guess the sanest solution is to somehow
move
> this test to the peerconnection unittests?

Discussed this with Per. We concluded that this test no longer makes much sense
in webrtcsession_unittests. However, it would be nice with some peerconnection
unittest that exercises the code looking for this constraint.

But both the peerconnection code and the test code is a bit intimitating...
Maybe one can inherit PeerConnectionFactory, override CreateMediaController with
a method that saves a copy of the MediaConfig.

It's not clear to me of the peerconnection_unittests and
peerconnectioninterface_unittest are intended as simple unittests mocking most
of the environment, or if they're actually exchanging real packets over
loopback. So another alternative is to try to get a FakeNetworkInterface in
somewhere, and check its dscp() method (like the non-working
WebRtcSessionTest.TestDscpConstraint in the current cl).

Or yet another alternative might be to extract the conversion from constraints
to MediaConfig to a separate method and write a unittest for that conversion
only.

[pthatcher1, 2016-02-08 18:58:39]

On 2016/02/08 14:34:08, nisse-webrtc wrote:
> On 2016/02/08 12:22:31, nisse-webrtc wrote:
> > But in WebRtcSessionTest.TestDscpConstraint, there's no PeerConnection
> involved,
> > and hence the code to handle the constraint, in PeerConnection::Initialize,
> > simply has no place in the test. I guess the sanest solution is to somehow
> move
> > this test to the peerconnection unittests?
> 
> Discussed this with Per. We concluded that this test no longer makes much
sense
> in webrtcsession_unittests. However, it would be nice with some peerconnection
> unittest that exercises the code looking for this constraint.
> 
> But both the peerconnection code and the test code is a bit intimitating...
> Maybe one can inherit PeerConnectionFactory, override CreateMediaController
with
> a method that saves a copy of the MediaConfig.
> 
> It's not clear to me of the peerconnection_unittests and
> peerconnectioninterface_unittest are intended as simple unittests mocking most
> of the environment, or if they're actually exchanging real packets over
> loopback. So another alternative is to try to get a FakeNetworkInterface in
> somewhere, and check its dscp() method (like the non-working
> WebRtcSessionTest.TestDscpConstraint in the current cl).
> 
> Or yet another alternative might be to extract the conversion from constraints
> to MediaConfig to a separate method and write a unittest for that conversion
> only.

I think it would be best to just test the "convert and pass into
MediaController" step.  The proposal you have of overriding
CreateMediaController sounds like a good option to me.

[pthatcher1, 2016-02-08 19:03:01]

Seems like the unit test is the only thing remaining.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession.cc
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession.cc:681: &audio_options_.combined_audio_video_bwe);
On 2016/02/08 09:02:32, nisse-webrtc wrote:
> On 2016/02/05 17:11:30, pthatcher1 wrote:
> > Shouldn't this also be part of MediaConfig?
> 
> Maybe. Can you explain briefly what it's intended to do?
> 
> Actually, it appears it is set here, from a constraint named
> "googCombinedAudioVideoBwe", but then it's never used for anything.
> 
> We might need a CL deleting unused members of AudioOptions?

I didn't realize it was dead code.  Can you put a TODO at least?  You can do
"TODO(pthatcher): Remove because it's not used anywhere".

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession_unittest.cc:4081: #if 0
On 2016/02/08 12:22:30, nisse-webrtc wrote:
> I'm getting into trouble with the webrtcsession unittests.
> 
> Problem is, the MediaController is created by PeerConnection::Initialize. So
> populating MediaConfig, and looking for the corresponding constraints, is
moved
> to this method. After creating the MediaController, it creates a WebRtcSession
> and passes the MediaController to it.
> 
> But in WebRtcSessionTest.TestDscpConstraint, there's no PeerConnection
involved,
> and hence the code to handle the constraint, in PeerConnection::Initialize,
> simply has no place in the test. I guess the sanest solution is to somehow
move
> this test to the peerconnection unittests?

I agree that the right thing to do is move it to peerconnection unittests.

https://codereview.webrtc.org/1670153003/diff/60001/webrtc/media/webrtc/webrt...
File webrtc/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/60001/webrtc/media/webrtc/webrt...
webrtc/media/webrtc/webrtcvideoengine2_unittest.cc:1722:
media_config.enable_cpu_overuse_detection = false;
In this area of the code, we use the {}s (don't let pbos tell you otherwise :).

if (foo) {
  bar;
}

[nisse-webrtc, 2016-02-09 09:01:38]

Wrote a new test. Not entirely sure where it belongs.

And I still think it would be nice to check that the dscp setting is propageted
down to the MediaChannel, but I don't know how to do that.

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession.cc
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1670153003/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsession.cc:681: &audio_options_.combined_audio_video_bwe);
On 2016/02/08 19:03:01, pthatcher1 wrote:
> On 2016/02/08 09:02:32, nisse-webrtc wrote:
> > On 2016/02/05 17:11:30, pthatcher1 wrote:
> > > Shouldn't this also be part of MediaConfig?
> > 
> > Maybe. Can you explain briefly what it's intended to do?
> > 
> > Actually, it appears it is set here, from a constraint named
> > "googCombinedAudioVideoBwe", but then it's never used for anything.
> > 
> > We might need a CL deleting unused members of AudioOptions?
> 
> I didn't realize it was dead code.  Can you put a TODO at least?  You can do
> "TODO(pthatcher): Remove because it's not used anywhere".

Done.

https://codereview.webrtc.org/1670153003/diff/60001/webrtc/media/webrtc/webrt...
File webrtc/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/60001/webrtc/media/webrtc/webrt...
webrtc/media/webrtc/webrtcvideoengine2_unittest.cc:1722:
media_config.enable_cpu_overuse_detection = false;
On 2016/02/08 19:03:01, pthatcher1 wrote:
> In this area of the code, we use the {}s (don't let pbos tell you otherwise
:).
> 
> if (foo) {
>   bar;
> }

Done.

[pbos-webrtc, 2016-02-09 15:31:44]

Think this tests makes sense, pthatcher@ might be more familiar with if this is
how they're written. Haven't done a test here.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2371: mutable bool
createmediacontroller_called = false;
create_media_controller_called

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2377: TEST(PeerConnection,
TestDscpConstraint) {
Can you add tests for the other options as well?

[perkj_webrtc, 2016-02-09 15:35:36]

perkj@webrtc.org changed reviewers:
+ perkj@webrtc.org

[perkj_webrtc, 2016-02-09 15:35:37]

drive be comments. Hopefully  the comments  make it easier to land this...

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/fakemedia...
File talk/app/webrtc/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/fakemedia...
talk/app/webrtc/fakemediacontroller.h:51: static const MediaConfig media_config
= MediaConfig();
nit: add as a member instead.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2371: mutable bool
createmediacontroller_called = false;
why mutable  ?

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2371: mutable bool
createmediacontroller_called = false;
member variables should end with _, iereatemediacontroller_called_

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2372: mutable
cricket::MediaConfig createmediacontroller_config;
dito

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2382:
MockPeerConnectionObserver observer;
move to where it is used.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2383:
scoped_refptr<PeerConnectionInterface> pc;
declare when used.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2385:
webrtc::MediaConstraintsInterface::kEnableDscp, true);
also test calling with kEnableDscp false ?

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:605:
rtc::Optional<bool>(rtc_configuration.disable_prerenderer_smoothing);
where do you set disable_prerenderer_smoothing now? It make sence to me to have
this kind of settings in PeerConnectionInterface::RTCConfiguration .

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:675:
SetOptionFromOptionalConstraint(constraints,
Add test for this as well?

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:108: // Enable WebRTC CPU Overuse Detection.
This flag comes from the
We normally descibe the purpose. Not the exact call chain since it will most
probably change over time.

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:121: // This flag comes from PeerConnection's
RtcConfiguration, but is
dito

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:462: private:
remove private:

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

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/webrtc/webrt...
webrtc/media/webrtc/webrtcvideoengine2.cc:634: signal_cpu_adaptation_ =
config.enable_cpu_overuse_detection;
move to colon list and make const

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/webrtc/webrt...
File webrtc/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/webrtc/webrt...
webrtc/media/webrtc/webrtcvideoengine2.h:503: bool signal_cpu_adaptation_;
const ?

[nisse-webrtc, 2016-02-10 08:53:00]

I think I've addressed everything now, except for the FakeMediaController
media_config nit.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/fakemedia...
File talk/app/webrtc/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/fakemedia...
talk/app/webrtc/fakemediacontroller.h:51: static const MediaConfig media_config
= MediaConfig();
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> nit: add as a member instead.

I can do that, but in my opinion, a local definition is more readable.

Update: I tried it, and C++ doesn't like me... I want this object to be
statically allocated read-only data, set to the default values specified in the
declaration of the MediaConfig struct. So I wrote 

 private:
  static const MediaConfig media_config_ = MediaConfig();

I write the initializer "= MediaConfig()" because I want it properly initialized
with default values (do I get that if I drop the initializer?). And then the
compiler complains, "in-class initializer for static data member of type 'const
cricket::MediaConfig' requires 'constexpr' specifier". But I don't want any
fancy compile-time computation, I just want an initializer equivalent to writing
"= { false, true, false }" in C.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2371: mutable bool
createmediacontroller_called = false;
On 2016/02/09 15:31:43, pbos-webrtc wrote:
> create_media_controller_called

Done.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2371: mutable bool
createmediacontroller_called = false;
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> member variables should end with _, iereatemediacontroller_called_

Done.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2371: mutable bool
createmediacontroller_called = false;
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> why mutable  ?

The CreateMediaController method is const. So I need to cheat to be able to
modify those members. One alternative is to move the variables out to a separate
struct and keep a reference to them, but using mutable seems simpler.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2377: TEST(PeerConnection,
TestDscpConstraint) {
On 2016/02/09 15:31:43, pbos-webrtc wrote:
> Can you add tests for the other options as well? 

I guess so. We just need to find the right balance. There are two constraints
which can be true, false or unset. And one flag copied from RTCConfiguration. So
a total of 18 combinations. 

I'm adding code to test a few selected combinations.

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1670153003/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:605:
rtc::Optional<bool>(rtc_configuration.disable_prerenderer_smoothing);
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> where do you set disable_prerenderer_smoothing now? It make sence to me to
have
> this kind of settings in PeerConnectionInterface::RTCConfiguration .

It's copied from RTCConfiguration to MediaConfig. But it is now done by
PeerConnection instead of WebRtcSession.

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:108: // Enable WebRTC CPU Overuse Detection.
This flag comes from the
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> We normally descibe the purpose. Not the exact call chain since it will most
> probably change over time.

I understand this comment is going to get less accurate over time, but I really
want to have some notes on the current use, and I think it's going to be worth
the effort to try to keep it updated as the code changes. Wording was copied
over from the previous location in VideoOptions.

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:462: private:
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> remove private:

Done.

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

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/webrtc/webrt...
webrtc/media/webrtc/webrtcvideoengine2.cc:634: signal_cpu_adaptation_ =
config.enable_cpu_overuse_detection;
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> move to colon list and make const

Done (and again, similarly, for MediaChannel::enable_dscp_).

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/webrtc/webrt...
File webrtc/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1670153003/diff/80001/webrtc/media/webrtc/webrt...
webrtc/media/webrtc/webrtcvideoengine2.h:503: bool signal_cpu_adaptation_;
On 2016/02/09 15:35:37, perkj_webrtc wrote:
> const ?

Done. And similarly for MediaChannel::enable_dscp_.

[perkj_webrtc, 2016-02-10 10:41:27]

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/fakemedi...
File talk/app/webrtc/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/fakemedi...
talk/app/webrtc/fakemediacontroller.h:51: static const MediaConfig media_config
= MediaConfig();
remove static. make  media_config_ a const member. 

const MediaConfig media_config = MediaConfig();

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2379: TEST(PeerConnection,
TestSettingOfMediaConfig) {
please split into separate tests. 
1. Test constrinats webrtc::MediaConstraintsInterface::kEnableDscp,
2. Test enable_cpu_overuse_detection 
webrtc::MediaConstraintsInterface::kCpuOveruseDetection
3. Test PeerConnectionInterface::RTCConfiguration config
disable_prerenderer_smoothing

https://codereview.webrtc.org/1670153003/diff/120001/webrtc/media/webrtc/null...
File webrtc/media/webrtc/nullwebrtcvideoengine.h (right):

https://codereview.webrtc.org/1670153003/diff/120001/webrtc/media/webrtc/null...
webrtc/media/webrtc/nullwebrtcvideoengine.h:39: }  // namespace webrtc
nit empty line before end of ns

[nisse-webrtc, 2016-02-10 13:42:01]

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/fakemedi...
File talk/app/webrtc/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/fakemedi...
talk/app/webrtc/fakemediacontroller.h:51: static const MediaConfig media_config
= MediaConfig();
On 2016/02/10 10:41:26, perkj_webrtc wrote:
> remove static. make  media_config_ a const member. 
> 
> const MediaConfig media_config = MediaConfig();

Done.

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2379: TEST(PeerConnection,
TestSettingOfMediaConfig) {
On 2016/02/10 10:41:26, perkj_webrtc wrote:
> please split into separate tests. 
> 1. Test constrinats webrtc::MediaConstraintsInterface::kEnableDscp,
> 2. Test enable_cpu_overuse_detection 
> webrtc::MediaConstraintsInterface::kCpuOveruseDetection
> 3. Test PeerConnectionInterface::RTCConfiguration config
> disable_prerenderer_smoothing

Done.

https://codereview.webrtc.org/1670153003/diff/120001/webrtc/media/webrtc/null...
File webrtc/media/webrtc/nullwebrtcvideoengine.h (right):

https://codereview.webrtc.org/1670153003/diff/120001/webrtc/media/webrtc/null...
webrtc/media/webrtc/nullwebrtcvideoengine.h:39: }  // namespace webrtc
On 2016/02/10 10:41:26, perkj_webrtc wrote:
> nit empty line before end of ns

Done.

[pthatcher1, 2016-02-10 19:24:23]

lgtm

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2377: mutable
cricket::MediaConfig create_media_controller_config_;
What's the purpose of marking these "mutable"?  So that you can change them on a
const value?

[pthatcher1, 2016-02-10 19:25:01]

lgtm

[nisse-webrtc, 2016-02-11 07:54:34]

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2377: mutable
cricket::MediaConfig create_media_controller_config_;
On 2016/02/10 19:24:23, pthatcher1 wrote:
> What's the purpose of marking these "mutable"?  So that you can change them on
a
> const value?

So I can change them in the CreateMediaController method, which is
const-declared. It's cheating, but I think it's the simplest way to solve the
problem.

[perkj_webrtc, 2016-02-11 08:04:06]

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2362: 
nit: remove empty line

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2377: mutable
cricket::MediaConfig create_media_controller_config_;
On 2016/02/11 07:54:33, nisse-webrtc wrote:
> On 2016/02/10 19:24:23, pthatcher1 wrote:
> > What's the purpose of marking these "mutable"?  So that you can change them
on
> a
> > const value?
> 
> So I can change them in the CreateMediaController method, which is
> const-declared. It's cheating, but I think it's the simplest way to solve the
> problem.

maybe add a comment.

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2389: // Without constraint,
default value is true
false

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2397: EXPECT_TRUE(pc.get()
!= nullptr);
nit: ASSERT_TRUE - otherwise the test will crash below if this fail.

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2401: // With constraint set
to false
to true

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2423: // Without constraint,
default value is false
true

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2431: EXPECT_TRUE(pc.get()
!= nullptr);
same as above

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2467: EXPECT_TRUE(pc.get()
!= nullptr);
ASSERT....

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:60: #include
"webrtc/media/base/fakenetworkinterface.h"
used?

[nisse-webrtc, 2016-02-11 09:16:00]

Rebased, maybe you want to have another look before I try to commit.

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface_unittest.cc:2397: EXPECT_TRUE(pc.get()
!= nullptr);
On 2016/02/11 08:04:06, perkj_webrtc wrote:
> nit: ASSERT_TRUE - otherwise the test will crash below if this fail.

Done. I didn't know, and I've had precisely that problem with other tests. Also
fixed the comments.

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/140001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:60: #include
"webrtc/media/base/fakenetworkinterface.h"
On 2016/02/11 08:04:06, perkj_webrtc wrote:
> used?

Done.

[pbos-webrtc, 2016-02-11 09:22:16]

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2349: mutable bool
create_media_controller_called_ = false;
If these aren't race prone then I think you can actually move these to static
global g_media_controller_called_; and initialize them in an overridden SetUp
method, but this is probably still cleaner.

[nisse-webrtc, 2016-02-11 09:31:32]

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2349: mutable bool
create_media_controller_called_ = false;
On 2016/02/11 09:22:16, pbos-webrtc wrote:
> If these aren't race prone then I think you can actually move these to static
> global g_media_controller_called_; and initialize them in an overridden SetUp
> method, but this is probably still cleaner.

To get a SetUp method, I'd need to define a "test fixture class" and use TEST_F
rather than TEST? I'd prefer to keep it as is.

As for races, I think everything happens on the same thread, but I'm not 100%
sure. I'm quite sure there's no actual "race"; the writes should happen before
CreatePeerConnection returns. But if needed, it should be no big deal to add a
critical section or whatever is needed to generate appropriate memory barriers
to ensure new values are visible to other threads.

[pbos-webrtc, 2016-02-11 11:34:10]

lgtm, this looks a lot better, woo!

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/fakemediacont...
File webrtc/api/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/fakemediacont...
webrtc/api/fakemediacontroller.h:38: const MediaConfig media_config_ =
MediaConfig();
Don't need the initialization here, media_config_ should default initialize.

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2349: mutable bool
create_media_controller_called_ = false;
On 2016/02/11 09:31:32, nisse-webrtc wrote:
> On 2016/02/11 09:22:16, pbos-webrtc wrote:
> > If these aren't race prone then I think you can actually move these to
static
> > global g_media_controller_called_; and initialize them in an overridden
SetUp
> > method, but this is probably still cleaner.
> 
> To get a SetUp method, I'd need to define a "test fixture class" and use
TEST_F
> rather than TEST? I'd prefer to keep it as is.
> 
> As for races, I think everything happens on the same thread, but I'm not 100%
> sure. I'm quite sure there's no actual "race"; the writes should happen before
> CreatePeerConnection returns. But if needed, it should be no big deal to add a
> critical section or whatever is needed to generate appropriate memory barriers
> to ensure new values are visible to other threads.

I think that's fine, you can also use rtc::AtomicOps on volatile ints, but given
that this is single threaded go with this.

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideoengine2.cc:610: : VideoMediaChannel(config),
call_(call),
git cl format

[nisse-webrtc, 2016-02-11 12:10:18]

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/fakemediacont...
File webrtc/api/fakemediacontroller.h (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/api/fakemediacont...
webrtc/api/fakemediacontroller.h:38: const MediaConfig media_config_ =
MediaConfig();
On 2016/02/11 11:34:10, pbos-webrtc wrote:
> Don't need the initialization here, media_config_ should default initialize.

Unfortunately, the compiler doesn't agree. If I remove the initializer I get 

error: constructor for 'cricket::FakeMediaController' must explicitly initialize
the const member 'media_config_'

I'm afraid I don't understand the fine details about default initialization in
C++. I could move the initializer to the constructor :-list, but I don't think
that makes the code clearer.

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1670153003/diff/200001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideoengine2.cc:610: : VideoMediaChannel(config),
call_(call),
On 2016/02/11 11:34:10, pbos-webrtc wrote:
> git cl format

Done.

[perkj_webrtc, 2016-02-11 12:32:08]

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2362: // Without constraint,
default value is false
nit add . Write complete sentences.

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2363:
scoped_refptr<PeerConnectionInterface> pc;
nit: move to where it is used. ie 
scoped_refptr<PeerConnectionInterface> pc( pcf->.....

here and below.

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2381: EXPECT_TRUE(pc.get() !=
nullptr);
ASSERT_TRUE

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2445:
pcf->create_media_controller_called_ = false;
right - this should actually be 6 tests instead to avoid this.

I would use TEST_F with a test fixture to setup the test. Then create a method
where I provide a RTCConfiguration and put in the constraints. Then only check
the expectations in each test.

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideoengine2.cc:1817: 1000 *
parameters_.options.screencast_min_bitrate_kbps.value_or(0);
is this intentional?  This looks like a change in behaviour.

[pbos-webrtc, 2016-02-11 12:50:29]

ack, lgtm

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideoengine2.cc:1817: 1000 *
parameters_.options.screencast_min_bitrate_kbps.value_or(0);
On 2016/02/11 12:32:08, perkj_webrtc wrote:
> is this intentional?  This looks like a change in behaviour. 

Looks fine, the default value was:

options_.screencast_min_bitrate_kbps = rtc::Optional<int>(0);

in SetDefaultOptions, so it was always set but would default to zero.

[nisse-webrtc, 2016-02-11 13:45:08]

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2362: // Without constraint,
default value is false
On 2016/02/11 12:32:08, perkj_webrtc wrote:
> nit add . Write complete sentences.

Done.

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2363:
scoped_refptr<PeerConnectionInterface> pc;
On 2016/02/11 12:32:07, perkj_webrtc wrote:
> nit: move to where it is used. ie 
> scoped_refptr<PeerConnectionInterface> pc( pcf->.....
> 
> here and below.

I did it this way because I wanted both calls 
pc = pcf->CreatePeerConnection to have the same form.

Anyway, this will likely be moot when I rewrite the tests per your other
comment.

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2381: EXPECT_TRUE(pc.get() !=
nullptr);
On 2016/02/11 12:32:08, perkj_webrtc wrote:
> ASSERT_TRUE

Done.

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2445:
pcf->create_media_controller_called_ = false;
On 2016/02/11 12:32:07, perkj_webrtc wrote:
> right - this should actually be 6 tests instead to avoid this.
> 
> I would use TEST_F with a test fixture to setup the test. Then create a method
> where I provide a RTCConfiguration and put in the constraints. Then only check
> the expectations in each test. 

I'll do. I guess we'll need a few iterations more...

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1670153003/diff/220001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideoengine2.cc:1817: 1000 *
parameters_.options.screencast_min_bitrate_kbps.value_or(0);
On 2016/02/11 12:32:08, perkj_webrtc wrote:
> is this intentional?  This looks like a change in behaviour. 

There are two changes here, the factor 1000 moved from the end to the start of
the expression, and the value_or thing. The latter replaces logic in the deleted
SetDefaultsOptions.

[perkj_webrtc, 2016-02-12 09:19:24]

lgtm

https://codereview.webrtc.org/1670153003/diff/300001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1670153003/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2359: const cricket::MediaConfig
TestCreatePeerConnection(
nit &

[perkj_webrtc, 2016-02-12 09:19:27]

lgtm

[nisse-webrtc, 2016-02-12 09:23:36]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-02-12 09:23:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pbos@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1670153003/#ps320001 (title:
"Addressed test nit; use reference.")

[commit-bot: I haz the power, 2016-02-12 09:23:44]

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

[nisse-webrtc, 2016-02-12 09:30:11]

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

[nisse-webrtc, 2016-02-12 09:30:12]

Magnus, can you have a look? I lack owner's approval for the change to
channelmanager.cc.

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

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

[commit-bot: I haz the power, 2016-02-12 09:36:37]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3401)

[nisse-webrtc, 2016-02-12 10:25:26]

Description was changed from

==========
Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

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

to

==========
Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

TBR=pthatcher@webrtc.org
BUG=webrtc:5438
==========

[nisse-webrtc, 2016-02-12 10:25:52]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-02-12 10:25:59]

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

[commit-bot: I haz the power, 2016-02-12 10:27:09]

Description was changed from

==========
Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

TBR=pthatcher@webrtc.org
BUG=webrtc:5438
==========

to

==========
Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

TBR=pthatcher@webrtc.org
BUG=webrtc:5438
==========

[commit-bot: I haz the power, 2016-02-12 10:27:11]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-02-12 10:27:20]

Description was changed from

==========
Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

TBR=pthatcher@webrtc.org
BUG=webrtc:5438
==========

to

==========
Introduce struct MediaConfig, with construction-time settings.

Pass it to MediaController constructor and down to WebRtcVideoEngine2
and WebRtcVoiceEngine.

Follows discussion on https://codereview.webrtc.org/1646253004/

TBR=pthatcher@webrtc.org
BUG=webrtc:5438

Committed: https://crrev.com/51542be8ce35503c3cec4755ded415ecb1da2d37
Cr-Commit-Position: refs/heads/master@{#11595}
==========

[commit-bot: I haz the power, 2016-02-12 10:27:22]

Patchset 17 (id:??) landed as
https://crrev.com/51542be8ce35503c3cec4755ded415ecb1da2d37
Cr-Commit-Position: refs/heads/master@{#11595}