Bitrate controller for VideoToolbox encoder.

Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc:4081
Committed: https://crrev.com/f75d0082350096077e6553a149bda7749593b9c7
Cr-Commit-Position: refs/heads/master@{#11729}

[tkchin_webrtc, 2016-02-03 15:39:14]

Description was changed from

==========
Bitrate controller for hardware encoders.

Parameters used in the implementation were tested using iOS H264
encoder and may not be appropriate for other encoders. We can add
different tuning in the future if needed.

BUG=webrtc::4081
==========

to

==========
Bitrate controller for hardware encoders.

Parameters used in the implementation were tested using iOS H264
encoder and may not be appropriate for other encoders. We can add
different tuning in the future if needed.

BUG=webrtc::4081
==========

[tkchin_webrtc, 2016-02-03 15:39:15]

tkchin@webrtc.org changed reviewers:
+ pbos@webrtc.org, stefan@webrtc.org

[tkchin_webrtc, 2016-02-03 15:39:35]

Patchset #1 (id:1) has been deleted

[tkchin_webrtc, 2016-02-03 15:39:47]

Patchset #1 (id:20001) has been deleted

[pbos-webrtc, 2016-02-03 15:50:07]

I don't think we should add another layer to VideoEncoders, if this is a bug in
VideoToolbox encoders then we should make it a component that can be integrated
into the VideoToolbox implementation rather than wrapped around it.

[pbos-webrtc, 2016-02-03 15:52:22]

https://codereview.webrtc.org/1660963002/diff/40001/webrtc/video/video_encode...
File webrtc/video/video_encoder_hardware.cc (right):

https://codereview.webrtc.org/1660963002/diff/40001/webrtc/video/video_encode...
webrtc/video/video_encoder_hardware.cc:90: int32_t
HardwareVideoEncoderWrapper::SetRates(uint32_t bitrate,
This one is permitted to be called often so you should not reset
adjusted_bitrate_ inside.

[tkchin_webrtc, 2016-02-03 16:00:40]

On 2016/02/03 15:50:07, pbos-webrtc wrote:
> I don't think we should add another layer to VideoEncoders, if this is a bug
in
> VideoToolbox encoders then we should make it a component that can be
integrated
> into the VideoToolbox implementation rather than wrapped around it.

It's a general problem with hardware encoders that they don't respect bitrates
very well; it's not specific to VideoToolbox. I can add it as a component
instead, but then you'd have to add it for other ones, e.g. encoders on some
Android devices. I don't mind either way. wdyt?

[pbos-webrtc, 2016-02-03 16:06:11]

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

[pbos-webrtc, 2016-02-03 16:06:11]

Prefer adding it as a component, if it's to be turned on for all HW codecs I'd
still like it done outside another wrapper. Adding mflodman@ who might have an
opinion as well. If we don't trust any encoders this could possibly go into
VCMGenericEncoder.

[stefan-webrtc, 2016-02-03 16:30:33]

On 2016/02/03 16:06:11, pbos-webrtc wrote:
> Prefer adding it as a component, if it's to be turned on for all HW codecs I'd
> still like it done outside another wrapper. Adding mflodman@ who might have an
> opinion as well. If we don't trust any encoders this could possibly go into
> VCMGenericEncoder.

We discussed this, and I think it's better to start off experimenting/evaluating
this for one implementation (toolbox encoder). I'm pretty sure it will a lot of
work to tune this for each implementation (even more than the quality scaler),
as the dynamics of the rate control on different codec implementations behaves
very differently (and sometimes even unpredictably...)

[stefan-webrtc, 2016-02-03 16:31:05]

On 2016/02/03 16:30:33, stefan-webrtc (holmer) wrote:
> On 2016/02/03 16:06:11, pbos-webrtc wrote:
> > Prefer adding it as a component, if it's to be turned on for all HW codecs
I'd
> > still like it done outside another wrapper. Adding mflodman@ who might have
an
> > opinion as well. If we don't trust any encoders this could possibly go into
> > VCMGenericEncoder.
> 
> We discussed this, and I think it's better to start off
experimenting/evaluating
> this for one implementation (toolbox encoder). I'm pretty sure it will a lot
of
> work to tune this for each implementation (even more than the quality scaler),
> as the dynamics of the rate control on different codec implementations behaves
> very differently (and sometimes even unpredictably...)

And I also wouldn't mind adding it as a component. :)

[tkchin_webrtc, 2016-02-05 14:53:09]

PTAL

[tkchin_webrtc, 2016-02-05 15:37:29]

Description was changed from

==========
Bitrate controller for hardware encoders.

Parameters used in the implementation were tested using iOS H264
encoder and may not be appropriate for other encoders. We can add
different tuning in the future if needed.

BUG=webrtc::4081
==========

to

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc::4081
==========

[tkchin_webrtc, 2016-02-08 15:53:58]

ping.

[stefan-webrtc, 2016-02-09 10:04:17]

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

[stefan-webrtc, 2016-02-09 10:04:37]

Erik will take a look.

[sprang, 2016-02-09 10:46:26]

sprang@google.com changed reviewers:
+ sprang@google.com

[sprang, 2016-02-09 10:46:26]

Could you please add a bitrate_adjuster_unittest.cc as well. Change the clock
implementation used so that you can use a SimulatedClock in the tests.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:69: if (bitrate_tracker_.count()
< 60) {
Please used a named constant for this, or derive from the existing ones

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:79: float error = target_bitrate
- bitrate;
Can we perhaps use webrtc::RateStatistics instead of rtc::RollingAccumulator
here? Then we can get an explicit time based window instead of constants
assuming 30fps.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:92: 100.;
Why do we need to clamp it to below target_bitrate?

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:105:
observer_->OnBitrateAdjusted(adjusted_bitrate);
Perhaps we should have a limit on how often this will be called, so that we
don't accidentally screw with the encoder by calling it too often. Max every N
frames?

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:310: //
Need to reset to that the session is invalidated and won't use the
s/to that/so that

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:311: //
callback anymore. Frames may continue to vend until it is torn down
s/vend/send

[tkchin_webrtc, 2016-02-11 00:03:21]

PTAL

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:69: if (bitrate_tracker_.count()
< 60) {
On 2016/02/09 10:46:26, sprang wrote:
> Please used a named constant for this, or derive from the existing ones

Done.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:79: float error = target_bitrate
- bitrate;
On 2016/02/09 10:46:26, sprang wrote:
> Can we perhaps use webrtc::RateStatistics instead of rtc::RollingAccumulator
> here? Then we can get an explicit time based window instead of constants
> assuming 30fps.

Sure, that seems like a good idea.
Weird to introduce dependency on remote_bitrate_estimator though. I'll move
ratestatistics to webrtc/base.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:92: 100.;
On 2016/02/09 10:46:26, sprang wrote:
> Why do we need to clamp it to below target_bitrate?

It seemed to reduce oscillations. We know that the encoder consistently
overshoots at 100%, so clamping it a bit under after it actually does so seems
to make sense.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:105:
observer_->OnBitrateAdjusted(adjusted_bitrate);
On 2016/02/09 10:46:26, sprang wrote:
> Perhaps we should have a limit on how often this will be called, so that we
> don't accidentally screw with the encoder by calling it too often. Max every N
> frames?

The update only happens once every t seconds, so it should be pretty safe.
Counting frames in addition to that seems fine to me as well.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:310: //
Need to reset to that the session is invalidated and won't use the
On 2016/02/09 10:46:26, sprang wrote:
> s/to that/so that

Done.

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:311: //
callback anymore. Frames may continue to vend until it is torn down
On 2016/02/09 10:46:26, sprang wrote:
> s/vend/send

Done.

[tkchin_webrtc, 2016-02-12 22:32:36]

On 2016/02/11 00:03:21, tkchin_webrtc wrote:
> PTAL
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> File webrtc/modules/video_coding/bitrate_adjuster.cc (right):
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/bitrate_adjuster.cc:69: if
(bitrate_tracker_.count()
> < 60) {
> On 2016/02/09 10:46:26, sprang wrote:
> > Please used a named constant for this, or derive from the existing ones
> 
> Done.
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/bitrate_adjuster.cc:79: float error =
target_bitrate
> - bitrate;
> On 2016/02/09 10:46:26, sprang wrote:
> > Can we perhaps use webrtc::RateStatistics instead of rtc::RollingAccumulator
> > here? Then we can get an explicit time based window instead of constants
> > assuming 30fps.
> 
> Sure, that seems like a good idea.
> Weird to introduce dependency on remote_bitrate_estimator though. I'll move
> ratestatistics to webrtc/base.
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/bitrate_adjuster.cc:92: 100.;
> On 2016/02/09 10:46:26, sprang wrote:
> > Why do we need to clamp it to below target_bitrate?
> 
> It seemed to reduce oscillations. We know that the encoder consistently
> overshoots at 100%, so clamping it a bit under after it actually does so seems
> to make sense.
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/bitrate_adjuster.cc:105:
> observer_->OnBitrateAdjusted(adjusted_bitrate);
> On 2016/02/09 10:46:26, sprang wrote:
> > Perhaps we should have a limit on how often this will be called, so that we
> > don't accidentally screw with the encoder by calling it too often. Max every
N
> > frames?
> 
> The update only happens once every t seconds, so it should be pretty safe.
> Counting frames in addition to that seems fine to me as well.
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
> (right):
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:310: //
> Need to reset to that the session is invalidated and won't use the
> On 2016/02/09 10:46:26, sprang wrote:
> > s/to that/so that
> 
> Done.
> 
>
https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:311: //
> callback anymore. Frames may continue to vend until it is torn down
> On 2016/02/09 10:46:26, sprang wrote:
> > s/vend/send
> 
> Done.

ping.

[sprang_webrtc, 2016-02-15 08:51:32]

sprang@webrtc.org changed reviewers:
- sprang@google.com

[sprang_webrtc, 2016-02-15 08:51:32]

lgtm
Sorry for the late reply, was sick a few day last week...
Thanks for moving RateStatistics!

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:79: float error = target_bitrate
- bitrate;
On 2016/02/11 00:03:20, tkchin_webrtc wrote:
> On 2016/02/09 10:46:26, sprang wrote:
> > Can we perhaps use webrtc::RateStatistics instead of rtc::RollingAccumulator
> > here? Then we can get an explicit time based window instead of constants
> > assuming 30fps.
> 
> Sure, that seems like a good idea.
> Weird to introduce dependency on remote_bitrate_estimator though. I'll move
> ratestatistics to webrtc/base.

That sounds great, thanks!

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:92: 100.;
On 2016/02/11 00:03:20, tkchin_webrtc wrote:
> On 2016/02/09 10:46:26, sprang wrote:
> > Why do we need to clamp it to below target_bitrate?
> 
> It seemed to reduce oscillations. We know that the encoder consistently
> overshoots at 100%, so clamping it a bit under after it actually does so seems
> to make sense.

Fair enough, perhaps a comment to that effect would be good for posterity tough?
:)

https://codereview.webrtc.org/1660963002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:105:
observer_->OnBitrateAdjusted(adjusted_bitrate);
On 2016/02/11 00:03:20, tkchin_webrtc wrote:
> On 2016/02/09 10:46:26, sprang wrote:
> > Perhaps we should have a limit on how often this will be called, so that we
> > don't accidentally screw with the encoder by calling it too often. Max every
N
> > frames?
> 
> The update only happens once every t seconds, so it should be pretty safe.
> Counting frames in addition to that seems fine to me as well.

Ah, right. I'm fine with either then.

[tkchin_webrtc, 2016-02-17 00:11:19]

tkchin@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[tkchin_webrtc, 2016-02-17 00:12:44]

tkchin@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[tkchin_webrtc, 2016-02-17 00:12:45]

Stefan/Peter - mind rubber stamping for OWNERS?

kjellander@ - mind stamping for BUILD.gn?

[kjellander_webrtc, 2016-02-17 06:28:41]

Description was changed from

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc::4081
==========

to

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc:4081
==========

[kjellander_webrtc, 2016-02-17 06:38:52]

lgtm, assuming the BUILD.gn differences are handled in another CL (not
necessarily by you since you didn't introduce them).

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/BUILD.gn:20: "../../base:rtc_base",
When comparing with
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi this is
not present so I guess it's not needed.

Actually, it seems this BUILD.gn is very out of sync, since the rbe_components
target is also no longer present in GYP. 
It seems it wasn't updated when making changes in
https://webrtc-codereview.appspot.com/40429004 and has continued to diverge
after that. Can you fix it up in this or a separate CL?

[tkchin_webrtc, 2016-02-17 21:55:41]

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/BUILD.gn:20: "../../base:rtc_base",
On 2016/02/17 06:38:52, kjellander (webrtc) wrote:
> When comparing with
> webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi this is
> not present so I guess it's not needed.
> 
> Actually, it seems this BUILD.gn is very out of sync, since the rbe_components
> target is also no longer present in GYP. 
> It seems it wasn't updated when making changes in
> https://webrtc-codereview.appspot.com/40429004 and has continued to diverge
> after that. Can you fix it up in this or a separate CL?

One of the bots broke without the extra rtc_base so it seems like it is needed.
Probably the gyp build has rtc_base added in somewhere, or never compiles the
h264 fuzzer by consuming only this target.

I'll do it in a separate CL, would like to just get this one in.

[pthatcher, 2016-02-17 22:19:51]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2016-02-17 22:19:51]

lgtm for webrtc/base

[stefan-webrtc, 2016-02-18 20:56:58]

Lgtm, but consider my comments

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:122: <<
(uint32_t)last_adjusted_bitrate
static_cast

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:123: << "\n  adjusted_bitrate:"
<< (uint32_t)adjusted_bitrate;
static_cast

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:467: int
result = callback_->Encoded(frame, &(codec_specific_info), header.get());
Remove parentheses around codec_specific_info

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h
(right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:31: public
BitrateAdjusterObserver {
I think I would have preferred simply asking the bitrate adjuster for the
bitrate in Encode() and set it to the toolbox encoder there before passing it
the frame. Avoids having to deal with this extra observer interface.

[pbos-webrtc, 2016-02-18 21:08:31]

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h
(right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:31: public
BitrateAdjusterObserver {
On 2016/02/18 20:56:58, stefan-webrtc (holmer) wrote:
> I think I would have preferred simply asking the bitrate adjuster for the
> bitrate in Encode() and set it to the toolbox encoder there before passing it
> the frame. Avoids having to deal with this extra observer interface.

I would prefer this as well.

[tkchin_webrtc, 2016-02-18 23:24:51]

Patchset #8 (id:180001) has been deleted

[tkchin_webrtc, 2016-02-18 23:50:39]

Thanks for looking.

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:122: <<
(uint32_t)last_adjusted_bitrate
On 2016/02/18 20:56:58, stefan-webrtc (holmer) wrote:
> static_cast

Done.

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:123: << "\n  adjusted_bitrate:"
<< (uint32_t)adjusted_bitrate;
On 2016/02/18 20:56:58, stefan-webrtc (holmer) wrote:
> static_cast

Done.

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:467: int
result = callback_->Encoded(frame, &(codec_specific_info), header.get());
On 2016/02/18 20:56:58, stefan-webrtc (holmer) wrote:
> Remove parentheses around codec_specific_info

Done.

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h
(right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:31: public
BitrateAdjusterObserver {
On 2016/02/18 21:08:30, pbos-webrtc wrote:
> On 2016/02/18 20:56:58, stefan-webrtc (holmer) wrote:
> > I think I would have preferred simply asking the bitrate adjuster for the
> > bitrate in Encode() and set it to the toolbox encoder there before passing
it
> > the frame. Avoids having to deal with this extra observer interface.
> 
> I would prefer this as well.

Done. Let me know if this is what you had in mind.

[stefan-webrtc, 2016-02-19 08:23:03]

lgtm

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h
(right):

https://codereview.webrtc.org/1660963002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:31: public
BitrateAdjusterObserver {
On 2016/02/18 23:50:39, tkchin_webrtc wrote:
> On 2016/02/18 21:08:30, pbos-webrtc wrote:
> > On 2016/02/18 20:56:58, stefan-webrtc (holmer) wrote:
> > > I think I would have preferred simply asking the bitrate adjuster for the
> > > bitrate in Encode() and set it to the toolbox encoder there before passing
> it
> > > the frame. Avoids having to deal with this extra observer interface.
> > 
> > I would prefer this as well.
> 
> Done. Let me know if this is what you had in mind.

Exactly.

[pbos-webrtc, 2016-02-19 11:26:40]

Looks a lot better. I'd like to get _bps units for the bitrate stuff (kbps in
the VideoEncoder interface is legacy)

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/BUILD.gn:20: "../../base:rtc_base",
Doesn't this cause Chromium to depend on rtc_base? Shouldn't this be
rtc_base_approved?

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:81: target_bitrate_kbit_ = 0;
Can this whole thing operate on bps_ instead?

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:112: adjusted_bitrate =
std::max(adjusted_bitrate, min_bitrate);
units on all of these (_bps preferred)

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:279:
SetBitrate(bitrate_adjuster_.GetAdjustedBitrate());
GetAdjustedBitrateBps if possible

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:307:
target_bitrate_kbit_ = new_bitrate_kbit;
kbps

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:307:
target_bitrate_kbit_ = new_bitrate_kbit;
Can we store bps_ here and do target_bitrate_bps_ = new_bitrate_kbps * 1000?

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:309:
SetEncoderBitrate(target_bitrate_kbit_);
Shouldn't this use the (new?) adjusted bitrate as well? This will make the
bitrate_adjuster_ ignored if called often enough. Unless the ::Encode() call is
sufficient, then drop the call from here.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:414: void
H264VideoToolboxEncoder::SetBitrate(uint32_t bitrate_kbit) {
SetBitrateBps imo

[tkchin_webrtc, 2016-02-19 18:51:10]

I used kbit because that's what was provided to the VideoEncoder iface. If
that's going away I can convert it later today - meanwhile would like your input
on the base_approved issue.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/BUILD.gn:20: "../../base:rtc_base",
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> Doesn't this cause Chromium to depend on rtc_base? Shouldn't this be
> rtc_base_approved?

If that is the case, what do you propose to do with ratestatistics.h? Presumably
I can't add that to rtc_base_approved. The alternative is to have this module
depend on remote_bitrate_estimator..

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:309:
SetEncoderBitrate(target_bitrate_kbit_);
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> Shouldn't this use the (new?) adjusted bitrate as well? This will make the
> bitrate_adjuster_ ignored if called often enough. Unless the ::Encode() call
is
> sufficient, then drop the call from here.

We can't use adjusted bitrate immediately because it takes time to settle. We
don't really know what the effect of the new rate will be on the encoder so the
value of setting something else is unknown. I feel like carrying forward
penalties is something we can do after more experimentation with current model.
(The old observer iface carried forward penalties..but it was more strange to do
it in non-observer land so I left it in interest of moving forward since this CL
has been out for more than two weeks)

[pbos-webrtc, 2016-02-19 18:57:49]

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:275: "rate_statistics.cc",
This should go into rtc_base_approved

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/base.gyp
File webrtc/base/base.gyp (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/base.gyp#new...
webrtc/base/base.gyp:109: 'rate_statistics.cc',
This is rtc_base_approved

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/BUILD.gn:20: "../../base:rtc_base",
On 2016/02/19 18:51:09, tkchin_webrtc wrote:
> On 2016/02/19 11:26:39, pbos-webrtc wrote:
> > Doesn't this cause Chromium to depend on rtc_base? Shouldn't this be
> > rtc_base_approved?
> 
> If that is the case, what do you propose to do with ratestatistics.h?
Presumably
> I can't add that to rtc_base_approved. The alternative is to have this module
> depend on remote_bitrate_estimator..

I see nothing scary in rate_statistics that wouldn't permit it to be in
rtc_base_approved, move it there. It's already in there in the gyp.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:309:
SetEncoderBitrate(target_bitrate_kbit_);
On 2016/02/19 18:51:09, tkchin_webrtc wrote:
> On 2016/02/19 11:26:39, pbos-webrtc wrote:
> > Shouldn't this use the (new?) adjusted bitrate as well? This will make the
> > bitrate_adjuster_ ignored if called often enough. Unless the ::Encode() call
> is
> > sufficient, then drop the call from here.
> 
> We can't use adjusted bitrate immediately because it takes time to settle. We
> don't really know what the effect of the new rate will be on the encoder so
the
> value of setting something else is unknown. I feel like carrying forward
> penalties is something we can do after more experimentation with current
model.
> (The old observer iface carried forward penalties..but it was more strange to
do
> it in non-observer land so I left it in interest of moving forward since this
CL
> has been out for more than two weeks)

Shouldn't this be handled inside BitrateAdjuster then? It can tell you to use
the target bitrate directly until it settles on a new one?

[tkchin_webrtc, 2016-02-19 23:30:05]

PTAL

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:275: "rate_statistics.cc",
On 2016/02/19 18:57:49, pbos-webrtc wrote:
> This should go into rtc_base_approved

Done.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/base.gyp
File webrtc/base/base.gyp (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/base/base.gyp#new...
webrtc/base/base.gyp:109: 'rate_statistics.cc',
On 2016/02/19 18:57:49, pbos-webrtc wrote:
> This is rtc_base_approved

Acknowledged.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/BUILD.gn (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/BUILD.gn:20: "../../base:rtc_base",
On 2016/02/19 18:57:49, pbos-webrtc wrote:
> On 2016/02/19 18:51:09, tkchin_webrtc wrote:
> > On 2016/02/19 11:26:39, pbos-webrtc wrote:
> > > Doesn't this cause Chromium to depend on rtc_base? Shouldn't this be
> > > rtc_base_approved?
> > 
> > If that is the case, what do you propose to do with ratestatistics.h?
> Presumably
> > I can't add that to rtc_base_approved. The alternative is to have this
module
> > depend on remote_bitrate_estimator..
> 
> I see nothing scary in rate_statistics that wouldn't permit it to be in
> rtc_base_approved, move it there. It's already in there in the gyp.

Done.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:81: target_bitrate_kbit_ = 0;
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> Can this whole thing operate on bps_ instead?

Done.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:112: adjusted_bitrate =
std::max(adjusted_bitrate, min_bitrate);
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> units on all of these (_bps preferred)

Done.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:279:
SetBitrate(bitrate_adjuster_.GetAdjustedBitrate());
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> GetAdjustedBitrateBps if possible

Done.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:307:
target_bitrate_kbit_ = new_bitrate_kbit;
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> kbps

Done.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:309:
SetEncoderBitrate(target_bitrate_kbit_);
On 2016/02/19 18:57:49, pbos-webrtc wrote:
> On 2016/02/19 18:51:09, tkchin_webrtc wrote:
> > On 2016/02/19 11:26:39, pbos-webrtc wrote:
> > > Shouldn't this use the (new?) adjusted bitrate as well? This will make the
> > > bitrate_adjuster_ ignored if called often enough. Unless the ::Encode()
call
> > is
> > > sufficient, then drop the call from here.
> > 
> > We can't use adjusted bitrate immediately because it takes time to settle.
We
> > don't really know what the effect of the new rate will be on the encoder so
> the
> > value of setting something else is unknown. I feel like carrying forward
> > penalties is something we can do after more experimentation with current
> model.
> > (The old observer iface carried forward penalties..but it was more strange
to
> do
> > it in non-observer land so I left it in interest of moving forward since
this
> CL
> > has been out for more than two weeks)
> 
> Shouldn't this be handled inside BitrateAdjuster then? It can tell you to use
> the target bitrate directly until it settles on a new one?

Aha I get what you're saying now. Yes, it is already handled inside
BitrateAdjuster.

https://codereview.webrtc.org/1660963002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:414: void
H264VideoToolboxEncoder::SetBitrate(uint32_t bitrate_kbit) {
On 2016/02/19 11:26:39, pbos-webrtc wrote:
> SetBitrateBps imo

Done.

[pbos-webrtc, 2016-02-22 15:44:18]

Bit confusing about max bitrate, shouldn't max adjustment be over target,
otherwise this can't increase for encoders that don't keep up?

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:30: bitrate_tracker_(1.5 *
kBitrateUpdateIntervalMs, 8 * 1000) {
What#s 8? constant please

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:68: return .95 *
target_bitrate_bps_;
Is it intentional that this thing can only operate on encoders that overshoot?
If they undershoot we'll need to compensate as well, no? If this is only used
when overshooting, then please update the name.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:104: error * 100 >
kBitrateTolerancePct * target_bitrate_bps) {
make kBitrateTolerance not be in percent.

[tkchin_webrtc, 2016-02-22 18:23:18]

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:30: bitrate_tracker_(1.5 *
kBitrateUpdateIntervalMs, 8 * 1000) {
On 2016/02/22 15:44:18, pbos-webrtc wrote:
> What#s 8? constant please

Done.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:68: return .95 *
target_bitrate_bps_;
On 2016/02/22 15:44:18, pbos-webrtc wrote:
> Is it intentional that this thing can only operate on encoders that overshoot?
> If they undershoot we'll need to compensate as well, no? If this is only used
> when overshooting, then please update the name.

As we talked about before, we wanted this for the iOS encoder specifically
first. As far as I know, the Android hardware encoder also regularly overshoots
so this should still be usable.

This can be used for both over and undershooting, there's just currently no way
to set it. The values are used purely for clamping. I had them as getters to
that it'd be easy to update the class later.

We can add a setter in a subsequent CL. I don't want to add any more to this CL
given that the the setter behavior is not being used at this point of time.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:104: error * 100 >
kBitrateTolerancePct * target_bitrate_bps) {
On 2016/02/22 15:44:18, pbos-webrtc wrote:
> make kBitrateTolerance not be in percent.

why? Do you have a particular value in mind?

[pbos-webrtc, 2016-02-22 18:37:19]

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:68: return .95 *
target_bitrate_bps_;
On 2016/02/22 18:23:18, tkchin_webrtc wrote:
> On 2016/02/22 15:44:18, pbos-webrtc wrote:
> > Is it intentional that this thing can only operate on encoders that
overshoot?
> > If they undershoot we'll need to compensate as well, no? If this is only
used
> > when overshooting, then please update the name.
> 
> As we talked about before, we wanted this for the iOS encoder specifically
> first. As far as I know, the Android hardware encoder also regularly
overshoots
> so this should still be usable.
> 
> This can be used for both over and undershooting, there's just currently no
way
> to set it. The values are used purely for clamping. I had them as getters to
> that it'd be easy to update the class later.
> 
> We can add a setter in a subsequent CL. I don't want to add any more to this
CL
> given that the the setter behavior is not being used at this point of time.
> 
> 

But this reads to me like it's allowed to clamp only between 0.95 and 0.5, why
can't it go to or above 1.0?

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:104: error * 100 >
kBitrateTolerancePct * target_bitrate_bps) {
On 2016/02/22 18:23:18, tkchin_webrtc wrote:
> On 2016/02/22 15:44:18, pbos-webrtc wrote:
> > make kBitrateTolerance not be in percent.
> 
> why? Do you have a particular value in mind?

Just make it 0.05 instead of 5 to avoid the multiplications etc.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:110: float min_bitrate_bps =
GetMinAdjustedBitrateBps();
This takes crit_ recursively, can you put EXCLUSIVE_LOCKS_REQUIRED(crit_) on the
clamping rates instead of taking the lock inside?

[tkchin_webrtc, 2016-02-23 00:05:27]

PTAL

Addressed offline comments re: fast changing rates.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:68: return .95 *
target_bitrate_bps_;
On 2016/02/22 18:37:19, pbos-webrtc wrote:
> On 2016/02/22 18:23:18, tkchin_webrtc wrote:
> > On 2016/02/22 15:44:18, pbos-webrtc wrote:
> > > Is it intentional that this thing can only operate on encoders that
> overshoot?
> > > If they undershoot we'll need to compensate as well, no? If this is only
> used
> > > when overshooting, then please update the name.
> > 
> > As we talked about before, we wanted this for the iOS encoder specifically
> > first. As far as I know, the Android hardware encoder also regularly
> overshoots
> > so this should still be usable.
> > 
> > This can be used for both over and undershooting, there's just currently no
> way
> > to set it. The values are used purely for clamping. I had them as getters to
> > that it'd be easy to update the class later.
> > 
> > We can add a setter in a subsequent CL. I don't want to add any more to this
> CL
> > given that the the setter behavior is not being used at this point of time.
> > 
> > 
> 
> But this reads to me like it's allowed to clamp only between 0.95 and 0.5, why
> can't it go to or above 1.0?

Done.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:104: error * 100 >
kBitrateTolerancePct * target_bitrate_bps) {
On 2016/02/22 18:37:19, pbos-webrtc wrote:
> On 2016/02/22 18:23:18, tkchin_webrtc wrote:
> > On 2016/02/22 15:44:18, pbos-webrtc wrote:
> > > make kBitrateTolerance not be in percent.
> > 
> > why? Do you have a particular value in mind?
> 
> Just make it 0.05 instead of 5 to avoid the multiplications etc.

Done.

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:110: float min_bitrate_bps =
GetMinAdjustedBitrateBps();
On 2016/02/22 18:37:19, pbos-webrtc wrote:
> This takes crit_ recursively, can you put EXCLUSIVE_LOCKS_REQUIRED(crit_) on
the
> clamping rates instead of taking the lock inside?

Done. But now requires friend class for test.

[tkchin_webrtc, 2016-02-23 00:09:07]

Patchset #12 (id:280001) has been deleted

[pbos-webrtc, 2016-02-23 15:04:40]

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:110: float min_bitrate_bps =
GetMinAdjustedBitrateBps();
On 2016/02/23 00:05:27, tkchin_webrtc wrote:
> On 2016/02/22 18:37:19, pbos-webrtc wrote:
> > This takes crit_ recursively, can you put EXCLUSIVE_LOCKS_REQUIRED(crit_) on
> the
> > clamping rates instead of taking the lock inside?
> 
> Done. But now requires friend class for test.

Can't you just make sure it's clamped from the outside? Loop and
overuse/underuse for 10s and see that it hits but doesn't go above/below the
threshold?

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

https://codereview.webrtc.org/1660963002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:43: float
current_target_bitrate_bps = target_bitrate_bps_;
Just use directly, static_cast?

https://codereview.webrtc.org/1660963002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:72: rtc::CritScope cs(&crit_);
Make const and set in the constructor instead.

[tkchin_webrtc, 2016-02-23 17:03:20]

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/bitrate_adjuster.cc (right):

https://codereview.webrtc.org/1660963002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:110: float min_bitrate_bps =
GetMinAdjustedBitrateBps();
On 2016/02/23 15:04:40, pbos-webrtc wrote:
> On 2016/02/23 00:05:27, tkchin_webrtc wrote:
> > On 2016/02/22 18:37:19, pbos-webrtc wrote:
> > > This takes crit_ recursively, can you put EXCLUSIVE_LOCKS_REQUIRED(crit_)
on
> > the
> > > clamping rates instead of taking the lock inside?
> > 
> > Done. But now requires friend class for test.
> 
> Can't you just make sure it's clamped from the outside? Loop and
> overuse/underuse for 10s and see that it hits but doesn't go above/below the
> threshold?

Done.

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

https://codereview.webrtc.org/1660963002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:43: float
current_target_bitrate_bps = target_bitrate_bps_;
On 2016/02/23 15:04:40, pbos-webrtc wrote:
> Just use directly, static_cast?

Done.

https://codereview.webrtc.org/1660963002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/bitrate_adjuster.cc:72: rtc::CritScope cs(&crit_);
On 2016/02/23 15:04:40, pbos-webrtc wrote:
> Make const and set in the constructor instead.

Done.

[pbos-webrtc, 2016-02-23 17:04:55]

lgtm

[tkchin_webrtc, 2016-02-23 18:12:49]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2016-02-23 18:12:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
kjellander@webrtc.org, pthatcher@google.com, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1660963002/#ps320001 (title:
"Move setters to ctor args instead.")

[commit-bot: I haz the power, 2016-02-23 18:12:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660963002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660963002/320001

[tkchin_webrtc, 2016-02-23 18:14:47]

The CQ bit was unchecked by tkchin@webrtc.org

[pthatcher1, 2016-02-23 20:26:01]

lgtm

[tkchin_webrtc, 2016-02-23 20:32:28]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2016-02-23 20:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
sprang@webrtc.org, stefan@webrtc.org, kjellander@webrtc.org,
pthatcher@google.com
Link to the patchset: https://codereview.webrtc.org/1660963002/#ps360001 (title:
"Fix win compile errors")

[commit-bot: I haz the power, 2016-02-23 20:32:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660963002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660963002/360001

[commit-bot: I haz the power, 2016-02-23 22:33:18]

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

[commit-bot: I haz the power, 2016-02-23 22:33:19]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kjellander_webrtc, 2016-02-24 06:46:52]

On 2016/02/23 22:33:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

We had a long q

[kjellander_webrtc, 2016-02-24 06:47:28]

On 2016/02/24 06:46:52, kjellander (webrtc) wrote:
> On 2016/02/23 22:33:19, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
> 
> We had a long q

We had a long queue yesterday, I'll CQ this one again.

[kjellander_webrtc, 2016-02-24 06:47:48]

The CQ bit was checked by kjellander@webrtc.org

[commit-bot: I haz the power, 2016-02-24 06:47:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660963002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660963002/360001

[commit-bot: I haz the power, 2016-02-24 06:49:46]

Description was changed from

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc:4081
==========

to

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc:4081
==========

[commit-bot: I haz the power, 2016-02-24 06:49:48]

Committed patchset #15 (id:360001)

[commit-bot: I haz the power, 2016-02-24 06:49:56]

Description was changed from

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc:4081
==========

to

==========
Bitrate controller for VideoToolbox encoder.

Also fixes a crash on encoder Release.

BUG=webrtc:4081

Committed: https://crrev.com/f75d0082350096077e6553a149bda7749593b9c7
Cr-Commit-Position: refs/heads/master@{#11729}
==========

[commit-bot: I haz the power, 2016-02-24 06:49:58]

Patchset 15 (id:??) landed as
https://crrev.com/f75d0082350096077e6553a149bda7749593b9c7
Cr-Commit-Position: refs/heads/master@{#11729}