Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760513002:
h264_decoder_unittests: Initial Change. (Closed)
Descriptionh264_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 #Messages
Total messages: 54 (27 generated)
Description was changed from ========== h264_decoder_unittests: Initial Change. BUG=b/35934865 TEST=Run the test on CrOS devices. ========== to ========== 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 ==========
owenlin@chromium.org changed reviewers: + johnylin@chromium.org, kcwu@chromium.org, posciak@chromium.org
PTAL. 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) { Is it possible to make the test run on all platforms and included in media_unittests? https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:23: const char* kBaselineFrame0 = "bear-320x192-baseline-frame-0.h264"; Forgot to git add? https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:64: typedef std::function<void(const Event& event)> EventHandler; Suggest to use c++11's "using" syntax. https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:104: class FakeH264Accelerator : public H264Decoder::H264Accelerator { Have you considered using gmock? https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:251: ASSERT_EQ(0u, expected_events_.size()); 1. IIRC, we prefer inline this kind of assertions to a separate function. If so, we can know which test failed easier. 2. EXPECT instead? 3. *_TRUE(expected_events_.empty());
The CQ bit was checked by owenlin@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:60001) has been deleted
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) { On 2017/03/17 06:47:43, kcwu wrote: > Is it possible to make the test run on all platforms and included in > media_unittests? Done. https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:23: const char* kBaselineFrame0 = "bear-320x192-baseline-frame-0.h264"; On 2017/03/17 06:47:44, kcwu wrote: > Forgot to git add? It was in another CL, but I have merge the two CL so that I can test on try bot. https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:64: typedef std::function<void(const Event& event)> EventHandler; On 2017/03/17 06:47:43, kcwu wrote: > Suggest to use c++11's "using" syntax. Done. https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:104: class FakeH264Accelerator : public H264Decoder::H264Accelerator { On 2017/03/17 06:47:43, kcwu wrote: > Have you considered using gmock? I don't think using mock will simplify the code or increasing the readability. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/1zktlTqFb6o https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:251: ASSERT_EQ(0u, expected_events_.size()); On 2017/03/17 06:47:43, kcwu wrote: > 1. IIRC, we prefer inline this kind of assertions to a separate function. If so, > we can know which test failed easier. > 2. EXPECT instead? > 3. *_TRUE(expected_events_.empty()); I agree.
https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:251: ASSERT_EQ(0u, expected_events_.size()); On 2017/03/20 08:00:04, Owen Lin wrote: > On 2017/03/17 06:47:43, kcwu wrote: > > 1. IIRC, we prefer inline this kind of assertions to a separate function. If > so, > > we can know which test failed easier. > > 2. EXPECT instead? > > 3. *_TRUE(expected_events_.empty()); > > I agree. You forgot to change code? https://codereview.chromium.org/2760513002/diff/80001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2760513002/diff/80001/media/gpu/BUILD.gn#newc... media/gpu/BUILD.gn:538: # The following sources are only used in chrome os and windows but they are IIUC, we can add them to sources unconditionally.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/1/media/gpu/h264_decoder_unit... media/gpu/h264_decoder_unittest.cc:251: ASSERT_EQ(0u, expected_events_.size()); On 2017/03/20 08:28:09, kcwu wrote: > On 2017/03/20 08:00:04, Owen Lin wrote: > > On 2017/03/17 06:47:43, kcwu wrote: > > > 1. IIRC, we prefer inline this kind of assertions to a separate function. If > > so, > > > we can know which test failed easier. > > > 2. EXPECT instead? > > > 3. *_TRUE(expected_events_.empty()); > > > > I agree. > > You forgot to change code? Done. https://codereview.chromium.org/2760513002/diff/80001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/2760513002/diff/80001/media/gpu/BUILD.gn#newc... media/gpu/BUILD.gn:538: # The following sources are only used in chrome os and windows but they are On 2017/03/20 08:28:09, kcwu wrote: > IIUC, we can add them to sources unconditionally. Done.
lgtm
The CQ bit was checked by owenlin@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
owenlin@chromium.org changed reviewers: + dalecurtis@chromium.org, wuchengli@chromium.org
PTAL. Thank you.
lgtm https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:86: size_t GetNumberOfExpectedEvents() const { return expected_events_.size(); } Inline should be hacker_style().
Description was changed from ========== 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 ========== to ========== 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 ==========
wuchengli@chromium.org changed reviewers: - wuchengli@chromium.org
https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:23: const char* kBaselineFrame0 = "bear-320x192-baseline-frame-0.h264"; Perhaps const std::string, as GetTestDataFilePath() also takes an std::string ? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:32: // Forward declaration Nit: comment perhaps not needed. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:35: class H264DecoderTest : public ::testing::Test { Please add a comment what this test is for. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:40: enum Type { enum class? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:52: Event(Type type) : type(type) {} explicit? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:64: using EventHandler = std::function<void(const Event& event)>; std::function is not allowed per Chromium coding style (https://chromium-cpp.appspot.com/#library-blacklist). Please use base::Callback. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:66: H264DecoderTest() {} = default; perhaps? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:88: protected: Do we need protected instead of private? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:111: return new H264Picture(); To mimic the actual behavior of the accelerator, we should perhaps have a simple subclass of H264Picture that increments picture_quota when all refs to it are dropped. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:185: static_cast<const H264DecoderTest::OutputPictureEvent&>(e); Perhaps we could instead have a virtual AsString() method in Event, that would be overriden in OutputPictureEvent to also output poc and call e.AsString() from here without having to check the type and cast? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:198: void H264DecoderTest::FeedData(std::vector<const char*> frames) { Perhaps const vector<std::string>&? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:239: void H264DecoderTest::ExpectEvent(Event::Type type, EventHandler handler) { Should this take ownership of handler here and when creating the ExpectedEvent? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:254: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); Perhaps have another specialization of Event for ALLOCATE_NEW_SURFACES that takes and verifies expected Size, just like the OUTPUT_PICTURE one does with PoC? https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:289: accelerator_->SetNewPictureQuota(4); Should we perhaps SetNewPictureQuota() in the ALLOCATE_NEW_SURFACES event instead?
https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:254: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); Please swap the arguments. They are read as EXPECT_*(expect, actual); https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:272: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); Please swap the arguments. They are read as EXPECT_*(expect, actual);
Sorry for the late update. Please take a look again. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:23: const char* kBaselineFrame0 = "bear-320x192-baseline-frame-0.h264"; On 2017/03/23 08:34:21, Pawel Osciak wrote: > Perhaps const std::string, as GetTestDataFilePath() also takes an std::string ? Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:32: // Forward declaration On 2017/03/23 08:34:20, Pawel Osciak wrote: > Nit: comment perhaps not needed. Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:35: class H264DecoderTest : public ::testing::Test { On 2017/03/23 08:34:21, Pawel Osciak wrote: > Please add a comment what this test is for. Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:40: enum Type { On 2017/03/23 08:34:20, Pawel Osciak wrote: > enum class? Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:52: Event(Type type) : type(type) {} On 2017/03/23 08:34:20, Pawel Osciak wrote: > explicit? You're right. Implicit conversion is not allowed. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:64: using EventHandler = std::function<void(const Event& event)>; On 2017/03/23 08:34:20, Pawel Osciak wrote: > std::function is not allowed per Chromium coding style > (https://chromium-cpp.appspot.com/#library-blacklist). Please use > base::Callback. Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:66: H264DecoderTest() {} On 2017/03/23 08:34:20, Pawel Osciak wrote: > = default; perhaps? Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:86: size_t GetNumberOfExpectedEvents() const { return expected_events_.size(); } On 2017/03/21 18:07:18, DaleCurtis wrote: > Inline should be hacker_style(). Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:88: protected: On 2017/03/23 08:34:20, Pawel Osciak wrote: > Do we need protected instead of private? Some tests can directly access these members. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:111: return new H264Picture(); On 2017/03/23 08:34:20, Pawel Osciak wrote: > To mimic the actual behavior of the accelerator, we should perhaps have a simple > subclass of H264Picture that increments picture_quota when all refs to it are > dropped. It sounds like we should add some tests about free H264Picture. In this test, I would like to test each part of the decoder individually. So, I would prefer to add other tests to see H264Picture is released as expected. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:185: static_cast<const H264DecoderTest::OutputPictureEvent&>(e); On 2017/03/23 08:34:20, Pawel Osciak wrote: > Perhaps we could instead have a virtual AsString() method in Event, that would > be overriden in OutputPictureEvent to also output poc and call e.AsString() from > here without having to check the type and cast? Done. But follow most namings in chromium as ToString(). https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:198: void H264DecoderTest::FeedData(std::vector<const char*> frames) { On 2017/03/23 08:34:20, Pawel Osciak wrote: > Perhaps const vector<std::string>&? Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:239: void H264DecoderTest::ExpectEvent(Event::Type type, EventHandler handler) { On 2017/03/23 08:34:20, Pawel Osciak wrote: > Should this take ownership of handler here and when creating the ExpectedEvent? I did some code study I think we should pass by reference and do a copy when creating the ExpectedEvent. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:254: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); On 2017/03/23 08:54:33, kcwu wrote: > Please swap the arguments. > They are read as EXPECT_*(expect, actual); Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:254: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); On 2017/03/23 08:34:20, Pawel Osciak wrote: > Perhaps have another specialization of Event for ALLOCATE_NEW_SURFACES that > takes and verifies expected Size, just like the OUTPUT_PICTURE one does with > PoC? Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:272: EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192)); On 2017/03/23 08:54:33, kcwu wrote: > Please swap the arguments. > They are read as EXPECT_*(expect, actual); Done. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:289: accelerator_->SetNewPictureQuota(4); On 2017/03/23 08:34:21, Pawel Osciak wrote: > Should we perhaps SetNewPictureQuota() in the ALLOCATE_NEW_SURFACES event > instead? I would prefer we focus on one thing we are testing with each test cases.
https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:111: return new H264Picture(); On 2017/06/21 07:37:29, Owen Lin wrote: > On 2017/03/23 08:34:20, Pawel Osciak wrote: > > To mimic the actual behavior of the accelerator, we should perhaps have a > simple > > subclass of H264Picture that increments picture_quota when all refs to it are > > dropped. > > It sounds like we should add some tests about free H264Picture. > > In this test, I would like to test each part of the decoder individually. So, I > would prefer to add other tests to see H264Picture is released as expected. Acknowledged. https://codereview.chromium.org/2760513002/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:289: accelerator_->SetNewPictureQuota(4); On 2017/06/21 07:37:29, Owen Lin wrote: > On 2017/03/23 08:34:21, Pawel Osciak wrote: > > Should we perhaps SetNewPictureQuota() in the ALLOCATE_NEW_SURFACES event > > instead? > > I would prefer we focus on one thing we are testing with each test cases. Acknowledged. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:40: // events are triggers in order. s/triggers/triggered/ Also probably s/in order/at appropriate times/? https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:41: enum class EventType { Could this be inside the Event class still? https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:69: using EventHandler = base::Callback<void(const Event& event)>; Nit: OnceCallback? https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:88: void ExpectEvent(EventType type, const EventHandler& handler = {}); To be honest it took me a bit to understand the idea behind different overloads, difference between EventHandler and the Closure case below, and also how the default param works. Could we perhaps consider calling this: void ExpectEvent(EventType type); and... https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:90: void ExpectEvent(EventType type, const base::Closure& closure); ... this ExpectEventAndRun() ? Do we also need to have a Callback and Closure versions? Could we just always take Callback, since Closure is a Callback? https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:263: ExpectEvent(type, base::Bind(RunClosureAndIgnoreEvent, closure)); Are we really ignoring the event here? I think we are still checking it in OnEvent? Should the method be called "*Ignore*"? https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:267: auto event = static_cast<const OutputPictureEvent&>(e); Could we instead create a specialized event in ExpectOutputPictureWithPoc to avoid casting?
https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:40: // events are triggers in order. On 2017/06/23 02:07:34, Pawel Osciak wrote: > s/triggers/triggered/ > Also probably s/in order/at appropriate times/? The code is refactored out. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:41: enum class EventType { On 2017/06/23 02:07:34, Pawel Osciak wrote: > Could this be inside the Event class still? Ditto. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:69: using EventHandler = base::Callback<void(const Event& event)>; On 2017/06/23 02:07:34, Pawel Osciak wrote: > Nit: OnceCallback? Done. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:88: void ExpectEvent(EventType type, const EventHandler& handler = {}); On 2017/06/23 02:07:34, Pawel Osciak wrote: > To be honest it took me a bit to understand the idea behind different overloads, > difference between EventHandler and the Closure case below, and also how the > default param works. Could we perhaps consider calling this: > > void ExpectEvent(EventType type); > > and... Same. Code has been removed. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:90: void ExpectEvent(EventType type, const base::Closure& closure); On 2017/06/23 02:07:34, Pawel Osciak wrote: > ... this ExpectEventAndRun() ? > > Do we also need to have a Callback and Closure versions? Could we just always > take Callback, since Closure is a Callback? Ditto. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:263: ExpectEvent(type, base::Bind(RunClosureAndIgnoreEvent, closure)); On 2017/06/23 02:07:34, Pawel Osciak wrote: > Are we really ignoring the event here? I think we are still checking it in > OnEvent? Should the method be called "*Ignore*"? Ditto. https://codereview.chromium.org/2760513002/diff/140001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:267: auto event = static_cast<const OutputPictureEvent&>(e); On 2017/06/23 02:07:34, Pawel Osciak wrote: > Could we instead create a specialized event in ExpectOutputPictureWithPoc to > avoid casting? Ditto.
Thanks, this looks great! Just two questions/suggestions. https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:177: TEST_F(H264DecoderTest, SkipNoneIDRFrames) { s/None/Non/ https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:201: EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(0))); If I understand the stream correctly, would this be what we expect: InSequence decode_seq, p0_seq, p2_seq, p4_seq, p6_seq; EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(0))). InSequence(decode_seq, p0_seq); EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(2))). InSequence(decode_seq, p2_seq); EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(4))). InSequence(decode_seq, p4_seq); EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(6))). InSequence(decode_seq, p6_seq); EXPECT_CALL(accelerator_, OutputPicture(WithPoc(0))). InSequence(p0_seq); EXPECT_CALL(accelerator_, OutputPicture(WithPoc(2))). InSequence(p2_seq); EXPECT_CALL(accelerator_, OutputPicture(WithPoc(4))). InSequence(p4_seq); EXPECT_CALL(accelerator_, OutputPicture(WithPoc(6))). InSequence(p6_seq); Not sure if this could be written in any more compact way... https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:251: EXPECT_EQ(gfx::Size(320, 192), decoder_->GetPicSize()); Should we also EXPECT for decoder_->GetRequiredNumOfPictures() (here and in other places)?
https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:177: TEST_F(H264DecoderTest, SkipNoneIDRFrames) { On 2017/06/28 07:59:25, Pawel Osciak wrote: > s/None/Non/ Done. https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:201: EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(0))); On 2017/06/28 07:59:25, Pawel Osciak wrote: > If I understand the stream correctly, would this be what we expect: > > InSequence decode_seq, p0_seq, p2_seq, p4_seq, p6_seq; > > EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(0))). > InSequence(decode_seq, p0_seq); > EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(2))). > InSequence(decode_seq, p2_seq); > EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(4))). > InSequence(decode_seq, p4_seq); > EXPECT_CALL(accelerator_, SubmitDecode(WithPoc(6))). > InSequence(decode_seq, p6_seq); > > EXPECT_CALL(accelerator_, OutputPicture(WithPoc(0))). > InSequence(p0_seq); > EXPECT_CALL(accelerator_, OutputPicture(WithPoc(2))). > InSequence(p2_seq); > EXPECT_CALL(accelerator_, OutputPicture(WithPoc(4))). > InSequence(p4_seq); > EXPECT_CALL(accelerator_, OutputPicture(WithPoc(6))). > InSequence(p6_seq); > > > Not sure if this could be written in any more compact way... Done. https://codereview.chromium.org/2760513002/diff/180001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:251: EXPECT_EQ(gfx::Size(320, 192), decoder_->GetPicSize()); On 2017/06/28 07:59:25, Pawel Osciak wrote: > Should we also EXPECT for decoder_->GetRequiredNumOfPictures() (here and in > other places)? Done.
Thanks, lgtm % 2 comments. https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:183: EXPECT_EQ(15U, decoder_->GetRequiredNumOfPictures()); Perhaps EXPECT_GE(9u, ) to ensure at least the minimum DPB size for this stream (9) is allocated, but also allow more. Here and for all baseline checks please. https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:226: EXPECT_EQ(22U, decoder_->GetRequiredNumOfPictures()); GE 16U and everywhere for high please.
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, dalecurtis@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2760513002/#ps220001 (title: "Address nits from Pawel.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder... File media/gpu/h264_decoder_unittest.cc (right): https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:183: EXPECT_EQ(15U, decoder_->GetRequiredNumOfPictures()); On 2017/06/29 04:57:44, Pawel Osciak wrote: > Perhaps EXPECT_GE(9u, ) to ensure at least the minimum DPB size for this stream > (9) is allocated, but also allow more. > > Here and for all baseline checks please. Done. https://codereview.chromium.org/2760513002/diff/200001/media/gpu/h264_decoder... media/gpu/h264_decoder_unittest.cc:226: EXPECT_EQ(22U, decoder_->GetRequiredNumOfPictures()); On 2017/06/29 04:57:44, Pawel Osciak wrote: > GE 16U and everywhere for high please. Done. Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
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...) 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_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, dalecurtis@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2760513002/#ps240001 (title: "Rebase")
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
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_...)
The CQ bit was checked by owenlin@chromium.org
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
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...)
The CQ bit was checked by owenlin@chromium.org
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": 240001, "attempt_start_ts": 1498786121322220,
"parent_rev": "a675706f0676775c0b13f26ea4398786f62599b0", "commit_rev":
"0fc85fd05e9607d8d1ca242b831abee0e4f4c318"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/0fc85fd05e9607d8d1ca242b831a... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/0fc85fd05e9607d8d1ca242b831a...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/2964193002/ by timloh@chromium.org. The reason for reverting is: Broke some bots: "CommandFailedError: Failed to list tests on any device" https://build.chromium.org/p/chromium.linux/builders/Android%20Tests https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%... Added tests all fail: https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests .
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...
Message was sent while issue was closed.
Description was changed from ========== 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/+/0fc85fd05e9607d8d1ca242b831a... ========== to ========== 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/+/0fc85fd05e9607d8d1ca242b831a... ========== |
