Android: enable WebRTC HW H264 with a flag by default

Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108 (cl
https://codereview.chromium.org/2358683002), disabled by default with
a combined flag for both VP8 and H264. Now we separate the flag into
two and still keep VP8 HW encoder disabled by default.

BUG=664652
Committed: https://crrev.com/dae42d9a0ed4b75e0c6064d715da8bcb26603b3f
Cr-Commit-Position: refs/heads/master@{#437665}

[braveyao, 2016-12-05 22:55:21]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 22:55:55]

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

[braveyao, 2016-12-05 22:58:52]

Description was changed from

==========
Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108, disabled by default with
a combined flag for both VP8 and H264. Now we seperate the flag into two and
keep VP8 HW encoder disable by default.

BUG=664652
==========

to

==========
Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108 (cl
https://codereview.chromium.org/2358683002), disabled by default with
a combined flag for both VP8 and H264. Now we separate the flag into
two and still keep VP8 HW encoder disabled by default.

BUG=664652
==========

[commit-bot: I haz the power, 2016-12-06 00:08:20]

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

[commit-bot: I haz the power, 2016-12-06 00:08:22]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-12-06 00:12:45]

braveyao@chromium.org changed reviewers:
+ piman@chromium.org

[braveyao, 2016-12-06 00:12:45]

Hi piman, please take a look when you get a chance.

[Pawel Osciak, 2016-12-06 04:13:00]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2016-12-06 04:13:01]

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
content/renderer/media/gpu/rtc_video_encoder_factory.cc:49:
codecs->push_back(cricket::WebRtcVideoEncoderFactory::VideoCodec(
Perhaps I'm misreading the logic of the decision tree here, but it looks like
right now having kDisableWebRtcHWEncoding only disables HW VP8 encode, but does
not disable H264 HW encode?

[braveyao, 2016-12-06 17:50:13]

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
content/renderer/media/gpu/rtc_video_encoder_factory.cc:49:
codecs->push_back(cricket::WebRtcVideoEncoderFactory::VideoCodec(
On 2016/12/06 04:13:00, Pawel Osciak wrote:
> Perhaps I'm misreading the logic of the decision tree here, but it looks like
> right now having kDisableWebRtcHWEncoding only disables HW VP8 encode, but
does
> not disable H264 HW encode?

That's true. We want to control H264 separately from other HW codecs(currently
only VP8) on Android. So a new feature added for H264 (enabled by default) and
the original kDisableWebRtcHWEncoding will only disable other codecs, e.g. VP8.

[piman, 2016-12-06 21:29:42]

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
content/renderer/media/gpu/rtc_video_encoder_factory.cc:49:
codecs->push_back(cricket::WebRtcVideoEncoderFactory::VideoCodec(
On 2016/12/06 17:50:13, braveyao wrote:
> On 2016/12/06 04:13:00, Pawel Osciak wrote:
> > Perhaps I'm misreading the logic of the decision tree here, but it looks
like
> > right now having kDisableWebRtcHWEncoding only disables HW VP8 encode, but
> does
> > not disable H264 HW encode?
> 
> That's true. We want to control H264 separately from other HW codecs(currently
> only VP8) on Android. So a new feature added for H264 (enabled by default) and
> the original kDisableWebRtcHWEncoding will only disable other codecs, e.g.
VP8.

Should we rename the flag to avoid confusion? E.g. kDisableWebRtcVP8HWEncoding

[braveyao, 2016-12-06 22:45:59]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:46:48]

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

[commit-bot: I haz the power, 2016-12-06 23:56:21]

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

[commit-bot: I haz the power, 2016-12-06 23:56:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[braveyao, 2016-12-07 00:07:53]

Patchset #2 (id:20001) has been deleted

[braveyao, 2016-12-07 00:08:00]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 00:08:57]

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

[commit-bot: I haz the power, 2016-12-07 00:17:02]

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

[commit-bot: I haz the power, 2016-12-07 00:17:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[braveyao, 2016-12-07 01:35:45]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 01:36:24]

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

[commit-bot: I haz the power, 2016-12-07 03:03:38]

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

[commit-bot: I haz the power, 2016-12-07 03:03:39]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-12-07 17:07:37]

Done. PTAL.

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/2549283002/diff/1/content/renderer/media/gpu/...
content/renderer/media/gpu/rtc_video_encoder_factory.cc:49:
codecs->push_back(cricket::WebRtcVideoEncoderFactory::VideoCodec(
On 2016/12/06 21:29:42, piman wrote:
> On 2016/12/06 17:50:13, braveyao wrote:
> > On 2016/12/06 04:13:00, Pawel Osciak wrote:
> > > Perhaps I'm misreading the logic of the decision tree here, but it looks
> like
> > > right now having kDisableWebRtcHWEncoding only disables HW VP8 encode, but
> > does
> > > not disable H264 HW encode?
> > 
> > That's true. We want to control H264 separately from other HW
codecs(currently
> > only VP8) on Android. So a new feature added for H264 (enabled by default)
and
> > the original kDisableWebRtcHWEncoding will only disable other codecs, e.g.
> VP8.
> 
> Should we rename the flag to avoid confusion? E.g. kDisableWebRtcVP8HWEncoding

Done.

[piman, 2016-12-07 19:16:34]

LGTM + nit if Pawel is happy.

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
File content/public/browser/gpu_utils.cc (right):

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
content/public/browser/gpu_utils.cc:48:
gpu_preferences.disable_web_rtc_hw_encoding =
nit: update the name of this field too, to accurately represent what it does.
E.g. disable_web_rtc_hw_vp8_encoding

[braveyao, 2016-12-07 20:29:52]

braveyao@chromium.org changed reviewers:
+ jwd@chromium.org

[braveyao, 2016-12-07 20:29:53]

jwd@, please help to take a look at histograms.xml. Thanks!

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
File content/public/browser/gpu_utils.cc (right):

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
content/public/browser/gpu_utils.cc:48:
gpu_preferences.disable_web_rtc_hw_encoding =
On 2016/12/07 19:16:33, piman wrote:
> nit: update the name of this field too, to accurately represent what it does.
> E.g. disable_web_rtc_hw_vp8_encoding

It's better to keep it as is. Because either VP8 or H264 HW encoding is enabled,
|disable_web_rtc_hw_encoding| won't be true and GPU will create VEA according to
it. So I suppose the name is right here.

[piman, 2016-12-07 20:35:33]

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
File content/public/browser/gpu_utils.cc (right):

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
content/public/browser/gpu_utils.cc:48:
gpu_preferences.disable_web_rtc_hw_encoding =
On 2016/12/07 20:29:53, braveyao wrote:
> On 2016/12/07 19:16:33, piman wrote:
> > nit: update the name of this field too, to accurately represent what it
does.
> > E.g. disable_web_rtc_hw_vp8_encoding
> 
> It's better to keep it as is. Because either VP8 or H264 HW encoding is
enabled,
> |disable_web_rtc_hw_encoding| won't be true and GPU will create VEA according
to
> it. So I suppose the name is right here.

The logic here is wrong then. If H264 HW encoding is enabled, and VP8 is
disabled, disable_web_rtc_hw_encoding will be set to true, and we will *not* use
HW encoding.

[braveyao, 2016-12-07 21:32:09]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 21:32:42]

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

[braveyao, 2016-12-07 21:34:02]

Hi piman@, that's the problem and fixed now. PTAL.

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
File content/public/browser/gpu_utils.cc (right):

https://codereview.chromium.org/2549283002/diff/40001/content/public/browser/...
content/public/browser/gpu_utils.cc:48:
gpu_preferences.disable_web_rtc_hw_encoding =
On 2016/12/07 20:35:33, piman wrote:
> On 2016/12/07 20:29:53, braveyao wrote:
> > On 2016/12/07 19:16:33, piman wrote:
> > > nit: update the name of this field too, to accurately represent what it
> does.
> > > E.g. disable_web_rtc_hw_vp8_encoding
> > 
> > It's better to keep it as is. Because either VP8 or H264 HW encoding is
> enabled,
> > |disable_web_rtc_hw_encoding| won't be true and GPU will create VEA
according
> to
> > it. So I suppose the name is right here.
> 
> The logic here is wrong then. If H264 HW encoding is enabled, and VP8 is
> disabled, disable_web_rtc_hw_encoding will be set to true, and we will *not*
use
> HW encoding.

Thanks so much! I neglected that case...
Done.

[commit-bot: I haz the power, 2016-12-07 21:48:14]

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

[commit-bot: I haz the power, 2016-12-07 21:48:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[braveyao, 2016-12-07 22:05:28]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[braveyao, 2016-12-07 22:05:39]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-12-07 22:05:53]

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

[piman, 2016-12-07 22:24:22]

lgtm

[braveyao, 2016-12-07 22:49:43]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:49:59]

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

[commit-bot: I haz the power, 2016-12-08 00:09:18]

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

[commit-bot: I haz the power, 2016-12-08 00:09:19]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[braveyao, 2016-12-08 17:38:07]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 17:38:56]

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

[commit-bot: I haz the power, 2016-12-08 19:27:11]

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

[commit-bot: I haz the power, 2016-12-08 19:27:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[braveyao, 2016-12-09 00:01:08]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 00:01:50]

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

[commit-bot: I haz the power, 2016-12-09 00:12:51]

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

[commit-bot: I haz the power, 2016-12-09 00:12:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[braveyao, 2016-12-09 01:42:17]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

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

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

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

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

[commit-bot: I haz the power, 2016-12-09 03:09:49]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-12-09 17:19:58]

lgtm

https://codereview.chromium.org/2549283002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2549283002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:92156: -  <int value="-2008272679"
label="disable-webrtc-hw-encoding"/>
Can you keep this value, and just add (deprecated) to the label.

[braveyao, 2016-12-09 17:27:11]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 17:27:24]

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

[braveyao, 2016-12-09 17:29:40]

Thanks jwd@. Didn't notice such practice before and will keep it in mind.

https://codereview.chromium.org/2549283002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2549283002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:92156: -  <int value="-2008272679"
label="disable-webrtc-hw-encoding"/>
On 2016/12/09 17:19:58, jwd wrote:
> Can you keep this value, and just add (deprecated) to the label.

Done.

[commit-bot: I haz the power, 2016-12-09 18:38:01]

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

[commit-bot: I haz the power, 2016-12-09 18:38:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[braveyao, 2016-12-09 18:45:39]

The CQ bit was checked by braveyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 18:46:15]

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

[commit-bot: I haz the power, 2016-12-09 20:41:05]

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

[commit-bot: I haz the power, 2016-12-09 20:41:06]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-12-09 21:47:30]

The CQ bit was checked by braveyao@chromium.org

[braveyao, 2016-12-09 21:47:30]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2549283002/#ps140001
(title: "fix histograms value")

[commit-bot: I haz the power, 2016-12-09 21:48:23]

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

[commit-bot: I haz the power, 2016-12-09 22:04:27]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481320050088480,
"parent_rev": "177a62115dad9a23b023fd87313bede83fa6348c", "commit_rev":
"901f7878d9183bfe444f77f6445b8c90e6053e76"}

[commit-bot: I haz the power, 2016-12-09 22:04:56]

Description was changed from

==========
Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108 (cl
https://codereview.chromium.org/2358683002), disabled by default with
a combined flag for both VP8 and H264. Now we separate the flag into
two and still keep VP8 HW encoder disabled by default.

BUG=664652
==========

to

==========
Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108 (cl
https://codereview.chromium.org/2358683002), disabled by default with
a combined flag for both VP8 and H264. Now we separate the flag into
two and still keep VP8 HW encoder disabled by default.

BUG=664652

Review-Url: https://codereview.chromium.org/2549283002
==========

[commit-bot: I haz the power, 2016-12-09 22:05:00]

Committed patchset #6 (id:140001)

[Pawel Osciak, 2016-12-12 01:42:11]

kDisableWebRtcHWEncoding was essential for Chrome OS as we had an ability to
have a global switch to disable all HW encoding with it, including both H264 and
VP8 encoding.

It's very useful and widely documented and known, and used by QA and developers.
We would really prefer to keep it please. Could we please restore it? Thank you.

[commit-bot: I haz the power, 2016-12-12 14:56:44]

Description was changed from

==========
Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108 (cl
https://codereview.chromium.org/2358683002), disabled by default with
a combined flag for both VP8 and H264. Now we separate the flag into
two and still keep VP8 HW encoder disabled by default.

BUG=664652

Review-Url: https://codereview.chromium.org/2549283002
==========

to

==========
Android: enable WebRTC HW H264 with a flag by default

This cl is to enable HW H264 encoding on Android by default, with a flag.
This feature is firstly introduced in crbug/615108 (cl
https://codereview.chromium.org/2358683002), disabled by default with
a combined flag for both VP8 and H264. Now we separate the flag into
two and still keep VP8 HW encoder disabled by default.

BUG=664652

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

[commit-bot: I haz the power, 2016-12-12 14:56:46]

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

[braveyao, 2016-12-12 18:14:24]

On 2016/12/12 01:42:11, Pawel Osciak wrote:
> kDisableWebRtcHWEncoding was essential for Chrome OS as we had an ability to
> have a global switch to disable all HW encoding with it, including both H264
and
> VP8 encoding.
> 
> It's very useful and widely documented and known, and used by QA and
developers.
> We would really prefer to keep it please. Could we please restore it? Thank
you.

OK. Good to know. I'll create another CL for this ASAP.

[braveyao, 2016-12-19 19:55:49]

Patchset #7 (id:160001) has been deleted