Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358683002:
Android: enable/disable WebRTC HW H264 with a flag. (Closed)
|
Created:
4 years, 3 months ago by braveyao Modified:
4 years, 2 months ago CC:
asvitkine+watch_chromium.org, avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: enable/disable WebRTC HW H264 with a flag.
This cl is to enable/disable HW H264 codec on Android by
expanding an existing flag.
Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage.
Also there is a black list to excludes known models with
poor performance.
This cl also includes some basic performance tuning to make
sure video works normally under most common scenarios.
More study on it will be done in the following working.
BUG = 615108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/899bc7e19a29e09965d6954a51dde29999ac815b
Cr-Commit-Position: refs/heads/master@{#425819}
Patch Set 1 #
Total comments: 2
Patch Set 2 : expanding existing switch flag instead of add one more feature #
Total comments: 18
Patch Set 3 : address comments #
Total comments: 2
Patch Set 4 : loop codecs in fineHWEncoder() and other fixes. #
Total comments: 26
Patch Set 5 : adopt enum instead and address other comments #
Total comments: 4
Patch Set 6 : add back the removed entry in software_rendering_list_jason.cc and remove the change in gpu_data_manager_impl_private.cc #
Total comments: 4
Patch Set 7 : address comments #Patch Set 8 : rebase to Oct17 #Messages
Total messages: 80 (50 generated)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android: enable WebRTC HW H264 with a flag. This cl is to enable HW H264 codec on Android with a seperate flag. Qcom and Exynos codecs will be supported at this stage with given minimum SDKs. This cl also incudes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 ========== to ========== Android: enable WebRTC HW H264 with a flag. This cl is to enable HW H264 codec on Android with a separate flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 ==========
braveyao@chromium.org changed reviewers: + dalecurtis@chromium.org
Hi dalecurtis@, could you please have a first look at this cl when you get a chance?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry this slipped off the active review list. Please ping if you don't hear from me in 24 hours. It's a bit odd that you're mixing both a switch and a feature. Why not just have one?
On 2016/09/23 17:56:39, DaleCurtis wrote: > Sorry this slipped off the active review list. Please ping if you don't hear > from me in 24 hours. > > It's a bit odd that you're mixing both a switch and a feature. Why not just have > one? Yes it's a bit odd. The purpose is to use a separate flag to control H264 on Android only, as I was told to. And we don't add new switch now, but feature only. So there is an old switch "kDisableWebRtcHWEncoding" for general HW encoding on CrOS and vpx on Android now, and a new feature "kWebRtcHWH264EncodingAndroid" for H264 on Android. Anyway, we can discuss on this.
How does having a separate flag let you control Android separately? Can't you just change the defaults based on compile flag?
On 2016/09/23 20:40:40, DaleCurtis wrote: > How does having a separate flag let you control Android separately? Can't you > just change the defaults based on compile flag? The newly added "kWebRtcHWH264EncodingAndroid" is only available on Android. And it will control the H264 On/Off only, despite the existing flag "kDisableWebRtcHWEncoding". Then "kDisableWebRtcHWEncoding" on Android will only control VPx(VP8 at present, maybe VP9 in the future.).(On CrOS it still can control both H264 and VPx.) It's a request to enable/disable H264 and VPx separately on Android. So I suppose one flag is not enough. Any other thinking?
https://codereview.chromium.org/2358683002/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:378: new MediaCodecProperties("OMX.Exynos.", Build.VERSION_CODES.M); You might want to consider adding Intel HW encoder to this list. It was disabled in WebRTC due to some crashes in the encoder. Since Chromium uses different wrapper around the encoder, it is possible this issue doesn't affect Chromium. https://codereview.webrtc.org/2249743007/
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi dalecurtis@, please take a look to the new patchset, in which I expanded the existing flag with equivalent 4 values. https://codereview.chromium.org/2358683002/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:378: new MediaCodecProperties("OMX.Exynos.", Build.VERSION_CODES.M); On 2016/09/26 11:42:04, sakal-chromium wrote: > You might want to consider adding Intel HW encoder to this list. It was disabled > in WebRTC due to some crashes in the encoder. Since Chromium uses different > wrapper around the encoder, it is possible this issue doesn't affect Chromium. > https://codereview.webrtc.org/2249743007/ Just curious: why do you need supporting to Intel HW encoder, since there are few models at present? Anyway, I'll find a Intel device for a test first. Maybe enable it in the following cl.
https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:19: const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess(); Pass in? https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:21: if (!cmd_line->HasSwitch(switches::kDisableWebRtcHWEncoding) || early return. store getswithvalueascii for comparison. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:44: if (!IsCodecDisabledByCommanLine(switches::kDisableWebRtcHWEncodingVPx)) { CommandLine. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:60: if (cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding) || Can this flag be deleted now? https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:261: !cmd_line->GetSwitchValueASCII(switches::kDisableWebRtcHWEncoding) Multiline if needs {} https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:438: String encoderName = getDefaultCodecName(mime, MEDIA_CODEC_ENCODER, false); Hmm instead of doing this, why not loop through potential encoders and find one not on the list? https://codereview.chromium.org/2358683002/diff/20001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2358683002/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:38: const int64_t kNumMicrosecsPerSec = INT64_C(1000000); INT64_C necessary?
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks so much! All addressed/responded. PTAL! https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:19: const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess(); On 2016/09/29 00:31:36, DaleCurtis wrote: > Pass in? Done. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:21: if (!cmd_line->HasSwitch(switches::kDisableWebRtcHWEncoding) || On 2016/09/29 00:31:35, DaleCurtis wrote: > early return. store getswithvalueascii for comparison. Done. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:44: if (!IsCodecDisabledByCommanLine(switches::kDisableWebRtcHWEncodingVPx)) { On 2016/09/29 00:31:35, DaleCurtis wrote: > CommandLine. Done. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:60: if (cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding) || On 2016/09/29 00:31:36, DaleCurtis wrote: > Can this flag be deleted now? No. This is for extension on Mac. please see the comments above. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:261: !cmd_line->GetSwitchValueASCII(switches::kDisableWebRtcHWEncoding) On 2016/09/29 00:31:36, DaleCurtis wrote: > Multiline if needs {} Done. https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:438: String encoderName = getDefaultCodecName(mime, MEDIA_CODEC_ENCODER, false); On 2016/09/29 00:31:36, DaleCurtis wrote: > Hmm instead of doing this, why not loop through potential encoders and find one > not on the list? Not understand your concern. Here the whole logic is: Find the HW codec is available on this device or not, then check if it can pass the blacklist/whitelist check. Any problem? https://codereview.chromium.org/2358683002/diff/20001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2358683002/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:38: const int64_t kNumMicrosecsPerSec = INT64_C(1000000); On 2016/09/29 00:31:36, DaleCurtis wrote: > INT64_C necessary? Done.
https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:60: if (cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding) || On 2016/09/29 at 19:26:26, braveyao wrote: > On 2016/09/29 00:31:36, DaleCurtis wrote: > > Can this flag be deleted now? > > No. This is for extension on Mac. please see the comments above. How is that flag passed? Can you remove that flag and specify --disable-...-=h264 on all platforms except mac? https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:438: String encoderName = getDefaultCodecName(mime, MEDIA_CODEC_ENCODER, false); On 2016/09/29 at 19:26:26, braveyao wrote: > On 2016/09/29 00:31:36, DaleCurtis wrote: > > Hmm instead of doing this, why not loop through potential encoders and find one > > not on the list? > > Not understand your concern. > Here the whole logic is: > Find the HW codec is available on this device or not, then check if it can pass the blacklist/whitelist check. Any problem? You're configuring the mediacodec by name, but instead of just looping through the list and finding one that works for you, you're requesting the default one and disabling encoding if it doesn't match. There may be other encoders in the list (other than the default) which are okay for your purposes, right? https://codereview.chromium.org/2358683002/diff/40001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2358683002/diff/40001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:359: input_timestamp_ += base::TimeDelta::FromMicroseconds(kNumMicrosecsPerSec / base::Time:: has this already for you.
dalecurtis@chromium.org changed reviewers: + watk@chromium.org
=>watk since I'll be OOO soon.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the comments! All addressed/responded. PTAL. https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2358683002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:60: if (cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding) || On 2016/09/29 19:34:14, DaleCurtis_OOO_Until_Oct_18 wrote: > On 2016/09/29 at 19:26:26, braveyao wrote: > > On 2016/09/29 00:31:36, DaleCurtis wrote: > > > Can this flag be deleted now? > > > > No. This is for extension on Mac. please see the comments above. > > How is that flag passed? Can you remove that flag and specify > --disable-...-=h264 on all platforms except mac? You are right. I'll discuss with other and follow it up in a separate cl. https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:438: String encoderName = getDefaultCodecName(mime, MEDIA_CODEC_ENCODER, false); On 2016/09/29 19:34:14, DaleCurtis_OOO_Until_Oct_18 wrote: > On 2016/09/29 at 19:26:26, braveyao wrote: > > On 2016/09/29 00:31:36, DaleCurtis wrote: > > > Hmm instead of doing this, why not loop through potential encoders and find > one > > > not on the list? > > > > Not understand your concern. > > Here the whole logic is: > > Find the HW codec is available on this device or not, then check if it can > pass the blacklist/whitelist check. Any problem? > > You're configuring the mediacodec by name, but instead of just looping through > the list and finding one that works for you, you're requesting the default one > and disabling encoding if it doesn't match. There may be other encoders in the > list (other than the default) which are okay for your purposes, right? Done. https://codereview.chromium.org/2358683002/diff/40001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2358683002/diff/40001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:359: input_timestamp_ += base::TimeDelta::FromMicroseconds(kNumMicrosecsPerSec / On 2016/09/29 19:34:14, DaleCurtis_OOO_Until_Oct_18 wrote: > base::Time:: has this already for you. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2358683002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2358683002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:678: switches::kDisableWebRtcHWEncodingNone}, The double negative is confusing. Can it be called kEnableWebRtcHWEncoding? https://codereview.chromium.org/2358683002/diff/60001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2358683002/diff/60001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:686: // Disabe HW encoding by default on Android. Disable https://codereview.chromium.org/2358683002/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2358683002/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:26: return false; nit: can return the expression in the if-statement instead https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:363: private static class MediaCodecProperties { I think this would be simpler if you used an enum instead, with three fields: mime, codecPrefix, minSdk. Then you don't need the two arrays for vp8 and h264, and you can call findHwEncoder once. findHwEncoder can iterate over the enums #values() and check whether the three things match. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:394: "SAMSUNG-SGH-I337", "Nexus 7", "Nexus 4"}; "Blacklist" is a more common term for this. What about "H264_ENCODER_MODEL_BLACKLIST". Could consider inlining it into where it's used for now, but up to you. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:406: assert result.mediaCodec == null; I see that you copied this from above, but I don't feel like we need it. I can't imagine what kind of mistake it would catch. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:419: result.mediaCodec = null; This is already null? https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:430: private static boolean findHwEncoder( Rename this to isEncoderSupportedByDevice to mirror the decoder one? https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:443: Log.w(TAG, "Model: " + Build.MODEL + " has black listed H.264 encoder."); s/black listed/blacklisted https://codereview.chromium.org/2358683002/diff/60001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2358683002/diff/60001/media/base/android/medi... media/base/android/media_codec_util.cc:90: static bool IsH264EncoderSupported() { I would mirror IsDecoderSupportedByDevice and call this IsEncoderSupportedByDevice https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:394: return; Did you mean to change these? You don't want to try dequeueing again if the buffers changed? Since all cases are returns now, you don't need the do {} while do you? https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.h (right): https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.h:57: INITIAL_FRAMERATE = 30, Should this just be called FRAMERATE? Not sure why it's INITIAL https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.h:98: // A monotonically-growing value in us. delete "in us". TimeDelta knows its units.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi,watk@, thanks so much for the comments! All addressed/responded. PTAL. https://codereview.chromium.org/2358683002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2358683002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:678: switches::kDisableWebRtcHWEncodingNone}, On 2016/10/01 01:06:53, watk wrote: > The double negative is confusing. Can it be called kEnableWebRtcHWEncoding? Yes it's a bit confusing. But it works as a pair to 'switches::kDisableWebRtcHWDecoding'. If I recall it correctly, they are both evolved from 'kEnableWebRtcHWEncoding/Decoding' quite a while ago. CrOS needs it to be 'kDisableWebRtcHWEncoding' as HW codec is the first choice on it. Android may need it soon. So I suppose this should be OK. WDYT? https://codereview.chromium.org/2358683002/diff/60001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2358683002/diff/60001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:686: // Disabe HW encoding by default on Android. On 2016/10/01 01:06:53, watk wrote: > Disable Done. https://codereview.chromium.org/2358683002/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2358683002/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:26: return false; On 2016/10/01 01:06:54, watk wrote: > nit: can return the expression in the if-statement instead Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:363: private static class MediaCodecProperties { On 2016/10/01 01:06:54, watk wrote: > I think this would be simpler if you used an enum instead, with three fields: > mime, codecPrefix, minSdk. > > Then you don't need the two arrays for vp8 and h264, and you can call > findHwEncoder once. findHwEncoder can iterate over the enums #values() and > check whether the three things match. Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:394: "SAMSUNG-SGH-I337", "Nexus 7", "Nexus 4"}; On 2016/10/01 01:06:54, watk wrote: > "Blacklist" is a more common term for this. What about > "H264_ENCODER_MODEL_BLACKLIST". > > Could consider inlining it into where it's used for now, but up to you. Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:406: assert result.mediaCodec == null; On 2016/10/01 01:06:54, watk wrote: > I see that you copied this from above, but I don't feel like we need it. I can't > imagine what kind of mistake it would catch. Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:419: result.mediaCodec = null; On 2016/10/01 01:06:54, watk wrote: > This is already null? Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:430: private static boolean findHwEncoder( On 2016/10/01 01:06:54, watk wrote: > Rename this to isEncoderSupportedByDevice to mirror the decoder one? Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:443: Log.w(TAG, "Model: " + Build.MODEL + " has black listed H.264 encoder."); On 2016/10/01 01:06:54, watk wrote: > s/black listed/blacklisted Done. https://codereview.chromium.org/2358683002/diff/60001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2358683002/diff/60001/media/base/android/medi... media/base/android/media_codec_util.cc:90: static bool IsH264EncoderSupported() { On 2016/10/01 01:06:54, watk wrote: > I would mirror IsDecoderSupportedByDevice and call this > IsEncoderSupportedByDevice Done. https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:394: return; On 2016/10/01 01:06:54, watk wrote: > Did you mean to change these? You don't want to try dequeueing again if the > buffers changed? > > Since all cases are returns now, you don't need the do {} while do you? Done removing do-while. Yes I incline to return instead of break here. The reason is with 'return' the bitrate will, with greater chance, ramp up more quickly and smoothly with the BWE setting, at least in all my testings. I'm still working on it with other performance tuning for the following up cl. We can make a final decision there. https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.h (right): https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.h:57: INITIAL_FRAMERATE = 30, On 2016/10/01 01:06:54, watk wrote: > Should this just be called FRAMERATE? Not sure why it's INITIAL Because this value is for initializing encoder only. The real bitrate and framerate will vary according to BWE. It's common in WebRtc for both HW and SW codecs. I suppose it's OK to keep it. https://codereview.chromium.org/2358683002/diff/60001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.h:98: // A monotonically-growing value in us. On 2016/10/01 01:06:54, watk wrote: > delete "in us". TimeDelta knows its units. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! lgtm
braveyao@chromium.org changed reviewers: + cpu@chromium.org
+ cpu@, for OWNERS' approval to content/browser and content/public. Thanks!
Description was changed from ========== Android: enable WebRTC HW H264 with a flag. This cl is to enable HW H264 codec on Android with a separate flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 ========== to ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 ==========
braveyao@chromium.org changed reviewers: + kbr@chromium.org
+ kbr@ for OWNER's approval to content/browser/gpu/gpu_data_manager_impl_private.cc.
https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:690: } Is there a reason to not just handle this with a new entry in src/gpu/config/software_rendering_list_json.cc ? If not, please add a new entry there with a description and link to the bug ID, and don't forget to increment the version number at the top of the file. Thanks.
Thanks kbr@. There's history for the change. Please help to suggest which one is better. https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:690: } On 2016/10/06 23:23:09, Ken Russell wrote: > Is there a reason to not just handle this with a new entry in > src/gpu/config/software_rendering_list_json.cc ? If not, please add a new entry > there with a description and link to the bug ID, and don't forget to increment > the version number at the top of the file. Thanks. There was one in software_rendering_list_json.cc which was recently removed in cl https://codereview.chromium.org/2288103003. When I firstly started this cl, it took me a while to find out the entry in the software_rendering_list_json.cc. So this time I added it here. Do you think it's better to add it back to the software_rendering_list_json.cc in this cl?
https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:690: } On 2016/10/06 23:31:05, braveyao wrote: > On 2016/10/06 23:23:09, Ken Russell wrote: > > Is there a reason to not just handle this with a new entry in > > src/gpu/config/software_rendering_list_json.cc ? If not, please add a new > entry > > there with a description and link to the bug ID, and don't forget to increment > > the version number at the top of the file. Thanks. > > There was one in software_rendering_list_json.cc which was recently removed in > cl https://codereview.chromium.org/2288103003. > When I firstly started this cl, it took me a while to find out the entry in the > software_rendering_list_json.cc. So this time I added it here. > Do you think it's better to add it back to the software_rendering_list_json.cc > in this cl? Yes, please just add the entry back in software_rendering_list_json.cc. There's already a way to do this so it's best not to add another way. Also, by putting it in software_rendering_list_json.cc there's a way to optionally enable it, where doing it this way that would require a recompile.
Description was changed from ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 ========== to ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Thanks kbr@ for the clarification! Comments addressed. PTAL. https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2358683002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:690: } On 2016/10/07 00:46:51, Ken Russell wrote: > On 2016/10/06 23:31:05, braveyao wrote: > > On 2016/10/06 23:23:09, Ken Russell wrote: > > > Is there a reason to not just handle this with a new entry in > > > src/gpu/config/software_rendering_list_json.cc ? If not, please add a new > > entry > > > there with a description and link to the bug ID, and don't forget to > increment > > > the version number at the top of the file. Thanks. > > > > There was one in software_rendering_list_json.cc which was recently removed in > > cl https://codereview.chromium.org/2288103003. > > When I firstly started this cl, it took me a while to find out the entry in > the > > software_rendering_list_json.cc. So this time I added it here. > > Do you think it's better to add it back to the software_rendering_list_json.cc > > in this cl? > > Yes, please just add the entry back in software_rendering_list_json.cc. There's > already a way to do this so it's best not to add another way. Also, by putting > it in software_rendering_list_json.cc there's a way to optionally enable it, > where doing it this way that would require a recompile. Done.
Thanks, looks better. Couple of small fixes please. https://codereview.chromium.org/2358683002/diff/100001/gpu/config/software_re... File gpu/config/software_rendering_list_json.cc (right): https://codereview.chromium.org/2358683002/diff/100001/gpu/config/software_re... gpu/config/software_rendering_list_json.cc:21: "version": "11.13", Please update this version number. https://codereview.chromium.org/2358683002/diff/100001/gpu/config/software_re... gpu/config/software_rendering_list_json.cc:695: "description": "MediaCodec is still too buggy to use for encoding (b/11536167)", Please add a "cr_bugs" field for this entry so about:gpu contains a link.
Thanks for the quick check! All addressed. PTAL. https://codereview.chromium.org/2358683002/diff/100001/gpu/config/software_re... File gpu/config/software_rendering_list_json.cc (right): https://codereview.chromium.org/2358683002/diff/100001/gpu/config/software_re... gpu/config/software_rendering_list_json.cc:21: "version": "11.13", On 2016/10/07 01:02:49, Ken Russell wrote: > Please update this version number. Done. https://codereview.chromium.org/2358683002/diff/100001/gpu/config/software_re... gpu/config/software_rendering_list_json.cc:695: "description": "MediaCodec is still too buggy to use for encoding (b/11536167)", On 2016/10/07 01:02:49, Ken Russell wrote: > Please add a "cr_bugs" field for this entry so about:gpu contains a link. Done. I'm using the crbug number of this cl, since when H264 is ready to release, we can remove this entry.
Thanks for the updates. LGTM
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
braveyao@chromium.org changed reviewers: - cpu@chromium.org
The CQ bit was unchecked by braveyao@chromium.org
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2358683002/#ps160001 (title: "rebase to Oct17")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
braveyao@chromium.org changed reviewers: + cpu@chromium.org
lgtm
The CQ bit was checked by braveyao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Android: enable/disable WebRTC HW H264 with a flag. This cl is to enable/disable HW H264 codec on Android by expanding an existing flag. Qcom and Exynos codecs (with given minimum SDKs) are white-listed at this stage. Also there is a black list to excludes known models with poor performance. This cl also includes some basic performance tuning to make sure video works normally under most common scenarios. More study on it will be done in the following working. BUG = 615108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/899bc7e19a29e09965d6954a51dde29999ac815b Cr-Commit-Position: refs/heads/master@{#425819} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/899bc7e19a29e09965d6954a51dde29999ac815b Cr-Commit-Position: refs/heads/master@{#425819} |
