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

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

This cl has a few features:
* Update SinkWants with a range struct (min, target, max) instead of
  having each component be optional.
* Both pixel count and framerate is now present as an optional range.
* Wire framerate restriction up in VideoAdapter
* Support both maintain-resolution and balanced in vie_encoder, but
  don't actually request lower framerate yet.

The new range makes the requests more explicit, as we can set max or
min as cap when adapting down or up respectievely, while having a
target that matches our desired step size and the possibility of not
taking a too large step up or down.

BUG=webrtc:4172

[sprang_webrtc, 2017-02-16 16:08:35]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 16:08:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sprang_webrtc, 2017-02-16 16:09:35]

sprang@webrtc.org changed reviewers:
+ nisse@webrtc.org

[sprang_webrtc, 2017-02-16 16:09:35]

nisse@, would you like to have a first shot at this since you've been involved
in it before?

[commit-bot: I haz the power, 2017-02-16 16:21:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 16:21:29]

Dry run: Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)

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

First round of comments, without looking carefully at the implementation.

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videoadapte...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videoadapte...
webrtc/media/base/videoadapter.cc:279: void
VideoAdapter::OnSinkWantsUpdated(const rtc::VideoSinkWants& sink_wants) {
I understand this change makes things look a little simpler, but it's not quite
the right level of abstraction. 

I think it's preferable to only pass and copy the entries of the SinkWants which
the video adapter needs, which would be everything but the |rotation_applied|
and |black_frames| flags. Using the Range type is fine, though.

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
webrtc/media/base/videosourceinterface.h:41: uint32_t target;
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:14]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 15:48:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

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

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videoadapte...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videoadapte...
webrtc/media/base/videoadapter.cc:279: void
VideoAdapter::OnSinkWantsUpdated(const rtc::VideoSinkWants& sink_wants) {
On 2017/02/17 08:55:21, nisse-webrtc wrote:
> I understand this change makes things look a little simpler, but it's not
quite
> the right level of abstraction. 
> 
> I think it's preferable to only pass and copy the entries of the SinkWants
which
> the video adapter needs, which would be everything but the |rotation_applied|
> and |black_frames| flags. Using the Range type is fine, though.

Fair enough. Just didn't want to pass along 6 optional<int> :)

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
webrtc/media/base/videosourceinterface.h:41: uint32_t target;
On 2017/02/17 08:55:21, nisse-webrtc wrote:
> 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();
> };

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.

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

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.
 
> So maybe you should define the types Range<type>, PixelCountRange ==
> Range<size_t>, FpsRange == Range<int> or something like that.

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.

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

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]

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
webrtc/media/base/videosourceinterface.h:41: uint32_t target;
On 2017/02/20 15:48:35, språng wrote:
> 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.

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.

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

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

I think it would be neat to use the same helper function/method for dealing with
both pixel ranges and fps ranges.

[commit-bot: I haz the power, 2017-02-20 17:48:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 17:48:53]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_more_configs on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

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

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
webrtc/media/base/videosourceinterface.h:41: uint32_t target;
On 2017/02/20 16:03:47, nisse-webrtc wrote:
> On 2017/02/20 15:48:35, språng wrote:
> > 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.
> 
> 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.

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?
 
> > 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.
> 
> 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 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 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.
> 
> I think it would be neat to use the same helper function/method for dealing
with
> both pixel ranges and fps ranges.

I did an attempt at that in videobroadcaster.cc
Might be possible to prettify it even further.

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

nisse@webrtc.org changed reviewers:
+ kthelgason@webrtc.org, perkj@webrtc.org

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

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2698203003/diff/1/webrtc/media/base/videosource...
webrtc/media/base/videosourceinterface.h:41: uint32_t target;
On 2017/02/21 09:11:03, språng wrote:
> Any opinion on whose opinion we should seek?

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

So start with the locals?

> I didn't think it was recommended to use the unisgned/unsigned long types
> directly, but rather use uint[x]_t?

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. 

> I'm in favor of size_t in that case anyhow.

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.

https://codereview.webrtc.org/2698203003/diff/40001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster.cc (right):

https://codereview.webrtc.org/2698203003/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.cc:82: }
I think this is in the right direction. Might be even prettier with a
non-destructive function like

  Range Intersect(const Range& a, const Range& b);

(And can we find a better name than Range for an object which is a range + a
target value? For lack of better ideas, maybe RangeWant ?).

[sprang_webrtc, 2017-02-21 09:42:38]

https://codereview.webrtc.org/2698203003/diff/40001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster.cc (right):

https://codereview.webrtc.org/2698203003/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.cc:82: }
On 2017/02/21 09:33:59, nisse-webrtc wrote:
> I think this is in the right direction. Might be even prettier with a
> non-destructive function like
> 
>   Range Intersect(const Range& a, const Range& b);
> 
> (And can we find a better name than Range for an object which is a range + a
> target value? For lack of better ideas, maybe RangeWant ?).

I think I'll revisit this when/if removing the Optional part.
RangeWant could work... or maybe TargetRange ? :)

[kthelgason, 2017-02-22 12:30:09]

This seems over-engineered to me, however I've followed the discussion and
realise that this problem has more nuances than immediately apparent. Here are
some thoughts in no particular order:

1) I don't really understand what the point of |target| is when we have min and
max, i.e. what is the difference between selecting the next resolution greater
than |min|?

2) When no resolution >= |min| is available, we return the greatest available
resolution, even if it's less than |min| right? That makes it very easy to
misunderstand what these terms mean.

3) We already have onOutputFormatRequest to cap the maximum resolution, why is
|max| also necessary?

I haven't taken a good look at the code, I just wanted to have a discussion
about the high-level design first.

[sprang_webrtc, 2017-02-22 13:23:02]

On 2017/02/22 12:30:09, kthelgason wrote:
> This seems over-engineered to me, however I've followed the discussion and
> realise that this problem has more nuances than immediately apparent. Here are
> some thoughts in no particular order:
> 
> 1) I don't really understand what the point of |target| is when we have min
and
> max, i.e. what is the difference between selecting the next resolution greater
> than |min|?

The point of target was to have a more direct control of what resolution to,
without a priori knowledge about available step sizes. We can just say "give me
a resolution close to X", rather than "give me a resolution lower than Y" over
and over again until we're reasonably close to X.
Right now, all scaling is done by VideoAdapter, but hopefully we will be able to
propagate these wants all the way to the capturer soon(ish), and then all bets
are off as to step size.

> 2) When no resolution >= |min| is available, we return the greatest available
> resolution, even if it's less than |min| right? That makes it very easy to
> misunderstand what these terms mean.

All of this is on a best effort basis, so yeah. When we enable the capturer to
handles this, we may need to add extra protection just in case it misbehaves.

> 3) We already have onOutputFormatRequest to cap the maximum resolution, why is
> |max| also necessary?

I'm not sure where OnOutputFormatRequest() comes into this, it seems to a
request for cropping that comes from elsewhere? I'm in favor of removing that
if it's duplicate (both max pixel count and output format request was there
before I started mucking around, so unsure of the context).

> I haven't taken a good look at the code, I just wanted to have a discussion
> about the high-level design first.

Thanks.
I really want to get the fps constraints in asap. Should we have a face to face
meeting so we can reach consensus faster?

[nisse-webrtc, 2017-02-22 13:27:08]

On 2017/02/22 13:23:02, språng wrote:
> I'm not sure where OnOutputFormatRequest() comes into this, it seems to a
> request for cropping that comes from elsewhere? I'm in favor of removing that
> if it's duplicate (both max pixel count and output format request was there
> before I started mucking around, so unsure of the context).

If I remember correctly, OnOutputFormatRequest is used by apps to specify
aspect-ratio clipping.
Something which SinkWants currently doesn't do.

[kthelgason, 2017-02-22 13:27:11]

On 2017/02/22 13:23:02, språng wrote:
> On 2017/02/22 12:30:09, kthelgason wrote:
> > This seems over-engineered to me, however I've followed the discussion and
> > realise that this problem has more nuances than immediately apparent. Here
are
> > some thoughts in no particular order:
> > 
> > 1) I don't really understand what the point of |target| is when we have min
> and
> > max, i.e. what is the difference between selecting the next resolution
greater
> > than |min|?
> 
> The point of target was to have a more direct control of what resolution to,
> without a priori knowledge about available step sizes. We can just say "give
me
> a resolution close to X", rather than "give me a resolution lower than Y" over
> and over again until we're reasonably close to X.
> Right now, all scaling is done by VideoAdapter, but hopefully we will be able
to
> propagate these wants all the way to the capturer soon(ish), and then all bets
> are off as to step size.
> 
> > 2) When no resolution >= |min| is available, we return the greatest
available
> > resolution, even if it's less than |min| right? That makes it very easy to
> > misunderstand what these terms mean.
> 
> All of this is on a best effort basis, so yeah. When we enable the capturer to
> handles this, we may need to add extra protection just in case it misbehaves.
> 
> > 3) We already have onOutputFormatRequest to cap the maximum resolution, why
is
> > |max| also necessary?
> 
> I'm not sure where OnOutputFormatRequest() comes into this, it seems to a
> request for cropping that comes from elsewhere? I'm in favor of removing that
> if it's duplicate (both max pixel count and output format request was there
> before I started mucking around, so unsure of the context).
> 
> > I haven't taken a good look at the code, I just wanted to have a discussion
> > about the high-level design first.
> 
> Thanks.
> I really want to get the fps constraints in asap. Should we have a face to
face
> meeting so we can reach consensus faster?

That sounds really good, I'm sure you've thought about this much more thoroughly
than I have, and it would be good to have a meatspace discussion about this. If
you schedule a meeting magjed may also be interested.