Update sink wants with ranges for both pixel count and frame rate.

webrtc/media/base/videosourceinterface.h

 /*
  *  Copyright (c) 2016 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.
  */
 
 #ifndef WEBRTC_MEDIA_BASE_VIDEOSOURCEINTERFACE_H_
 #define WEBRTC_MEDIA_BASE_VIDEOSOURCEINTERFACE_H_
 
 #include "webrtc/media/base/videosinkinterface.h"
 #include "webrtc/base/optional.h"
 
 namespace rtc {
 
 // VideoSinkWants is used for notifying the source of properties a video frame
 // should have when it is delivered to a certain sink.
 struct VideoSinkWants {
   // Tells the source whether the sink wants frames with rotation applied.
   // By default, any rotation must be applied by the sink.
   bool rotation_applied = false;
 
   // Tells the source that the sink only wants black frames.
   bool black_frames = false;
 
-  // Tells the source the maximum number of pixels the sink wants.
-  rtc::Optional<int> max_pixel_count;
+  // Indicates a range some value should be constrained to, such as resolution
+  // (pixel count) or frame rate. It is up to the application code to make sure
+  // |min| <= |target| <= |max|.
+  struct ValueRange {
+    ValueRange()
+        : min(0), target(std::numeric_limits<uint32_t>::max()), max(target) {}
+    ValueRange(uint32_t min, uint32_t target, uint32_t max)
+        : min(min), target(target), max(max) {}
+
+    // |min| = 0, indicates no lower bound.
+    uint32_t min;
+    // |target| = maxint by default - requesting highest option available.
+    uint32_t target;

[nisse-webrtc, 2017/02/17 08:55:21]

This makes it impossible to request min and max values without setting a target.
Which seems like a reasonable usecase to me. If you want to simplify this, I'd
suggest dropping the outer optionals and use

struct ValueRange {
  uint32_t min = 0;
  rtc::Optional<uint32_t> target;
  uint32_t max = std::numeric_limits<uint32_t>::max();
};

Some other stylistic comments: "Value" is almost always redundant in type names,
VideoSinkWants::Range should be sufficient.

You might want to consider carefully what types to use, uint32_t seems a bit odd
to me. Old code used int for pixel counts, where maybe size_t would be more
appropriate. While fps have a very different range.

So maybe you should define the types Range<type>, PixelCountRange ==
Range<size_t>, FpsRange == Range<int> or something like that.

And if you make ranges a real type to unify handling of fps and pixel count,
define merging of ranges as on operation on the Range type, either here or in
the broadcaster (which currently is the only place needing that).

[sprang_webrtc, 2017/02/20 15:48:35]

I think it's reasonable to explicitly define the target instead of having it as
an optional, so we don't have to assume what the target should be. In the case
of pixel count, the default would be as high as possible, so the same as max.
A chose an unsigned so that the entire value range makes sense. Lower bound is
naturally 0 and upper bound is set by the type. This makes sense for both
framerate and framerate. Then we don't have to deal with special values like -1,
or unset if using optionals. size_t might be option.
 
This sounds a bit overly complex to me. I think it would be better if we
explicitly handled any cases where -1 would be valid.
I left the logic in the broadcaster for that reason. I can probably clean it up
a bit though, just didn't want to make too many modification in one cl.

[nisse-webrtc, 2017/02/20 16:03:47]

I think we need to get a second opinion (or third, rather) on that. 

If we agree target doesn't need to be optional, then we have no use at all for
optional, right? Just use default integer values, 0 for the range minimum, and
maximum for the type in question for target and max.

Optional was only needed for the step_up thing which you're replacing.
I think uint32_t is rarely appropriate, except when representing values from
some file format or wire protocol. One of unsigned, unsigned long, or size_t
would be better.
I think it would be neat to use the same helper function/method for dealing with
both pixel ranges and fps ranges.

[sprang_webrtc, 2017/02/21 09:11:03]

Yes, I think avoid optional entirely in this case would be better. It was just
convenient to test if anything was set, but we can work around that.

Any opinion on whose opinion we should seek?
 
I didn't think it was recommended to use the unisgned/unsigned long types
directly, but rather use uint[x]_t?
I'm in favor of size_t in that case anyhow.
 
I did an attempt at that in videobroadcaster.cc
Might be possible to prettify it even further.

[nisse-webrtc, 2017/02/21 09:33:59]

The original SinkWants cabal were myself, perkj, pbos, and pthatcher. More
recently, kthelgason have been working with these.

So start with the locals?
Not sure, it seems odd to me that unsigned would be discouraged when we use int,
also with system-dependent size, all over the place. 
Go for size_t then. It's definitely appropriate for pixel count. It's overkill
for fps, but storage overhead shouldn't matter here. 

In the rest of the code, it looks like all of int, unsigned int, uint32_t,
size_t, float and double are used to represent the frame rate in different
places.

+    // |max| = maxint, indicates to upper bound.
+    uint32_t max;
+  };
+
   // Tells the source the desired number of pixels the sinks wants. This will
-  // typically be used when stepping the resolution up again when conditions
-  // have improved after an earlier downgrade. The source should select the
-  // closest resolution to this pixel count, but if max_pixel_count is set, it
-  // still sets the absolute upper bound.
-  rtc::Optional<int> target_pixel_count;
+  // typically be used when stepping the resolution down or up again as resource
+  // constraints are imposed or lifted.
+  //
+  // For example, when stepping down resolution on CPU overuse, it's preferable
+  // to set the max just below the current resolution but the target to a
+  // resolution that it is assumed would result in a utilization the system can
+  // handle. The source should select a resolution as close to |target| as
+  // possible, but no higher than |max|. Then |min| limit can then be set so
+  // that user requested minimum resolution bounds aren't violated.
+  //
+  // Similarly, when stepping up again, the |min| limit can be set one higher
+  // than the current resolution, and |target| to something reasonably higher
+  // (probably matching the step size down). The |max| limit can be set to
+  // avoid too large input.
+  rtc::Optional<ValueRange> pixel_count;
+
+  // These constraints work in just the same manner as the pixel_count ones, but
+  // instead of limiting resolution they limit the framerate.
+  rtc::Optional<ValueRange> framerate_fps_;
 };
 
 template <typename VideoFrameT>
 class VideoSourceInterface {
  public:
   virtual void AddOrUpdateSink(VideoSinkInterface<VideoFrameT>* sink,
                                const VideoSinkWants& wants) = 0;
   // RemoveSink must guarantee that at the time the method returns,
   // there is no current and no future calls to VideoSinkInterface::OnFrame.
   virtual void RemoveSink(VideoSinkInterface<VideoFrameT>* sink) = 0;
 
  protected:
   virtual ~VideoSourceInterface() {}
 };
 
 }  // namespace rtc
 #endif  // WEBRTC_MEDIA_BASE_VIDEOSOURCEINTERFACE_H_