Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2927893002:
Remove FORMAT_RGB from gfx::PngCodec (Closed)
DescriptionRemove FORMAT_RGB from gfx::PngCodec
This is only used in test code.
BUG=724616
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2927893002
Cr-Original-Commit-Position: refs/heads/master@{#479124}
Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201930221ddbe8a
Review-Url: https://codereview.chromium.org/2927893002
Cr-Commit-Position: refs/heads/master@{#480024}
Committed: https://chromium.googlesource.com/chromium/src/+/64759b2876675ce91e2049cc2658b8c8068ec6c8
Patch Set 1 #Patch Set 2 : Fix win compile errors #
Total comments: 4
Patch Set 3 : Response to comments #
Total comments: 8
Patch Set 4 : Use appropriate pointer in test #Patch Set 5 : Preserve md5 value #Patch Set 6 : Compile fixes #
Messages
Total messages: 51 (36 generated)
Description was changed from ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 ========== to ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
The CQ bit was checked by msarett@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by msarett@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by msarett@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.
msarett@chromium.org changed reviewers: + dcheng@chromium.org, sandersd@chromium.org, scroggo@chromium.org
dcheng: ui/gfx sandersd: media/gpu
media/ lgtm
dcheng@, scroggo@, please take a look.
lgtm https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... File media/gpu/rendering_helper.cc (right): https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... media/gpu/rendering_helper.cc:734: solid = solid && ((*rgba)[4 * i + 3] == 0xff); Once solid is false, there's no need to continue this loop, right? Taking a step back, this method is only called in one place, this method isn't doing anything the client couldn't do themselves to detect alpha, and it's not doing it along the way (e.g. it used to detect alpha while it iterated through the pixels and copied the other components). Why not move this check into the client code? https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... File media/gpu/rendering_helper.h (right): https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... media/gpu/rendering_helper.h:142: // Get rendered thumbnails as RGB. RGB -> RGBA
The CQ bit was checked by msarett@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/2927893002/diff/20001/media/gpu/rendering_hel... File media/gpu/rendering_helper.cc (right): https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... media/gpu/rendering_helper.cc:734: solid = solid && ((*rgba)[4 * i + 3] == 0xff); On 2017/06/13 14:54:00, scroggo_chromium wrote: > Once solid is false, there's no need to continue this loop, right? > > Taking a step back, this method is only called in one place, this method isn't > doing anything the client couldn't do themselves to detect alpha, and it's not > doing it along the way (e.g. it used to detect alpha while it iterated through > the pixels and copied the other components). Why not move this check into the > client code? sgtm, done. https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... File media/gpu/rendering_helper.h (right): https://codereview.chromium.org/2927893002/diff/20001/media/gpu/rendering_hel... media/gpu/rendering_helper.h:142: // Get rendered thumbnails as RGB. On 2017/06/13 14:54:00, scroggo_chromium wrote: > RGB -> RGBA Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by msarett@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...
lgtm
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 msarett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2927893002/#ps40001 (title: "Response to comments")
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": 40001, "attempt_start_ts": 1497384775627900,
"parent_rev": "0901195b98d8c46db5a0d164e27a896345dc943c", "commit_rev":
"a9cbb594d5b28bdc51b7b505d201930221ddbe8a"}
Message was sent while issue was closed.
Description was changed from ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2927893002 Cr-Commit-Position: refs/heads/master@{#479124} Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2943433002/ by owenlin@chromium.org. The reason for reverting is: Break the VDA unittest, see http://crbug.com/733522..
Message was sent while issue was closed.
owenlin@chromium.org changed reviewers: + owenlin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2927893002/diff/40001/media/gpu/rendering_hel... File media/gpu/rendering_helper.cc (right): https://codereview.chromium.org/2927893002/diff/40001/media/gpu/rendering_hel... media/gpu/rendering_helper.cc:726: &rgba[0]); &(*rgba)[0] https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1546: base::StringPiece(reinterpret_cast<char*>(&rgba[0]), rgba.size())); This will changes the md5 checksum. You need update the golden md5s in that case.
Message was sent while issue was closed.
Description was changed from ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2927893002 Cr-Commit-Position: refs/heads/master@{#479124} Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201... ========== to ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2927893002 Cr-Commit-Position: refs/heads/master@{#479124} Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201... ==========
https://codereview.chromium.org/2927893002/diff/40001/media/gpu/rendering_hel... File media/gpu/rendering_helper.cc (right): https://codereview.chromium.org/2927893002/diff/40001/media/gpu/rendering_hel... media/gpu/rendering_helper.cc:726: &rgba[0]); On 2017/06/15 07:28:05, Owen Lin wrote: > &(*rgba)[0] Done. https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1546: base::StringPiece(reinterpret_cast<char*>(&rgba[0]), rgba.size())); On 2017/06/15 07:28:05, Owen Lin wrote: > This will changes the md5 checksum. You need update the golden md5s in that > case. Can you help with how to do this? I am able to build and run the tests from a Windows checkout of chromium. But they fail for me at ToT, it looks like I am missing test resources. Note: Google Test filter = ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ResourceExhaustion/VideoDecodeAcceleratorParamTest [ RUN ] ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 [432:8612:0615/140309.549:16680359:FATAL:video_decode_accelerator_unittest.cc(1261)] Assert failed: base::ReadFileToString(GetTestDataFile(filepath), &video_file->data_str). test_video_file: test-25fps.h264
https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1541: EXPECT_EQ(rgba[4 * i + 3], 0xff) << "RGBA frame had incorrect alpha"; If the values of alpha are wrong, this may leave tons of logs. https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1546: base::StringPiece(reinterpret_cast<char*>(&rgba[0]), rgba.size())); On 2017/06/15 18:06:44, msarett wrote: > On 2017/06/15 07:28:05, Owen Lin wrote: > > This will changes the md5 checksum. You need update the golden md5s in that > > case. > > Can you help with how to do this? I am able to build and run the tests from a > Windows checkout of chromium. But they fail for me at ToT, it looks like I am > missing test resources. > > Note: Google Test filter = > ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from ResourceExhaustion/VideoDecodeAcceleratorParamTest > [ RUN ] > ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 > [432:8612:0615/140309.549:16680359:FATAL:video_decode_accelerator_unittest.cc(1261)] > Assert failed: base::ReadFileToString(GetTestDataFile(filepath), > &video_file->data_str). test_video_file: test-25fps.h264 The test data stream and golden md5 golden value are in media/test/data/, for example test-25fps.h264(.md5). But I would really suggest not to change the way of calculating md5s. Please see test-25fps.h264, we will need to run the test on many different boards again to get the golden values. I would suggest to have a function to convert RGBA and RGB here to make sure the md5 is not changed.
https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1541: EXPECT_EQ(rgba[4 * i + 3], 0xff) << "RGBA frame had incorrect alpha"; On 2017/06/16 01:24:05, Owen Lin wrote: > If the values of alpha are wrong, this may leave tons of logs. FIxed. https://codereview.chromium.org/2927893002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1546: base::StringPiece(reinterpret_cast<char*>(&rgba[0]), rgba.size())); On 2017/06/16 01:24:05, Owen Lin wrote: > On 2017/06/15 18:06:44, msarett wrote: > > On 2017/06/15 07:28:05, Owen Lin wrote: > > > This will changes the md5 checksum. You need update the golden md5s in that > > > case. > > > > Can you help with how to do this? I am able to build and run the tests from a > > Windows checkout of chromium. But they fail for me at ToT, it looks like I am > > missing test resources. > > > > Note: Google Test filter = > > ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 > > [==========] Running 1 test from 1 test case. > > [----------] Global test environment set-up. > > [----------] 1 test from ResourceExhaustion/VideoDecodeAcceleratorParamTest > > [ RUN ] > > ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 > > > [432:8612:0615/140309.549:16680359:FATAL:video_decode_accelerator_unittest.cc(1261)] > > Assert failed: base::ReadFileToString(GetTestDataFile(filepath), > > &video_file->data_str). test_video_file: test-25fps.h264 > > The test data stream and golden md5 golden value are in media/test/data/, for > example test-25fps.h264(.md5). > > But I would really suggest not to change the way of calculating md5s. Please see > test-25fps.h264, we will need to run the test on many different boards again to > get the golden values. > > I would suggest to have a function to convert RGBA and RGB here to make sure the > md5 is not changed. > > Ahh yes, that's a much better idea. Done. I think this should work now!
The CQ bit was checked by msarett@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 msarett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, dcheng@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2927893002/#ps100001 (title: "Compile fixes")
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": 100001, "attempt_start_ts": 1497615355534050,
"parent_rev": "c8d2a63048345a021592fa0afe5211f12bfffe82", "commit_rev":
"64759b2876675ce91e2049cc2658b8c8068ec6c8"}
Message was sent while issue was closed.
Description was changed from ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2927893002 Cr-Commit-Position: refs/heads/master@{#479124} Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201... ========== to ========== Remove FORMAT_RGB from gfx::PngCodec This is only used in test code. BUG=724616 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2927893002 Cr-Original-Commit-Position: refs/heads/master@{#479124} Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201... Review-Url: https://codereview.chromium.org/2927893002 Cr-Commit-Position: refs/heads/master@{#480024} Committed: https://chromium.googlesource.com/chromium/src/+/64759b2876675ce91e2049cc2658... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/64759b2876675ce91e2049cc2658... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
