|
|
Chromium Code Reviews
DescriptionAdd AVDA unittests to media_unittests
The AVDA unit tests were previously only included in the
video_decode_accelerator_unittest target, which isn't run automatically.
Now they're in a seperate source_set that's also included in
media_unittests so they'll be run automatically with the rest of our
tests.
BUG=642972
Committed: https://crrev.com/722f30f115b2ba519d003cdeac6ba75c7ac1a8e5
Cr-Commit-Position: refs/heads/master@{#430107}
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments #Patch Set 3 : make gn check pass #
Total comments: 2
Patch Set 4 : move it #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Add AVDA unittests to media_unittests The AVDA unit tests were previously only included in the video_decode_accelerator_unittest target, which isn't run automatically. Now they're in a seperate source_set that's also included in media_unittests so they'll be run automatically with the rest of our tests. ========== to ========== Add AVDA unittests to media_unittests The AVDA unit tests were previously only included in the video_decode_accelerator_unittest target, which isn't run automatically. Now they're in a seperate source_set that's also included in media_unittests so they'll be run automatically with the rest of our tests. BUG=642972 ==========
watk@chromium.org changed reviewers: + xhwang@chromium.org
PTAL
looking good. just have a question. https://codereview.chromium.org/2481563002/diff/1/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/BUILD.gn#newcode372 media/gpu/BUILD.gn:372: test("video_decode_accelerator_unittest") { Is this running on bots today? If not, I wonder why... https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator_unittest.cc:110: ASSERT_TRUE(Initialize(H264PROFILE_BASELINE)); Is it valuable to still cover VP8?
https://codereview.chromium.org/2481563002/diff/1/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/BUILD.gn#newcode372 media/gpu/BUILD.gn:372: test("video_decode_accelerator_unittest") { On 2016/11/04 22:20:44, xhwang wrote: > Is this running on bots today? If not, I wonder why... No :( I broke it the other day on ChromeOS. I'm guessing no one has prioritized it. I think it would be a good idea. Especially now that jbauman added DXVA support. https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator_unittest.cc:110: ASSERT_TRUE(Initialize(H264PROFILE_BASELINE)); On 2016/11/04 22:20:44, xhwang wrote: > Is it valuable to still cover VP8? Forgot to comment on this. The reason I changed it is that it's not true that AVDA should always succeed to initialize with VP8 any more. If the device doesn't have a HW accelerated VP8 MediaCodec we reject initialiation so we can use libvpx. H264 is always accepted though.
LGTM % nit https://codereview.chromium.org/2481563002/diff/1/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/BUILD.gn#newcode372 media/gpu/BUILD.gn:372: test("video_decode_accelerator_unittest") { On 2016/11/04 22:25:45, watk wrote: > On 2016/11/04 22:20:44, xhwang wrote: > > Is this running on bots today? If not, I wonder why... > > No :( I broke it the other day on ChromeOS. I'm guessing no one has prioritized > it. I think it would be a good idea. Especially now that jbauman added DXVA > support. Makes sense. Could you please file a bug and add a TODO here? https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator_unittest.cc:110: ASSERT_TRUE(Initialize(H264PROFILE_BASELINE)); On 2016/11/04 22:25:45, watk wrote: > On 2016/11/04 22:20:44, xhwang wrote: > > Is it valuable to still cover VP8? > > Forgot to comment on this. The reason I changed it is that it's not true that > AVDA should always succeed to initialize with VP8 any more. If the device > doesn't have a HW accelerated VP8 MediaCodec we reject initialiation so we can > use libvpx. > > H264 is always accepted though. Thanks! Could you please add a comment here about VP8 since "vp8" in general should be supported on all devices.
The CQ bit was checked by watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2481563002/#ps20001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2481563002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator_unittest.cc:110: ASSERT_TRUE(Initialize(H264PROFILE_BASELINE)); On 2016/11/04 22:42:36, xhwang wrote: > On 2016/11/04 22:25:45, watk wrote: > > On 2016/11/04 22:20:44, xhwang wrote: > > > Is it valuable to still cover VP8? > > > > Forgot to comment on this. The reason I changed it is that it's not true that > > AVDA should always succeed to initialize with VP8 any more. If the device > > doesn't have a HW accelerated VP8 MediaCodec we reject initialiation so we can > > use libvpx. > > > > H264 is always accepted though. > > Thanks! Could you please add a comment here about VP8 since "vp8" in general > should be supported on all devices. Since it's not strange for VDAs to reject codecs in certain cases, the interesting fact here is that h264 is always supported, so I wrote that. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2481563002/diff/40001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2481563002/diff/40001/media/BUILD.gn#newcode718 media/BUILD.gn:718: "//media/gpu:android_video_decode_accelerator_unittests", Whether I put this here or inside "if (is_android)", it's the same thing. Do you have a preference? It's kind of surprising to see it here because it has android in the name I guess.
https://codereview.chromium.org/2481563002/diff/40001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2481563002/diff/40001/media/BUILD.gn#newcode718 media/BUILD.gn:718: "//media/gpu:android_video_decode_accelerator_unittests", On 2016/11/04 23:12:31, watk wrote: > Whether I put this here or inside "if (is_android)", it's the same thing. Do you > have a preference? It's kind of surprising to see it here because it has android > in the name I guess. yes, put it in is_android seems more logical.
On 2016/11/04 23:14:20, xhwang wrote: > https://codereview.chromium.org/2481563002/diff/40001/media/BUILD.gn > File media/BUILD.gn (right): > > https://codereview.chromium.org/2481563002/diff/40001/media/BUILD.gn#newcode718 > media/BUILD.gn:718: "//media/gpu:android_video_decode_accelerator_unittests", > On 2016/11/04 23:12:31, watk wrote: > > Whether I put this here or inside "if (is_android)", it's the same thing. Do > you > > have a preference? It's kind of surprising to see it here because it has > android > > in the name I guess. > > yes, put it in is_android seems more logical. Thanks, not sure why I uploaded it when I already knew that I should have put it there :P
The CQ bit was checked by watk@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 watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2481563002/#ps60001 (title: "move it")
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 ========== Add AVDA unittests to media_unittests The AVDA unit tests were previously only included in the video_decode_accelerator_unittest target, which isn't run automatically. Now they're in a seperate source_set that's also included in media_unittests so they'll be run automatically with the rest of our tests. BUG=642972 ========== to ========== Add AVDA unittests to media_unittests The AVDA unit tests were previously only included in the video_decode_accelerator_unittest target, which isn't run automatically. Now they're in a seperate source_set that's also included in media_unittests so they'll be run automatically with the rest of our tests. BUG=642972 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add AVDA unittests to media_unittests The AVDA unit tests were previously only included in the video_decode_accelerator_unittest target, which isn't run automatically. Now they're in a seperate source_set that's also included in media_unittests so they'll be run automatically with the rest of our tests. BUG=642972 ========== to ========== Add AVDA unittests to media_unittests The AVDA unit tests were previously only included in the video_decode_accelerator_unittest target, which isn't run automatically. Now they're in a seperate source_set that's also included in media_unittests so they'll be run automatically with the rest of our tests. BUG=642972 Committed: https://crrev.com/722f30f115b2ba519d003cdeac6ba75c7ac1a8e5 Cr-Commit-Position: refs/heads/master@{#430107} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/722f30f115b2ba519d003cdeac6ba75c7ac1a8e5 Cr-Commit-Position: refs/heads/master@{#430107} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
