|
|
Created:
4 years, 9 months ago by DaleCurtis Modified:
4 years, 9 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, mlamouri+watch-media_chromium.org, piman, posciak+watch_chromium.org, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@better_known Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup MediaCodecUtil::GetDefaultCodecName().
There's no need to build an entire list of codecs and transport that
across JNI when we always want the first one in the list.
Notably this reduces the cost of GpuChildThread::OnInitialize() by
over 400ms on an N5 running Marshmallow (450.208ms vs 39.64ms)! The
reduction comes from VideoEncodeAccelerator no longer creating ~3
MediaCodec instances to determine the codec name during the call to
VideoEncodeAccelerator::GetSupportedProfiles() during child thread
startup. I suspect this will translate into real GPU process startup
improvements.
BUG=594814
TEST=codec lists match before after on KitKat below and above.
Committed: https://crrev.com/5c1bee20afe6113087fc9a5781b5773a8894bbd4
Cr-Commit-Position: refs/heads/master@{#381805}
Patch Set 1 : Cleanup. #
Total comments: 6
Patch Set 2 : Switch to faster getCodecInfos. Comments. #
Total comments: 14
Patch Set 3 : Comments. #
Messages
Total messages: 38 (10 generated)
Patchset #1 (id:1) has been deleted
dalecurtis@chromium.org changed reviewers: + timav@chromium.org
WDYT about switching to use MediaCodecList::getCodecInfos() instead of actually creating a MediaCodec to determine the name. It seems like we might be incurring some unnecessary overhead for spinning up a MediaCodec just for this information.
Timing wise getName() + creation + teardown seems to take ~70-180ms.
GetCodecInfos / iterating is much faster (45 ms first call, 0ms all subsequent calls, vs 100+ms for each call). Since encoders and decoders are enumerated at GPU process startup this can significantly reduce initialization time. I'll go ahead and replace this with a helper class which selects between GetCodecInfos on Lollipop and just plain iterate on lower versions.
timav@chromium.org changed reviewers: + qinmin@chromium.org
https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:58: Log.w(TAG, "getDefaultCodecName: Failed to create MediaCodec: %s, direction: %d", nit: Does it make sense to add ", getting first match from the codec list" here in the log line? https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:76: return ""; nit: Log that it could not determine the name? https://codereview.chromium.org/1805163002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1805163002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:127: return base::StartsWith(codec_name, "OMX.google.", I think codec_name can be empty string? In this case the old code returned true and the new will return false. I'd keep the functionality unchanged.
On 2016/03/15 20:50:02, DaleCurtis wrote: > WDYT about switching to use MediaCodecList::getCodecInfos() instead of actually > creating a MediaCodec to determine the name. It seems like we might be incurring > some unnecessary overhead for spinning up a MediaCodec just for this > information. Can it be several codecs for the same mime type? +qinmin@
Patchset #2 (id:40001) has been deleted
On 2016/03/15 21:39:59, Tima Vaisburd wrote: > On 2016/03/15 20:50:02, DaleCurtis wrote: > > WDYT about switching to use MediaCodecList::getCodecInfos() instead of > actually > > creating a MediaCodec to determine the name. It seems like we might be > incurring > > some unnecessary overhead for spinning up a MediaCodec just for this > > information. > > Can it be several codecs for the same mime type? > +qinmin@ Yes, that could happen
Latest patch set switches to getCodecInfo(). I don't think it matters that there might be multiple codecs per mime-type. Do you have examples where it does? I'd guess that unless you provide more information than the mime-type (i.e. constructing a MediaFormat) MediaCodec should be doing the same thing as this iteration. On Lollipop+ we can switch to find(Decoder|Encoder)ForFormat(MediaFormat) if you prefer. https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:58: Log.w(TAG, "getDefaultCodecName: Failed to create MediaCodec: %s, direction: %d", On 2016/03/15 at 21:38:17, Tima Vaisburd wrote: > nit: Does it make sense to add ", getting first match from the codec list" here in the log line? I've deleted this code entirely in the latest patch set. Calling createDecoder() / createEncoder() is too slow. ~100ms per call on an N5. https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:76: return ""; On 2016/03/15 at 21:38:17, Tima Vaisburd wrote: > nit: Log that it could not determine the name? Done. Used the wording from similar messages elsewhere in the file. https://codereview.chromium.org/1805163002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1805163002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:127: return base::StartsWith(codec_name, "OMX.google.", On 2016/03/15 at 21:38:17, Tima Vaisburd wrote: > I think codec_name can be empty string? > In this case the old code returned true and the new will return false. I'd keep the functionality unchanged. Whoops, done.
On 2016/03/15 23:02:45, DaleCurtis wrote: timav> Can it be several codecs for the same mime type? timav> +qinmin@ qinmin> Yes, that could happen > Latest patch set switches to getCodecInfo(). I don't think it matters that there > might be multiple codecs per mime-type. Do you have examples where it does? I'd > guess that unless you provide more information than the mime-type (i.e. > constructing a MediaFormat) MediaCodec should be doing the same thing as this > iteration. The code in PS2 looks good. I'd be interested to know that too, because I think it can validate or refute the approach.
dalecurtis@chromium.org changed reviewers: + glaznev@chromium.org
+glaznev who added a lot of this code in https://codereview.chromium.org/605883002
Ping glaznev@. I've spent some time looking over the original bugs and it's not clear to me what the change to use getName() actually solved. It appears that the GPU blacklist entries resolved the main problem. glaznev@ do you recall the details of the cases where getName() was superior to choosing the first codec?
lgtm
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) This changes the supported android versions, we still need to support JB devices.
glaznev@ and I chatted offline. The root issue the original CLs were trying to fix is that createDecoderByType() would not select the first decoder in the list on Galaxy S4 devices; instead picking the broken vp8 decoder that timav@ is now familiar with :) Now that we're blacklisting vp8 entirely for MediaCodec on these devices we don't have any issues with this anymore. On L+ the MediaCodecList is required to be in order of performance. b/9735008 has some more details on this. Given the availability of the software vp8 decoders and the fact that MSE MediaCodec was never given the protections afforded WebRTC from this path and has only seen issues on the S4 devices, this should be good to go. We can always add more devices to the vp8 blacklist as issues arise.
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2016/03/17 at 00:03:38, qinmin wrote: > This changes the supported android versions, we still need to support JB devices. Sorry I'm a bit of a noob with these macros. What's the expected annotation for "targets jellybean, but we have a runtime check to use lollipop" code :)
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2016/03/17 00:06:34, DaleCurtis wrote: > On 2016/03/17 at 00:03:38, qinmin wrote: > > This changes the supported android versions, we still need to support JB > devices. > > Sorry I'm a bit of a noob with these macros. What's the expected annotation for > "targets jellybean, but we have a runtime check to use lollipop" code :) The annotation is ok, but the issue is that now mCodecList is an empty list for JB_MR2, but previously this is not empty. You mentioned vp8 in your comments to this CL, but what about h.264?
We don't have software h264 decoders, so it doesn't matter if h264 is known unaccelerated. https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2016/03/17 at 00:13:11, qinmin wrote: > On 2016/03/17 00:06:34, DaleCurtis wrote: > > On 2016/03/17 at 00:03:38, qinmin wrote: > > > This changes the supported android versions, we still need to support JB > > devices. > > > > Sorry I'm a bit of a noob with these macros. What's the expected annotation for > > "targets jellybean, but we have a runtime check to use lollipop" code :) > > The annotation is ok, but the issue is that now mCodecList is an empty list for JB_MR2, but previously this is not empty. You mentioned vp8 in your comments to this CL, but what about h.264? Sorry I don't understand? mCodecList is only used on lollipop. On lower versions we'll call the static MediaCodecList.getCodecCount() .getCodecInfoAt() methods. See the conditionals below on if (hasNewMediaCodecList()).
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2016/03/17 00:13:11, qinmin wrote: > On 2016/03/17 00:06:34, DaleCurtis wrote: > > On 2016/03/17 at 00:03:38, qinmin wrote: > > > This changes the supported android versions, we still need to support JB > > devices. > > > > Sorry I'm a bit of a noob with these macros. What's the expected annotation > for > > "targets jellybean, but we have a runtime check to use lollipop" code :) > > The annotation is ok, but the issue is that now mCodecList is an empty list for > JB_MR2, but previously this is not empty. You mentioned vp8 in your comments to > this CL, but what about h.264? > I thought that if hasNewMediaCodecList() returns false the |mCodecList| is not used at all and the iteration goes with the static methods of MediaCodecList class?
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2016/03/17 at 00:20:37, Tima Vaisburd wrote: > On 2016/03/17 00:13:11, qinmin wrote: > > On 2016/03/17 00:06:34, DaleCurtis wrote: > > > On 2016/03/17 at 00:03:38, qinmin wrote: > > > > This changes the supported android versions, we still need to support JB > > > devices. > > > > > > Sorry I'm a bit of a noob with these macros. What's the expected annotation > > for > > > "targets jellybean, but we have a runtime check to use lollipop" code :) > > > > The annotation is ok, but the issue is that now mCodecList is an empty list for > > JB_MR2, but previously this is not empty. You mentioned vp8 in your comments to > > this CL, but what about h.264? > > > > I thought that if hasNewMediaCodecList() returns false the |mCodecList| is not used at all and the iteration goes with the static methods of MediaCodecList class? Correct, that's what should be happening unless I've missed something here.
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) On 2016/03/17 02:12:55, DaleCurtis wrote: > On 2016/03/17 at 00:20:37, Tima Vaisburd wrote: > > On 2016/03/17 00:13:11, qinmin wrote: > > > On 2016/03/17 00:06:34, DaleCurtis wrote: > > > > On 2016/03/17 at 00:03:38, qinmin wrote: > > > > > This changes the supported android versions, we still need to support JB > > > > devices. > > > > > > > > Sorry I'm a bit of a noob with these macros. What's the expected > annotation > > > for > > > > "targets jellybean, but we have a runtime check to use lollipop" code :) > > > > > > The annotation is ok, but the issue is that now mCodecList is an empty list > for > > > JB_MR2, but previously this is not empty. You mentioned vp8 in your comments > to > > > this CL, but what about h.264? > > > > > > > I thought that if hasNewMediaCodecList() returns false the |mCodecList| is not > used at all and the iteration goes with the static methods of MediaCodecList > class? > > Correct, that's what should be happening unless I've missed something here. ah... I messed MediaCodecList with mCodecList in the calls below. Forget what I've said above https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:81: int codec_count = codecListHelper.getCodecCount(); java uses camelcase for variable name https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:85: int codec_direction = info.isEncoder() ? MEDIA_CODEC_ENCODER : MEDIA_CODEC_DECODER; ditto https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:106: int codec_count = codecListHelper.getCodecCount(); ditto https://codereview.chromium.org/1805163002/diff/60001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/medi... media/base/android/media_codec_util.cc:120: if (!codec_name.size()) codec_name.empty()
Description was changed from ========== Cleanup MediaCodecUtil::GetDefaultCodecName(). There's no need to build an entire list of codecs and transport that across JNI when we always want the first one in the list. BUG=none TEST=codec lists match before after on KitKat below and above. ========== to ========== Cleanup MediaCodecUtil::GetDefaultCodecName(). There's no need to build an entire list of codecs and transport that across JNI when we always want the first one in the list. Notably this reduces the cost of GpuChildThread::OnInitialize() by over 400ms on an N5 running Marshmallow (450.208ms vs 39.64ms)! The reduction comes from VideoEncodeAccelerator no longer creating ~3 MediaCodec instances to determine the codec name during the call to VideoEncodeAccelerator::GetSupportedProfiles() during child thread startup. I suspect this will translate into real GPU process startup improvements. BUG=none TEST=codec lists match before after on KitKat below and above. ==========
cc:sievers, piman as an FYI since this might reduce gpu startup time on Android by over 400ms. Do either of you know if we have a UMA recording GPU process startup time? If not it might make sense to send another CL to add a ScopedHistogramTimer to GpuChildThread::OnInitialize(). https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:81: int codec_count = codecListHelper.getCodecCount(); On 2016/03/17 at 04:11:29, qinmin wrote: > java uses camelcase for variable name Done. https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:85: int codec_direction = info.isEncoder() ? MEDIA_CODEC_ENCODER : MEDIA_CODEC_DECODER; On 2016/03/17 at 04:11:28, qinmin wrote: > ditto Done. https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:106: int codec_count = codecListHelper.getCodecCount(); On 2016/03/17 at 04:11:29, qinmin wrote: > ditto Done.
lgtm
lgtm
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@chromium.org Link to the patchset: https://codereview.chromium.org/1805163002/#ps80001 (title: "Comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805163002/80001
Looks like there is a UMA for GPU process start time.... but it's not actually registered with histograms.xml! https://code.google.com/p/chromium/codesearch#search/&q=GPU.GPUProcessLaunchT...
https://codereview.chromium.org/1812073002 to fix the .xml issue.
Description was changed from ========== Cleanup MediaCodecUtil::GetDefaultCodecName(). There's no need to build an entire list of codecs and transport that across JNI when we always want the first one in the list. Notably this reduces the cost of GpuChildThread::OnInitialize() by over 400ms on an N5 running Marshmallow (450.208ms vs 39.64ms)! The reduction comes from VideoEncodeAccelerator no longer creating ~3 MediaCodec instances to determine the codec name during the call to VideoEncodeAccelerator::GetSupportedProfiles() during child thread startup. I suspect this will translate into real GPU process startup improvements. BUG=none TEST=codec lists match before after on KitKat below and above. ========== to ========== Cleanup MediaCodecUtil::GetDefaultCodecName(). There's no need to build an entire list of codecs and transport that across JNI when we always want the first one in the list. Notably this reduces the cost of GpuChildThread::OnInitialize() by over 400ms on an N5 running Marshmallow (450.208ms vs 39.64ms)! The reduction comes from VideoEncodeAccelerator no longer creating ~3 MediaCodec instances to determine the codec name during the call to VideoEncodeAccelerator::GetSupportedProfiles() during child thread startup. I suspect this will translate into real GPU process startup improvements. BUG=594814 TEST=codec lists match before after on KitKat below and above. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup MediaCodecUtil::GetDefaultCodecName(). There's no need to build an entire list of codecs and transport that across JNI when we always want the first one in the list. Notably this reduces the cost of GpuChildThread::OnInitialize() by over 400ms on an N5 running Marshmallow (450.208ms vs 39.64ms)! The reduction comes from VideoEncodeAccelerator no longer creating ~3 MediaCodec instances to determine the codec name during the call to VideoEncodeAccelerator::GetSupportedProfiles() during child thread startup. I suspect this will translate into real GPU process startup improvements. BUG=594814 TEST=codec lists match before after on KitKat below and above. ========== to ========== Cleanup MediaCodecUtil::GetDefaultCodecName(). There's no need to build an entire list of codecs and transport that across JNI when we always want the first one in the list. Notably this reduces the cost of GpuChildThread::OnInitialize() by over 400ms on an N5 running Marshmallow (450.208ms vs 39.64ms)! The reduction comes from VideoEncodeAccelerator no longer creating ~3 MediaCodec instances to determine the codec name during the call to VideoEncodeAccelerator::GetSupportedProfiles() during child thread startup. I suspect this will translate into real GPU process startup improvements. BUG=594814 TEST=codec lists match before after on KitKat below and above. Committed: https://crrev.com/5c1bee20afe6113087fc9a5781b5773a8894bbd4 Cr-Commit-Position: refs/heads/master@{#381805} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5c1bee20afe6113087fc9a5781b5773a8894bbd4 Cr-Commit-Position: refs/heads/master@{#381805} |