Add dynamic bitrate tracker and adjustment for Exynos VP8 HW encoder.

Add dynamic bitrate tracker and adjustment for Exynos VP8 HW encoder.

Exynos VP8 HW encoder may generate a bitrate which deviates too
much from target value causing frame drops and fps reduction or
BWE stuck at low values.

Add one more option to bitrate adjustment in HW encoder wrapper
which allows to track and dynamicaly scale bitrate used for
codec configuration.

BUG=b/30951236
R=wzh@webrtc.org

Committed: https://crrev.com/cfaca03b4b67c407d43aadc4bdd4f1eeb5bf5eb9
Cr-Commit-Position: refs/heads/master@{#14095}

[AlexG, 2016-09-02 21:54:43]

glaznev@webrtc.org changed reviewers:
+ magjed@webrtc.org, wzh@webrtc.org

[AlexG, 2016-09-02 21:54:43]

PTAL

[wzh, 2016-09-02 22:40:13]

lgtm

[magjed_webrtc, 2016-09-03 16:45:38]

https://codereview.webrtc.org/2308843002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/MediaCodecVideoEncoder.java (right):

https://codereview.webrtc.org/2308843002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/MediaCodecVideoEncoder.java:169: //
Variables used for dynamic bitrate adjustment.
I would like to encapsulate this state and the associated logic. So instead of a
BitrateAdjustmentType enum in MediaCodecProperties, there would be a
'BitrateAdjuster bitrateAdjuster', where BitrateAdjuster is something like:
interface BitrateAdjuster {
  int startFps(int targetFps);
  int getAdjustedBitrate(int targetBitRate, int targetFps);
  void reportEncodedFrame(int encodedFrameSize);
}
and in setRates():
private boolean setRates(int kbps, int frameRate) {
  final int adjustedBitrate = bitrateAdjuster.getAdjustedBitrate(kbps * 1000,
frameRate);
  if (adjustedBitrate == this.adjustedTargetBitrate) {
    return;
  }
  ...
  params.putInt(MediaCodec.PARAMETER_KEY_VIDEO_BITRATE, adjustedTargetBitrate);
  ...
}
and in dequeueOutputBuffer():
  ...
  bitrateAdjuster.reportEncodedFrame(info.size);
  setRates(kbps, frameRate);
  ...
and then make one BitrateAdjuster implementation for each of NO_ADJUSTMENT,
FRAMERATE_ADJUSTMENT, and DYNAMIC_ADJUSTMENT. I'm not sure how it would turn out
without actually implementing it, but what do you think?

[AlexG, 2016-09-06 17:41:18]

On 2016/09/03 16:45:38, magjed_webrtc wrote:
>
https://codereview.webrtc.org/2308843002/diff/1/webrtc/api/android/java/src/o...
> File webrtc/api/android/java/src/org/webrtc/MediaCodecVideoEncoder.java
(right):
> 
>
https://codereview.webrtc.org/2308843002/diff/1/webrtc/api/android/java/src/o...
> webrtc/api/android/java/src/org/webrtc/MediaCodecVideoEncoder.java:169: //
> Variables used for dynamic bitrate adjustment.
> I would like to encapsulate this state and the associated logic. So instead of
a
> BitrateAdjustmentType enum in MediaCodecProperties, there would be a
> 'BitrateAdjuster bitrateAdjuster', where BitrateAdjuster is something like:
> interface BitrateAdjuster {
>   int startFps(int targetFps);
>   int getAdjustedBitrate(int targetBitRate, int targetFps);
>   void reportEncodedFrame(int encodedFrameSize);
> }
> and in setRates():
> private boolean setRates(int kbps, int frameRate) {
>   final int adjustedBitrate = bitrateAdjuster.getAdjustedBitrate(kbps * 1000,
> frameRate);
>   if (adjustedBitrate == this.adjustedTargetBitrate) {
>     return;
>   }
>   ...
>   params.putInt(MediaCodec.PARAMETER_KEY_VIDEO_BITRATE,
adjustedTargetBitrate);
>   ...
> }
> and in dequeueOutputBuffer():
>   ...
>   bitrateAdjuster.reportEncodedFrame(info.size);
>   setRates(kbps, frameRate);
>   ...
> and then make one BitrateAdjuster implementation for each of NO_ADJUSTMENT,
> FRAMERATE_ADJUSTMENT, and DYNAMIC_ADJUSTMENT. I'm not sure how it would turn
out
> without actually implementing it, but what do you think?

Yes, I thought about moving bitrate adjuster logic into separate class. The
problem is that there are currently not too much common functions for frame rate
based and dynamic bitrate adjustment - frame rate adjusted does not need
reportEncodedFrame call, on the other side dynamic bitrate adjuster need a
special event to notify that bitrate deviates too much and setRates() need to be
called. So in this common interface there will be definitions which are needed
for one bitrate adjuster type, but not needed for another one.

One more thing is that I think this is a kind of experimental design, which I
want to soak for some time first. I am thinking of combining both types of
adjustment and using it for Exynos H.264 encoder since apart from not using
frame timestamps it also sometime have too much bitrate deviations even after
frame rate based correction. This will be a better candidate for adding common
interface then.

[AlexG, 2016-09-06 17:41:54]

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

[commit-bot: I haz the power, 2016-09-06 17:41:59]

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-09-06 18:25:28]

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

[commit-bot: I haz the power, 2016-09-06 18:25:29]

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

[AlexG, 2016-09-06 18:36:30]

The CQ bit was checked by glaznev@webrtc.org

[AlexG, 2016-09-06 18:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from wzh@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2308843002/#ps20001 (title:
"Shorten log message")

[commit-bot: I haz the power, 2016-09-06 18:36:40]

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-09-06 20:37:17]

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

[commit-bot: I haz the power, 2016-09-06 20:37:18]

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

[AlexG, 2016-09-06 21:08:30]

Description was changed from

==========
Add dynamic bitrate tracker and adjustment for Exynos VP8 HW encoder.

Exynos VP8 HW encoder may generate a bitrate which deviates too
much from target value causing frame drops and fps reduction or
BWE stuck at low values.

Add one more option to bitrate adjustment in HW encoder wrapper
which allows to track and dynamicaly scale bitrate used for
codec configuration.

BUG=b/30951236
==========

to

==========
Add dynamic bitrate tracker and adjustment for Exynos VP8 HW encoder.

Exynos VP8 HW encoder may generate a bitrate which deviates too
much from target value causing frame drops and fps reduction or
BWE stuck at low values.

Add one more option to bitrate adjustment in HW encoder wrapper
which allows to track and dynamicaly scale bitrate used for
codec configuration.

BUG=b/30951236
R=wzh@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/cfaca03b4b67c407d43aadc4b...
==========

[AlexG, 2016-09-06 21:08:32]

Committed patchset #2 (id:20001) manually as
cfaca03b4b67c407d43aadc4bdd4f1eeb5bf5eb9 (presubmit successful).

[commit-bot: I haz the power, 2016-09-06 21:08:33]

Description was changed from

==========
Add dynamic bitrate tracker and adjustment for Exynos VP8 HW encoder.

Exynos VP8 HW encoder may generate a bitrate which deviates too
much from target value causing frame drops and fps reduction or
BWE stuck at low values.

Add one more option to bitrate adjustment in HW encoder wrapper
which allows to track and dynamicaly scale bitrate used for
codec configuration.

BUG=b/30951236
R=wzh@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/cfaca03b4b67c407d43aadc4b...
==========

to

==========
Add dynamic bitrate tracker and adjustment for Exynos VP8 HW encoder.

Exynos VP8 HW encoder may generate a bitrate which deviates too
much from target value causing frame drops and fps reduction or
BWE stuck at low values.

Add one more option to bitrate adjustment in HW encoder wrapper
which allows to track and dynamicaly scale bitrate used for
codec configuration.

BUG=b/30951236
R=wzh@webrtc.org

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

[magjed_webrtc, 2016-09-07 07:55:00]

On 2016/09/06 17:41:18, AlexG wrote:
> On 2016/09/03 16:45:38, magjed_webrtc wrote:
> >
>
https://codereview.webrtc.org/2308843002/diff/1/webrtc/api/android/java/src/o...
> > File webrtc/api/android/java/src/org/webrtc/MediaCodecVideoEncoder.java
> (right):
> > 
> >
>
https://codereview.webrtc.org/2308843002/diff/1/webrtc/api/android/java/src/o...
> > webrtc/api/android/java/src/org/webrtc/MediaCodecVideoEncoder.java:169: //
> > Variables used for dynamic bitrate adjustment.
> > I would like to encapsulate this state and the associated logic. So instead
of
> a
> > BitrateAdjustmentType enum in MediaCodecProperties, there would be a
> > 'BitrateAdjuster bitrateAdjuster', where BitrateAdjuster is something like:
> > interface BitrateAdjuster {
> >   int startFps(int targetFps);
> >   int getAdjustedBitrate(int targetBitRate, int targetFps);
> >   void reportEncodedFrame(int encodedFrameSize);
> > }
> > and in setRates():
> > private boolean setRates(int kbps, int frameRate) {
> >   final int adjustedBitrate = bitrateAdjuster.getAdjustedBitrate(kbps *
1000,
> > frameRate);
> >   if (adjustedBitrate == this.adjustedTargetBitrate) {
> >     return;
> >   }
> >   ...
> >   params.putInt(MediaCodec.PARAMETER_KEY_VIDEO_BITRATE,
> adjustedTargetBitrate);
> >   ...
> > }
> > and in dequeueOutputBuffer():
> >   ...
> >   bitrateAdjuster.reportEncodedFrame(info.size);
> >   setRates(kbps, frameRate);
> >   ...
> > and then make one BitrateAdjuster implementation for each of NO_ADJUSTMENT,
> > FRAMERATE_ADJUSTMENT, and DYNAMIC_ADJUSTMENT. I'm not sure how it would turn
> out
> > without actually implementing it, but what do you think?
> 
> Yes, I thought about moving bitrate adjuster logic into separate class. The
> problem is that there are currently not too much common functions for frame
rate
> based and dynamic bitrate adjustment - frame rate adjusted does not need
> reportEncodedFrame call, on the other side dynamic bitrate adjuster need a
> special event to notify that bitrate deviates too much and setRates() need to
be
> called. So in this common interface there will be definitions which are needed
> for one bitrate adjuster type, but not needed for another one.
> 
> One more thing is that I think this is a kind of experimental design, which I
> want to soak for some time first. I am thinking of combining both types of
> adjustment and using it for Exynos H.264 encoder since apart from not using
> frame timestamps it also sometime have too much bitrate deviations even after
> frame rate based correction. This will be a better candidate for adding common
> interface then.

Ok, sgtm.