Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2397001:
enable omx_codec_unittest based on new omx engine (Closed)
|
Created:
10 years, 6 months ago by wjia(left Chromium) Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., fbarchard, awong, Alpha Left Google, scherkus (not reviewing) Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
Descriptionenable omx_codec_unittest based on new omx engine
BUG=none
TEST=try bot
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48741
Patch Set 1 #
Total comments: 55
Patch Set 2 : fix lint complaints #
Total comments: 24
Patch Set 3 : for codereview #Patch Set 4 : more codereview #Messages
Total messages: 8 (0 generated)
LGTM. probably try linux_valgrind bot before commit.
Also need to rename the file to omx_video_decode_engine_unittest.cc http://codereview.chromium.org/2397001/diff/1/2 File media/omx/omx_codec_unittest.cc (right): http://codereview.chromium.org/2397001/diff/1/2#newcode21 media/omx/omx_codec_unittest.cc:21: using media::VideoDecodeEngine; All the test cases are in media namespace so this line is not needed. http://codereview.chromium.org/2397001/diff/1/2#newcode33 media/omx/omx_codec_unittest.cc:33: // Temporarily disable omx_codec_unittests during heavy refactoring. Remove this line. http://codereview.chromium.org/2397001/diff/1/2#newcode38 media/omx/omx_codec_unittest.cc:38: // const char* kRoleName = "awsome"; Why comment this out? Remove it if not needed. http://codereview.chromium.org/2397001/diff/1/2#newcode44 media/omx/omx_codec_unittest.cc:44: static std::deque<OMX_BUFFERHEADERTYPE*> output_pool; Try not to use static variable. http://codereview.chromium.org/2397001/diff/1/2#newcode101 media/omx/omx_codec_unittest.cc:101: ACTION(EmptyBuffer) { Please rename this to EmptyBufferDone. I named it badly in the first place. http://codereview.chromium.org/2397001/diff/1/2#newcode102 media/omx/omx_codec_unittest.cc:102: // arg0->nFlags = 0; I think these two lines are not useful so remove them. http://codereview.chromium.org/2397001/diff/1/2#newcode108 media/omx/omx_codec_unittest.cc:108: OMX_BUFFERHEADERTYPE* out_buffer = output_pool.front(); I don't think we need to use global variable here. You can have the action accept an argument like: ACTION_P(EmptyBuffer, queue) and then use |queue| in this mock action. http://codereview.chromium.org/2397001/diff/1/2#newcode109 media/omx/omx_codec_unittest.cc:109: output_pool.pop_front(); Why calling FillBufferDone in this action? They should be in a different action. http://codereview.chromium.org/2397001/diff/1/2#newcode118 media/omx/omx_codec_unittest.cc:118: ACTION(FillBuffer) { Looks like this method is used only for enqueuing output buffer, so call it EnqueueOutputBuffer. Also don't think we need to set values of the buffer here. Exact the part in EmptyBuffer action to make the new FillBufferAction. http://codereview.chromium.org/2397001/diff/1/2#newcode152 media/omx/omx_codec_unittest.cc:152: uint8* data_; use scoped_array instead of pointer. http://codereview.chromium.org/2397001/diff/1/2#newcode165 media/omx/omx_codec_unittest.cc:165: NewCallback(this, &OmxCodecTest::FeedDoneCallback); nit: indent by 4 spaces http://codereview.chromium.org/2397001/diff/1/2#newcode167 media/omx/omx_codec_unittest.cc:167: NewCallback(this, &OmxCodecTest::DecodeDoneCallback); nit: indent by 4 spaces http://codereview.chromium.org/2397001/diff/1/2#newcode171 media/omx/omx_codec_unittest.cc:171: // CHECK(output_units_.size() == 0); Why this check is not valid? http://codereview.chromium.org/2397001/diff/1/2#newcode174 media/omx/omx_codec_unittest.cc:174: // protected: I think it's still good to have protected. http://codereview.chromium.org/2397001/diff/1/2#newcode308 media/omx/omx_codec_unittest.cc:308: void FeedDoneCallback(scoped_refptr<Buffer> buffer) { This should be named EmptyBufferDoneCallback http://codereview.chromium.org/2397001/diff/1/2#newcode312 media/omx/omx_codec_unittest.cc:312: void DecodeDoneCallback(scoped_refptr<VideoFrame> frame) { This should be named FillBufferDoneCallback http://codereview.chromium.org/2397001/diff/1/2#newcode332 media/omx/omx_codec_unittest.cc:332: Remove this empty line. http://codereview.chromium.org/2397001/diff/1/2#newcode356 media/omx/omx_codec_unittest.cc:356: output_pool.clear(); Try to make this variable local instead of global. http://codereview.chromium.org/2397001/diff/1/2#newcode377 media/omx/omx_codec_unittest.cc:377: output_pool.clear(); Try to make this variable local instead of global. http://codereview.chromium.org/2397001/diff/1/2#newcode397 media/omx/omx_codec_unittest.cc:397: // Give input buffers to OmxCodec. OmxCodec will make a new Should be OmxVideoDecodeEngine. http://codereview.chromium.org/2397001/diff/1/2#newcode408 media/omx/omx_codec_unittest.cc:408: CHECK(kBufferCount == static_cast<int>(input_units_.size())); I think EXPECT_EQ is better than CHECK here. I used CHECK because it is an assertion to prevent runtime failure. If we are just checking results to fail the test we should use EXPECT_XX. http://codereview.chromium.org/2397001/diff/1/2#newcode413 media/omx/omx_codec_unittest.cc:413: // send EndOfStream, expect eos flag nit: capital letter at the beginning of line, period at the end. http://codereview.chromium.org/2397001/diff/1/2#newcode413 media/omx/omx_codec_unittest.cc:413: // send EndOfStream, expect eos flag This block of EOS code seems to be the same as below, maybe add a method to share the code. http://codereview.chromium.org/2397001/diff/1/2#newcode427 media/omx/omx_codec_unittest.cc:427: // shutdown nit: capital letter at the beginning of line, period at the end. http://codereview.chromium.org/2397001/diff/1/2#newcode457 media/omx/omx_codec_unittest.cc:457: // Give input buffers to OmxCodec. OmxCodec will make a new This is not OmxCodec any more. http://codereview.chromium.org/2397001/diff/1/2#newcode467 media/omx/omx_codec_unittest.cc:467: CHECK(kBufferCount == static_cast<int>(input_units_.size())); These lines shold be EXPECT_EQ. http://codereview.chromium.org/2397001/diff/1/2#newcode472 media/omx/omx_codec_unittest.cc:472: CHECK(kBufferCount == static_cast<int>(input_units_.size())); These lines should be EXPECT_EQ http://codereview.chromium.org/2397001/diff/1/2#newcode486 media/omx/omx_codec_unittest.cc:486: CHECK(kBufferCount == static_cast<int>(input_units_.size())); These lines should be EXPECT_EQ
http://codereview.chromium.org/2397001/diff/1/2 File media/omx/omx_codec_unittest.cc (right): http://codereview.chromium.org/2397001/diff/1/2#newcode21 media/omx/omx_codec_unittest.cc:21: using media::VideoDecodeEngine; On 2010/06/01 19:36:09, Alpha wrote: > All the test cases are in media namespace so this line is not needed. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode33 media/omx/omx_codec_unittest.cc:33: // Temporarily disable omx_codec_unittests during heavy refactoring. On 2010/06/01 19:36:09, Alpha wrote: > Remove this line. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode38 media/omx/omx_codec_unittest.cc:38: // const char* kRoleName = "awsome"; On 2010/06/01 19:36:09, Alpha wrote: > Why comment this out? Remove it if not needed. This line is removed since the configurator is internal to omx_video_decode_engine. http://codereview.chromium.org/2397001/diff/1/2#newcode101 media/omx/omx_codec_unittest.cc:101: ACTION(EmptyBuffer) { On 2010/06/01 19:36:09, Alpha wrote: > Please rename this to EmptyBufferDone. I named it badly in the first place. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode102 media/omx/omx_codec_unittest.cc:102: // arg0->nFlags = 0; On 2010/06/01 19:36:09, Alpha wrote: > I think these two lines are not useful so remove them. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode108 media/omx/omx_codec_unittest.cc:108: OMX_BUFFERHEADERTYPE* out_buffer = output_pool.front(); On 2010/06/01 19:36:09, Alpha wrote: > I don't think we need to use global variable here. > > You can have the action accept an argument like: > ACTION_P(EmptyBuffer, queue) and then use |queue| in this mock action. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode109 media/omx/omx_codec_unittest.cc:109: output_pool.pop_front(); On 2010/06/01 19:36:09, Alpha wrote: > Why calling FillBufferDone in this action? They should be in a different action. This is to simulate how OMX component works. Assuming output buffer is ready to fill, one input buffer will cause OMX component to both return input buffer itself and fill an output buffer. Previous mocking (return output buffer immediately when receiving OMX_FillThisBuffer) is too simplified. If needed, we can even improve this one to really simulate a component by checking if output buffer is available here. Also add checking if input buffer is available when OMX_FillThisBuffer is called. http://codereview.chromium.org/2397001/diff/1/2#newcode118 media/omx/omx_codec_unittest.cc:118: ACTION(FillBuffer) { On 2010/06/01 19:36:09, Alpha wrote: > Looks like this method is used only for enqueuing output buffer, so call it > EnqueueOutputBuffer. Also don't think we need to set values of the buffer here. > > Exact the part in EmptyBuffer action to make the new FillBufferAction. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode152 media/omx/omx_codec_unittest.cc:152: uint8* data_; On 2010/06/01 19:36:09, Alpha wrote: > use scoped_array instead of pointer. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode165 media/omx/omx_codec_unittest.cc:165: NewCallback(this, &OmxCodecTest::FeedDoneCallback); On 2010/06/01 19:36:09, Alpha wrote: > nit: indent by 4 spaces Done. http://codereview.chromium.org/2397001/diff/1/2#newcode167 media/omx/omx_codec_unittest.cc:167: NewCallback(this, &OmxCodecTest::DecodeDoneCallback); On 2010/06/01 19:36:09, Alpha wrote: > nit: indent by 4 spaces Done. http://codereview.chromium.org/2397001/diff/1/2#newcode171 media/omx/omx_codec_unittest.cc:171: // CHECK(output_units_.size() == 0); On 2010/06/01 19:36:09, Alpha wrote: > Why this check is not valid? Now, output_units_ is the VideoFrame's returned to test app. And it's not recycled. http://codereview.chromium.org/2397001/diff/1/2#newcode174 media/omx/omx_codec_unittest.cc:174: // protected: On 2010/06/01 19:36:09, Alpha wrote: > I think it's still good to have protected. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode308 media/omx/omx_codec_unittest.cc:308: void FeedDoneCallback(scoped_refptr<Buffer> buffer) { On 2010/06/01 19:36:09, Alpha wrote: > This should be named EmptyBufferDoneCallback Done. http://codereview.chromium.org/2397001/diff/1/2#newcode312 media/omx/omx_codec_unittest.cc:312: void DecodeDoneCallback(scoped_refptr<VideoFrame> frame) { On 2010/06/01 19:36:09, Alpha wrote: > This should be named FillBufferDoneCallback Done. http://codereview.chromium.org/2397001/diff/1/2#newcode332 media/omx/omx_codec_unittest.cc:332: On 2010/06/01 19:36:09, Alpha wrote: > Remove this empty line. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode356 media/omx/omx_codec_unittest.cc:356: output_pool.clear(); On 2010/06/01 19:36:09, Alpha wrote: > Try to make this variable local instead of global. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode377 media/omx/omx_codec_unittest.cc:377: output_pool.clear(); On 2010/06/01 19:36:09, Alpha wrote: > Try to make this variable local instead of global. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode397 media/omx/omx_codec_unittest.cc:397: // Give input buffers to OmxCodec. OmxCodec will make a new On 2010/06/01 19:36:09, Alpha wrote: > Should be OmxVideoDecodeEngine. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode408 media/omx/omx_codec_unittest.cc:408: CHECK(kBufferCount == static_cast<int>(input_units_.size())); On 2010/06/01 19:36:09, Alpha wrote: > I think EXPECT_EQ is better than CHECK here. > > I used CHECK because it is an assertion to prevent runtime failure. If we are > just checking results to fail the test we should use EXPECT_XX. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode413 media/omx/omx_codec_unittest.cc:413: // send EndOfStream, expect eos flag On 2010/06/01 19:36:09, Alpha wrote: > nit: capital letter at the beginning of line, period at the end. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode413 media/omx/omx_codec_unittest.cc:413: // send EndOfStream, expect eos flag On 2010/06/01 19:36:09, Alpha wrote: > This block of EOS code seems to be the same as below, maybe add a method to > share the code. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode427 media/omx/omx_codec_unittest.cc:427: // shutdown On 2010/06/01 19:36:09, Alpha wrote: > nit: capital letter at the beginning of line, period at the end. > Done. http://codereview.chromium.org/2397001/diff/1/2#newcode457 media/omx/omx_codec_unittest.cc:457: // Give input buffers to OmxCodec. OmxCodec will make a new On 2010/06/01 19:36:09, Alpha wrote: > This is not OmxCodec any more. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode467 media/omx/omx_codec_unittest.cc:467: CHECK(kBufferCount == static_cast<int>(input_units_.size())); On 2010/06/01 19:36:09, Alpha wrote: > These lines shold be EXPECT_EQ. Done. http://codereview.chromium.org/2397001/diff/1/2#newcode472 media/omx/omx_codec_unittest.cc:472: CHECK(kBufferCount == static_cast<int>(input_units_.size())); On 2010/06/01 19:36:09, Alpha wrote: > These lines should be EXPECT_EQ Done. http://codereview.chromium.org/2397001/diff/1/2#newcode486 media/omx/omx_codec_unittest.cc:486: CHECK(kBufferCount == static_cast<int>(input_units_.size())); On 2010/06/01 19:36:09, Alpha wrote: > These lines should be EXPECT_EQ Done.
LGTM. Thanks for fixing this test. Please make sure this passes the valgrind test. You can run media_unittests under valgrind, please read this and set up valgrind locally: http://www.chromium.org/developers/how-tos/using-valgrind You can then run the test with valgrind like: valgrind out/Debug/media_unittests This will check for memory errors in the code.
don't forget to try and format comments properly including capitalization, periods, etc... http://codereview.chromium.org/2397001/diff/4001/5001 File media/omx/omx_codec_unittest.cc (right): http://codereview.chromium.org/2397001/diff/4001/5001#newcode21 media/omx/omx_codec_unittest.cc:21: using media::VideoDecodeEngine; this shouldn't be needed as the tests are inside the media namespace http://codereview.chromium.org/2397001/diff/4001/5001#newcode38 media/omx/omx_codec_unittest.cc:38: // const char* kRoleName = "awsome"; commented out? http://codereview.chromium.org/2397001/diff/4001/5001#newcode44 media/omx/omx_codec_unittest.cc:44: static std::deque<OMX_BUFFERHEADERTYPE*> output_pool; global non-basic-types (i.e., int, char, float) are disallowed furthermore such globals can lead to bugs in test cases if state is not reset between tests I'd recommend making it a static pointer member inside OmxCodecTest where it is created + assigned in the ctor (test set up) and delete + set to NULL in the dtor (test tear down) http://codereview.chromium.org/2397001/diff/4001/5001#newcode165 media/omx/omx_codec_unittest.cc:165: NewCallback(this, &OmxCodecTest::FeedDoneCallback); indent by extra 2 spaces http://codereview.chromium.org/2397001/diff/4001/5001#newcode167 media/omx/omx_codec_unittest.cc:167: NewCallback(this, &OmxCodecTest::DecodeDoneCallback); indent by extra 2 spaces http://codereview.chromium.org/2397001/diff/4001/5001#newcode171 media/omx/omx_codec_unittest.cc:171: // CHECK(output_units_.size() == 0); commented out? http://codereview.chromium.org/2397001/diff/4001/5001#newcode332 media/omx/omx_codec_unittest.cc:332: nit: remove blank line http://codereview.chromium.org/2397001/diff/4001/5001#newcode345 media/omx/omx_codec_unittest.cc:345: VideoDecodeEngine::EmptyThisBufferCallback* feed_done_cb_; these should be scoped_ptr as you're not deleting them and leaking memory http://codereview.chromium.org/2397001/diff/4001/5001#newcode361 media/omx/omx_codec_unittest.cc:361: &av_stream_, align parameters at ( http://codereview.chromium.org/2397001/diff/4001/5001#newcode381 media/omx/omx_codec_unittest.cc:381: omx_engine_->Initialize(&message_loop_, is it possible to move this inside ExpectStart? http://codereview.chromium.org/2397001/diff/4001/5001#newcode382 media/omx/omx_codec_unittest.cc:382: &av_stream_, align parameters at ( http://codereview.chromium.org/2397001/diff/4001/5001#newcode428 media/omx/omx_codec_unittest.cc:428: EXPECT_CALL(stop_callback_, OnFilterCallback()); is it possible to move this stuff into ExpectStop?
http://codereview.chromium.org/2397001/diff/4001/5001 File media/omx/omx_codec_unittest.cc (right): http://codereview.chromium.org/2397001/diff/4001/5001#newcode21 media/omx/omx_codec_unittest.cc:21: using media::VideoDecodeEngine; On 2010/06/02 01:51:58, scherkus wrote: > this shouldn't be needed as the tests are inside the media namespace Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode38 media/omx/omx_codec_unittest.cc:38: // const char* kRoleName = "awsome"; On 2010/06/02 01:51:58, scherkus wrote: > commented out? Removed. http://codereview.chromium.org/2397001/diff/4001/5001#newcode44 media/omx/omx_codec_unittest.cc:44: static std::deque<OMX_BUFFERHEADERTYPE*> output_pool; On 2010/06/02 01:51:58, scherkus wrote: > global non-basic-types (i.e., int, char, float) are disallowed > > furthermore such globals can lead to bugs in test cases if state is not reset > between tests > > I'd recommend making it a static pointer member inside OmxCodecTest where it is > created + assigned in the ctor (test set up) and delete + set to NULL in the > dtor (test tear down) > Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode165 media/omx/omx_codec_unittest.cc:165: NewCallback(this, &OmxCodecTest::FeedDoneCallback); On 2010/06/02 01:51:58, scherkus wrote: > indent by extra 2 spaces Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode167 media/omx/omx_codec_unittest.cc:167: NewCallback(this, &OmxCodecTest::DecodeDoneCallback); On 2010/06/02 01:51:58, scherkus wrote: > indent by extra 2 spaces Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode171 media/omx/omx_codec_unittest.cc:171: // CHECK(output_units_.size() == 0); On 2010/06/02 01:51:58, scherkus wrote: > commented out? Removed. http://codereview.chromium.org/2397001/diff/4001/5001#newcode332 media/omx/omx_codec_unittest.cc:332: On 2010/06/02 01:51:58, scherkus wrote: > nit: remove blank line Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode345 media/omx/omx_codec_unittest.cc:345: VideoDecodeEngine::EmptyThisBufferCallback* feed_done_cb_; On 2010/06/02 01:51:58, scherkus wrote: > these should be scoped_ptr as you're not deleting them and leaking memory Those callbacks are deleted inside engine. http://codereview.chromium.org/2397001/diff/4001/5001#newcode361 media/omx/omx_codec_unittest.cc:361: &av_stream_, On 2010/06/02 01:51:58, scherkus wrote: > align parameters at ( Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode381 media/omx/omx_codec_unittest.cc:381: omx_engine_->Initialize(&message_loop_, On 2010/06/02 01:51:58, scherkus wrote: > is it possible to move this inside ExpectStart? Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode382 media/omx/omx_codec_unittest.cc:382: &av_stream_, On 2010/06/02 01:51:58, scherkus wrote: > align parameters at ( Done. http://codereview.chromium.org/2397001/diff/4001/5001#newcode428 media/omx/omx_codec_unittest.cc:428: EXPECT_CALL(stop_callback_, OnFilterCallback()); On 2010/06/02 01:51:58, scherkus wrote: > is it possible to move this stuff into ExpectStop? Done.
LGTM |
