Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506363002:
vda_unittest: Add a new test case to verify the visible size. (Closed)
Descriptionvda_unittest: Add a new test case to verify the visible size.
Add a new test case to verify the visible rectangles returned
in PictureReady() are correct. It could be an empty rectangle if
the information is not available from the VDA.
BUG=658654
Test=Run and pass the vda_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
Patch Set 1 #
Total comments: 12
Messages
Total messages: 13 (5 generated)
Description was changed from ========== vda_unittest: Add a new test case to verify the visible size. Add a new test case to verify the visible rectangles returned in PictureReady() are correct. It could be an empty rectangle if the information is not available from the VDA. BUG=658654 Test=Run and pass the vda_unittest ========== to ========== vda_unittest: Add a new test case to verify the visible size. Add a new test case to verify the visible rectangles returned in PictureReady() are correct. It could be an empty rectangle if the information is not available from the VDA. BUG=658654 Test=Run and pass the vda_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 ==========
owenlin@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
PTAL. Thanks.
wuchengli@chromium.org changed reviewers: + kcwu@chromium.org
Kuang-che. Please help review this.
https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:185: struct VisibleRectEntry { Please add a short sentence describing what this is, and also what count means. https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:188: VisibleRectEntry(const gfx::Rect rect, int count) const& https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:747: visible_rects_.push_back(VisibleRectEntry(picture.visible_rect(), 1)); Perhaps just have count=1 as default when constructing? Up to you. https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1730: // This test Logs the visible rectangles of the picture returned from s/Logs/verifies/ https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1751: CreateAndStartDecoder(client, note); I wonder if we need an additional test case just for this, we could maybe just verify this in SimpleDecode, otherwise we are making the automated tests run longer... https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1759: LOG(INFO) << output_string; Perhaps just DVLOG? https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1768: ASSERT_TRUE(entry.rect.IsEmpty() || (entry.rect == expected_rect)); I think we could just EXPECT?
https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1757: base::StringPrintf("(%d, %d, %d, %d) %d", r.x(), r.y(), r.width(), How about LOG(INFO) << entry.rect << " " << entry.count; https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1768: ASSERT_TRUE(entry.rect.IsEmpty() || (entry.rect == expected_rect)); EXPECT_TRUE https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1769: ASSERT_EQ(test_video_files_[0]->num_frames, entry.count); EXPECT_EQ
https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1751: CreateAndStartDecoder(client, note); On 2016/11/17 07:50:01, Pawel Osciak wrote: > I wonder if we need an additional test case just for this, we could maybe just > verify this in SimpleDecode, otherwise we are making the automated tests run > longer... Adding a test case has a benefits that it's clear what is broken. TestDecodeTimeMedian is a performance test and runs very long. We can remove TestDecodeTimeMedian from the default test suite to save some time.
https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1751: CreateAndStartDecoder(client, note); On 2016/11/17 08:23:41, wuchengli wrote: > On 2016/11/17 07:50:01, Pawel Osciak wrote: > > I wonder if we need an additional test case just for this, we could maybe just > > verify this in SimpleDecode, otherwise we are making the automated tests run > > longer... > Adding a test case has a benefits that it's clear what is broken. Yes, but I believe making the test run longer outweights this advantage, and proper logging via EXPECT() should be enough to see what went wrong. > TestDecodeTimeMedian is a performance test and runs very long. We can remove > TestDecodeTimeMedian from the default test suite to save some time. That's an orthogonal issue. The test stream is 10 seconds long. By adding this test, we are making each run of this test 10 seconds longer in the test lab for each run, but we could achieve this by adding this verification to one of the existing test cases, just like we are already verifying multiple things in them, like the number of frames decoded.
On 2016/11/17 08:40:32, Pawel Osciak wrote: > https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... > File media/gpu/video_decode_accelerator_unittest.cc (right): > > https://codereview.chromium.org/2506363002/diff/1/media/gpu/video_decode_acce... > media/gpu/video_decode_accelerator_unittest.cc:1751: > CreateAndStartDecoder(client, note); > On 2016/11/17 08:23:41, wuchengli wrote: > > On 2016/11/17 07:50:01, Pawel Osciak wrote: > > > I wonder if we need an additional test case just for this, we could maybe > just > > > verify this in SimpleDecode, otherwise we are making the automated tests run > > > longer... > > Adding a test case has a benefits that it's clear what is broken. > > Yes, but I believe making the test run longer outweights this advantage, and > proper logging via EXPECT() should be enough to see what went wrong. > > > TestDecodeTimeMedian is a performance test and runs very long. We can remove > > TestDecodeTimeMedian from the default test suite to save some time. > > That's an orthogonal issue. The test stream is 10 seconds long. By adding this > test, we are making each run of this test 10 seconds longer in the test lab for > each run, but we could achieve this by adding this verification to one of the > existing test cases, just like we are already verifying multiple things in them, > like the number of frames decoded. I agree with you. But we don't need to render the video frames, so it only took about 1 seconds to decode on ELM.
Description was changed from ========== vda_unittest: Add a new test case to verify the visible size. Add a new test case to verify the visible rectangles returned in PictureReady() are correct. It could be an empty rectangle if the information is not available from the VDA. BUG=658654 Test=Run and pass the vda_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 ========== vda_unittest: Add a new test case to verify the visible size. Add a new test case to verify the visible rectangles returned in PictureReady() are correct. It could be an empty rectangle if the information is not available from the VDA. BUG=658654 Test=Run and pass the vda_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 ==========
wuchengli@chromium.org changed reviewers: - wuchengli@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
