Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1717583002:
Non-constraint interfaces for all constrainable interfaces (Closed)
|
Created:
4 years, 10 months ago by hta-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, pthatcher1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis CL provides interfaces that do not use constraints for
all interfaces that formerly took constraints parameters
in name=value form.
This is in preparation for making Chrome only use these
explicit interfaces.
BUG=webrtc:4906
Committed: https://crrev.com/a2a49d9d9c65cf6d080811e6c2755cba9cb52d32
Cr-Commit-Position: refs/heads/master@{#11870}
Patch Set 1 : Working APIs for all formerly constrainable APIs #Patch Set 2 : Fix an ambiguous function #
Total comments: 32
Patch Set 3 : Some review comments addressed #Patch Set 4 : Changed API to use rtc::Optional #Patch Set 5 : Rebase result (no intentional change) #
Total comments: 5
Patch Set 6 : Some redesign of PeerConnection creation API #
Total comments: 6
Patch Set 7 : Removing a TODO #
Total comments: 14
Patch Set 8 : Default implementation for deprecated virtual functions #Patch Set 9 : Rebased, moved a helper function #
Total comments: 4
Patch Set 10 : Redesigned webrtcsession_unittest #Patch Set 11 : Renaming a function #
Total comments: 2
Patch Set 12 : Fix videosource API change #Patch Set 13 : Work around an Android gtest issue #Patch Set 14 : Redesign of CopyConstraintsIntoRtcConfiguration #
Total comments: 10
Patch Set 15 : Review comments #
Messages
Total messages: 103 (43 generated)
Description was changed from ========== LocalAudio with audio options This adds a constructor for LocalAudio that takes cricket::audio_options rather than a Constraints construct. Long term goal is to remove the Constraints construct. BUG= ========== to ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=4906 ==========
hta@webrtc.org changed reviewers: + tommi@webrtc.org
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Dry run: Exceeded global retry quota
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7632) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7550)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5338) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7668) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11493)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5340) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7670) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7587) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11495)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hta@webrtc.org changed reviewers: + perkj@webrtc.org
Description was changed from ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=4906 ========== to ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=webrtc:4906 ==========
perkj@webrtc.org changed reviewers: + nisse@webrtc.org, pthatcher@webrtc.org
I think it would be nice to get rid of the generic constraints if they are no longer needed. But please also let phatcher and nisse take a look since nisse is also working in this area. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... webrtc/api/localaudiosource.cc:108: if (!audio_options) I think you should allow audio_options == nullptr ie- source_state_ = kLive. Even better- make sure source_state_(kLive) in the ctor and remove setting it here and in the default Initialize. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1584: if (!ConvertRtcOptionsForAnswer(options, session_options)) { This is code duplication. Please create a helper method that both version uses. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:2135: void CopyConstraintsIntoRtcConfiguration( Please move this to the factory where it is used in an anonymous ns. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:223: rtc::scoped_refptr<VideoSource> source( Just call the other implementation with constraints nullptr? Why add this method at all? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:278: CopyConstraintsIntoRtcConfiguration(constraints, &configuration); Move the implementation of CopyConstraintsIntoRtcConfiguration to this file. Also move CopyConstraintsIntoRtcConfiguration to the line below Interface::RTCConfiguration configuration =... https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactoryproxy.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactoryproxy.h:83: cricket::PortAllocator* a3, a2, a3 .... https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:322: struct CreateOptions {}; unused? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:437: virtual bool UpdateIce(const IceServers& configuration) { return false; } not part of this cl? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:557: virtual rtc::scoped_refptr<PeerConnectionInterface> CreatePeerConnection( Mark as deprecated? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:575: virtual rtc::scoped_refptr<AudioSourceInterface> CreateAudioSource( Mark as deprecated? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:583: virtual rtc::scoped_refptr<VideoSourceInterface> CreateVideoSource( Mark as deprecated? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/videosource.h File webrtc/api/videosource.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/videosource.h#... webrtc/api/videosource.h:58: static rtc::scoped_refptr<VideoSource> Create( Remove this method? Just use the above with constraints = nullptr where it is needed. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:390: CopyConstraintsIntoRtcConfiguration(constraints_.get(), &rtc_configuration); replace all use of constraints in this file since this file no longer use constraints. Might need to add separate tests where you do the actual conversion.
https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:223: rtc::scoped_refptr<VideoSource> source( On 2016/02/23 11:40:18, perkj_webrtc wrote: > Just call the other implementation with constraints nullptr? > > Why add this method at all? I would have expected this function to get some type of options-struct as argument, instead of the the constraints. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:250: bool enable_dscp; If an "override"-flag *really* is needed, I guess using rtc::Optional is the right tool. I don't understand why that would be needed here, though. Some of these flags, in particular enable_dscp, enable_cpu_overuse_detection, and disable_prerenderer_smoothing now live in struct MediaConfig (see cl https://codereview.webrtc.org/1670153003/). There's code in PeerConnection::Initialize to populate MediaConfig from constraints and from RTCConfiguration. If constraints-parsing is moved elsewhere, maybe it would make sense to have RTCConfiguration include a struct MediaConfig? The fewer places we need to declare the flags and copy them around, the better.
Some comments on the comments. Addressed the simplest ones. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... webrtc/api/localaudiosource.cc:108: if (!audio_options) On 2016/02/23 11:40:18, perkj_webrtc wrote: > I think you should allow audio_options == nullptr > > ie- source_state_ = kLive. > > Even better- make sure source_state_(kLive) in the ctor and remove setting it > here and in the default Initialize. The constructor sets source_state_(kInitializing). I thought this code path strange, but didn't want to change behavior, so I reproduced the previous behavior, even though it's bizarre. TODO? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1584: if (!ConvertRtcOptionsForAnswer(options, session_options)) { On 2016/02/23 11:40:18, perkj_webrtc wrote: > This is code duplication. Please create a helper method that both version uses. Done. We can inline it again when there's only one caller. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:2135: void CopyConstraintsIntoRtcConfiguration( On 2016/02/23 11:40:18, perkj_webrtc wrote: > Please move this to the factory where it is used in an anonymous ns. The other user is webrtcsession_unittest, which doesn't know about peerconnections. Suggestions? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:223: rtc::scoped_refptr<VideoSource> source( On 2016/02/23 13:05:32, nisse-webrtc wrote: > On 2016/02/23 11:40:18, perkj_webrtc wrote: > > Just call the other implementation with constraints nullptr? > > > > Why add this method at all? > > I would have expected this function to get some type of options-struct as > argument, instead of the the constraints. This pair is one where the functionality we reach at the bottom is slightly different - the constraint-less version can't filter formats on width/height/framerate. See comments in videosource.h. But it doesn't use any other constraints. So I'd like to keep the two paths separate, and the fact that they call the same code at the bottom as an implementation detail. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory.cc:278: CopyConstraintsIntoRtcConfiguration(constraints, &configuration); On 2016/02/23 11:40:18, perkj_webrtc wrote: > Move the implementation of CopyConstraintsIntoRtcConfiguration to this file. > Also move CopyConstraintsIntoRtcConfiguration to the line below > Interface::RTCConfiguration configuration =... See comments about unittests. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:250: bool enable_dscp; On 2016/02/23 13:05:32, nisse-webrtc wrote: > If an "override"-flag *really* is needed, I guess using rtc::Optional is the > right tool. I don't understand why that would be needed here, though. One of these (I think it was dscp) is controlled by a Finch experiment unless set by constraint. So I need override or rtc::optional for that. At the moment there isn't a single rtc::optional being constructed in Chromium (as far as I can see): codesearch for "rtc::optional -file:third_party/webrtc" gave no results. Are we exposing them on the WebRTC external interface already? > Some of these flags, in particular enable_dscp, enable_cpu_overuse_detection, > and disable_prerenderer_smoothing now live in struct MediaConfig (see cl > https://codereview.webrtc.org/1670153003/). > > There's code in PeerConnection::Initialize to populate MediaConfig from > constraints and from RTCConfiguration. If constraints-parsing is moved > elsewhere, maybe it would make sense to have RTCConfiguration include a struct > MediaConfig? > > The fewer places we need to declare the flags and copy them around, the better. Yes. Are there more flags that can be eaten by a MediaConfig? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:322: struct CreateOptions {}; On 2016/02/23 11:40:18, perkj_webrtc wrote: > unused? Removing. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:437: virtual bool UpdateIce(const IceServers& configuration) { return false; } On 2016/02/23 11:40:18, perkj_webrtc wrote: > not part of this cl? Needed to remove all APIs with MediaConstraintsInterface* parameters. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:557: virtual rtc::scoped_refptr<PeerConnectionInterface> CreatePeerConnection( On 2016/02/23 11:40:18, perkj_webrtc wrote: > Mark as deprecated? Just add // deprecated in a comment above, or is there another way to do it? https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/videosource.h File webrtc/api/videosource.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/videosource.h#... webrtc/api/videosource.h:58: static rtc::scoped_refptr<VideoSource> Create( On 2016/02/23 11:40:18, perkj_webrtc wrote: > Remove this method? Just use the above with constraints = nullptr where it is > needed. Part of the video path - see previous comment about differing functionality. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:390: CopyConstraintsIntoRtcConfiguration(constraints_.get(), &rtc_configuration); On 2016/02/23 11:40:18, perkj_webrtc wrote: > replace all use of constraints in this file since this file no longer use > constraints. > Might need to add separate tests where you do the actual conversion. Unfortunately there's an awful lot of constraints in this file :-( They're just hidden in the "test/fakeconstraints.cc" source file. It's a major job to reconstruct the tests for non-constraints use.
https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... webrtc/api/localaudiosource.cc:108: if (!audio_options) On 2016/02/23 14:30:34, hta-webrtc wrote: > On 2016/02/23 11:40:18, perkj_webrtc wrote: > > I think you should allow audio_options == nullptr > > > > ie- source_state_ = kLive. > > > > Even better- make sure source_state_(kLive) in the ctor and remove setting it > > here and in the default Initialize. > > The constructor sets source_state_(kInitializing). I thought this code path > strange, but didn't want to change behavior, so I reproduced the previous > behavior, even though it's bizarre. TODO? If it's possible to delete all code dealing with a state of "existing, but somehow invalid", that would be nice. I'm not sure what the conventions are in our code, but it seems a static Create method which does new + Initialize is used when initialization needs to call virtual functions, which doesn't quite work in the constructor. But when doing things this way, the constructor is usually not public, and hopefully subclasses can be trusted to do correct initialization so we don't need code to check for misuse or track the unititialized state. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:250: bool enable_dscp; On 2016/02/23 14:30:34, hta-webrtc wrote: > On 2016/02/23 13:05:32, nisse-webrtc wrote: > > If an "override"-flag *really* is needed, I guess using rtc::Optional is the > > right tool. I don't understand why that would be needed here, though. > > One of these (I think it was dscp) is controlled by a Finch experiment unless > set by constraint. So I need override or rtc::optional for that. If the default value needs to be configurable at runtime, I'd prefer to have a separate setting for that. Something like foo_flag = settings.default_foo; /* Override the compile-time default */ process_foo_constraint (&foo_flag, kFooConstraint, constraints); where assignment order is the order of increasing precedence. For dscp, I think it should be straight forward. The run-time default would be a member of RTCConfiguration, I guess, and the place for the above logic is where populating MediaConfig, in PeerConnection::Initialize, before calling CreateMediaController. > Yes. Are there more flags that can be eaten by a MediaConfig? Not sure. MediaConfig is intended for construction time settings for MediaChannel. For video, there are only three (four once my is_screencast cl is landed) remaining members in VideoOptions. They can be split by several different dimensions: By origin (from video source properties or from constraints). By time of application (construction time, or set midstream using SetOptions or SetSendParameters). By scope (applied too all streams in a peerconnections, or possibly different settings for different streams streams). Still a bit unclear what should happen, but I think it will be split in some way, and I'd prefer to get rid of rtc::Optional in the process. So in current RTCConfiguration, I see nothing besides disable_prerendering_smoothing which obviously could be eaten by MediaConfig. And for AudioOptions, some initial cleaning is needed before considering a reorganization, since not all current members are in use.
Commentary about initialization code... https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:250: bool enable_dscp; On 2016/02/23 15:11:02, nisse-webrtc wrote: > On 2016/02/23 14:30:34, hta-webrtc wrote: > > On 2016/02/23 13:05:32, nisse-webrtc wrote: > > > If an "override"-flag *really* is needed, I guess using rtc::Optional is the > > > right tool. I don't understand why that would be needed here, though. > > > > One of these (I think it was dscp) is controlled by a Finch experiment unless > > set by constraint. So I need override or rtc::optional for that. > > If the default value needs to be configurable at runtime, I'd prefer to have a > separate setting for that. Something like > > foo_flag = settings.default_foo; /* Override the compile-time default */ > process_foo_constraint (&foo_flag, kFooConstraint, constraints); > > where assignment order is the order of increasing precedence. > > For dscp, I think it should be straight forward. The run-time default would be a > member of RTCConfiguration, I guess, and the place for the above logic is where > populating MediaConfig, in PeerConnection::Initialize, before calling > CreateMediaController. I don't think you're confused enough yet :-) I don't know when the Finch flag is read. But I know that it's after the creation of the RTCPeerConnection. At that time, the Javascript has already decided long ago whether or not it will pass down a constraint to the PeerConnnection initializer - so we have to pass the information about whether it is true, false or absent; when it's absent, the Finch flag applies. I think it's much nicer to use rtc::Optional for this than two booleans, but worry about introducing code in Chrome that relies on rtc::Optional given that we don't have it before. I'll check with Tommi.
After checking with Tommi, I think it's OK to start infecting Chrome with rtc::Optional by exposing them at the API. This is the result of that redesign. PTAL.
https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:250: bool enable_dscp; On 2016/02/24 12:40:38, hta-webrtc wrote: > I don't think you're confused enough yet :-) Very likely. > I don't know when the Finch flag is read. But I know that it's after the > creation of the RTCPeerConnection. At that time, the Javascript has already > decided long ago whether or not it will pass down a constraint to the > PeerConnnection initializer - so we have to pass the information about whether > it is true, false or absent; when it's absent, the Finch flag applies. I wonder if that can work at all now. In MediaChannel, enable_dscp is now a construction-time option, there's no way (at least not intended) to set it later.
https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:625: media_config.enable_dscp = *(configuration.enable_dscp); I agree this use of rtc::Optional makes some sense. Since it represents a "raw" user config. However, I think the call-site in PeerConnectionFactory is a more natural place to merge config values from different sources. See my other comment on that file. I'm also missing the logic for dscp experiments; in the current cl, if the constraint is unset, media_config.enable_dscp always get the compiled-in default of false (from the declaration in mediachannel.h). So it's not clear to me how you intend to make the default configurable by chrome (or other applications). https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.cc:258: // We merge constraints and configuration into a single configuration. I think this is a logical place to handle run-time configurable defaults. Sketch: Let RTCConfiguration include a struct MediaConfig. In configuration_in, the values represent run-time defaults, e.g., set by chrome experiments. Copy this (like the current code does), and for any present constraints, override the default values by simply assigning to it. No rtc::Optional needed, only a call to FindConstraint, we don't even need to examine the return value. Finally, pass the modified configuration on to pc->Initialize. This method then doesn't have to touch MediaConfig flags at all, it can just pass on a reference to RTCConfiguration::media_config with it's call to CreateMediaController. My thinking is that this method gets the full responsibility for deciding which value to use for every option. By the call to PeerConnection::Initialize, everything is decided, and the settings should just be applied and passed on to whereever they are used. Does that make sense? Doing that for MediaConfig flags seems reasonable to me for this cl, not sure if it should do the same for all other options or if that's for later. Hmm. Or maybe I'm misunderstanding your intent, I don't understand the intended responsibilities of the factory class. If you really want the responsibility for config merging logic in PeerConnection::Initialize, that's fine too, I guess. But in that case, I think this method should avoid do any of that, it should just pass on the RTCConfiguration unchanged, together with a struct using rtc::Optional to represent the constraints present. So that responsibility and logic is in one place.
Let's see if this is more readable. I kept the optionals (so far), but peerconnection.Initialize doesn't look at some of them - they're resolved into a MediaConfig in the factory, and Initialize doesn't touch the MediaConfig, it just passes it on. Makes more sense? https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:625: media_config.enable_dscp = *(configuration.enable_dscp); On 2016/02/25 08:26:51, nisse-webrtc wrote: > I agree this use of rtc::Optional makes some sense. Since it represents a "raw" > user config. > > However, I think the call-site in PeerConnectionFactory is a more natural place > to merge config values from different sources. See my other comment on that > file. > > I'm also missing the logic for dscp experiments; in the current cl, if the > constraint is unset, media_config.enable_dscp always get the compiled-in default > of false (from the declaration in mediachannel.h). > So it's not clear to me how you intend to make the default configurable by > chrome (or other applications). I was misremembering (it is the IPv6 that is controlled by experiment - webrtc::field_trial::FindFullName - see above.) I'll see what i can do about lifting the logic. https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.cc:258: // We merge constraints and configuration into a single configuration. I think I see one reason why this review has been so confusing. The diff function has *marked the wrong function as new*. The genuine new function is the CreatePeerConnection *below* this one, which has no "constraints" parameter. This one is the *old* function, which I intend to delete when I can. I'll rewrite to make this plainer (and reduce code duplication). https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:224: // TODO(hbos): Change into class with private data and public getters. Since this class forms part of webrtc's public interface for createPeerConnection, I have misgivings about using it for "what's been decided". I don't want that logic to move out of webrtc. (if webrtc changes the default for dscp from false to true, I want that change to take effect without any code changes to the consumers that haven't stated a preference, for instance).
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/140001
https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:2143: configuration->enable_dscp = ConstraintToOptionalBool( I like this version better. Have you considered including a MediaConfig inside RTCConfiguration, is there any reason that won't work (compatibility...)? Then this function should just write the corresponding flags directly into that struct (enable_dscp, enable_cpu_adaptation, disable_prerenderer_smoothing) without any Optional. The way it would work, an application which just creates a RTCConfigurations gets the values determined by the default constructor, and possibly overridden by constraints. And if the application sets specific values, they're used instead of the compiled-in defaults (in the absense of constraints). I'm also curious about how constraints will be applied without the MediaConstraintsInterface? There will be code somewhere parsing the wire protocol (is that sdp?), and storing values directly in an RTCConfiguration?
Pushback, I'm afraid. I don't see how to do the change you're suggesting without moving decisions between Chrome and WebRTC, and I don't want to do that as part of this refactoring. https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:2143: configuration->enable_dscp = ConstraintToOptionalBool( On 2016/02/25 15:25:54, nisse-webrtc wrote: > I like this version better. Have you considered including a MediaConfig inside > RTCConfiguration, is there any reason that won't work (compatibility...)? Then > this function should just write the corresponding flags directly into that > struct (enable_dscp, enable_cpu_adaptation, disable_prerenderer_smoothing) > without any Optional. But then the logic for deciding what was the default, which is currently in webrtc, moves to the application. That's the part I don't like. > > The way it would work, an application which just creates a RTCConfigurations > gets the values determined by the default constructor, and possibly overridden > by constraints. Remember: There are (will be) no more constraints in webrtc. Either it's optionals, or it's straight booleans. If we want webrtc to make the decision, it's got to be optionals. If the client app makes the decision *always*, booleans are good enough. And if the application sets specific values, they're used > instead of the compiled-in defaults (in the absense of constraints). > > I'm also curious about how constraints will be applied without the > MediaConstraintsInterface? There will be code somewhere parsing the wire > protocol (is that sdp?), and storing values directly in an RTCConfiguration? The creation of the PeerConnection is done before SDP enters the picture - SDP comes in with setLocalDescription and setRemoteDescription.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
On 2016/02/25 16:06:07, hta-webrtc wrote: > I don't see how to do the change you're suggesting without moving decisions > between Chrome and WebRTC, and I don't want to do that as part of this > refactoring. Ok, lets leave that for later. > But then the logic for deciding what was the default, which is currently in > webrtc, moves to the application. That's the part I don't like. I was thinking that any config struct would have a default constructor, belonging to webrtc, which is responsible for webrtc's idea of what the defaults are. And then the application can overwrite 0 or more of these settings with it's preferred values, to specify the application's idea of the default, before passing the config into webrtc. And then the defaults might be overridden again with per call settings (from javascript or constraints or whatever, and this is the only place where I see a potential need for rtc::Optional). Let us get back to that later.
I should say that I'm still not convinced it's the right thing to have CopyConstraintsIntoRtcConfiguration convert constraints into optionals. But I think it might be ok for now, since this is an interface-cl; if others think it's ok, I won't insist. I'd also like to know where the "webrtc defaults" are located. Do I understand it right, that the defaults are what's specified where RTCConfiguration is declared in peerconnectioninterface.h? If so, there should be no functional difference switching from rtc::Optional<bool> to plain bool in this cl; in either case, they values set by the constructor will act as the defaults in absence of constraints. https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/localaudiosou... File webrtc/api/localaudiosource.h (right): https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/localaudiosou... webrtc/api/localaudiosource.h:47: LocalAudioSource() : source_state_(kInitializing) {} I guess that's also for a separate refactoring cl, but I'd really like to kill all "kInitializing" states. When creating an object, just make sure that it is valid and fully initialized before taking it into use. https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:458: // TODO(hta): Figure out if there's a difference Will you figure it out before landing?
Response to comments. On redesign of initialization: On second thoughts, I don't think moving experiment detection into the constructor for options structs is the right thing. C++ consructors need to be simple and isolated, and figuring out which experiments are running is neither. But other designs (such as private constructor and a Create method) are possible. So I think rtc::Optional has the right semantics, for now. https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/localaudiosou... File webrtc/api/localaudiosource.h (right): https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/localaudiosou... webrtc/api/localaudiosource.h:47: LocalAudioSource() : source_state_(kInitializing) {} On 2016/02/26 08:53:45, nisse-webrtc wrote: > I guess that's also for a separate refactoring cl, but I'd really like to kill > all "kInitializing" states. When creating an object, just make sure that it is > valid and fully initialized before taking it into use. Yes, I think it is separate. Filing a bug would be appropriate (so that we do not forget). Bug webrtc:5604 filed. https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:458: // TODO(hta): Figure out if there's a difference On 2016/02/26 08:53:45, nisse-webrtc wrote: > Will you figure it out before landing? Yes, I've decided that there is no difference we care about. Will delete.
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... webrtc/api/localaudiosource.cc:108: if (!audio_options) On 2016/02/23 14:30:34, hta-webrtc wrote: > On 2016/02/23 11:40:18, perkj_webrtc wrote: > > I think you should allow audio_options == nullptr > > > > ie- source_state_ = kLive. > > > > Even better- make sure source_state_(kLive) in the ctor and remove setting it > > here and in the default Initialize. > > The constructor sets source_state_(kInitializing). I thought this code path > strange, but didn't want to change behavior, so I reproduced the previous > behavior, even though it's bizarre. TODO? It is probably correct in the base class since for video the state changes depending on how it went to parse the contraints and start the camera. But here I dont think it make sence. The LocalAudioSource should set it to someting that make sence. So please fix it in your new code. Also- if audio_options is a pointer - I would have suspected that null is ok and therefore have the state be kLive even if it is null. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/localaudiosou... File webrtc/api/localaudiosource_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/localaudiosou... webrtc/api/localaudiosource_unittest.cc:105: EXPECT_EQ(rtc::Optional<bool>(true), source->options().highpass_filter); check state please https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/localaudiosou... webrtc/api/localaudiosource_unittest.cc:112: EXPECT_EQ(rtc::Optional<bool>(), source->options().highpass_filter); check state please. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/mediaconstrai... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface.cc:141: if (mandatory_constraints) nit : {} https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:475: // if it is an answer and video is offered, we should still accept video Is this no longer true? ie is your ConvertRtcOptionsForAnswer correct? https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:1545: void PeerConnection::FinishOptionsForAnswer( FillTransportOptionsForAnswer ? https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:2127: void CopyConstraintsIntoRtcConfiguration( I still think this should be moved to where it is used and dont pollute the webrtc ns. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnection.h File webrtc/api/peerconnection.h (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnection.h:42: // Deprecated. Can you please add a todo and explain when this will be removed? https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.h (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.h:66: // Deprecated, use version without constraints. So you suggest Android and IOS and none chrome desktop does not use constraints to decide frame rate and resolution? The question is then how.... https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/webrtcsession... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/webrtcsession... webrtc/api/webrtcsession_unittest.cc:390: CopyConstraintsIntoRtcConfiguration(constraints_.get(), &rtc_configuration); webrtcsession no longer uses constraints so you should be able to just replace constraints_ everywhere in this file with your new configuration. Add a a test in PeerConnectionFactory that you correctly translate constraints to rtc_configuration.
PTAL - I also added one default implementation and bug references. https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/1717583002/diff/60001/webrtc/api/localaudiosour... webrtc/api/localaudiosource.cc:108: if (!audio_options) On 2016/03/01 08:41:26, perkj_webrtc wrote: > On 2016/02/23 14:30:34, hta-webrtc wrote: > > On 2016/02/23 11:40:18, perkj_webrtc wrote: > > > I think you should allow audio_options == nullptr > > > > > > ie- source_state_ = kLive. > > > > > > Even better- make sure source_state_(kLive) in the ctor and remove setting > it > > > here and in the default Initialize. > > > > The constructor sets source_state_(kInitializing). I thought this code path > > strange, but didn't want to change behavior, so I reproduced the previous > > behavior, even though it's bizarre. TODO? > > It is probably correct in the base class since for video the state changes > depending on how it went to parse the contraints and start the camera. But here > I dont think it make sence. The LocalAudioSource should set it to someting that > make sence. So please fix it in your new code. > Also- if audio_options is a pointer - I would have suspected that null is ok and > therefore have the state be kLive even if it is null. It is actually not present in the base class. It's a private field of the LocalAudioSource class. If the logic above holds, it serves no purpose and should be removed. OK. Done. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/localaudiosou... File webrtc/api/localaudiosource_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/localaudiosou... webrtc/api/localaudiosource_unittest.cc:105: EXPECT_EQ(rtc::Optional<bool>(true), source->options().highpass_filter); On 2016/03/01 08:41:26, perkj_webrtc wrote: > check state please Gone. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/localaudiosou... webrtc/api/localaudiosource_unittest.cc:112: EXPECT_EQ(rtc::Optional<bool>(), source->options().highpass_filter); On 2016/03/01 08:41:26, perkj_webrtc wrote: > check state please. Gone. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/mediaconstrai... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface.cc:141: if (mandatory_constraints) On 2016/03/01 08:41:26, perkj_webrtc wrote: > nit : {} Done (here and in existing code above). https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnection.h File webrtc/api/peerconnection.h (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnection.h:42: // Deprecated. On 2016/03/01 08:41:26, perkj_webrtc wrote: > Can you please add a todo and explain when this will be removed? Done. Bug 5617 filed. https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.h (right): https://codereview.webrtc.org/1717583002/diff/160001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.h:66: // Deprecated, use version without constraints. On 2016/03/01 08:41:26, perkj_webrtc wrote: > So you suggest Android and IOS and none chrome desktop does not use constraints > to decide frame rate and resolution? The question is then how.... Added a comment (removed the "D" word) and referenced bug 5617.
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7827) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11737) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7463)
Patchset #9 (id:200001) has been deleted
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good. Please fix webrtcsession_unittest as we discussed. https://codereview.webrtc.org/1717583002/diff/220001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1717583002/diff/220001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:430: bool ConvertRtcOptionsForOffer( suggest rename to ConvertToMediaSessionOptions and remove ConvertRtcOptionsForAnswer. https://codereview.webrtc.org/1717583002/diff/220001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/220001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.cc:260: CopyConstraintsIntoRtcConfiguration(constraints, &configuration); This Copy... might need a test.
hta@chromium.org changed reviewers: + hta@chromium.org
New try ... are we ready to go now? https://codereview.chromium.org/1717583002/diff/220001/webrtc/api/peerconnect... File webrtc/api/peerconnection.cc (right): https://codereview.chromium.org/1717583002/diff/220001/webrtc/api/peerconnect... webrtc/api/peerconnection.cc:430: bool ConvertRtcOptionsForOffer( On 2016/03/02 14:46:26, perkj_webrtc wrote: > suggest rename to ConvertToMediaSessionOptions and remove > ConvertRtcOptionsForAnswer. ExtractMediaSessionOptions. Calling it a conversion is not really correct when I think about it. It's used in a few places. Changed. https://codereview.chromium.org/1717583002/diff/220001/webrtc/api/peerconnect... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.chromium.org/1717583002/diff/220001/webrtc/api/peerconnect... webrtc/api/peerconnectionfactory.cc:260: CopyConstraintsIntoRtcConfiguration(constraints, &configuration); On 2016/03/02 14:46:27, perkj_webrtc wrote: > This Copy... might need a test. Test added (some aspects). Look at test to see behavior.
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
https://codereview.webrtc.org/1717583002/diff/260001/webrtc/api/mediaconstrai... File webrtc/api/mediaconstraintsinterface_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/260001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface_unittest.cc:49: // but leave other fields alone. I don't understand the logic. If no constraint related to dtls_srtp is present, why is the configuration flag modified? If that's intended, I think it ought to be explained why, in a comment in the CopyCon... function.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/1287)
Responding to the "why", but not doing anything about it yet. https://codereview.webrtc.org/1717583002/diff/260001/webrtc/api/mediaconstrai... File webrtc/api/mediaconstraintsinterface_unittest.cc (right): https://codereview.webrtc.org/1717583002/diff/260001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface_unittest.cc:49: // but leave other fields alone. On 2016/03/03 12:09:39, nisse-webrtc wrote: > I don't understand the logic. If no constraint related to dtls_srtp is present, > why is the configuration flag modified? If that's intended, I think it ought to > be explained why, in a comment in the CopyCon... function. It came out when I wrote the unittest... when I added the fields, I thought "they will be empty, setting them to empty again in the CopyCon.. won't harm anything, nothing else writes to them." Now that I see it from this side, it looks odd. So I chose to document the behavior. The alternative is to rewrite the functions inside mediaconstraintsinterface.cc to not overwrite.
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5633) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
Patchset #12 (id:280001) has been deleted
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
The CQ bit was checked by hta@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x64_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Updated according to nisse's comments. Yes, this looks better.
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
lgtm Although, if you have time to make the Optional vs. not consistent, I would appreciate that. Also, it might be good for Taylor to take a look. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:251: bool enable_rtp_data_channel; Why are two of these optional, but the rest not?
There's *some* logic to the optional usage.... thanks for the quick turnaround! Will give Nisse and Taylor a chance to comment. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:251: bool enable_rtp_data_channel; On 2016/03/03 17:51:17, pthatcher1 wrote: > Why are two of these optional, but the rest not? The DSCP one was the one where I found that it didn't have a hardwired default, so that I had to preserve the information about whether or not the client had asked for a specific value. Thinking more about it, I decided that if we were ever to change a default on one of the other ones, we'd likely use a Finch experiment, which would need exactly the same thing. But I couldn't imagine changing our minds on defaults for IPv6 and RTP data channels, which is why I left them as boolean. So there's a certain logic to it. If a consistent treatment is important, I can make them all optional. (Nisse pushed back on the use of optional, btw)
lgtm https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:251: bool enable_rtp_data_channel; On 2016/03/03 17:51:17, pthatcher1 wrote: > Why are two of these optional, but the rest not? For the ones that map to MediaConfig flags, I think the right way is to embed a MediaConfig in RTCConfiguration, without any rtc::Optional. And refactor the code so that values are assigned to MediaConfig flags in the order 1. Webrtc compiled-in defaults, set in the MediaConfig default constructor. 2. Overriding defaults set by the application, e.g., configuration of chrome experiments. 3. Constraints (or other means of configuration) specific to a single PeerConnection. And I'd expect similar logic can be applied to other settings too. But I've agreed with Harald that investigation of that path can be a later cl.
lgtm https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/mediaconstrai... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface.cc:154: void ConstraintToOptionalBool(const MediaConstraintsInterface* constraints, nit: move to anonymous ns. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface.cc:164: void ConstraintToOptionalInt(const MediaConstraintsInterface* constraints, nit: move to anonymous ns. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.cc:17: #include "webrtc/api/mediaconstraintsinterface.h" nit: I would expect this to be included in the header file already? https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:422: virtual bool UpdateIce(const IceServers& configuration) { return false; } nit: Is this really needed?
lgtm
On 2016/03/04 08:23:30, nisse-webrtc wrote: > lgtm > > https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... > File webrtc/api/peerconnectioninterface.h (right): > > https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... > webrtc/api/peerconnectioninterface.h:251: bool enable_rtp_data_channel; > On 2016/03/03 17:51:17, pthatcher1 wrote: > > Why are two of these optional, but the rest not? > > For the ones that map to MediaConfig flags, I think the right way is to embed a > MediaConfig in RTCConfiguration, without any rtc::Optional. And refactor the > code so that values are assigned to MediaConfig flags in the order > > 1. Webrtc compiled-in defaults, set in the MediaConfig default constructor. > > 2. Overriding defaults set by the application, e.g., configuration of chrome > experiments. > > 3. Constraints (or other means of configuration) specific to a single > PeerConnection. > > And I'd expect similar logic can be applied to other settings too. But I've > agreed with Harald that investigation of that path can be a later cl. And I argued that in step #3, we need rtc::Optional() or a similar mechanism to pass the information, given that we intend to stop using constraints. But yes, we've agreed that it's a later issue to address.
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1717583002/#ps360001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1717583002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1717583002/360001
And with that, I think we're ready. Addressed the last style comments; leaving the parameter issue (including consistency) for a later round. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/mediaconstrai... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface.cc:154: void ConstraintToOptionalBool(const MediaConstraintsInterface* constraints, On 2016/03/04 08:44:36, perkj_webrtc wrote: > nit: move to anonymous ns. Done. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/mediaconstrai... webrtc/api/mediaconstraintsinterface.cc:164: void ConstraintToOptionalInt(const MediaConstraintsInterface* constraints, On 2016/03/04 08:44:36, perkj_webrtc wrote: > nit: move to anonymous ns. Done. https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1717583002/diff/340001/webrtc/api/peerconnectio... webrtc/api/peerconnectionfactory.cc:17: #include "webrtc/api/mediaconstraintsinterface.h" On 2016/03/04 08:44:36, perkj_webrtc wrote: > nit: I would expect this to be included in the header file already? Nope, it isn't. Just a forward declaration.
Message was sent while issue was closed.
Description was changed from ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=webrtc:4906 ========== to ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=webrtc:4906 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=webrtc:4906 ========== to ========== This CL provides interfaces that do not use constraints for all interfaces that formerly took constraints parameters in name=value form. This is in preparation for making Chrome only use these explicit interfaces. BUG=webrtc:4906 Committed: https://crrev.com/a2a49d9d9c65cf6d080811e6c2755cba9cb52d32 Cr-Commit-Position: refs/heads/master@{#11870} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/a2a49d9d9c65cf6d080811e6c2755cba9cb52d32 Cr-Commit-Position: refs/heads/master@{#11870} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
