Extract bitrate allocation of spatial/temporal layers out of codec impl.

Extract bitrate allocation of spatial/temporal layers out of codec impl.

This CL makes a number of intervowen changes:

* Add BitrateAllocation struct, that contains a codec independent view
  of how the target bitrate is distributed over spatial and temporal
  layers.

* Adds the BitrateAllocator interface, which takes a bitrate and frame
  rate and produces a BitrateAllocation.

* A default (non layered) implementation is added, and
  SimulcastRateAllocator is extended to fully handle VP8 allocation.
  This includes capturing TemporalLayer instances created by the
  encoder.

* ViEEncoder now owns both the bitrate allocator and the temporal layer
  factories for VP8. This allows allocation to happen fully outside of
  the encoder implementation.

This refactoring will make it possible for ViEEncoder to signal the
full picture of target bitrates to the RTCP module.

BUG=webrtc:6301
Committed: https://crrev.com/8f46c679d24a05b3f08e02c6d91ec9637f34e24f
Cr-Commit-Position: refs/heads/master@{#14998}

[sprang_webrtc, 2016-10-21 07:22:19]

sprang@webrtc.org changed reviewers:
+ perkj@webrtc.org, stefan@webrtc.org

[sprang_webrtc, 2016-10-21 07:22:19]

I did "a perkj" and created a massive refactoring cl. ptal :)

[perkj_webrtc, 2016-10-21 08:24:29]

Its large but make sense as one cl I think.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h#n...
webrtc/common_types.h:625: struct BitrateAllocation {
please use class.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h#n...
webrtc/common_types.h:632: assert(spatial_index < kMaxSpatialLayers);
 CHECK or DCHECK instead of assert.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h#n...
webrtc/common_types.h:639: sum -= bitrates[spatial_index][temporal_index];
please move implementation to a cc file.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h#n...
webrtc/common_types.h:646: assert(spatial_index < kMaxSpatialLayers);
dito

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h#n...
webrtc/common_types.h:671: uint32_t sum = 0;
sum_

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_types.h#n...
webrtc/common_types.h:672: uint32_t
bitrates[kMaxSpatialLayers][kMaxTemporalStreams] = {};
bitrates_

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_video/inc...
File webrtc/common_video/include/video_bitrate_allocator.h (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/common_video/inc...
webrtc/common_video/include/video_bitrate_allocator.h:21: virtual
~VideoBitrateAllocator() {}
make destructor protected unless it should be possible to delete the
implementation through this interface.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
File webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h:282:
ASSERT_TRUE(settings);
DCHECK or CHECK

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:206: if
(codec_.numberOfSimulcastStreams > 0 &&
dont' we always set this to at least 1.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:297: // TODO(andresp): crash
if layers is invalid.
fix or remove this todo?

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
File webrtc/modules/video_coding/generic_encoder.cc (left):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
webrtc/modules/video_coding/generic_encoder.cc:97:
encoder_->SetRates(target_bitrate_kbps, params.input_frame_rate);
Deprecate SetRates? Create a default implementation that crash and remove the
implementation from the files you change here.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
File webrtc/modules/video_coding/include/video_coding.h (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/modules/video_co...
webrtc/modules/video_coding/include/video_coding.h:170: int64_t rtt,
We no longer use the module. Instead we use the sender directly.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video/vie_encode...
File webrtc/video/vie_encoder.cc (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video/vie_encode...
webrtc/video/vie_encoder.cc:460: switch (codec.codecType) {
Can we try to keep ViEEncoder codec agnostic? 
Ie- create a static Create method in the BitrateAllocator that creates the
correct instance of a BitrateAllocator given the codecType.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video/vie_encode...
webrtc/video/vie_encoder.cc:464: rate_allocator_.reset(rate_allocator);
rate_allocator_.reset(new SimulcastRateAllocator(codec));

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video/vie_encode...
webrtc/video/vie_encoder.cc:479: vp8_tl_factory_->SetListener(rate_allocator);
Can we hide this from ViEEncoder by hiding it in SimulcastRateAllocator? 
SimulcastRateAllocator can own and setup the tl_factory. 
You can add a method on the rate allocator that lets it modify VideoCodec.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video/vie_encode...
webrtc/video/vie_encoder.cc:496: frame_rate = codec.maxFramerate;
codec.maxFramerate does not have a real meaning. It is just a fake hard coded
value.

It seems a little bit weird to require a  frame rate in order to get the
preferedbitrate. The frame rate is hard to know.

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video_encoder.h
File webrtc/video_encoder.h (right):

https://codereview.chromium.org/2434073003/diff/20001/webrtc/video_encoder.h#...
webrtc/video_encoder.h:164: virtual int32_t SetRates(uint32_t bitrate, uint32_t
framerate) = 0;
Deprecate SetRates and add a default implementation that DCHECK since we no
longer call it.

[sprang_webrtc, 2016-10-25 10:44:26]

Deprecated SetRates() and fixed many test cases that broke because of that. ptal

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:625: struct BitrateAllocation {
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> please use class.

Agreed. This started out as a simple struct (that's why the naming a style i
struct-based), but it's grown a bit too large. I'll change it.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:632: assert(spatial_index < kMaxSpatialLayers);
On 2016/10/21 08:24:28, perkj_webrtc wrote:
>  CHECK or DCHECK instead of assert.

For some reason, everything here used assert and checks.h wasn't included. I was
assuming there was some dependency reason for this. Changed to DCHECK when I
moved this out to the .cc

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:639: sum -= bitrates[spatial_index][temporal_index];
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> please move implementation to a cc file.

Done.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:646: assert(spatial_index < kMaxSpatialLayers);
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> dito

Done.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:671: uint32_t sum = 0;
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> sum_

Done.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:672: uint32_t
bitrates[kMaxSpatialLayers][kMaxTemporalStreams] = {};
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> bitrates_

Done.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_video/inclu...
File webrtc/common_video/include/video_bitrate_allocator.h (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/common_video/inclu...
webrtc/common_video/include/video_bitrate_allocator.h:21: virtual
~VideoBitrateAllocator() {}
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> make destructor protected unless it should be possible to delete the
> implementation through this interface.

ViEEncoder has a std::unique_ptr<VideoBitrateAllocator>, so yes - the destructor
needs to be public.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h:282:
ASSERT_TRUE(settings);
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> DCHECK or CHECK

Done.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:206: if
(codec_.numberOfSimulcastStreams > 0 &&
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> dont' we always set this to at least 1.

I don't think so. At least there seem to be many test cases which don't. Maybe
that can be a followup cl, this is really quite large as it is?

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:297: // TODO(andresp): crash
if layers is invalid.
On 2016/10/21 08:24:28, perkj_webrtc wrote:
> fix or remove this todo?

Done.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.cc (left):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.cc:97:
encoder_->SetRates(target_bitrate_kbps, params.input_frame_rate);
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> Deprecate SetRates? Create a default implementation that crash and remove the
> implementation from the files you change here. 

Done. That was a LOT of test cases to fix :)

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding.h (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding.h:170: int64_t rtt,
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> We no longer use the module. Instead we use the sender directly.

Acknowledged.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:460: switch (codec.codecType) {
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> Can we try to keep ViEEncoder codec agnostic? 
> Ie- create a static Create method in the BitrateAllocator that creates the
> correct instance of a BitrateAllocator given the codecType.

I created a separate factory class for this, since it couldn't place it directly
in the interface.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:464: rate_allocator_.reset(rate_allocator);
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> rate_allocator_.reset(new SimulcastRateAllocator(codec));

Done. This was to avoid a few static_cast when accessing simulcast-specific
fields, but since there's only one usage left of those I'll bring the cast back.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:479: vp8_tl_factory_->SetListener(rate_allocator);
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> Can we hide this from ViEEncoder by hiding it in SimulcastRateAllocator? 
> SimulcastRateAllocator can own and setup the tl_factory. 
> You can add a method on the rate allocator that lets it modify VideoCodec.

I can do it so it can optionally hold the tl factory, but they can still be
propagated from the outside so can't always rely on that.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:496: frame_rate = codec.maxFramerate;
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> codec.maxFramerate does not have a real meaning. It is just a fake hard coded
> value.

I know, this is just a placeholder until we have valid stats. 

> It seems a little bit weird to require a  frame rate in order to get the
> preferedbitrate. The frame rate is hard to know. 

The rate allocator wants the frame rate because if it is too low we may change
the temporal layer layout. With the current implementations that may not change
the preferred bitrate, but we will still update the registered TemporalLayers
instances though - so it can impact the encoder.

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video_encoder.h
File webrtc/video_encoder.h (right):

https://codereview.webrtc.org/2434073003/diff/20001/webrtc/video_encoder.h#ne...
webrtc/video_encoder.h:164: virtual int32_t SetRates(uint32_t bitrate, uint32_t
framerate) = 0;
On 2016/10/21 08:24:29, perkj_webrtc wrote:
> Deprecate SetRates and add a default implementation that DCHECK since we no
> longer call it.

Done.

[perkj_webrtc, 2016-10-25 11:02:27]

I have not looked at anything but ViEEncoder but that looks good. Just one
suggestion/question.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:443: rate_allocator_ =
VideoBitrateAllocatorFactory::GetBitrateAllocator(&codec);
This looks much better I think. 
But one more question - Do we want to move towards using VideoEncoderConfig ? 
If so, I think it might make sense have the new VideoBitrateAllocatorFactory
take that as an argument instead of VideoCodec. 

It might make sense to move VideoEncoderConfigToVideoCodec to there until
VideoSender also operate on VideoEncoderConfig or has been deprecated?

[sprang_webrtc, 2016-10-25 11:43:03]

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:443: rate_allocator_ =
VideoBitrateAllocatorFactory::GetBitrateAllocator(&codec);
On 2016/10/25 11:02:27, perkj_webrtc wrote:
> This looks much better I think. 
> But one more question - Do we want to move towards using VideoEncoderConfig ? 
> If so, I think it might make sense have the new VideoBitrateAllocatorFactory
> take that as an argument instead of VideoCodec. 
> 
> It might make sense to move VideoEncoderConfigToVideoCodec to there until
> VideoSender also operate on VideoEncoderConfig or has been deprecated? 

Hm, I'm not sure. It'll become a little messy because of how
encoder_specific_settings are handled. I might have to create a default instance
in another place just to populate the tl_factory field. Or move that out, which
brings back the codec specific part.

Can we please postpone that?

[sprang_webrtc, 2016-10-26 13:43:00]

ping?

[perkj_webrtc, 2016-10-27 12:57:32]

I think Stefan should take a look as well.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/common_types.h#new...
webrtc/common_types.h:627: static constexpr size_t kMaxBitrateBps = 100000000;
nit : add comment 100 Mbit/s

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/i420/include/i420.h (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/i420/include/i420.h:16: #include
"webrtc/base/checks.h"
seems to be included in the wrong file.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_bitrate_allocator_factory.h
(right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_bitrate_allocator_factory.h:21: class
VideoBitrateAllocatorFactory {
Please add documementation.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_bitrate_allocator_factory.cc (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_bitrate_allocator_factory.cc:40:
VideoBitrateAllocatorFactory::GetBitrateAllocator(VideoCodec* codec) {
it feels weird that GetBitrateAllocator take a none const pointer and also
modify it. 

I still think it would make sence if this method took a just the payload name
and created a VideoBitrateAllocator. 

The VideoBitrateAllocator can then construct the VideoCodec. In the VP8
VideoBitrateAllocator your can set codec->codecSpecific.VP8.tl_factory.

Change in ViEEncoder::ReconfigureVideoEncoder would be something like:

encoder_config_, streams, settings_.payload_name, settings_.payload_type

rate_allocator_.reset(VideoBitrateAllocatorFactory::GetBitrateAllocator(encoder_config_,
streams, settings_.payload_name,settings_.payload_type);
VideoCodec codec = rate_allocator->GetVideoCodec();

[sprang_webrtc, 2016-11-01 18:03:16]

Updated how VideoCodec is created, ptal.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/common_types.h#new...
webrtc/common_types.h:627: static constexpr size_t kMaxBitrateBps = 100000000;
On 2016/10/27 12:57:31, perkj_webrtc wrote:
> nit : add comment 100 Mbit/s

Done.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/i420/include/i420.h (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/i420/include/i420.h:16: #include
"webrtc/base/checks.h"
On 2016/10/27 12:57:31, perkj_webrtc wrote:
> seems to be included in the wrong file.

Done.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_bitrate_allocator_factory.h
(right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_bitrate_allocator_factory.h:21: class
VideoBitrateAllocatorFactory {
On 2016/10/27 12:57:31, perkj_webrtc wrote:
> Please add documementation.

Done.

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_bitrate_allocator_factory.cc (right):

https://codereview.webrtc.org/2434073003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_bitrate_allocator_factory.cc:40:
VideoBitrateAllocatorFactory::GetBitrateAllocator(VideoCodec* codec) {
On 2016/10/27 12:57:31, perkj_webrtc wrote:
> it feels weird that GetBitrateAllocator take a none const pointer and also
> modify it. 
> 
> I still think it would make sence if this method took a just the payload name
> and created a VideoBitrateAllocator. 
> 
> The VideoBitrateAllocator can then construct the VideoCodec. In the VP8
> VideoBitrateAllocator your can set codec->codecSpecific.VP8.tl_factory.
> 
> Change in ViEEncoder::ReconfigureVideoEncoder would be something like:
> 
> encoder_config_, streams, settings_.payload_name, settings_.payload_type
> 
>
rate_allocator_.reset(VideoBitrateAllocatorFactory::GetBitrateAllocator(encoder_config_,
> streams, settings_.payload_name,settings_.payload_type);
> VideoCodec codec = rate_allocator->GetVideoCodec(); 

I don't think it makes sense to have the rate allocator itself create the
VideoCodec instance.
I changed this class instead to VideoCodecInitializer, which can handle all of
VideoEncoderConfigToVideoCodec() for vie_encoder, and also creates the
VideoBitrateAllocator which goes with that.
Makes sense?

[perkj_webrtc, 2016-11-01 18:40:10]

lgtm with the comments addressed.

I have not looked in detail on the video_coding changes since I am not that
familiar with the codec implementations.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:567: rtc::Optional<std::string>
CodecTypeToPayloadName(VideoCodecType type);
nit: I would prefer empty string and unknown as return values and DCHECK  if the
conversion does not exist.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_codec_initializer.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_codec_initializer.h:29: // Takes an
EncoderSettings a VideoEncoderConfig and the VideoStream
nit // EncoderSettings, a Video..

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_codec_initializer.h:42: // Create a
bitrate allocator for the specified codec. tl_factory is optional,
|tl_factory| is optional. If it...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_codec_initializer.h:45: static
std::unique_ptr<VideoBitrateAllocator> GetBitrateAllocator(
Should this be private or is the intention that one day, this is what survives?

[stefan-webrtc, 2016-11-02 10:26:36]

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:14: #include "webrtc/base/checks.h"
Included twice.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:151: if (bitrate_bps > kMaxBitrateBps ||
Is it really needed to have a max? Can we use
std::numeric_limits<uint32_t>::max() instead? We have so many max bitrates in
our code that increasing the max would be a mess. :)

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:156: sum_ -= bitrates_[spatial_index][temporal_index];
Maybe DCHECK that sum_ is greater than bitrates_... before you subtract?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:661: uint32_t get_sum_kbps() const { return (sum_ + 500) /
1000; }
Maybe skip get_ or call them GetSum*()? I at least think that's the
convention...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_video/inclu...
File webrtc/common_video/include/video_bitrate_allocator.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_video/inclu...
webrtc/common_video/include/video_bitrate_allocator.h:24: uint32_t framerate) =
0;
Take the opportunity to align the framerate type here with the one below?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_video/inclu...
webrtc/common_video/include/video_bitrate_allocator.h:25: virtual uint32_t
GetPreferedBitrate(int frame_rate) = 0;
Preferred

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:29:
DefaultTemporalLayers::DefaultTemporalLayers(int numberOfTemporalLayers,
Please update this to non_camel_case

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:52: int
bitrateKbit,
Here too please :) Perhaps also change to kbps instead of kbit...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:68:
bitrates[i] -= sum;
DCHECK that bitrates[i] > 0 after the subtraction, or that sum is less than
bitrates[i] before subtraction.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:93:
cfg->ts_target_bitrate[0] = (*new_bitrates_kbps_)[0];
Should we call this current_allocation_kbps_ or current_bitrates_kbps_ instead?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:199:
new_bitrates_kbps_ = rtc::Optional<std::vector<uint32_t>>();
Why is this needed?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/realtime_temporal_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/realtime_temporal_layers.cc:180:
bitrates[i] -= sum;
Add the same DCHECK here as in the default layers.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/realtime_temporal_layers.cc:307:
listener_->OnTemporalLayersCreated(simulcast_id, tl);
I find this a bit weird, but it might become clearer later on...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:151:
std::vector<uint32_t> ScreenshareLayers::OnRatesUpdated(int bitrate_kbit,
I prefer kbps... :)

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:156: frame_rate_ =
framerate;
Rename framerate_

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
(right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:64:
layers_->OnRatesUpdated(kDefaultTl0BitrateKbps, kDefaultTl1BitrateKbps,
Should you expect something on the allocation returned?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:66:
layers_->UpdateConfiguration(&vpx_cfg);
EXPECT_TRUE?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:377:
layers_->OnRatesUpdated(100, 1000, 5);
Same for these

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (left):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:381: //
TODO(holmer): This is a temporary hack for screensharing, where we
Is it safe to remove the hack? :)

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h (left):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h:150: class
SkipEncodingUnusedStreamsTest {
Could you comment on why this is no longer tested?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:187: if (codec_.maxBitrate >
0 && bitrate.get_sum_kbps() > codec_.maxBitrate)
Should we DCHECK instead if it should match at this point?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:56: // temporal
layer 0. We'll then distribute this stream allocation is done.
"...distribute this when stream allocation is done"?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:67:
left_to_allocate -= allocation;
DCHECK to make sure left_to_allocate doesn't end up wrapping around?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:81:
left_to_allocate -= allocation;
here too

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:95: continue; 
// TODO(sprang): If > 1 SS, assume default TL alloc?
Does this change current behavior? Seems reasonable that you can create
simulcast without temporal layers.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:110: // frames.
This hack should be improved.
:/

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.h:37: uint32_t
GetPreferedBitrate(int frame_rate) override;
framerate

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_codec_initializer.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_codec_initializer.cc:67: //              
codec types.
Why not warn now? Are we expected to get here?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:17: #include
"webrtc/common_video/include/video_bitrate_allocator.h"
Do you have to include both this one and default_video_bitrate_allocator.h?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocator*
bitrate_allocator) {
Does this change on every channel parameter update? Should it be set in the ctor
instead?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:168: .value_or(VideoCodecType::kVideoCodecUnknown)),
I guess unknown should generate a warning?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:314: int frame_rate =
stats_proxy_->GetSendFrameRate();
framerate

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video_encoder.h
File webrtc/video_encoder.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video_encoder.h#ne...
webrtc/video_encoder.h:219: BitrateAllocation bitrate_;
Rename bitrate_allocation_;

[sprang_webrtc, 2016-11-02 13:28:34]

Addressed comments and rebased.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:14: #include "webrtc/base/checks.h"
On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> Included twice.

Oops. Rebase error...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:151: if (bitrate_bps > kMaxBitrateBps ||
On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> Is it really needed to have a max? Can we use
> std::numeric_limits<uint32_t>::max() instead? We have so many max bitrates in
> our code that increasing the max would be a mess. :)

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:156: sum_ -= bitrates_[spatial_index][temporal_index];
On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> Maybe DCHECK that sum_ is greater than bitrates_... before you subtract?

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:567: rtc::Optional<std::string>
CodecTypeToPayloadName(VideoCodecType type);
On 2016/11/01 18:40:10, perkj_webrtc wrote:
> nit: I would prefer empty string and unknown as return values and DCHECK  if
the
> conversion does not exist. 

I'm not sure I agree. There was at least one usage where "generic" should be
used instead of unknown, so it's a bit ambiguous what these values indicate.
Especially the conversion from string to type should force the caller to handle
the error case imo.
I'd like to change this to an enum class longer term, and get compile time error
for incorrect type to string.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:661: uint32_t get_sum_kbps() const { return (sum_ + 500) /
1000; }
On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> Maybe skip get_ or call them GetSum*()? I at least think that's the
> convention...

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_video/inclu...
File webrtc/common_video/include/video_bitrate_allocator.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_video/inclu...
webrtc/common_video/include/video_bitrate_allocator.h:24: uint32_t framerate) =
0;
On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> Take the opportunity to align the framerate type here with the one below?

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_video/inclu...
webrtc/common_video/include/video_bitrate_allocator.h:25: virtual uint32_t
GetPreferedBitrate(int frame_rate) = 0;
On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> Preferred

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:29:
DefaultTemporalLayers::DefaultTemporalLayers(int numberOfTemporalLayers,
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Please update this to non_camel_case

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:52: int
bitrateKbit,
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Here too please :) Perhaps also change to kbps instead of kbit...

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:68:
bitrates[i] -= sum;
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> DCHECK that bitrates[i] > 0 after the subtraction, or that sum is less than
> bitrates[i] before subtraction.

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:93:
cfg->ts_target_bitrate[0] = (*new_bitrates_kbps_)[0];
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Should we call this current_allocation_kbps_ or current_bitrates_kbps_
instead?

They are new bitrates though, as the encoder has not yet been updated with these
values, and they will be removed after that..

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:199:
new_bitrates_kbps_ = rtc::Optional<std::vector<uint32_t>>();
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Why is this needed?

To not have to redo all this work for every frame.

Previously the encoder implementation would call ConfigureBirates directly from
its RatesUpdated() method, this would both calculate the allocation and update
the vpx_codec_enc_cfg_t struct.

Now, OnRatesUpdated() will be called externally (from ViEEncoder) when bitrates
are updated.
The encoder will call UpdateConfiguration() to update the config struct both on
RatesUpdated() and from Encode(), but we only want to reset the temporal pattern
etc if the rates have actually changed since the last UpdateoConfiguration()
call...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/realtime_temporal_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/realtime_temporal_layers.cc:180:
bitrates[i] -= sum;
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Add the same DCHECK here as in the default layers.

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/realtime_temporal_layers.cc:307:
listener_->OnTemporalLayersCreated(simulcast_id, tl);
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> I find this a bit weird, but it might become clearer later on...

Acknowledged.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:151:
std::vector<uint32_t> ScreenshareLayers::OnRatesUpdated(int bitrate_kbit,
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> I prefer kbps... :)

Me too. This was probably a copy/paste thing from the interface, which uses
kbit. Changed where used.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:156: frame_rate_ =
framerate;
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Rename framerate_

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
(right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:64:
layers_->OnRatesUpdated(kDefaultTl0BitrateKbps, kDefaultTl1BitrateKbps,
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Should you expect something on the allocation returned?

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:66:
layers_->UpdateConfiguration(&vpx_cfg);
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> EXPECT_TRUE?

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:377:
layers_->OnRatesUpdated(100, 1000, 5);
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Same for these

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (left):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:381: //
TODO(holmer): This is a temporary hack for screensharing, where we
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Is it safe to remove the hack? :)

It's not removed; it's only been moved into the RateAllocator class. And removed
from a handful of other places where the logic was duplicated...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h (left):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h:150: class
SkipEncodingUnusedStreamsTest {
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Could you comment on why this is no longer tested?

The purpose of this test was to make sure the codec was configured with 0
bitrate for layers that weren't gonna be encoded, and required catching this via
callbacks.
As that rate allocation is now handled entirely outside of the codec, this
convoluted test doesn't really have a purpose anymore....

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:187: if (codec_.maxBitrate >
0 && bitrate.get_sum_kbps() > codec_.maxBitrate)
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Should we DCHECK instead if it should match at this point?

Yes, that is probably a good idea.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_codec_initializer.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_codec_initializer.h:29: // Takes an
EncoderSettings a VideoEncoderConfig and the VideoStream
On 2016/11/01 18:40:10, perkj_webrtc wrote:
> nit // EncoderSettings, a Video.. 

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_codec_initializer.h:42: // Create a
bitrate allocator for the specified codec. tl_factory is optional,
On 2016/11/01 18:40:10, perkj_webrtc wrote:
> |tl_factory| is optional. If it...

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_codec_initializer.h:45: static
std::unique_ptr<VideoBitrateAllocator> GetBitrateAllocator(
On 2016/11/01 18:40:10, perkj_webrtc wrote:
> Should this be private or is the intention that one day, this is what
survives? 

The intention is that this should become private, once some test code that uses
it can be ditched.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:56: // temporal
layer 0. We'll then distribute this stream allocation is done.
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> "...distribute this when stream allocation is done"?

For great justice!

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:67:
left_to_allocate -= allocation;
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> DCHECK to make sure left_to_allocate doesn't end up wrapping around?

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:81:
left_to_allocate -= allocation;
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> here too

We do cap allocation to left_to_allocate two lines above, but ok :)

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:95: continue; 
// TODO(sprang): If > 1 SS, assume default TL alloc?
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> Does this change current behavior? Seems reasonable that you can create
> simulcast without temporal layers.

This should not change the behavior, I think. 
The question I was ponder was, if no temporal layers have been configured
(rather than explicitly setting a single-layer temporal layers factory), should
we assume single layer or the default temporal layer strategy we use with
simulcast today?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:110: // frames.
This hack should be improved.
On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> :/

I know :(
Should be easier to get rid of after this refactoring cl though. For now I want
to make sure this is actually a 2k+ line nop....

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.h:37: uint32_t
GetPreferedBitrate(int frame_rate) override;
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> framerate

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_codec_initializer.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_codec_initializer.cc:67: //              
codec types.
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> Why not warn now? Are we expected to get here?

My thinking is that once we have some form of TL allocation support for VP9 and
H264(?), we should initialize that here. 
Right now we only care about VP8 however, and leave the other ones to handle
that internally if at all.
At least VP9 should follow in another CL, unless you prefer this one to grow?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:17: #include
"webrtc/common_video/include/video_bitrate_allocator.h"
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> Do you have to include both this one and default_video_bitrate_allocator.h?

I don't have to, but I think this follows the "include what you use" rule. We
use both a raw VideoBitrateAllocator interface and the
DefaultVideoBitrateAllocator class, so I prefer not to include the interface
implicitly.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocator*
bitrate_allocator) {
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> Does this change on every channel parameter update? Should it be set in the
ctor
> instead?

The bitrate_allocator is owned by ViEEncoder, and may change on
ReconfigureEncoder(). I included it as a parameter here rather than set up an
explicit setter method and keeping a state.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:168: .value_or(VideoCodecType::kVideoCodecUnknown)),
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> I guess unknown should generate a warning?

It's probably the fake encoder. Do we always want to warn about that?

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:314: int frame_rate =
stats_proxy_->GetSendFrameRate();
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> framerate

Done.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video_encoder.h
File webrtc/video_encoder.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/video_encoder.h#ne...
webrtc/video_encoder.h:219: BitrateAllocation bitrate_;
On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> Rename bitrate_allocation_;

Done.

[sprang_webrtc, 2016-11-03 09:19:28]

Found an interesting race when I DCHECK'ed (rather than just return en error
coder) in the encoder that the allocated bitrate is correct.

1. ViEEncoder::OnBitrateUpdated is called, causing a bitrate allocation to be
stored as pending in VideoSender.
2. ViEEncoder::ReconfigureEncoder is called, setting up a new VideoCodec
instance which is incompatible with the pending bitrate allocation.
3. ViEEncoder::OnFrame is called, which will pick up the pending bitrate config
and try to use it with VideoEncoder::SetRateAllocation.
4. SetRateAllocation fails, so an error code is returned to GenericEncoder,
which silently ignores it...

This was probably a problem even before this CL, but likely somewhat benign
since it will recover at the next SetRates.

I've fixed this by forcing a bitrate update after reconfiguring the encoder, and
fixed a few test cases as well. Also added logging of failed calls in
GenericEncoder :)

As a side not, we should try to get rid of the pending encoder settings thingy,
it's really useful now that we have a task queue running the show. But I don't
want to grow this CL any further...

[stefan-webrtc, 2016-11-03 13:34:58]

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:661: uint32_t get_sum_kbps() const { return (sum_ + 500) /
1000; }
On 2016/11/02 13:28:32, språng wrote:
> On 2016/11/02 10:26:34, stefan-webrtc (holmer) wrote:
> > Maybe skip get_ or call them GetSum*()? I at least think that's the
> > convention...
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Ack, things have apparently changed since I read this thing. :)

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc:199:
new_bitrates_kbps_ = rtc::Optional<std::vector<uint32_t>>();
On 2016/11/02 13:28:32, språng wrote:
> On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> > Why is this needed?
> 
> To not have to redo all this work for every frame.
> 
> Previously the encoder implementation would call ConfigureBirates directly
from
> its RatesUpdated() method, this would both calculate the allocation and update
> the vpx_codec_enc_cfg_t struct.
> 
> Now, OnRatesUpdated() will be called externally (from ViEEncoder) when
bitrates
> are updated.
> The encoder will call UpdateConfiguration() to update the config struct both
on
> RatesUpdated() and from Encode(), but we only want to reset the temporal
pattern
> etc if the rates have actually changed since the last UpdateoConfiguration()
> call...

Acknowledged.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:81:
left_to_allocate -= allocation;
On 2016/11/02 13:28:33, språng wrote:
> On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> > here too
> 
> We do cap allocation to left_to_allocate two lines above, but ok :)

I guess I missed that :)

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:95: continue; 
// TODO(sprang): If > 1 SS, assume default TL alloc?
On 2016/11/02 13:28:33, språng wrote:
> On 2016/11/02 10:26:35, stefan-webrtc (holmer) wrote:
> > Does this change current behavior? Seems reasonable that you can create
> > simulcast without temporal layers.
> 
> This should not change the behavior, I think. 
> The question I was ponder was, if no temporal layers have been configured
> (rather than explicitly setting a single-layer temporal layers factory),
should
> we assume single layer or the default temporal layer strategy we use with
> simulcast today?

I would have expected no temporal layers...

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_codec_initializer.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_codec_initializer.cc:67: //              
codec types.
On 2016/11/02 13:28:33, språng wrote:
> On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> > Why not warn now? Are we expected to get here?
> 
> My thinking is that once we have some form of TL allocation support for VP9
and
> H264(?), we should initialize that here. 
> Right now we only care about VP8 however, and leave the other ones to handle
> that internally if at all.
> At least VP9 should follow in another CL, unless you prefer this one to grow?

Acknowledged.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:17: #include
"webrtc/common_video/include/video_bitrate_allocator.h"
On 2016/11/02 13:28:33, språng wrote:
> On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> > Do you have to include both this one and default_video_bitrate_allocator.h?
> 
> I don't have to, but I think this follows the "include what you use" rule. We
> use both a raw VideoBitrateAllocator interface and the
> DefaultVideoBitrateAllocator class, so I prefer not to include the interface
> implicitly.

Acknowledged.

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocator*
bitrate_allocator) {
On 2016/11/02 13:28:33, språng wrote:
> On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> > Does this change on every channel parameter update? Should it be set in the
> ctor
> > instead?
> 
> The bitrate_allocator is owned by ViEEncoder, and may change on
> ReconfigureEncoder(). I included it as a parameter here rather than set up an
> explicit setter method and keeping a state.

Probably good, or perhaps you should pass in a BitrateAllocation instead? And if
it's nullptr you create a default allocation?

https://codereview.webrtc.org/2434073003/diff/220001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2434073003/diff/220001/webrtc/common_types.cc#n...
webrtc/common_types.cc:157: if (new_bitrate_sum_bps > kMaxBitrateBps)
Now the question is, can this ever happen when new_bitrate_sum_bps is a size_t?
:)

[sprang_webrtc, 2016-11-03 14:36:59]

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocator*
bitrate_allocator) {
On 2016/11/03 13:34:58, stefan-webrtc (holmer) wrote:
> On 2016/11/02 13:28:33, språng wrote:
> > On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> > > Does this change on every channel parameter update? Should it be set in
the
> > ctor
> > > instead?
> > 
> > The bitrate_allocator is owned by ViEEncoder, and may change on
> > ReconfigureEncoder(). I included it as a parameter here rather than set up
an
> > explicit setter method and keeping a state.
> 
> Probably good, or perhaps you should pass in a BitrateAllocation instead? And
if
> it's nullptr you create a default allocation?

The reason I pass down the allocator reference is that it needs the
input_frame_rate as argument. That is calculated by _mediaOpt in VideoSender...
The default allocator path is just used in tests.

I hope we can clean this up if we merge VideoSender ViEEncoder.

https://codereview.webrtc.org/2434073003/diff/220001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2434073003/diff/220001/webrtc/common_types.cc#n...
webrtc/common_types.cc:157: if (new_bitrate_sum_bps > kMaxBitrateBps)
On 2016/11/03 13:34:58, stefan-webrtc (holmer) wrote:
> Now the question is, can this ever happen when new_bitrate_sum_bps is a
size_t?
> :)

Alright, let's do uint64_t instead then.

[stefan-webrtc, 2016-11-03 15:32:08]

lgtm

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2434073003/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocator*
bitrate_allocator) {
On 2016/11/03 14:36:59, språng wrote:
> On 2016/11/03 13:34:58, stefan-webrtc (holmer) wrote:
> > On 2016/11/02 13:28:33, språng wrote:
> > > On 2016/11/02 10:26:36, stefan-webrtc (holmer) wrote:
> > > > Does this change on every channel parameter update? Should it be set in
> the
> > > ctor
> > > > instead?
> > > 
> > > The bitrate_allocator is owned by ViEEncoder, and may change on
> > > ReconfigureEncoder(). I included it as a parameter here rather than set up
> an
> > > explicit setter method and keeping a state.
> > 
> > Probably good, or perhaps you should pass in a BitrateAllocation instead?
And
> if
> > it's nullptr you create a default allocation?
> 
> The reason I pass down the allocator reference is that it needs the
> input_frame_rate as argument. That is calculated by _mediaOpt in
VideoSender...
> The default allocator path is just used in tests.
> 
> I hope we can clean this up if we merge VideoSender ViEEncoder.

I don't know how, but I missed that.

https://codereview.webrtc.org/2434073003/diff/220001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2434073003/diff/220001/webrtc/common_types.cc#n...
webrtc/common_types.cc:157: if (new_bitrate_sum_bps > kMaxBitrateBps)
On 2016/11/03 14:36:59, språng wrote:
> On 2016/11/03 13:34:58, stefan-webrtc (holmer) wrote:
> > Now the question is, can this ever happen when new_bitrate_sum_bps is a
> size_t?
> > :)
> 
> Alright, let's do uint64_t instead then.

Hehe... Or can we just assume it's safe? I just meant checking against max() was
useless.

[sprang_webrtc, 2016-11-07 15:55:20]

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

[sprang_webrtc, 2016-11-07 15:55:21]

+mflodman for webrtc/common*

[sprang_webrtc, 2016-11-08 13:56:52]

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

[commit-bot: I haz the power, 2016-11-08 13:57:00]

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

[commit-bot: I haz the power, 2016-11-08 14:07:33]

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

[commit-bot: I haz the power, 2016-11-08 14:07:34]

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

[sprang_webrtc, 2016-11-08 16:28:47]

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

[commit-bot: I haz the power, 2016-11-08 16:28:54]

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

[commit-bot: I haz the power, 2016-11-08 16:52:05]

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

[commit-bot: I haz the power, 2016-11-08 16:52:06]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-11-09 11:58:12]

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_codec_initializer.cc (right):

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_codec_initializer.cc:77:
VideoCodecInitializer::GetBitrateAllocator(
I think this method should be called CreateBitrateAllocator

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.cc (right):

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.cc:119: maxPayloadSize);
I find it a bit difficult to understand lifetime of objects here. tl_factory
ownership is handed over to rate_allocator_, which lives up until
VideoCodingModuleImpl is destroyed? Or does it live only until RegisterSendCodec
is called again? Is it safe for vp8_codec to have a pointer to it? Maybe at
least worth commenting on.

[sprang_webrtc, 2016-11-09 12:26:19]

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_codec_initializer.cc (right):

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_codec_initializer.cc:77:
VideoCodecInitializer::GetBitrateAllocator(
On 2016/11/09 11:58:12, stefan-webrtc (holmer) wrote:
> I think this method should be called CreateBitrateAllocator

Done.

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.cc (right):

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.cc:119: maxPayloadSize);
On 2016/11/09 11:58:12, stefan-webrtc (holmer) wrote:
> I find it a bit difficult to understand lifetime of objects here. tl_factory
> ownership is handed over to rate_allocator_, which lives up until
> VideoCodingModuleImpl is destroyed? Or does it live only until
RegisterSendCodec
> is called again? Is it safe for vp8_codec to have a pointer to it? Maybe at
> least worth commenting on.

The TemporalLayersFactory is owned by the BitrateAllocator yes, and it must live
when InitEncode() is called on the codec implementation. Since this is done on a
task queue we can't really know when that is, so we let the instance live until
we destroy the video coding module or we register a new send codec.
I can add a comment.

[stefan-webrtc, 2016-11-09 12:40:26]

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.cc (right):

https://codereview.webrtc.org/2434073003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.cc:119: maxPayloadSize);
On 2016/11/09 12:26:18, språng wrote:
> On 2016/11/09 11:58:12, stefan-webrtc (holmer) wrote:
> > I find it a bit difficult to understand lifetime of objects here. tl_factory
> > ownership is handed over to rate_allocator_, which lives up until
> > VideoCodingModuleImpl is destroyed? Or does it live only until
> RegisterSendCodec
> > is called again? Is it safe for vp8_codec to have a pointer to it? Maybe at
> > least worth commenting on.
> 
> The TemporalLayersFactory is owned by the BitrateAllocator yes, and it must
live
> when InitEncode() is called on the codec implementation. Since this is done on
a
> task queue we can't really know when that is, so we let the instance live
until
> we destroy the video coding module or we register a new send codec.
> I can add a comment.

Does it make more sense to let the codec own the allocator? It seems to be the
only one really using it?

[stefan-webrtc, 2016-11-09 12:43:08]

lgtm after offline discussion

[sprang_webrtc, 2016-11-09 12:44:36]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-11-09 12:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2434073003/#ps320001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2016-11-09 12:44:48]

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

[commit-bot: I haz the power, 2016-11-09 13:09:12]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-11-09 13:09:16]

Description was changed from

==========
Extract bitrate allocation of spatial/temporal layers out of codec impl.

This CL makes a number of intervowen changes:

* Add BitrateAllocation struct, that contains a codec independent view
  of how the target bitrate is distributed over spatial and temporal
  layers.

* Adds the BitrateAllocator interface, which takes a bitrate and frame
  rate and produces a BitrateAllocation.

* A default (non layered) implementation is added, and
  SimulcastRateAllocator is extended to fully handle VP8 allocation.
  This includes capturing TemporalLayer instances created by the
  encoder.

* ViEEncoder now owns both the bitrate allocator and the temporal layer
  factories for VP8. This allows allocation to happen fully outside of
  the encoder implementation.

This refactoring will make it possible for ViEEncoder to signal the
full picture of target bitrates to the RTCP module.

BUG=webrtc:6301
==========

to

==========
Extract bitrate allocation of spatial/temporal layers out of codec impl.

This CL makes a number of intervowen changes:

* Add BitrateAllocation struct, that contains a codec independent view
  of how the target bitrate is distributed over spatial and temporal
  layers.

* Adds the BitrateAllocator interface, which takes a bitrate and frame
  rate and produces a BitrateAllocation.

* A default (non layered) implementation is added, and
  SimulcastRateAllocator is extended to fully handle VP8 allocation.
  This includes capturing TemporalLayer instances created by the
  encoder.

* ViEEncoder now owns both the bitrate allocator and the temporal layer
  factories for VP8. This allows allocation to happen fully outside of
  the encoder implementation.

This refactoring will make it possible for ViEEncoder to signal the
full picture of target bitrates to the RTCP module.

BUG=webrtc:6301

Committed: https://crrev.com/8f46c679d24a05b3f08e02c6d91ec9637f34e24f
Cr-Commit-Position: refs/heads/master@{#14998}
==========

[commit-bot: I haz the power, 2016-11-09 13:09:17]

Patchset 17 (id:??) landed as
https://crrev.com/8f46c679d24a05b3f08e02c6d91ec9637f34e24f
Cr-Commit-Position: refs/heads/master@{#14998}

[sprang_webrtc, 2016-11-09 14:14:29]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.webrtc.org/2489843002/ by sprang@webrtc.org.

The reason for reverting is: Breaks perf tests..