Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(25)

Issue 1805163002: Cleanup MediaCodecUtil::GetDefaultCodecName(). (Closed)

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.

Description

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}

Patch Set 1 : Cleanup. #

Total comments: 6

Patch Set 2 : Switch to faster getCodecInfos. Comments. #

Total comments: 14

Patch Set 3 : Comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -228 lines) Patch
M media/base/android/java/src/org/chromium/media/MediaCodecUtil.java View 1 2 6 chunks +49 lines, -129 lines 0 comments Download
M media/base/android/media_codec_util.cc View 1 4 chunks +17 lines, -99 lines 0 comments Download

Messages

Total messages: 38 (10 generated)
DaleCurtis
4 years, 9 months ago (2016-03-15 20:47:46 UTC) #3
DaleCurtis
WDYT about switching to use MediaCodecList::getCodecInfos() instead of actually creating a MediaCodec to determine the ...
4 years, 9 months ago (2016-03-15 20:50:02 UTC) #4
DaleCurtis
Timing wise getName() + creation + teardown seems to take ~70-180ms.
4 years, 9 months ago (2016-03-15 21:20:43 UTC) #5
DaleCurtis
GetCodecInfos / iterating is much faster (45 ms first call, 0ms all subsequent calls, vs ...
4 years, 9 months ago (2016-03-15 21:33:58 UTC) #6
Tima Vaisburd
https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/20001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode58 media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:58: Log.w(TAG, "getDefaultCodecName: Failed to create MediaCodec: %s, direction: %d", ...
4 years, 9 months ago (2016-03-15 21:38:17 UTC) #8
Tima Vaisburd
On 2016/03/15 20:50:02, DaleCurtis wrote: > WDYT about switching to use MediaCodecList::getCodecInfos() instead of actually ...
4 years, 9 months ago (2016-03-15 21:39:59 UTC) #9
qinmin
On 2016/03/15 21:39:59, Tima Vaisburd wrote: > On 2016/03/15 20:50:02, DaleCurtis wrote: > > WDYT ...
4 years, 9 months ago (2016-03-15 22:58:35 UTC) #11
DaleCurtis
Latest patch set switches to getCodecInfo(). I don't think it matters that there might be ...
4 years, 9 months ago (2016-03-15 23:02:45 UTC) #12
Tima Vaisburd
On 2016/03/15 23:02:45, DaleCurtis wrote: timav> Can it be several codecs for the same mime ...
4 years, 9 months ago (2016-03-15 23:32:51 UTC) #13
DaleCurtis
+glaznev who added a lot of this code in https://codereview.chromium.org/605883002
4 years, 9 months ago (2016-03-15 23:36:19 UTC) #15
DaleCurtis
Ping glaznev@. I've spent some time looking over the original bugs and it's not clear ...
4 years, 9 months ago (2016-03-16 22:16:57 UTC) #16
AlexGlaznev
lgtm
4 years, 9 months ago (2016-03-16 23:52:02 UTC) #17
qinmin
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode46 media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:46: @TargetApi(Build.VERSION_CODES.LOLLIPOP) This changes the supported android versions, we still ...
4 years, 9 months ago (2016-03-17 00:03:38 UTC) #18
DaleCurtis
glaznev@ and I chatted offline. The root issue the original CLs were trying to fix ...
4 years, 9 months ago (2016-03-17 00:05:27 UTC) #19
DaleCurtis
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode46 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 ...
4 years, 9 months ago (2016-03-17 00:06:34 UTC) #20
qinmin
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode46 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 ...
4 years, 9 months ago (2016-03-17 00:13:12 UTC) #21
DaleCurtis
We don't have software h264 decoders, so it doesn't matter if h264 is known unaccelerated. ...
4 years, 9 months ago (2016-03-17 00:18:42 UTC) #22
Tima Vaisburd
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode46 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 ...
4 years, 9 months ago (2016-03-17 00:20:37 UTC) #23
DaleCurtis
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode46 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: > ...
4 years, 9 months ago (2016-03-17 02:12:55 UTC) #24
qinmin
https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/1805163002/diff/60001/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java#newcode46 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 ...
4 years, 9 months ago (2016-03-17 04:11:29 UTC) #25
DaleCurtis
cc:sievers, piman as an FYI since this might reduce gpu startup time on Android by ...
4 years, 9 months ago (2016-03-17 20:32:25 UTC) #27
qinmin
lgtm
4 years, 9 months ago (2016-03-17 20:39:19 UTC) #28
Tima Vaisburd
lgtm
4 years, 9 months ago (2016-03-17 20:40:45 UTC) #29
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-17 20:41:56 UTC) #32
DaleCurtis
Looks like there is a UMA for GPU process start time.... but it's not actually ...
4 years, 9 months ago (2016-03-17 20:50:16 UTC) #33
DaleCurtis
https://codereview.chromium.org/1812073002 to fix the .xml issue.
4 years, 9 months ago (2016-03-17 20:56:43 UTC) #34
commit-bot: I haz the power
Committed patchset #3 (id:80001)
4 years, 9 months ago (2016-03-17 22:03:44 UTC) #36
commit-bot: I haz the power
4 years, 9 months ago (2016-03-17 22:04:59 UTC) #38
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/5c1bee20afe6113087fc9a5781b5773a8894bbd4
Cr-Commit-Position: refs/heads/master@{#381805}

Powered by Google App Engine
This is Rietveld 408576698