Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by nisse-webrtc Modified:
4 years, 10 months ago Reviewers:
pbos-webrtc, perkj_webrtc, mflodman, pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntroduce 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}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Addressed simpler nits and naming issues. #Patch Set 3 : Move MediaConfig to mediachannel.h, and to the cricket namespace. #Patch Set 4 : Fix tests related to MediaConfig. #
Total comments: 2
Patch Set 5 : Added new peerconnection test for dscp constraint. #
Total comments: 26
Patch Set 6 : Improved test. #Patch Set 7 : Addressed Per's comments. #
Total comments: 6
Patch Set 8 : Split into three separate tests. #
Total comments: 13
Patch Set 9 : Fixed comments and other nits. #Patch Set 10 : Rebase (lots of files moving around). #Patch Set 11 : Formatting tweaks. #
Total comments: 7
Patch Set 12 : git cl format #
Total comments: 11
Patch Set 13 : Reorganize unittests. #Patch Set 14 : Rebase. #Patch Set 15 : Consolidate tests of default values. #Patch Set 16 : Deleted a left over comment. #
Total comments: 1
Patch Set 17 : Addressed test nit; use reference. #
Created: 4 years, 10 months ago
Messages
Total messages: 44 (10 generated)
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, pthatcher@webrtc.org
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?
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()?
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()?
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.
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.
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.
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.
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; }
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.
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.org changed reviewers: + perkj@webrtc.org
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 ?
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_.
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
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.
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?
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_; 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.
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?
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.
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.
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.
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
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.
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.
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.
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.
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 &
lgtm
The CQ bit was checked by nisse@webrtc.org
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.")
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.org changed reviewers: + mflodman@webrtc.org
Magnus, can you have a look? I lack owner's approval for the change to channelmanager.cc.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3401)
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 ==========
The CQ bit was checked by nisse@webrtc.org
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/51542be8ce35503c3cec4755ded415ecb1da2d37 Cr-Commit-Position: refs/heads/master@{#11595} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
