Updated structures and functions for setting the max bitrate limit to take rtc::Optional<int>

webrtc/media/engine/webrtcvideoengine2.h

 /*
  *  Copyright (c) 2014 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.
  */
 
@@ skipping 173 matching lines @@
 
     VideoCodec codec;
     webrtc::FecConfig fec;
     int rtx_payload_type;
   };
 
   struct ChangedSendParameters {
     // These optionals are unset if not changed.
     rtc::Optional<VideoCodecSettings> codec;
     rtc::Optional<std::vector<webrtc::RtpExtension>> rtp_header_extensions;
-    rtc::Optional<int> max_bandwidth_bps;
+    rtc::Optional<rtc::Optional<int>> max_bandwidth_bps;

[stefan-webrtc, 2016/03/18 08:28:40]

What is an optional optional? This looks super weird to me.

[skvlad, 2016/03/18 18:01:52]

As Peter said, this structure represents the diff from the previous values - and
it uses rtc::Optional<> with no value to indicate that there was no change.
The bandwidth limit parameter itself is optional - so it can not change at all
(outer rtc::Optional has no value), or it can change to a new limit
(rtc::Optional with rtc::Optional with a new value), or it can become unset
after it was set before - in which case it will be an rtc::Optional
(representing the diff) with an rtc::Optional (representing the setting) with no
value (no limit).

Do you think if we add a type alias - like rtc::Change<T> - for this particular
case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will the
overhead of the new type make it harder to read?

[pthatcher1, 2016/03/21 17:41:13]

I think that would make it less clear about what's going on.  

[Taylor Brandstetter, 2016/03/29 02:26:58]

I think just a comment above this line explaining the semantics of each
"Optional" would suffice.

[skvlad, 2016/03/30 19:40:44]

Done.

     rtc::Optional<bool> conference_mode;
     rtc::Optional<webrtc::RtcpMode> rtcp_mode;
   };
 
   struct ChangedRecvParameters {
     // These optionals are unset if not changed.
     rtc::Optional<std::vector<VideoCodecSettings>> codec_settings;
     rtc::Optional<std::vector<webrtc::RtpExtension>> rtp_header_extensions;
     rtc::Optional<webrtc::RtcpMode> rtcp_mode;
   };
@@ skipping 27 matching lines @@
       : public rtc::VideoSinkInterface<cricket::VideoFrame>,
         public webrtc::LoadObserver {
    public:
     WebRtcVideoSendStream(
         webrtc::Call* call,
         const StreamParams& sp,
         const webrtc::VideoSendStream::Config& config,
         const VideoOptions& options,
         WebRtcVideoEncoderFactory* external_encoder_factory,
         bool enable_cpu_overuse_detection,
-        int max_bitrate_bps,
+        rtc::Optional<int> max_bitrate_bps,
         const rtc::Optional<VideoCodecSettings>& codec_settings,
         const std::vector<webrtc::RtpExtension>& rtp_extensions,
         const VideoSendParameters& send_params);
     virtual ~WebRtcVideoSendStream();
 
     void SetOptions(const VideoOptions& options);
     // TODO(pbos): Move logic from SetOptions into this method.
     void SetSendParameters(const ChangedSendParameters& send_params);
     bool SetRtpParameters(const webrtc::RtpParameters& parameters);
 
@@ skipping 16 matching lines @@
 
    private:
     // Parameters needed to reconstruct the underlying stream.
     // webrtc::VideoSendStream doesn't support setting a lot of options on the
     // fly, so when those need to be changed we tear down and reconstruct with
     // similar parameters depending on which options changed etc.
     struct VideoSendStreamParameters {
       VideoSendStreamParameters(
           const webrtc::VideoSendStream::Config& config,
           const VideoOptions& options,
-          int max_bitrate_bps,
+          rtc::Optional<int> max_bitrate_bps,
           const rtc::Optional<VideoCodecSettings>& codec_settings);
       webrtc::VideoSendStream::Config config;
       VideoOptions options;
-      int max_bitrate_bps;
+      rtc::Optional<int> max_bitrate_bps;
       bool conference_mode;
       rtc::Optional<VideoCodecSettings> codec_settings;
       // Sent resolutions + bitrates etc. by the underlying VideoSendStream,
       // typically changes when setting a new resolution or reconfiguring
       // bitrates.
       webrtc::VideoEncoderConfig encoder_config;
     };
 
     struct AllocatedEncoder {
       AllocatedEncoder(webrtc::VideoEncoder* encoder,
@@ skipping 20 matching lines @@
 
     union VideoEncoderSettings {
       webrtc::VideoCodecH264 h264;
       webrtc::VideoCodecVP8 vp8;
       webrtc::VideoCodecVP9 vp9;
     };
 
     static std::vector<webrtc::VideoStream> CreateVideoStreams(
         const VideoCodec& codec,
         const VideoOptions& options,
-        int max_bitrate_bps,
+        rtc::Optional<int> max_bitrate_bps,
         size_t num_streams);
     static std::vector<webrtc::VideoStream> CreateSimulcastVideoStreams(
         const VideoCodec& codec,
         const VideoOptions& options,
-        int max_bitrate_bps,
+        rtc::Optional<int> max_bitrate_bps,
         size_t num_streams);
 
     void* ConfigureVideoEncoderSettings(const VideoCodec& codec)
         EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
     AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec)
         EXCLUSIVE_LOCKS_REQUIRED(lock_);
     void DestroyVideoEncoder(AllocatedEncoder* encoder)
         EXCLUSIVE_LOCKS_REQUIRED(lock_);
     void SetCodec(const VideoCodecSettings& codec)
@@ skipping 191 matching lines @@
   // TODO(deadbeef): Don't duplicate information between
   // send_params/recv_params, rtp_extensions, options, etc.
   VideoSendParameters send_params_;
   VideoOptions default_send_options_;
   VideoRecvParameters recv_params_;
 };
 
 }  // namespace cricket
 
 #endif  // WEBRTC_MEDIA_ENGINE_WEBRTCVIDEOENGINE2_H_