Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2760513002: h264_decoder_unittests: Initial Change. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by Owen Lin
Modified:
2 months, 2 weeks ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

h264_decoder_unittests: Initial Change. BUG=b/35934865 TEST=Run the test on CrOS devices. 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/2760513002 Cr-Commit-Position: refs/heads/master@{#483623} Committed: https://chromium.googlesource.com/chromium/src/+/0fc85fd05e9607d8d1ca242b831abee0e4f4c318

Patch Set 1 #

Total comments: 12

Patch Set 2 : Address kcwu's comments #

Patch Set 3 : incliude binary files #

Patch Set 4 : Fix build issue on win_clang #

Total comments: 2

Patch Set 5 : Address kcwu's comments. #

Total comments: 36

Patch Set 6 : Address review comments from posciak. #

Total comments: 14

Patch Set 7 : Refactor: uses gmock for expected calls. #

Patch Set 8 : Refactor: uses gmock for expected calls. #

Total comments: 6

Patch Set 9 : Address comments from Pawel #

Total comments: 4

Patch Set 10 : Address nits from Pawel. #

Patch Set 11 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+364 lines, -20 lines) Patch
M media/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -10 lines 0 comments Download
M media/gpu/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 4 chunks +29 lines, -9 lines 0 comments Download
A media/gpu/h264_decoder_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +332 lines, -0 lines 0 comments Download
M media/gpu/h264_dpb.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
A media/test/data/bear-320x192-baseline-frame-0.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-baseline-frame-1.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-baseline-frame-2.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-baseline-frame-3.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-high-frame-0.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-high-frame-1.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-high-frame-2.h264 View 1 2 Binary file 0 comments Download
A media/test/data/bear-320x192-high-frame-3.h264 View 1 2 Binary file 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 54 (27 generated)
Owen Lin
PTAL. Thanks.
6 months, 1 week ago (2017-03-17 03:03:47 UTC) #3
kcwu
https://codereview.chromium.org/2760513002/diff/1/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/BUILD.gn#newcode526 media/gpu/BUILD.gn:526: if (is_chromeos || is_win) { Is it possible to ...
6 months, 1 week ago (2017-03-17 06:47:44 UTC) #4
Owen Lin
Please take another look, thanks. https://codereview.chromium.org/2760513002/diff/1/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/BUILD.gn#newcode526 media/gpu/BUILD.gn:526: if (is_chromeos || is_win) ...
6 months, 1 week ago (2017-03-20 08:00:05 UTC) #10
kcwu
https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unittest.cc#newcode251 media/gpu/h264_decoder_unittest.cc:251: ASSERT_EQ(0u, expected_events_.size()); On 2017/03/20 08:00:04, Owen Lin wrote: > ...
6 months, 1 week ago (2017-03-20 08:28:09 UTC) #11
Owen Lin
https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unittest.cc#newcode251 media/gpu/h264_decoder_unittest.cc:251: ASSERT_EQ(0u, expected_events_.size()); On 2017/03/20 08:28:09, kcwu wrote: > On ...
6 months, 1 week ago (2017-03-20 09:29:27 UTC) #13
kcwu
lgtm
6 months, 1 week ago (2017-03-20 09:32:58 UTC) #14
Owen Lin
PTAL. Thank you.
6 months, 1 week ago (2017-03-21 02:02:57 UTC) #20
DaleCurtis
lgtm https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc#newcode86 media/gpu/h264_decoder_unittest.cc:86: size_t GetNumberOfExpectedEvents() const { return expected_events_.size(); } Inline ...
6 months ago (2017-03-21 18:07:18 UTC) #21
Pawel Osciak
https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc#newcode23 media/gpu/h264_decoder_unittest.cc:23: const char* kBaselineFrame0 = "bear-320x192-baseline-frame-0.h264"; Perhaps const std::string, as ...
6 months ago (2017-03-23 08:34:21 UTC) #24
kcwu
https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc#newcode254 media/gpu/h264_decoder_unittest.cc:254: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); Please swap the arguments. They are ...
6 months ago (2017-03-23 08:54:33 UTC) #25
Owen Lin
Sorry for the late update. Please take a look again. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc#newcode23 ...
3 months ago (2017-06-21 07:37:29 UTC) #26
Pawel Osciak
https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder_unittest.cc#newcode111 media/gpu/h264_decoder_unittest.cc:111: return new H264Picture(); On 2017/06/21 07:37:29, Owen Lin wrote: ...
3 months ago (2017-06-23 02:07:34 UTC) #27
Owen Lin
https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder_unittest.cc#newcode40 media/gpu/h264_decoder_unittest.cc:40: // events are triggers in order. On 2017/06/23 02:07:34, ...
2 months, 4 weeks ago (2017-06-26 09:09:00 UTC) #28
Pawel Osciak
Thanks, this looks great! Just two questions/suggestions. https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder_unittest.cc#newcode177 media/gpu/h264_decoder_unittest.cc:177: TEST_F(H264DecoderTest, SkipNoneIDRFrames) ...
2 months, 3 weeks ago (2017-06-28 07:59:25 UTC) #29
Owen Lin
https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder_unittest.cc#newcode177 media/gpu/h264_decoder_unittest.cc:177: TEST_F(H264DecoderTest, SkipNoneIDRFrames) { On 2017/06/28 07:59:25, Pawel Osciak wrote: ...
2 months, 3 weeks ago (2017-06-28 09:03:38 UTC) #30
Pawel Osciak
Thanks, lgtm % 2 comments. https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder_unittest.cc#newcode183 media/gpu/h264_decoder_unittest.cc:183: EXPECT_EQ(15U, decoder_->GetRequiredNumOfPictures()); Perhaps EXPECT_GE(9u, ...
2 months, 3 weeks ago (2017-06-29 04:57:45 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2760513002/220001
2 months, 3 weeks ago (2017-06-29 05:19:41 UTC) #34
Owen Lin
https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder_unittest.cc File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder_unittest.cc#newcode183 media/gpu/h264_decoder_unittest.cc:183: EXPECT_EQ(15U, decoder_->GetRequiredNumOfPictures()); On 2017/06/29 04:57:44, Pawel Osciak wrote: > ...
2 months, 3 weeks ago (2017-06-29 05:19:51 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/242236) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
2 months, 3 weeks ago (2017-06-29 05:21:50 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2760513002/240001
2 months, 3 weeks ago (2017-06-29 07:08:07 UTC) #40
commit-bot: I haz the power
Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_tests_rel/builds/11937)
2 months, 3 weeks ago (2017-06-29 07:44:53 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2760513002/240001
2 months, 3 weeks ago (2017-06-29 09:03:48 UTC) #44
commit-bot: I haz the power
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_android_rel_ng/builds/328432)
2 months, 3 weeks ago (2017-06-29 09:32:11 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2760513002/240001
2 months, 3 weeks ago (2017-06-30 01:28:55 UTC) #48
commit-bot: I haz the power
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/0fc85fd05e9607d8d1ca242b831abee0e4f4c318
2 months, 3 weeks ago (2017-06-30 03:41:36 UTC) #51
Timothy Loh
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/2964193002/ by timloh@chromium.org. ...
2 months, 3 weeks ago (2017-06-30 05:24:54 UTC) #52
findit-for-me
2 months, 3 weeks ago (2017-07-01 04:40:44 UTC) #53
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 483623 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b