Adapt encoder behavior to target bitrate

Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=

Committed: https://crrev.com/3b9c097391f9e068446b691e101a756459e8e898
Cr-Commit-Position: refs/heads/master@{#392985}

[Irfan, 2016-04-21 22:10:25]

Description was changed from

==========
Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=
==========

to

==========
Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=
==========

[Irfan, 2016-04-21 22:10:25]

isheriff@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Irfan, 2016-04-21 22:10:59]

Lmk if you see any obvious issues with this approach. Am still trying to further
tune the behavior.

[Sergey Ulanov, 2016-04-21 22:55:49]

I don't think we want to change behavior of encoder in our current protocol
(which for example doesn't support top-off for VP8).

Potentially we could have a set of flags to control VideoEncoderVpx behavior to
distinguish between the two use cases, but it's probably better to make a new
copy of that code specifically for WebRTC. That we could also avoid
remoting::VideoEncoder and avoid dependency on VideoPacket protobufs. E.g.
encoder would spit out webrtc::EncodedImage directly. 

Haven't looked at details in this CL yet.

https://codereview.chromium.org/1908203002/diff/1/remoting/codec/video_encode...
File remoting/codec/video_encoder_vpx.cc (right):

https://codereview.chromium.org/1908203002/diff/1/remoting/codec/video_encode...
remoting/codec/video_encoder_vpx.cc:302: // to top-off.
nit: comment formatting

[Sergey Ulanov, 2016-05-10 00:00:59]

lgtm

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/video_en...
remoting/codec/video_encoder.h:38: virtual void UpdateTargetBitrate(uint32_t
bitrate) {}
bitrate_kbps
also make it int
(https://google.github.io/styleguide/cppguide.html#Integer_Types)

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/webrtc_v...
File remoting/codec/webrtc_video_encoder_vpx.cc (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder_vpx.cc:333: // bandwidth requiremetn. We
specify a target rate of 1 / 15 fps here.
why is it 1/15 fps instead of 30 fps?

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder_vpx.cc:333: // bandwidth requiremetn. We
specify a target rate of 1 / 15 fps here.
typo: requiremetn

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.cc:109:
encoded_image.capture_time_ms_ = frame->capture_time_ms();
VideoPacket::capture_time_ms() returns time it took the capture the frame.
EncodedImage::capture_time_ms is the frame's timestamp.

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.cc:111:
static_cast<uint32_t>(frame->capture_time_ms() * 90);
same here. capture_time_ms() is not timestamp

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_encoder_factory.h (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.h:19: using TargetBitrateCallback
= base::Callback<void(uint32_t)>;
s/uint32_t/int bitrate_kbps/

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.h:25: class WebRtcVideoEncoder :
public webrtc::VideoEncoder {
maybe move this to the .cc file.

[Irfan, 2016-05-10 16:30:46]

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/video_en...
remoting/codec/video_encoder.h:38: virtual void UpdateTargetBitrate(uint32_t
bitrate) {}
On 2016/05/10 00:00:58, Sergey Ulanov wrote:
> bitrate_kbps
> also make it int
> (https://google.github.io/styleguide/cppguide.html#Integer_Types)

Done.

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/webrtc_v...
File remoting/codec/webrtc_video_encoder_vpx.cc (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder_vpx.cc:333: // bandwidth requiremetn. We
specify a target rate of 1 / 15 fps here.
On 2016/05/10 00:00:58, Sergey Ulanov wrote:
> why is it 1/15 fps instead of 30 fps?

I added a TODO here. Basically, we need to further experiment. I chose an
expected target bitrate rather than max target bitrate.

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.cc:109:
encoded_image.capture_time_ms_ = frame->capture_time_ms();
On 2016/05/10 00:00:58, Sergey Ulanov wrote:
> VideoPacket::capture_time_ms() returns time it took the capture the frame.
> EncodedImage::capture_time_ms is the frame's timestamp.

Agree that this is abuse of the VideoPacket capture_time_ms. The right thing
here is to actually fix to avoid using VideoPacket and have a class appropriate
for webrtc use (since the capture timestamp now is correctly moved from the time
of capture). I am going to leave this as is with a comment since this interface
is getting refactored.

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_encoder_factory.h (right):

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.h:19: using TargetBitrateCallback
= base::Callback<void(uint32_t)>;
On 2016/05/10 00:00:58, Sergey Ulanov wrote:
> s/uint32_t/int bitrate_kbps/

Done.

https://codereview.chromium.org/1908203002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_encoder_factory.h:25: class WebRtcVideoEncoder :
public webrtc::VideoEncoder {
On 2016/05/10 00:00:58, Sergey Ulanov wrote:
> maybe move this to the .cc file.

hm, think i prefer it here unless you strongly feel otherwise

[Sergey Ulanov, 2016-05-10 21:18:46]

lgtm

[Irfan, 2016-05-11 06:05:48]

The CQ bit was checked by isheriff@chromium.org

[commit-bot: I haz the power, 2016-05-11 06:06:04]

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

[commit-bot: I haz the power, 2016-05-11 06:12:08]

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

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

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Irfan, 2016-05-11 17:08:42]

I had brought in media/capture to aggregate frame duration, but I realized that
is mostly unused in the latest code (other than logging). removed that until we
really need it.

[Irfan, 2016-05-11 18:02:23]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-05-11 18:02:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1908203002/#ps60001
(title: "Remove unnecessary code")

[commit-bot: I haz the power, 2016-05-11 18:03:13]

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

[commit-bot: I haz the power, 2016-05-11 18:09:10]

Description was changed from

==========
Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=
==========

to

==========
Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=
==========

[commit-bot: I haz the power, 2016-05-11 18:09:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-11 18:10:55]

Description was changed from

==========
Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=
==========

to

==========
Adapt encoder behavior to target bitrate

This change utilizes the target bandwidth signal provided by webrtc
to tune the encoder behavior. We now support top-off on VP8 to enable
better interactive behavior. Also, the frame rate is adapted based
on feedback from the encoded data.

BUG=

Committed: https://crrev.com/3b9c097391f9e068446b691e101a756459e8e898
Cr-Commit-Position: refs/heads/master@{#392985}
==========

[commit-bot: I haz the power, 2016-05-11 18:10:56]

Patchset 4 (id:??) landed as
https://crrev.com/3b9c097391f9e068446b691e101a756459e8e898
Cr-Commit-Position: refs/heads/master@{#392985}

[Sergey Ulanov, 2016-05-30 11:59:56]

Looks like with this patch we now get significantly worse numbers in the perf
test. Is that expected?

[chromium-reviews, 2016-05-31 15:12:38]

I created https://bugs.chromium.org/p/chromium/issues/detail?id=616103 to
quantify the behavioral change

On Mon, May 30, 2016 at 4:59 AM <sergeyu@chromium.org> wrote:

> Looks like with this patch we now get significantly worse numbers in the
> perf
> test. Is that expected?
>
> https://codereview.chromium.org/1908203002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sergey Ulanov, 2016-05-31 19:23:39]

Thanks. FWIW running those tests with your jitter-buffer change does
improve perf test results.

On Tue, May 31, 2016 at 5:12 PM, Irfan Sheriff <isheriff@google.com> wrote:

> I created https://bugs.chromium.org/p/chromium/issues/detail?id=616103 to
> quantify the behavioral change
>
> On Mon, May 30, 2016 at 4:59 AM <sergeyu@chromium.org> wrote:
>
>> Looks like with this patch we now get significantly worse numbers in the
>> perf
>> test. Is that expected?
>>
>> https://codereview.chromium.org/1908203002/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.