Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104883005:
VEA unit test: Add timestamp test (Closed)
|
Created:
4 years, 5 months ago by shenghao Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVEA unit test: Add timestamp test
Verify that one input timestamps match at least one output timestamp.
BUG=620564
TEST=Verify that the test pass on minnie
Committed: https://crrev.com/d701f00d4b691011bb7a019ea3a0e2fbd65c0611
Cr-Commit-Position: refs/heads/master@{#404806}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 7
Patch Set 3 : Address Wucheng's comments #
Total comments: 4
Patch Set 4 : Removed rewind check #
Total comments: 8
Patch Set 5 : Change starting timestamp #Patch Set 6 : change commit message #
Total comments: 2
Patch Set 7 : change test parameter #Messages
Total messages: 35 (14 generated)
Description was changed from ========== VEA unit test: Add back timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ========== to ========== VEA unit test: Add back timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ==========
shenghao@google.com changed reviewers: + dalecurtis@chromium.org, posciak@chromium.org, wuchengli@chromium.org
shenghao@google.com changed reviewers: + shenghao@google.com
shenghao@google.com changed reviewers: - dalecurtis@chromium.org, shenghao@google.com
shenghao@google.com changed reviewers: + shenghao@google.com
Sorry, added Dale Curtis by mistake.
shenghao@google.com changed reviewers: + henryhsu@google.com
henryhsu@chromium.org changed reviewers: + henryhsu@chromium.org
https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1240: CHECK(!frame_timestamps_.empty()); s/CHECK/EXPECT_TRUE/ https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1247: CHECK_EQ(timestamp, frame_timestamps_.front()); s/CHECK_EQ/EXPECT_EQ/ to fail this test case and run the rest test cases. https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1579: // - If true, verify the output frames of encoder. Add comment for the new parameter https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1665: INSTANTIATE_TEST_CASE_P(ForceKeyframes, Is this generated by "git cl format"?
https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1240: CHECK(!frame_timestamps_.empty()); On 2016/07/07 07:44:58, henryhsu wrote: > s/CHECK/EXPECT_TRUE/ Done. https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1247: CHECK_EQ(timestamp, frame_timestamps_.front()); On 2016/07/07 07:44:58, henryhsu wrote: > s/CHECK_EQ/EXPECT_EQ/ to fail this test case and run the rest test cases. Done. https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1579: // - If true, verify the output frames of encoder. On 2016/07/07 07:44:58, henryhsu wrote: > Add comment for the new parameter Done. https://codereview.chromium.org/2104883005/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1665: INSTANTIATE_TEST_CASE_P(ForceKeyframes, On 2016/07/07 07:44:58, henryhsu wrote: > Is this generated by "git cl format"? yes
lgtm
The CQ bit was checked by shenghao@google.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
minnie only supports VP8 encode. Please find a device to test H264 encode with this patch. https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. For change description: s/Add back timestamp test/Add timestamp test/ No need to say we are adding "back" the test. The information won't be useful when we look at the git history in the future. https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1277: if (timestamp != frame_timestamps_.front()) { ASSERT abort the function when they fail. EXPECT do not abort the funciton. This line will crash if frame_timestamps_ is empty. It's easier to make this a function so you can use ASSERT. void VerifyOutputTimestamp(base::TimeDelta timestamp) { ASSERT_TRUE(!frame_timestamps_.empty()); if (timestamp != frame_timestamps_.front()) { frame_timestamps_.pop(); ASSERT_TRUE(!frame_timestamps_.empty()); EXPECT_EQ(timestamp, frame_timestamps_.front()); } } https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1279: EXPECT_EQ(timestamp, frame_timestamps_.front()); The first parameter is expected value and the second is actual value. We should swap the order. Otherwise the error message will be confusing. EXPECT_EQ(zzz, frame_timestamps_.front()); For example, ../../media/gpu/video_encode_accelerator_unittest.cc:nnnn: Failure Value of: frame_timestamps_.front() Actual: yyy Expected: zzz https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1282: Fail the test if the size of frame_timestamps_ is not zero when test finishes. Otherwise, test would pass if the driver return all 0 timestamps.
https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1277: if (timestamp != frame_timestamps_.front()) { On 2016/07/11 05:30:08, wuchengli wrote: > ASSERT abort the function when they fail. EXPECT do not abort the funciton. This > line will crash if frame_timestamps_ is empty. > > It's easier to make this a function so you can use ASSERT. > > void VerifyOutputTimestamp(base::TimeDelta timestamp) { > ASSERT_TRUE(!frame_timestamps_.empty()); > if (timestamp != frame_timestamps_.front()) { > frame_timestamps_.pop(); > ASSERT_TRUE(!frame_timestamps_.empty()); > EXPECT_EQ(timestamp, frame_timestamps_.front()); > } > } Done. https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1279: EXPECT_EQ(timestamp, frame_timestamps_.front()); On 2016/07/11 05:30:08, wuchengli wrote: > The first parameter is expected value and the second is actual value. We should > swap the order. Otherwise the error message will be confusing. > > EXPECT_EQ(zzz, frame_timestamps_.front()); > For example, > ../../media/gpu/video_encode_accelerator_unittest.cc:nnnn: Failure > Value of: frame_timestamps_.front() > Actual: yyy > Expected: zzz Done. https://codereview.chromium.org/2104883005/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1282: On 2016/07/11 05:30:08, wuchengli wrote: > Fail the test if the size of frame_timestamps_ is not zero when test finishes. > Otherwise, test would pass if the driver return all 0 timestamps. Done.
https://codereview.chromium.org/2104883005/diff/40001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1433: frame_timestamps_.push(video_frame->timestamp()); We should always push the timestamps. We can use |next_input_id_| - |num_frames_to_encode_| to know the number of elements remained in |frame_timestamps_| when finish. https://codereview.chromium.org/2104883005/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1530: EXPECT_LE(frame_timestamps_.size(), static_cast<size_t>(1)); The driver will pass if they don't return the last timestamp. Better to have an additional variable to store the previous timestamp. void VEAClient::VerifyOutputTimestamp(base::TimeDelta timestamp) { if (timestamp != previous_timestamp_) { ASSERT_TRUE(!frame_timestamps_.empty()); EXPECT_EQ(frame_timestamps_.front(), timestamp); previous_timestamp_ = frame_timestamps_.top(); frame_timestamps_.pop(); } }
https://codereview.chromium.org/2104883005/diff/40001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1433: frame_timestamps_.push(video_frame->timestamp()); On 2016/07/12 05:56:56, wuchengli wrote: > We should always push the timestamps. We can use |next_input_id_| - > |num_frames_to_encode_| to know the number of elements remained in > |frame_timestamps_| when finish. Done. https://codereview.chromium.org/2104883005/diff/40001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1530: EXPECT_LE(frame_timestamps_.size(), static_cast<size_t>(1)); On 2016/07/12 05:56:56, wuchengli wrote: > The driver will pass if they don't return the last timestamp. Better to have an > additional variable to store the previous timestamp. > > void VEAClient::VerifyOutputTimestamp(base::TimeDelta timestamp) { > if (timestamp != previous_timestamp_) { > ASSERT_TRUE(!frame_timestamps_.empty()); > EXPECT_EQ(frame_timestamps_.front(), timestamp); > previous_timestamp_ = frame_timestamps_.top(); > frame_timestamps_.pop(); > } > } Done.
Description was changed from ========== VEA unit test: Add back timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ========== to ========== VEA unit test: Add timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ==========
https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1266: if (num_encoded_frames_ == 0 || timestamp != previous_timestamp_) { This failed on oak for H264. Did you test any device with H264? The first input buffer produces two outputs buffers. First output buffer is SPS, so ProcessStreamBuffer does not call HandleEncodedFrame and num_encoded_frames_ is not increased. The timestamps are like this: num_encoded_frames_=0, timetsamp=0 num_encoded_frames_=0, timetsamp=0 num_encoded_frames_=1, timetsamp=33000 You can start the timestamp of first buffer at 33000. So you can do this here: if (timestamp != previous_timestamp_) { } https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1374: base::TimeDelta().FromMilliseconds(next_input_id_ * (next_input_id_ + 1) See the another comment. https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1524: // The last timestamp in |frame_timestamps_| won't be popped out. Update the comment. https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1781: false))); Try testing with you CL with verify_output_timestamp=true for all test cases. So we can be sure there's no dependency between parameters.
FYI. This is the test command I used. ./video_encode_accelerator_unittest --test_stream_data=bear_320x192_40frames.yuv:320:192:1:out1.h264:200000:30:200000:30
https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1266: if (num_encoded_frames_ == 0 || timestamp != previous_timestamp_) { On 2016/07/12 09:28:06, wuchengli wrote: > This failed on oak for H264. Did you test any device with H264? The first input > buffer produces two outputs buffers. First output buffer is SPS, so > ProcessStreamBuffer does not call HandleEncodedFrame and num_encoded_frames_ is > not increased. The timestamps are like this: > num_encoded_frames_=0, timetsamp=0 > num_encoded_frames_=0, timetsamp=0 > num_encoded_frames_=1, timetsamp=33000 > > You can start the timestamp of first buffer at 33000. So you can do this here: > if (timestamp != previous_timestamp_) { > } Done. https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1374: base::TimeDelta().FromMilliseconds(next_input_id_ * On 2016/07/12 09:28:07, wuchengli wrote: > (next_input_id_ + 1) > See the another comment. Done. https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1524: // The last timestamp in |frame_timestamps_| won't be popped out. On 2016/07/12 09:28:07, wuchengli wrote: > Update the comment. Done. https://codereview.chromium.org/2104883005/diff/60001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1781: false))); On 2016/07/12 09:28:07, wuchengli wrote: > Try testing with you CL with verify_output_timestamp=true for all test cases. So > we can be sure there's no dependency between parameters. Verified that they all pass.
LGTM https://codereview.chromium.org/2104883005/diff/100001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:1757: std::make_tuple(1, true, 0, false, false, false, false, false, true))); No need to save the output to a file. std::make_tuple(1, false, 0, false, false, false, false, false, true)
https://codereview.chromium.org/2104883005/diff/100001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2104883005/diff/100001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:1757: std::make_tuple(1, true, 0, false, false, false, false, false, true))); On 2016/07/12 11:46:13, wuchengli wrote: > No need to save the output to a file. > > std::make_tuple(1, false, 0, false, false, false, false, false, true) Done.
The CQ bit was checked by shenghao@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from henryhsu@chromium.org, wuchengli@chromium.org Link to the patchset: https://codereview.chromium.org/2104883005/#ps120001 (title: "change test parameter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Message was sent while issue was closed.
Description was changed from ========== VEA unit test: Add timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ========== to ========== VEA unit test: Add timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== VEA unit test: Add timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie ========== to ========== VEA unit test: Add timestamp test Verify that one input timestamps match at least one output timestamp. BUG=620564 TEST=Verify that the test pass on minnie Committed: https://crrev.com/d701f00d4b691011bb7a019ea3a0e2fbd65c0611 Cr-Commit-Position: refs/heads/master@{#404806} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d701f00d4b691011bb7a019ea3a0e2fbd65c0611 Cr-Commit-Position: refs/heads/master@{#404806} |
