Non-constraint interfaces for all constrainable interfaces

webrtc/api/peerconnectioninterface.h

Index: webrtc/api/peerconnectioninterface.h
diff --git a/webrtc/api/peerconnectioninterface.h b/webrtc/api/peerconnectioninterface.h
index 78635e4a56390953917c4b11eb3016b3dcb5f596..449c58cb4fb89c5664a7783c3da13d8b9082f56a 100644
--- a/webrtc/api/peerconnectioninterface.h
+++ b/webrtc/api/peerconnectioninterface.h
@@ -242,6 +242,23 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
     ContinualGatheringPolicy continual_gathering_policy;
     std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates;
     bool disable_prerenderer_smoothing;
+    // Flags corresponding to values set by constraint flags.
+    // If there is an "override" flag, the "enable/disable" flag is only acted
+    // on if the "override" flag is true.
+    bool disable_ipv6;
+    bool override_dscp;
+    bool enable_dscp;

[nisse-webrtc, 2016/02/23 13:05:32]

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.

[hta-webrtc, 2016/02/23 14:30:34]

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?
Yes. Are there more flags that can be eaten by a MediaConfig?

[nisse-webrtc, 2016/02/23 15:11:02]

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

[hta-webrtc, 2016/02/24 12:40:38]

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.

[nisse-webrtc, 2016/02/24 15:48:19]

Very likely.
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.

+    bool enable_rtp_data_channel;
+    bool override_cpu_overuse_detection;
+    bool enable_cpu_overuse_detection;
+    bool override_suspend_below_min_bitrate;
+    bool suspend_below_min_bitrate;
+    bool override_screencast_min_bitrate;
+    int screencast_min_bitrate;
+    bool override_combined_audio_video_bwe;
+    bool combined_audio_video_bwe;
+    bool override_enable_dtls_srtp;
+    bool enable_dtls_srtp;
     RTCConfiguration()
         : type(kAll),
           bundle_policy(kBundlePolicyBalanced),
@@ -252,7 +269,21 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
           ice_connection_receiving_timeout(kUndefined),
           ice_backup_candidate_pair_ping_interval(kUndefined),
           continual_gathering_policy(GATHER_ONCE),
-          disable_prerenderer_smoothing(false) {}
+          disable_prerenderer_smoothing(false),
+          disable_ipv6(false),
+          override_dscp(false),
+          enable_dscp(false),
+          enable_rtp_data_channel(false),
+          override_cpu_overuse_detection(false),
+          enable_cpu_overuse_detection(false),
+          override_suspend_below_min_bitrate(false),
+          suspend_below_min_bitrate(false),
+          override_screencast_min_bitrate(false),
+          screencast_min_bitrate(0),
+          override_combined_audio_video_bwe(false),
+          combined_audio_video_bwe(false),
+          override_enable_dtls_srtp(false),
+          enable_dtls_srtp(false) {}
   };
 
   struct RTCOfferAnswerOptions {
@@ -287,6 +318,9 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
           use_rtp_mux(use_rtp_mux) {}
   };
 
+  // Argument to the CreatePeerConnection call.
+  struct CreateOptions {};

[perkj_webrtc, 2016/02/23 11:40:18]

unused? 

[hta-webrtc, 2016/02/23 14:30:34]

Removing.

+
   // Used by GetStats to decide which stats to include in the stats reports.
   // |kStatsOutputLevelStandard| includes the standard stats for Javascript API;
   // |kStatsOutputLevelDebug| includes both the standard stats and additional
@@ -381,6 +415,8 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
   // The CreateSessionDescriptionObserver callback will be called when done.
   virtual void CreateAnswer(CreateSessionDescriptionObserver* observer,
                             const MediaConstraintsInterface* constraints) = 0;
+  virtual void CreateAnswer(CreateSessionDescriptionObserver* observer,
+                            const RTCOfferAnswerOptions& options) = 0;
   // Sets the local session description.
   // JsepInterface takes the ownership of |desc| even if it fails.
   // The |observer| callback will be called when done.
@@ -398,6 +434,7 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
                          const MediaConstraintsInterface* constraints) {
     return false;
   }
+  virtual bool UpdateIce(const IceServers& configuration) { return false; }

[perkj_webrtc, 2016/02/23 11:40:18]

not part of this cl?

[hta-webrtc, 2016/02/23 14:30:34]

Needed to remove all APIs with MediaConstraintsInterface* parameters.

   // Sets the PeerConnection's global configuration to |config|.
   // Any changes to STUN/TURN servers or ICE candidate policy will affect the
   // next gathering phase, and cause the next call to createOffer to generate
@@ -524,6 +561,12 @@ class PeerConnectionFactoryInterface : public rtc::RefCountInterface {
       rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
       PeerConnectionObserver* observer) = 0;
 
+  virtual rtc::scoped_refptr<PeerConnectionInterface> CreatePeerConnection(
+      const PeerConnectionInterface::RTCConfiguration& configuration,
+      rtc::scoped_ptr<cricket::PortAllocator> allocator,
+      rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
+      PeerConnectionObserver* observer) = 0;
+
   virtual rtc::scoped_refptr<MediaStreamInterface>
       CreateLocalMediaStream(const std::string& label) = 0;
 
@@ -531,6 +574,8 @@ class PeerConnectionFactoryInterface : public rtc::RefCountInterface {
   // |constraints| decides audio processing settings but can be NULL.
   virtual rtc::scoped_refptr<AudioSourceInterface> CreateAudioSource(

[perkj_webrtc, 2016/02/23 11:40:18]

Mark as deprecated?

       const MediaConstraintsInterface* constraints) = 0;
+  virtual rtc::scoped_refptr<AudioSourceInterface> CreateAudioSource(
+      const cricket::AudioOptions& options) = 0;
 
   // Creates a VideoSourceInterface. The new source take ownership of
   // |capturer|. |constraints| decides video resolution and frame rate but can
@@ -538,6 +583,8 @@ class PeerConnectionFactoryInterface : public rtc::RefCountInterface {
   virtual rtc::scoped_refptr<VideoSourceInterface> CreateVideoSource(

[perkj_webrtc, 2016/02/23 11:40:18]

Mark as deprecated?

       cricket::VideoCapturer* capturer,
       const MediaConstraintsInterface* constraints) = 0;
+  virtual rtc::scoped_refptr<VideoSourceInterface> CreateVideoSource(
+      cricket::VideoCapturer* capturer) = 0;
 
   // Creates a new local VideoTrack. The same |source| can be used in several
   // tracks.