Non-constraint interfaces for all constrainable interfaces

webrtc/api/peerconnectionfactory.cc

 /*
  *  Copyright 2004 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/api/peerconnectionfactory.h"
 
 #include <utility>
 
 #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"
 #include "webrtc/api/peerconnection.h"
 #include "webrtc/api/peerconnectionfactoryproxy.h"
 #include "webrtc/api/peerconnectionproxy.h"
 #include "webrtc/api/videosource.h"
 #include "webrtc/api/videosourceproxy.h"
 #include "webrtc/api/videotrack.h"
 #include "webrtc/base/bind.h"
@@ skipping 164 matching lines @@
 
 rtc::scoped_refptr<AudioSourceInterface>
 PeerConnectionFactory::CreateAudioSource(
     const MediaConstraintsInterface* constraints) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   rtc::scoped_refptr<LocalAudioSource> source(
       LocalAudioSource::Create(options_, constraints));
   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,
     const MediaConstraintsInterface* constraints) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   rtc::scoped_refptr<VideoSource> source(VideoSource::Create(
       channel_manager_.get(), capturer, constraints, false));
   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());
   return channel_manager_->StartAecDump(file, max_size_bytes);
 }
 
 void PeerConnectionFactory::StopAecDump() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   channel_manager_->StopAecDump();
 }
 
 bool PeerConnectionFactory::StartRtcEventLog(rtc::PlatformFile file) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   return channel_manager_->StartRtcEventLog(file);
 }
 
 void PeerConnectionFactory::StopRtcEventLog() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   channel_manager_->StopRtcEventLog();
 }
 
 rtc::scoped_refptr<PeerConnectionInterface>
 PeerConnectionFactory::CreatePeerConnection(
+    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,
-    const MediaConstraintsInterface* constraints,
     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
     // |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));
-  if (!pc->Initialize(configuration, constraints, std::move(allocator),
+  if (!pc->Initialize(configuration, std::move(allocator),
                       std::move(dtls_identity_store), observer)) {
     return nullptr;
   }
   return PeerConnectionProxy::Create(signaling_thread(), pc);
 }
 
 rtc::scoped_refptr<MediaStreamInterface>
 PeerConnectionFactory::CreateLocalMediaStream(const std::string& label) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   return MediaStreamProxy::Create(signaling_thread_,
@@ skipping 37 matching lines @@
 }
 
 cricket::MediaEngineInterface* PeerConnectionFactory::CreateMediaEngine_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   return cricket::WebRtcMediaEngineFactory::Create(
       default_adm_.get(), video_encoder_factory_.get(),
       video_decoder_factory_.get());
 }
 
 }  // namespace webrtc