Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1269473002:
Extract IVF parser from VP8 parser unittest (Closed)
DescriptionAdd an IVF parser
The IVF parser would be shared among VP8 and VP9 unittests.
BUG=509500
Committed: https://crrev.com/1c051bb37f94f150a7b268cbb9d66b6e49647b60
Cr-Commit-Position: refs/heads/master@{#342341}
Patch Set 1 #
Total comments: 24
Patch Set 2 : #Patch Set 3 : add copyright header #Patch Set 4 : nits #
Total comments: 4
Patch Set 5 : #
Total comments: 8
Patch Set 6 : DKIF #
Total comments: 4
Patch Set 7 : #Patch Set 8 : fix compile #
Messages
Total messages: 32 (11 generated)
kcwu@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org, xhwang@chromium.org
Looking good. Just a few comments. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:5: namespace { nit: Move this whole block into namespace media. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:59: file_header->unused = ReadU32(ptr_ + 28); Does media::BitReader work for you? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/bit_rea... Manually adding offsets is error prone.... https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:73: frame_header->timestamp = ReadU64(ptr_); ptr_ + 4 ? https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:34: class MEDIA_EXPORT IvfParser { Can you add some comments explaining what IVF stands for? https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:52: const uint8_t* end_; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:52: const uint8_t* end_; Comment on what ptr_ and end_ are.
https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:7: const size_t kIvfFileHeaderSize = 32; Can we use sizeof(IvfFileHeader)? Same for kIvfFrameHeaderSize. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:16: struct MEDIA_EXPORT IvfFileHeader { Remove IvfFileHeader and IvfFrameHeader in video_encode_accelerator_unittest.cc. Move the comments of IvfFileHeader and IvfFrameHeader of video_encode_accelerator_unittest.cc to here. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:26: }; __attribute__((packed)) for video_encode_accelerator_unittest.cc https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... File media/filters/vp8_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... media/filters/vp8_parser_unittest.cc:24: IvfFileHeader ivf_file_header; memset to 0 https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... media/filters/vp8_parser_unittest.cc:27: ASSERT_EQ(ivf_file_header.fourcc, 0x30385056u); // VP80 Make 0x30385056 a constant and share it with video_encode_accelerator_unittest.cc. https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... media/filters/vp8_parser_unittest.cc:30: IvfFrameHeader ivf_frame_header; memset to 0
I changed the implementation and provide ByteSwap() method according to the user case of video_encode_accelerator_unittest.cc. Also added ivf_parser_unittest. PTAL https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:5: namespace { On 2015/07/29 16:43:42, xhwang wrote: > nit: Move this whole block into namespace media. Done. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:7: const size_t kIvfFileHeaderSize = 32; On 2015/07/30 06:26:13, wuchengli wrote: > Can we use sizeof(IvfFileHeader)? Same for kIvfFrameHeaderSize. Done. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:59: file_header->unused = ReadU32(ptr_ + 28); On 2015/07/29 16:43:42, xhwang wrote: > Does media::BitReader work for you? Ah, great! I was finding it in base/ . I will use it in my other CLs. > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/bit_rea... > > Manually adding offsets is error prone.... Revised. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.cc... media/filters/ivf_parser.cc:73: frame_header->timestamp = ReadU64(ptr_); On 2015/07/29 16:43:42, xhwang wrote: > ptr_ + 4 ? Revised. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:16: struct MEDIA_EXPORT IvfFileHeader { On 2015/07/30 06:26:13, wuchengli wrote: > Remove IvfFileHeader and IvfFrameHeader in video_encode_accelerator_unittest.cc. > Move the comments of IvfFileHeader and IvfFrameHeader of > video_encode_accelerator_unittest.cc to here. Done. I preferred timebase_denum and timebase_num name because they are less confusing. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:26: }; On 2015/07/30 06:26:13, wuchengli wrote: > __attribute__((packed)) for video_encode_accelerator_unittest.cc I found #pragma pack is portable (across chrome platforms) way to control padding. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:34: class MEDIA_EXPORT IvfParser { On 2015/07/29 16:43:42, xhwang wrote: > Can you add some comments explaining what IVF stands for? Done. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:52: const uint8_t* end_; On 2015/07/29 16:43:42, xhwang wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1269473002/diff/1/media/filters/ivf_parser.h#... media/filters/ivf_parser.h:52: const uint8_t* end_; On 2015/07/29 16:43:42, xhwang wrote: > Comment on what ptr_ and end_ are. Done. https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... File media/filters/vp8_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... media/filters/vp8_parser_unittest.cc:24: IvfFileHeader ivf_file_header; On 2015/07/30 06:26:13, wuchengli wrote: > memset to 0 Done. https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... media/filters/vp8_parser_unittest.cc:27: ASSERT_EQ(ivf_file_header.fourcc, 0x30385056u); // VP80 On 2015/07/30 06:26:13, wuchengli wrote: > Make 0x30385056 a constant and share it with > video_encode_accelerator_unittest.cc. Where do you suggest to put these fourcc values? https://codereview.chromium.org/1269473002/diff/1/media/filters/vp8_parser_un... media/filters/vp8_parser_unittest.cc:30: IvfFrameHeader ivf_frame_header; On 2015/07/30 06:26:13, wuchengli wrote: > memset to 0 Done.
henryhsu@chromium.org changed reviewers: + henryhsu@chromium.org
https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parse... media/filters/ivf_parser.h:65: // error. |frame_header| will be filled with the frame header. |payload| |payload| what? seems the comment is not finished. https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parse... File media/filters/ivf_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parse... media/filters/ivf_parser_unittest.cc:47: EXPECT_EQ(44, payload - stream.data()); s/44/sizeof(file_header) + sizeof(frame_header)/
https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parse... media/filters/ivf_parser.h:65: // error. |frame_header| will be filled with the frame header. |payload| On 2015/08/05 06:22:55, henryhsu wrote: > |payload| what? seems the comment is not finished. Done. https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parse... File media/filters/ivf_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/60001/media/filters/ivf_parse... media/filters/ivf_parser_unittest.cc:47: EXPECT_EQ(44, payload - stream.data()); On 2015/08/05 06:22:55, henryhsu wrote: > s/44/sizeof(file_header) + sizeof(frame_header)/ Done.
https://codereview.chromium.org/1269473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1274: header.signature[3] = 'F'; memcpy(header.signature, media::kIvfHeaderSignature); https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... media/filters/ivf_parser.cc:47: if (memcmp(file_header->signature, "DKIF", 4) != 0) { s/"DKIF"/media::kIvfHeaderSignature/ https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... media/filters/ivf_parser.h:21: char signature[4]; // signature: 'DKIF' Can we have a constant to store DKIF? Then we can use it in many places. https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... File media/filters/ivf_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... media/filters/ivf_parser_unittest.cc:26: EXPECT_EQ(0, memcmp(file_header.signature, "DKIF", 4)); s/DKIF/media::kIvfHeaderSignature/
https://codereview.chromium.org/1269473002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1274: header.signature[3] = 'F'; On 2015/08/05 10:45:13, henryhsu wrote: > memcpy(header.signature, media::kIvfHeaderSignature); Done. https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... media/filters/ivf_parser.cc:47: if (memcmp(file_header->signature, "DKIF", 4) != 0) { On 2015/08/05 10:45:14, henryhsu wrote: > s/"DKIF"/media::kIvfHeaderSignature/ Done. https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parser.h File media/filters/ivf_parser.h (right): https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... media/filters/ivf_parser.h:21: char signature[4]; // signature: 'DKIF' On 2015/08/05 10:45:14, henryhsu wrote: > Can we have a constant to store DKIF? > Then we can use it in many places. Done. https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... File media/filters/ivf_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/80001/media/filters/ivf_parse... media/filters/ivf_parser_unittest.cc:26: EXPECT_EQ(0, memcmp(file_header.signature, "DKIF", 4)); On 2015/08/05 10:45:14, henryhsu wrote: > s/DKIF/media::kIvfHeaderSignature/ Done.
lgtm
@xhwang, owner review, please. Thanks
rs lgtm with nits https://codereview.chromium.org/1269473002/diff/100001/media/filters/ivf_pars... File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/100001/media/filters/ivf_pars... media/filters/ivf_parser.cc:7: #include "media/base/bit_reader.h" Do you actually use this? https://codereview.chromium.org/1269473002/diff/100001/media/filters/vp8_pars... File media/filters/vp8_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/100001/media/filters/vp8_pars... media/filters/vp8_parser_unittest.cc:28: IvfFrameHeader ivf_frame_header; Here and in other places, can you just do IvfFrameHeader ivf_frame_header = {0}; ?
https://codereview.chromium.org/1269473002/diff/100001/media/filters/ivf_pars... File media/filters/ivf_parser.cc (right): https://codereview.chromium.org/1269473002/diff/100001/media/filters/ivf_pars... media/filters/ivf_parser.cc:7: #include "media/base/bit_reader.h" On 2015/08/06 21:56:10, xhwang wrote: > Do you actually use this? Done. https://codereview.chromium.org/1269473002/diff/100001/media/filters/vp8_pars... File media/filters/vp8_parser_unittest.cc (right): https://codereview.chromium.org/1269473002/diff/100001/media/filters/vp8_pars... media/filters/vp8_parser_unittest.cc:28: IvfFrameHeader ivf_frame_header; On 2015/08/06 21:56:10, xhwang wrote: > Here and in other places, can you just do > > IvfFrameHeader ivf_frame_header = {0}; ? Done.
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, henryhsu@chromium.org Link to the patchset: https://codereview.chromium.org/1269473002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269473002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM for content/common/gpu/media
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269473002/120001
The CQ bit was unchecked by kcwu@chromium.org
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wuchengli@chromium.org, henryhsu@chromium.org Link to the patchset: https://codereview.chromium.org/1269473002/#ps140001 (title: "fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269473002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269473002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1c051bb37f94f150a7b268cbb9d66b6e49647b60 Cr-Commit-Position: refs/heads/master@{#342341} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
