Non-constraint interfaces for all constrainable interfaces

webrtc/api/peerconnectionfactory.cc

Index: webrtc/api/peerconnectionfactory.cc
diff --git a/webrtc/api/peerconnectionfactory.cc b/webrtc/api/peerconnectionfactory.cc
index c05cd852f605faada5dbd47e68e7b2ec8afd94e8..85714087e91f58d882c47f503084fb025a68ce55 100644
--- a/webrtc/api/peerconnectionfactory.cc
+++ b/webrtc/api/peerconnectionfactory.cc
@@ -14,6 +14,7 @@
 
 #include "webrtc/api/audiotrack.h"
 #include "webrtc/api/localaudiosource.h"
+#include "webrtc/api/mediaconstraintsinterface.h"
 #include "webrtc/api/mediastream.h"
 #include "webrtc/api/mediastreamproxy.h"
 #include "webrtc/api/mediastreamtrackproxy.h"
@@ -198,6 +199,14 @@ PeerConnectionFactory::CreateAudioSource(
   return source;
 }
 
+rtc::scoped_refptr<AudioSourceInterface>
+PeerConnectionFactory::CreateAudioSource(const cricket::AudioOptions& options) {
+  RTC_DCHECK(signaling_thread_->IsCurrent());
+  rtc::scoped_refptr<LocalAudioSource> source(
+      LocalAudioSource::Create(options_, &options));
+  return source;
+}
+
 rtc::scoped_refptr<VideoSourceInterface>
 PeerConnectionFactory::CreateVideoSource(
     cricket::VideoCapturer* capturer,
@@ -208,6 +217,14 @@ PeerConnectionFactory::CreateVideoSource(
   return VideoSourceProxy::Create(signaling_thread_, source);
 }
 
+rtc::scoped_refptr<VideoSourceInterface>
+PeerConnectionFactory::CreateVideoSource(cricket::VideoCapturer* capturer) {
+  RTC_DCHECK(signaling_thread_->IsCurrent());
+  rtc::scoped_refptr<VideoSource> source(
+      VideoSource::Create(channel_manager_.get(), capturer, false));
+  return VideoSourceProxy::Create(signaling_thread_, source);
+}
+
 bool PeerConnectionFactory::StartAecDump(rtc::PlatformFile file,
                                          int64_t max_size_bytes) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
@@ -231,13 +248,50 @@ void PeerConnectionFactory::StopRtcEventLog() {
 
 rtc::scoped_refptr<PeerConnectionInterface>
 PeerConnectionFactory::CreatePeerConnection(
-    const PeerConnectionInterface::RTCConfiguration& configuration,
+    const PeerConnectionInterface::RTCConfiguration& configuration_in,
     const MediaConstraintsInterface* constraints,
     rtc::scoped_ptr<cricket::PortAllocator> allocator,
     rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
     PeerConnectionObserver* observer) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
 
+  // We merge constraints and configuration into a single configuration.

[nisse-webrtc, 2016/02/25 08:26:51]

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.

[hta-webrtc, 2016/02/25 14:32:42]

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).

+  PeerConnectionInterface::RTCConfiguration configuration = configuration_in;
+
+  if (!dtls_identity_store.get()) {
+    // Because |pc|->Initialize takes ownership of the store we need a new
+    // wrapper object that can be deleted without deleting the underlying
+    // |dtls_identity_store_|, protecting it from being deleted multiple times.
+    dtls_identity_store.reset(
+        new DtlsIdentityStoreWrapper(dtls_identity_store_));
+  }
+
+  if (!allocator) {
+    allocator.reset(new cricket::BasicPortAllocator(
+        default_network_manager_.get(), default_socket_factory_.get()));
+  }
+  allocator->SetNetworkIgnoreMask(options_.network_ignore_mask);
+
+  rtc::scoped_refptr<PeerConnection> pc(
+      new rtc::RefCountedObject<PeerConnection>(this));
+
+  CopyConstraintsIntoRtcConfiguration(constraints, &configuration);
+
+  if (!pc->Initialize(configuration, std::move(allocator),
+                      std::move(dtls_identity_store), observer)) {
+    return nullptr;
+  }
+  return PeerConnectionProxy::Create(signaling_thread(), pc);
+}
+
+rtc::scoped_refptr<PeerConnectionInterface>
+PeerConnectionFactory::CreatePeerConnection(
+    const PeerConnectionInterface::RTCConfiguration& configuration,
+    rtc::scoped_ptr<cricket::PortAllocator> allocator,
+    rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
+    PeerConnectionObserver* observer) {
+  RTC_DCHECK(signaling_thread_->IsCurrent());
+
   if (!dtls_identity_store.get()) {
     // Because |pc|->Initialize takes ownership of the store we need a new
     // wrapper object that can be deleted without deleting the underlying
@@ -254,7 +308,7 @@ PeerConnectionFactory::CreatePeerConnection(
 
   rtc::scoped_refptr<PeerConnection> pc(
       new rtc::RefCountedObject<PeerConnection>(this));
-  if (!pc->Initialize(configuration, constraints, std::move(allocator),
+  if (!pc->Initialize(configuration, std::move(allocator),
                       std::move(dtls_identity_store), observer)) {
     return nullptr;
   }