Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686963002:
Fix and update MediaCodec blacklist. (Closed)
|
Created:
3 years, 10 months ago by liberato (no reviews please) Modified:
3 years, 10 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mlamouri+watch-media_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix and update MediaCodec blacklist.
Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots
of blacklist entries. Also add an entry for LGMS330, and a unit
test for it.
Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well
via software rendering list.
BUG=654905
TEST=media_codec_util_unittest
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
Review-Url: https://codereview.chromium.org/2686963002
Cr-Commit-Position: refs/heads/master@{#450169}
Committed: https://chromium.googlesource.com/chromium/src/+/aee34d71dd1d23104cf16ae86bd3756f644b10f2
Patch Set 1 #
Total comments: 4
Patch Set 2 : when i left you, i was but the test. now i am the unit. #
Total comments: 4
Patch Set 3 : only a mapper of strings, media_codec_util. #Patch Set 4 : cleanup #Patch Set 5 : cleanup #
Total comments: 4
Patch Set 6 : moved model_for_test into IsMediaCodecAvailable #
Total comments: 2
Patch Set 7 : map => vector #Patch Set 8 : added E-TAB4 #
Total comments: 10
Patch Set 9 : removed E-TAB4 based on dremel #Patch Set 10 : cl feedback #Patch Set 11 : rebased. #
Messages
Total messages: 50 (32 generated)
Description was changed from ========== Fix and update MediaCodec blacklist. Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots of blacklist entries. Also add an entry for LGMS330, and a unit test for it. Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well via software rendering list. BUG=654905 TEST=media_codec_util_unittest ========== to ========== Fix and update MediaCodec blacklist. Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots of blacklist entries. Also add an entry for LGMS330, and a unit test for it. Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well via software rendering list. BUG=654905 TEST=media_codec_util_unittest 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 ==========
Description was changed from ========== Fix and update MediaCodec blacklist. Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots of blacklist entries. Also add an entry for LGMS330, and a unit test for it. Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well via software rendering list. BUG=654905 TEST=media_codec_util_unittest 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 ========== Fix and update MediaCodec blacklist. Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots of blacklist entries. Also add an entry for LGMS330, and a unit test for it. Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well via software rendering list. BUG=654905 TEST=media_codec_util_unittest 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 ==========
liberato@chromium.org changed reviewers: + crouleau@chromium.org, dalecurtis@chromium.org
The CQ bit was checked by liberato@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...
now with unit test. i didn't try to fiddle with BuildInfo directly for the test, to limit the scope and maybe merge back. thanks -fl
LGTM Thanks for adding tests! https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:133: model != "GT-I9100" && model != "GT-I9300" && model != "GT-N7000" && Nit: Prefer ordering the operations like: J K L or L K J to keep things in alphabetical order.
watk@chromium.org changed reviewers: + watk@chromium.org
https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util_unittest.cc (right): https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... media/base/android/media_codec_util_unittest.cc:70: {"always_works", 0}, // Some codec that works everywhere. Did you consider changing the blacklist implementation to do a lookup in an array or map like this? Then we don't need much of a test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util_unittest.cc (right): https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... media/base/android/media_codec_util_unittest.cc:70: {"always_works", 0}, // Some codec that works everywhere. On 2017/02/09 00:44:59, watk wrote: > Did you consider changing the blacklist implementation to do a lookup in an > array or map like this? Then we don't need much of a test. yeah, after seeing the test it dimly crossed my mind that the test structure was better than the thing it was testing :) one one hand, i like the simplicity of the current code. on the other, that's also exactly why it didn't work before. i'll give it a try and see what it ends up looking like.
The CQ bit was checked by liberato@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...
Like the patch set message :) https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:133: static std::unordered_map<base::StringPiece, int, base::StringPieceHash> static const. https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util_unittest.cc (right): https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... media/base/android/media_codec_util_unittest.cc:11: /* Cleanup?
The CQ bit was checked by liberato@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...
https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:133: model != "GT-I9100" && model != "GT-I9300" && model != "GT-N7000" && On 2017/02/09 00:12:09, crouleau wrote: > Nit: Prefer ordering the operations like: > > J > K > L > > or > > L > K > J > > to keep things in alphabetical order. the (new) map is initialized in order. https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:133: static std::unordered_map<base::StringPiece, int, base::StringPieceHash> On 2017/02/09 18:31:14, DaleCurtis wrote: > static const. Done. https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util_unittest.cc (right): https://codereview.chromium.org/2686963002/diff/20001/media/base/android/medi... media/base/android/media_codec_util_unittest.cc:11: /* On 2017/02/09 18:31:14, DaleCurtis wrote: > Cleanup? oops, thanks.
LGTM https://codereview.chromium.org/2686963002/diff/80001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/80001/media/base/android/medi... media/base/android/media_codec_util.cc:118: const char* model_for_test) { offline comment https://codereview.chromium.org/2686963002/diff/80001/media/base/android/medi... media/base/android/media_codec_util.cc:131: // ["model name"] == las bad revision. We will blacklist the model on any sdk last
The CQ bit was checked by liberato@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...
crouleau: thanks for the offline discussion. -fl https://codereview.chromium.org/2686963002/diff/80001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/80001/media/base/android/medi... media/base/android/media_codec_util.cc:118: const char* model_for_test) { On 2017/02/09 19:01:57, crouleau wrote: > offline comment Done. https://codereview.chromium.org/2686963002/diff/80001/media/base/android/medi... media/base/android/media_codec_util.cc:131: // ["model name"] == las bad revision. We will blacklist the model on any sdk On 2017/02/09 19:01:57, crouleau wrote: > last Done.
lgtm % optional change. https://codereview.chromium.org/2686963002/diff/100001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/100001/media/base/android/med... media/base/android/media_codec_util.cc:133: static const std::unordered_map<base::StringPiece, int, base::StringPieceHash> For small maps like this it's better to use a std::vector() and std::find(), it's more performant and memory efficient under all cases at small sizes. https://codereview.chromium.org/2686963002/diff/100001/media/base/android/med... media/base/android/media_codec_util.cc:153: {"e-tab4", SDK_VERSION_JELLY_BEAN_MR2}, Should model always be uppercased?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
liberato@chromium.org changed reviewers: + aelias@chromium.org
The CQ bit was checked by liberato@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...
aelias: PTAL @ software_rendering_list_json.cc dale: i added E-TAB4 to be safe. i think that the original was lowercase, though. thanks -fl
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/10 at 18:27:00, liberato wrote: > aelias: PTAL @ software_rendering_list_json.cc > > dale: i added E-TAB4 to be safe. i think that the original was lowercase, though. Err, why not just always uppercase the Build.MODEL? > > thanks > -fl
gpu/config/ lgtm https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:128: if (sdk >= base::android::SDK_VERSION_MARSHMALLOW) This line doesn't do anything and it might cause a bug someday when something fails on M, please delete it.
lgtm with nites https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:118: bool MediaCodecUtil::IsMediaCodecAvailableForSdk(int sdk, const char* model) { nit: Either make it "ForDevice" or remove the suffix and make it an overload. Since it's not just the sdk you're passing in. https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:125: // Blacklist LGMS330 on L for crbug.com/654905 . Why don't we just intersperse the crbug # comments with the blacklist entries below. Like: {"GT-N7100", SDK_VERSION_KITKAT}, // http://crbug.com/628059 {"A6600", SDK_VERSION_KITKAT}, {"A6800", SDK_VERSION_KITKAT}, https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:131: // ["model name"] == last bad revision. We will blacklist the model on any "["model name"] == last bad revision." no longer relevant because it's not a map. Fine to just delete. https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... File media/base/android/media_codec_util.h (right): https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.h:50: // |model| as the model. This is provided primarily for unit tests; you nit: Remove "primarily". Makes it sound like there's another reason to use it, which I can't think of.
The CQ bit was checked by liberato@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 for the feedback. uppercase: i verified with dremel that it's actually "e-tab4". didn't want to uppercase the inputs since seems like a lot of work that essentially never changes the outcome. similarly, it didn't seem worth it to re-implement the StringPiece comparison code that we've got to ignore case. so, i left it as "e-tab4" only. thanks -fl https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:118: bool MediaCodecUtil::IsMediaCodecAvailableForSdk(int sdk, const char* model) { On 2017/02/10 22:14:13, watk wrote: > nit: Either make it "ForDevice" or remove the suffix and make it an overload. > Since it's not just the sdk you're passing in. true, but it's not just the device, either. :) i don't like overloads for cases like this, so i'll rename it to 'Is..For()'. it reads kinda nice IsMediaCodecAvailableFor(LOLLIPOP, "e-tab4"). https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:125: // Blacklist LGMS330 on L for crbug.com/654905 . On 2017/02/10 22:14:13, watk wrote: > Why don't we just intersperse the crbug # comments with the blacklist entries > below. Like: > > {"GT-N7100", SDK_VERSION_KITKAT}, > // http://crbug.com/628059 > {"A6600", SDK_VERSION_KITKAT}, > {"A6800", SDK_VERSION_KITKAT}, Done. https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:128: if (sdk >= base::android::SDK_VERSION_MARSHMALLOW) On 2017/02/10 21:57:46, aelias wrote: > This line doesn't do anything and it might cause a bug someday when something > fails on M, please delete it. Done. https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.cc:131: // ["model name"] == last bad revision. We will blacklist the model on any On 2017/02/10 22:14:13, watk wrote: > "["model name"] == last bad revision." no longer relevant because it's not a > map. Fine to just delete. Done. https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... File media/base/android/media_codec_util.h (right): https://codereview.chromium.org/2686963002/diff/140001/media/base/android/med... media/base/android/media_codec_util.h:50: // |model| as the model. This is provided primarily for unit tests; you On 2017/02/10 22:14:13, watk wrote: > nit: Remove "primarily". Makes it sound like there's another reason to use it, > which I can't think of. Done.
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from crouleau@chromium.org, dalecurtis@chromium.org, watk@chromium.org, aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2686963002/#ps180001 (title: "cl feedback")
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...)
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, aelias@chromium.org, crouleau@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2686963002/#ps200001 (title: "rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487024464801720,
"parent_rev": "09c1c7e0c3dff51be0c613a8163ab31f19f4a7a6", "commit_rev":
"aee34d71dd1d23104cf16ae86bd3756f644b10f2"}
Message was sent while issue was closed.
Description was changed from ========== Fix and update MediaCodec blacklist. Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots of blacklist entries. Also add an entry for LGMS330, and a unit test for it. Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well via software rendering list. BUG=654905 TEST=media_codec_util_unittest 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 ========== Fix and update MediaCodec blacklist. Fix MediaCodecUtil::IsMediaCodecAvailable, which was skipping lots of blacklist entries. Also add an entry for LGMS330, and a unit test for it. Blacklist Broadcom VideoCore IV GPU from using MediaCodec as well via software rendering list. BUG=654905 TEST=media_codec_util_unittest 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 Review-Url: https://codereview.chromium.org/2686963002 Cr-Commit-Position: refs/heads/master@{#450169} Committed: https://chromium.googlesource.com/chromium/src/+/aee34d71dd1d23104cf16ae86bd3... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/aee34d71dd1d23104cf16ae86bd3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
