Move the QP scaling thresholds to the relevant encoders

Move the QP scaling thresholds to the relevant encoders.

Also provide a new set of thresholds for the VideoToolbox encoder. The new thresholds were experimentally determined to work well on the iPhone 6S, and also adequately on the iPhone 5S.

BUG=webrtc:5678
Committed: https://crrev.com/478681e1e6e6c5d66366672614d58b10ad7df083
Cr-Commit-Position: refs/heads/master@{#14420}

[kthelgason, 2016-09-05 09:02:48]

kthelgason@webrtc.org changed reviewers:
+ magjed@webrtc.org, mflodman@webrtc.org

[magjed_webrtc, 2016-09-06 07:49:09]

lgtm

Since we need encoder specific thresholds, I think this is a cleaner approach
then using '#if defined(WEBRTC_IOS)' that we do right now.

https://codereview.webrtc.org/2309743002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/2309743002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:41: 
nit: remove extra line

[kthelgason, 2016-09-06 12:30:54]

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

[commit-bot: I haz the power, 2016-09-06 12:30:56]

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 12:38:22]

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

[commit-bot: I haz the power, 2016-09-06 12:38:23]

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

[kthelgason, 2016-09-06 13:05:40]

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

[commit-bot: I haz the power, 2016-09-06 13:05:49]

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 13:40:31]

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

[commit-bot: I haz the power, 2016-09-06 13:40:35]

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/16384)

[pbos-webrtc, 2016-09-07 19:09:25]

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

[pbos-webrtc, 2016-09-07 19:09:26]

I think that the encoder specific thresholds should only be added in files where
they deviate, e.g. keep default values in QualityScaler.

For VP8 I think the QPs map to actual quality, so I think they should be the
same everywhere.

For H264 the QP values are doing a heuristic on parts of the slices, and there
the metric might vary more based on implementation, here it makes more sense to
add it to VideoToolbox if it should be using a threshold that deviates from
OpenH264/MediaCodec.

[kthelgason, 2016-09-09 08:43:50]

On 2016/09/07 19:09:26, pbos-webrtc wrote:
> I think that the encoder specific thresholds should only be added in files
where
> they deviate, e.g. keep default values in QualityScaler.

Do you mean the scaler should keep one set of defaults for each codec, and if we
have several implementations of each codec, the implementations where QP differs
would have their own overriding settings?

> For VP8 I think the QPs map to actual quality, so I think they should be the
> same everywhere.
> For H264 the QP values are doing a heuristic on parts of the slices, and there
> the metric might vary more based on implementation, here it makes more sense
to
> add it to VideoToolbox if it should be using a threshold that deviates from
> OpenH264/MediaCodec.

From my perspective, the improvement here is a separation of concerns. Adding a
new codec should not require changing the quality scaler.

[pbos-webrtc, 2016-09-12 19:51:49]

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:29: const int
kLowH264QpThreshold = 24;
I want these to be declared in one place for this and
androidmediaencoder_jni.cc, since they are believed to work well as defaults

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(right):

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:36: const
int kLowH264QpThreshold = 28;
Can you add a comment here + rename to explicitly state that these deviate from
the thresholds on other platforms and why it was added?

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

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:39: static const int
kLowVp8QpThreshold = 29;
Should be declared in the same place as the android...jni.cc ones.

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/quality_scaler.h (left):

https://codereview.webrtc.org/2309743002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/quality_scaler.h:43: static const int
kLowVp8QpThreshold;
I think this might be the best place for the default values, and only make
VideoToolbox thresholds deviate, and make why they deviate more explicit
(average QPs different on certain iOS devices)

[kthelgason, 2016-09-19 13:37:16]

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

[commit-bot: I haz the power, 2016-09-19 13:37:24]

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

[kthelgason, 2016-09-19 13:37:37]

Updated after comments from pbos@.
PTAL

[commit-bot: I haz the power, 2016-09-19 13:38:34]

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

[commit-bot: I haz the power, 2016-09-19 13:38:35]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/641)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/13031)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/16943)
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/17838)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/13208)
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/15318)
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/5176)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/17760)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/14467)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_dbg/builds/745)
  mac_gyp_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_rel/builds/748)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18379)

[kthelgason, 2016-09-19 13:49:27]

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

[commit-bot: I haz the power, 2016-09-19 13:49:34]

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

[pbos-webrtc, 2016-09-19 14:06:57]

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

https://codereview.webrtc.org/2309743002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/quality_scaler.cc:64: ClearSamples();
Have this function just get low/high thresholds and then call the other Init
function with found high/low parameters.

E.g. this function just gets the default parameters for QP, then forwards to the
other Init().

https://codereview.webrtc.org/2309743002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/quality_scaler.cc:107: int highQpThreshold,
high_qp_threshold, same for low

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

https://codereview.webrtc.org/2309743002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/quality_scaler.h:37: int highQpThreshold,
hiqh_qp_threshold,
low_qp_threshold

[pbos-webrtc, 2016-09-19 14:31:38]

lgtm, thanks

[kthelgason, 2016-09-20 19:33:35]

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

[commit-bot: I haz the power, 2016-09-20 19:33:43]

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-20 19:37:15]

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

[commit-bot: I haz the power, 2016-09-20 19:37:16]

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

[kthelgason, 2016-09-20 19:48:00]

Description was changed from

==========
Move the QP scaling thresholds to the relevant encoders

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

to

==========
Move the QP scaling thresholds to the relevant encoders

BUG=webrtc:5678
==========

[kthelgason, 2016-09-20 19:48:16]

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

[commit-bot: I haz the power, 2016-09-20 19:48:27]

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-20 21:19:53]

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

[commit-bot: I haz the power, 2016-09-20 21:19:54]

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/16898)

[kthelgason, 2016-09-21 08:42:05]

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

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

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-21 10:42:36]

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

[commit-bot: I haz the power, 2016-09-21 10:42:37]

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

[kthelgason, 2016-09-28 09:04:56]

kthelgason@webrtc.org changed reviewers:
+ tommi@webrtc.org
- mflodman@webrtc.org

[kthelgason, 2016-09-28 09:04:56]

Adding tommi@ as a reviewer to get this landed.

[mflodman, 2016-09-28 11:54:06]

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

[mflodman, 2016-09-28 11:54:07]

One comment, then LGTM.

https://codereview.chromium.org/2309743002/diff/160001/webrtc/modules/video_c...
File webrtc/modules/video_coding/utility/quality_scaler.cc (right):

https://codereview.chromium.org/2309743002/diff/160001/webrtc/modules/video_c...
webrtc/modules/video_coding/utility/quality_scaler.cc:86: ClearSamples();
You can remove this or the one on line 93 I guess?

[mflodman, 2016-09-28 11:57:14]

Also, please mention in the description that the toolbox thresholds are actually
changed in this CL.

[kthelgason, 2016-09-28 13:46:16]

Description was changed from

==========
Move the QP scaling thresholds to the relevant encoders

BUG=webrtc:5678
==========

to

==========
Move the QP scaling thresholds to the relevant encoders.

Also provide a new set of thresholds for the VideoToolbox encoder. The new
thresholds were experimentally determined to work well on the iPhone 6S, and
also adequately on the iPhone 5S. 

BUG=webrtc:5678
==========

[kthelgason, 2016-09-28 13:46:24]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2016-09-28 13:46:25]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pbos@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2309743002/#ps180001 (title:
"Remove redundant call")

[commit-bot: I haz the power, 2016-09-28 13:46:31]

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-28 15:17:49]

Description was changed from

==========
Move the QP scaling thresholds to the relevant encoders.

Also provide a new set of thresholds for the VideoToolbox encoder. The new
thresholds were experimentally determined to work well on the iPhone 6S, and
also adequately on the iPhone 5S. 

BUG=webrtc:5678
==========

to

==========
Move the QP scaling thresholds to the relevant encoders.

Also provide a new set of thresholds for the VideoToolbox encoder. The new
thresholds were experimentally determined to work well on the iPhone 6S, and
also adequately on the iPhone 5S. 

BUG=webrtc:5678
==========

[commit-bot: I haz the power, 2016-09-28 15:17:50]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-28 15:17:58]

Description was changed from

==========
Move the QP scaling thresholds to the relevant encoders.

Also provide a new set of thresholds for the VideoToolbox encoder. The new
thresholds were experimentally determined to work well on the iPhone 6S, and
also adequately on the iPhone 5S. 

BUG=webrtc:5678
==========

to

==========
Move the QP scaling thresholds to the relevant encoders.

Also provide a new set of thresholds for the VideoToolbox encoder. The new
thresholds were experimentally determined to work well on the iPhone 6S, and
also adequately on the iPhone 5S.

BUG=webrtc:5678

Committed: https://crrev.com/478681e1e6e6c5d66366672614d58b10ad7df083
Cr-Commit-Position: refs/heads/master@{#14420}
==========

[commit-bot: I haz the power, 2016-09-28 15:17:59]

Patchset 10 (id:??) landed as
https://crrev.com/478681e1e6e6c5d66366672614d58b10ad7df083
Cr-Commit-Position: refs/heads/master@{#14420}