Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
748023002:
Add JPEG parser for JPEG decode acceleration (Closed)
|
Created:
6 years, 1 month ago by kcwu Modified:
5 years, 11 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd JPEG parser for JPEG decode acceleration
BUG=335778
TEST=build and run media_unittests --gtest_filter=JpegParserTest.*
R=posciak@chromium.org, wuchengli@chromium.org
Committed: https://crrev.com/ecfdf4bec75d0c0b4a8c52cb2655c051f6998963
Cr-Commit-Position: refs/heads/master@{#311444}
Patch Set 1 #
Total comments: 63
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 61
Patch Set 4 : revise code #Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : move to media/filters/ #
Total comments: 23
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 2
Patch Set 10 : revert change of gfx::Size #Patch Set 11 : #Patch Set 12 : const char* for data #
Total comments: 2
Patch Set 13 : #
Total comments: 23
Patch Set 14 : addressed xhwang's comments #
Total comments: 4
Patch Set 15 : #Patch Set 16 : rebase #
Messages
Total messages: 38 (8 generated)
https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:5: #include "base/big_endian.h" vaapi_jpeg_parser.h already includes this. remove https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:34: DLOG(ERROR) << "Only support 8 bit precision"; print the actual precision https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:38: result_.num_component >= arraysize(result_.components)) add DLOG https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:41: // 64k*64k is max. in the JPEG standard. VAAPI doesn't support larger than s/max./the maximum/. s/64k*64k/Size 64k*64k/ https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:43: if (result_.visible_height > 16384 || result_.visible_width > 16384) { Should 16384 come from va.h or somewhere else? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:44: DLOG(ERROR) << "VAAPI don't support size larger than 16k*16k"; print the actual size https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:49: JpegComponent* component = &result_.components[i]; nit: use JpegComponent&? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:53: DLOG(ERROR) << "Only allow component id <= num_component: " Print both numbers at the end is not very clear. Maybe "Component id (xx) should be <= num_component(xx)"? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:58: uint8_t hv; Is hv a common Jpeg terminology? If not, use more descriptive name. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:118: if (table_class >= 2) // invalid Add DLOG. Add DLOG for all non reader.ReadU8 failures. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:120: if (table_id >= 2) // only support baseline Add DLOG https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:125: if (is_ac_table) just use table_class == 1 here? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:133: for (unsigned i = 0; i < arraysize(table->code_length); i++) s/unsigned/size_t/ https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:136: if (count > sizeof(table->code_value)) add DLOG https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:154: if (result_.scan.num_component != result_.num_component) add DLOG https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:216: if (reader.remaining() < size) add DLOG https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:220: if (size < sizeof(size)) add DLOG https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:258: DVLOG(4) << "unknown marker " << static_cast<int>(marker2); Why do we need this cast? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:269: // Scan data is following scan header immediately. s/is following/follows/ https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:281: uint8_t marker2; nit: combine with the previous line https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. - How big is content/test/data/jpeg/pixel-1280x720.bin? - Most media test files are in media/test/data. Are some of them used by vda or vea tests? If yes, pixel-1280x720.bin can be moved there. - Need a README to explain pixel-1280x720.bin. See the example in media/test/data/README. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:41: // Parsing result of JPEG component s/of JPEG/of a JPEG/ https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:77: class CONTENT_EXPORT VaapiJpegParser { The only thing this only applies to vaapi is the maximum width and height? If yes, I think this can be renamed to JpegParse. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:79: VaapiJpegParser(const uint8_t* buffer, size_t length); Document who's the owner of memory in |buffer|. Add documentation for the constructor. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:84: bool Parse(); add blank line https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:87: const JpegParseResult* GetParsedResult() { return &result_; } Can we combine Parse and GetParsedResult? Make Parse() return JpegParseResult*. Return NULL if failed. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:90: bool ParseSOI(base::BigEndianReader reader); Keep the same order of functions in vaapi_jpeg_parser.h and .cc. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:90: bool ParseSOI(base::BigEndianReader reader); Does it save a copy if we use const base::BigEndianReader&? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:98: size_t length_; Need comments for buffer_ and length_. Who owns the memory of |buffer_|? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_jpeg_parser_unittest.cc (right): https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:14: TEST(VaapiJpegParserTest, Parsing) { Is there any Parse() failure case worth testing? https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:23: base::FilePath file_path = data_dir.AppendASCII("pixel-1280x720.bin"); Combine this with previous statement. base::FilePath file_path = data_dir.AppendASCII("content") .AppendASCII("test") .AppendASCII("data") .AppendASCII("jpeg") AppendASCII("pixel-1280x720.bin"); https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:37: ASSERT_EQ(result->visible_width, 1280); Use EXPECT_ for all the following cases to verify more fields in one test. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:51: ASSERT_EQ(result->scan.data_size, 121150u); You should verify all or most the fields in vaapi_jpeg_parser.h.
I will add more test cases later. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:5: #include "base/big_endian.h" On 2014/12/08 09:25:10, wuchengli wrote: > vaapi_jpeg_parser.h already includes this. remove Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:34: DLOG(ERROR) << "Only support 8 bit precision"; On 2014/12/08 09:25:11, wuchengli wrote: > print the actual precision Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:38: result_.num_component >= arraysize(result_.components)) On 2014/12/08 09:25:10, wuchengli wrote: > add DLOG Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:41: // 64k*64k is max. in the JPEG standard. VAAPI doesn't support larger than On 2014/12/08 09:25:10, wuchengli wrote: > s/max./the maximum/. s/64k*64k/Size 64k*64k/ Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:43: if (result_.visible_height > 16384 || result_.visible_width > 16384) { On 2014/12/08 09:25:10, wuchengli wrote: > Should 16384 come from va.h or somewhere else? There is no contant from va.h for this. It's heard from Intel in email. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:44: DLOG(ERROR) << "VAAPI don't support size larger than 16k*16k"; On 2014/12/08 09:25:10, wuchengli wrote: > print the actual size Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:49: JpegComponent* component = &result_.components[i]; On 2014/12/08 09:25:10, wuchengli wrote: > nit: use JpegComponent&? Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:53: DLOG(ERROR) << "Only allow component id <= num_component: " On 2014/12/08 09:25:11, wuchengli wrote: > Print both numbers at the end is not very clear. Maybe "Component id (xx) should > be <= num_component(xx)"? Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:58: uint8_t hv; On 2014/12/08 09:25:10, wuchengli wrote: > Is hv a common Jpeg terminology? If not, use more descriptive name. It's not so meaningful to give a name for this. They are just two 4-bit numbers packed into a byte. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:118: if (table_class >= 2) // invalid On 2014/12/08 09:25:10, wuchengli wrote: > Add DLOG. Add DLOG for all non reader.ReadU8 failures. Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:120: if (table_id >= 2) // only support baseline On 2014/12/08 09:25:10, wuchengli wrote: > Add DLOG Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:125: if (is_ac_table) On 2014/12/08 09:25:10, wuchengli wrote: > just use table_class == 1 here? Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:133: for (unsigned i = 0; i < arraysize(table->code_length); i++) On 2014/12/08 09:25:10, wuchengli wrote: > s/unsigned/size_t/ Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:136: if (count > sizeof(table->code_value)) On 2014/12/08 09:25:11, wuchengli wrote: > add DLOG Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:154: if (result_.scan.num_component != result_.num_component) On 2014/12/08 09:25:10, wuchengli wrote: > add DLOG Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:216: if (reader.remaining() < size) On 2014/12/08 09:25:10, wuchengli wrote: > add DLOG Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:220: if (size < sizeof(size)) On 2014/12/08 09:25:10, wuchengli wrote: > add DLOG Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:258: DVLOG(4) << "unknown marker " << static_cast<int>(marker2); On 2014/12/08 09:25:10, wuchengli wrote: > Why do we need this cast? marker2 is char. It will print the character if no casting. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:269: // Scan data is following scan header immediately. On 2014/12/08 09:25:11, wuchengli wrote: > s/is following/follows/ Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.cc:281: uint8_t marker2; On 2014/12/08 09:25:10, wuchengli wrote: > nit: combine with the previous line Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/12/08 09:25:11, wuchengli wrote: > - How big is content/test/data/jpeg/pixel-1280x720.bin? it's 120KB. > - Most media test files are in media/test/data. Are some of them used by vda or > vea tests? If yes, pixel-1280x720.bin can be moved there. I found content/test/data/media have many media test files as well. So I think it's ok to put file here. And since contest/test/data/media looks like single self-contained test files, I put file in jpeg/ directory instead of putting in content/test/data/media > - Need a README to explain pixel-1280x720.bin. See the example in > media/test/data/README. done https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:41: // Parsing result of JPEG component On 2014/12/08 09:25:11, wuchengli wrote: > s/of JPEG/of a JPEG/ Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:77: class CONTENT_EXPORT VaapiJpegParser { On 2014/12/08 09:25:11, wuchengli wrote: > The only thing this only applies to vaapi is the maximum width and height? If > yes, I think this can be renamed to JpegParse. And some sampling factor (see the end of ParseSOF). It's also limited to baseline profile due to vaapi. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:79: VaapiJpegParser(const uint8_t* buffer, size_t length); On 2014/12/08 09:25:11, wuchengli wrote: > Document who's the owner of memory in |buffer|. Add documentation for the > constructor. Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:84: bool Parse(); On 2014/12/08 09:25:11, wuchengli wrote: > add blank line Acknowledged. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:87: const JpegParseResult* GetParsedResult() { return &result_; } On 2014/12/08 09:25:11, wuchengli wrote: > Can we combine Parse and GetParsedResult? Make Parse() return JpegParseResult*. > Return NULL if failed. Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:90: bool ParseSOI(base::BigEndianReader reader); On 2014/12/08 09:25:11, wuchengli wrote: > Does it save a copy if we use const base::BigEndianReader&? No, it is not constant since Read() will modify its internal state. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:90: bool ParseSOI(base::BigEndianReader reader); On 2014/12/08 09:25:11, wuchengli wrote: > Keep the same order of functions in vaapi_jpeg_parser.h and .cc. Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser.h:98: size_t length_; On 2014/12/08 09:25:11, wuchengli wrote: > Need comments for buffer_ and length_. Who owns the memory of |buffer_|? Done. https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_jpeg_parser_unittest.cc (right): https://codereview.chromium.org/748023002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:37: ASSERT_EQ(result->visible_width, 1280); On 2014/12/08 09:25:11, wuchengli wrote: > Use EXPECT_ for all the following cases to verify more fields in one test. Done.
Looking good. I only have some minor comments. I'll review again after you add the tests. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:37: if (result_.num_component != 3 || Follow the (read) order of precesion, width/height, and num_component. Swap the order of if (result_.num_component != 3) and if (result_.visible_height > 16384). https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:46: // 16k*16k. combine with the previous line https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:104: if (table_id >= kJpegQuantizationTableNum) add a DLOG https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:118: uint8_t t; nit: s/t/tmp/ to be consistent with ParseDQT https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:235: } while (marker2 == MARKER1); // skip padding s/padding/fill bytes/ according to the spec. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:241: DLOG(ERROR) << "Ill-formed JPEG. Remain size (" << reader.remaining() s/Remain/Remaining/ https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:19: SOF0 = 0xC0, // start of image (baseline) s/image/frame/ https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:96: // JPEG buffer. It is still owned by caller. It's not obviously who the caller is here. Maybe change to "The parser doesn't own the memory." https://codereview.chromium.org/748023002/diff/20001/content/test/data/jpeg/R... File content/test/data/jpeg/README (right): https://codereview.chromium.org/748023002/diff/20001/content/test/data/jpeg/R... content/test/data/jpeg/README:5: pixel-1280x720.bin - single MJEPG encoded frame of 1280x720, captured on Pixel. How about renaming it to pixel-1280x720.jpg? Some applications (including chrome?) can show it directly.
https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:37: if (result_.num_component != 3 || On 2014/12/17 09:03:02, wuchengli wrote: > Follow the (read) order of precesion, width/height, and num_component. Swap the > order of if (result_.num_component != 3) and if (result_.visible_height > > 16384). Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:46: // 16k*16k. On 2014/12/17 09:03:02, wuchengli wrote: > combine with the previous line Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:104: if (table_id >= kJpegQuantizationTableNum) On 2014/12/17 09:03:02, wuchengli wrote: > add a DLOG Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:118: uint8_t t; On 2014/12/17 09:03:02, wuchengli wrote: > nit: s/t/tmp/ to be consistent with ParseDQT Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:235: } while (marker2 == MARKER1); // skip padding On 2014/12/17 09:03:02, wuchengli wrote: > s/padding/fill bytes/ according to the spec. Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:241: DLOG(ERROR) << "Ill-formed JPEG. Remain size (" << reader.remaining() On 2014/12/17 09:03:02, wuchengli wrote: > s/Remain/Remaining/ Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:19: SOF0 = 0xC0, // start of image (baseline) On 2014/12/17 09:03:02, wuchengli wrote: > s/image/frame/ Done. https://codereview.chromium.org/748023002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:96: // JPEG buffer. It is still owned by caller. On 2014/12/17 09:03:02, wuchengli wrote: > It's not obviously who the caller is here. Maybe change to "The parser doesn't > own the memory." Done. https://codereview.chromium.org/748023002/diff/20001/content/test/data/jpeg/R... File content/test/data/jpeg/README (right): https://codereview.chromium.org/748023002/diff/20001/content/test/data/jpeg/R... content/test/data/jpeg/README:5: pixel-1280x720.bin - single MJEPG encoded frame of 1280x720, captured on Pixel. On 2014/12/17 09:03:02, wuchengli wrote: > How about renaming it to pixel-1280x720.jpg? Some applications (including > chrome?) can show it directly. I was naming it not as "jpg" because many applications cannot show it. But I don't know chrome can show it. Now I'm convinced.
kcwu@chromium.org changed reviewers: + owenlin@chromium.org
https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:19: bool VaapiJpegParser::ParseSOF(BigEndianReader reader) { Do we really want to copy readers passing them to methods? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:24: return false; Perhaps you could have a macro READ_U8_OR_RETURN_FALSE() https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:39: if (result_.visible_height > 16384 || result_.visible_width > 16384) { We should not have vaapi specifics in the parser. Please keep it generic. Vaapi client can check this after parsing and decide to bail out. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:44: if (result_.num_component != 3 || make this a constant? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:45: result_.num_component >= arraysize(result_.components)) { What's the max num components by spec? Can we just have max in result? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:51: for (int i = 0; i < result_.num_component; i++) { size_t i https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:64: component.horizontal_sampling_factor = hv / 16; Do we need to check for more things, like if it's 0, etc? In general, whenever we read *anything* from the stream, and the spec gives you clear range of allowed values, we have to verify the value is correct. See for example in H264Parser IN_RANGE_OR_RETURN(). https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:74: DLOG(ERROR) << "VAAPI don't supports horizontal sampling factor of Y" No vaapi anywhere please. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:141: if (!reader.ReadBytes(&table->code_length, sizeof(table->code_length))) Where does code_length come from? Do we need to validate values in table in order to not have overflows in loop below? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:199: // unused fields, only for value checking Capital letter at beginning, dot at end. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:318: if (ParseSOI(BigEndianReader(reader.ptr(), reader.remaining()))) Why are we creating a separate reader for each parse method? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. This class could be in media/filters. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:14: static const int kJpegHuffmanTableNum_baseline = 2; all size_t please. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:28: // Parsing result of JPEG DHT marker Dots at end of sentences everywhere please. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:38: uint8_t value[64]; // only support 8 bits quantization table Is this a limitation of this parser, or is this according to spec? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:57: const char* data; const uint8_t* https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:62: uint16_t visible_width; gfx::Size ? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:64: uint8_t num_component; num_components. Please describe how this differs from kJpegComponentNum. I'm guessing you probably want to s/kJpegComponentNum/kJpegMaxComponents/. Not sure about other constants? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:79: // JPEG buffer and its size. The parser doesn't own the memory. |buffer| |sizes| What does the buffer have to contain, a JPEG picture? Can it take any JPEG picture? What happens if the picture is not supported? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:80: VaapiJpegParser(const uint8_t* buffer, size_t length); Feels to me that we could easily make this not Vaapi-specific.
https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:13: : buffer_(buffer), length_(length) { DCHECK on buffer and length ? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:129: DLOG(ERROR) << "Table id(" << table_id << ") >= 2 is invalid for " Is this the result of git cl format ? I thought there are some better format. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:292: reader.Skip(size); We should check the return code. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:18: enum JpegMarker { Do we need to export this ? Or it should be hide in the c++ file. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:73: // VaapiJpegParser parses JPEG header. It's not a full feature JPEG parser full featured ? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:85: // the returned result memory. It is not convincing that the life time of the parsed result should be same as the parser. How about passing the result as an output argument to this function. Return true iff the result is successfully parsed. bool Parse(JpegParseResult* result); or make this class can be reused: bool Parse(const uint8_t* buffer, size_t length, JpegParseResult *result) https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:89: bool ParseSOF(base::BigEndianReader reader); Why |reader| is not base::BigEndianReader* or const base::BigEndianReader& ? Or just ParseSOF(const uint8_t* data, size_t size) and you can create the BigEndianReaders inside those functions. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser_unittest.cc (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:17: data_dir = data_dir.AppendASCII("content") How about data_dir.Append(FILE_PATH_LITERAL('content/test/data/jpeg/pixel-1280x720.jpg')); ? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:90: int main(int argc, char** argv) { I think it could be part of the content_unittests. (Instead of a standalone executible). Add it to content/content_tests.gypi.
addressed most of comments in patch 4-6. and move the code to media/filters in patch 7. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:13: : buffer_(buffer), length_(length) { On 2014/12/26 07:13:39, Owen Lin wrote: > DCHECK on buffer and length ? Acknowledged. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:19: bool VaapiJpegParser::ParseSOF(BigEndianReader reader) { On 2014/12/26 04:47:40, Pawel Osciak wrote: > Do we really want to copy readers passing them to methods? I want to make sure the reader of one marker won't read beyond the length of that marker. So one reader for one marker is necessary (easier). Regarding to copy, it depends on compiler. I believed modern compilers will do this kind of simple optimization for us. Anyway, I changed it to plain pointer and size. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:24: return false; On 2014/12/26 04:47:40, Pawel Osciak wrote: > Perhaps you could have a macro > READ_U8_OR_RETURN_FALSE() Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:39: if (result_.visible_height > 16384 || result_.visible_width > 16384) { On 2014/12/26 04:47:39, Pawel Osciak wrote: > We should not have vaapi specifics in the parser. Please keep it generic. > Vaapi client can check this after parsing and decide to bail out. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:44: if (result_.num_component != 3 || On 2014/12/26 04:47:39, Pawel Osciak wrote: > make this a constant? Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:45: result_.num_component >= arraysize(result_.components)) { On 2014/12/26 04:47:40, Pawel Osciak wrote: > What's the max num components by spec? Can we just have max in result? In theory (generic JPEG spec), it could be 255. But in specific format, the number is much smaller. For example, JFIF is 1 and 3. In practice, the max is 4 (ref: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe... ) VAAPI only support 3. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:51: for (int i = 0; i < result_.num_component; i++) { On 2014/12/26 04:47:40, Pawel Osciak wrote: > size_t i Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:64: component.horizontal_sampling_factor = hv / 16; On 2014/12/26 04:47:40, Pawel Osciak wrote: > Do we need to check for more things, like if it's 0, etc? > > In general, whenever we read *anything* from the stream, and the spec gives you > clear range of allowed values, we have to verify the value is correct. > See for example in H264Parser IN_RANGE_OR_RETURN(). Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:74: DLOG(ERROR) << "VAAPI don't supports horizontal sampling factor of Y" On 2014/12/26 04:47:40, Pawel Osciak wrote: > No vaapi anywhere please. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:129: DLOG(ERROR) << "Table id(" << table_id << ") >= 2 is invalid for " On 2014/12/26 07:13:39, Owen Lin wrote: > Is this the result of git cl format ? I thought there are some better format. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:141: if (!reader.ReadBytes(&table->code_length, sizeof(table->code_length))) On 2014/12/26 04:47:40, Pawel Osciak wrote: > Where does code_length come from? Do we need to validate values in table in > order to not have overflows in loop below? I don't understand how to overflow. Please clarify? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:199: // unused fields, only for value checking On 2014/12/26 04:47:40, Pawel Osciak wrote: > Capital letter at beginning, dot at end. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:292: reader.Skip(size); On 2014/12/26 07:13:39, Owen Lin wrote: > We should check the return code. I already check "reader.remaining() < size" above. And if there is really something wrong, it will also failed when next read. Do you still think we should check the return value here? https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:318: if (ParseSOI(BigEndianReader(reader.ptr(), reader.remaining()))) On 2014/12/26 04:47:40, Pawel Osciak wrote: > Why are we creating a separate reader for each parse method? Acknowledged. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/12/26 04:47:40, Pawel Osciak wrote: > This class could be in media/filters. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:14: static const int kJpegHuffmanTableNum_baseline = 2; On 2014/12/26 04:47:40, Pawel Osciak wrote: > all size_t please. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:18: enum JpegMarker { On 2014/12/26 07:13:39, Owen Lin wrote: > Do we need to export this ? Or it should be hide in the c++ file. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:28: // Parsing result of JPEG DHT marker On 2014/12/26 04:47:40, Pawel Osciak wrote: > Dots at end of sentences everywhere please. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:38: uint8_t value[64]; // only support 8 bits quantization table On 2014/12/26 04:47:40, Pawel Osciak wrote: > Is this a limitation of this parser, or is this according to spec? Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:57: const char* data; On 2014/12/26 04:47:40, Pawel Osciak wrote: > const uint8_t* Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:62: uint16_t visible_width; On 2014/12/26 04:47:40, Pawel Osciak wrote: > gfx::Size ? Done. The drawback is needing ctor since it is no longer plain data. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:64: uint8_t num_component; On 2014/12/26 04:47:40, Pawel Osciak wrote: > num_components. Please describe how this differs from kJpegComponentNum. I'm > guessing you probably want to s/kJpegComponentNum/kJpegMaxComponents/. Not sure > about other constants? Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:73: // VaapiJpegParser parses JPEG header. It's not a full feature JPEG parser On 2014/12/26 07:13:39, Owen Lin wrote: > full featured ? Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:79: // JPEG buffer and its size. The parser doesn't own the memory. On 2014/12/26 04:47:40, Pawel Osciak wrote: > |buffer| |sizes| > What does the buffer have to contain, a JPEG picture? Can it take any JPEG > picture? What happens if the picture is not supported? Acknowledged. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:80: VaapiJpegParser(const uint8_t* buffer, size_t length); On 2014/12/26 04:47:40, Pawel Osciak wrote: > Feels to me that we could easily make this not Vaapi-specific. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:85: // the returned result memory. On 2014/12/26 07:13:39, Owen Lin wrote: > It is not convincing that the life time of the parsed result should be same as > the parser. How about passing the result as an output argument to this function. > Return true iff the result is successfully parsed. > > bool Parse(JpegParseResult* result); > > or make this class can be reused: > > bool Parse(const uint8_t* buffer, size_t length, JpegParseResult *result) Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:89: bool ParseSOF(base::BigEndianReader reader); On 2014/12/26 07:13:39, Owen Lin wrote: > Why |reader| is not base::BigEndianReader* or const base::BigEndianReader& ? > > Or just ParseSOF(const uint8_t* data, size_t size) > and you can create the BigEndianReaders inside those functions. Done. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser_unittest.cc (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:17: data_dir = data_dir.AppendASCII("content") On 2014/12/26 07:13:39, Owen Lin wrote: > How about > data_dir.Append(FILE_PATH_LITERAL('content/test/data/jpeg/pixel-1280x720.jpg')); > ? Make the code more portable. Windows use "\" path separator. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser_unittest.cc:90: int main(int argc, char** argv) { On 2014/12/26 07:13:39, Owen Lin wrote: > I think it could be part of the content_unittests. (Instead of a standalone > executible). > > Add it to content/content_tests.gypi. moved to media_unittests
https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:141: if (!reader.ReadBytes(&table->code_length, sizeof(table->code_length))) On 2014/12/27 13:27:34, kcwu wrote: > On 2014/12/26 04:47:40, Pawel Osciak wrote: > > Where does code_length come from? Do we need to validate values in table in > > order to not have overflows in loop below? > > I don't understand how to overflow. Please clarify? It's actually the length of the |code_length| ... and is a value decided in compiling time. I believe it should be fine. https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.cc:292: reader.Skip(size); On 2014/12/27 13:27:34, kcwu wrote: > On 2014/12/26 07:13:39, Owen Lin wrote: > > We should check the return code. > > I already check "reader.remaining() < size" above. And if there is really > something wrong, it will also failed when next read. Do you still think we > should check the return value here? Acknowledged. https://codereview.chromium.org/748023002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_parser.cc:79: result->visible_size = gfx::Size(visible_width, visible_height); Why not result->visible_size.SetSize()? https://codereview.chromium.org/748023002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_parser.h:70: JpegParser(); If all members are static, why we need constructor and destructor? https://codereview.chromium.org/748023002/diff/120001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/748023002/diff/120001/media/media.gyp#newcode3 media/media.gyp:3: # found in the LICENSE file. How about the media/BUILD.gn? Should we update it as well?
https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:6: #include "media/filters/jpeg_parser.h" This should be moved to the first include. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:82: DLOG(ERROR) << "Only support 8 bit precision, not " nit: s/8 bit/8-bit/ https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:122: READ_U8_OR_RETURN_FALSE(&tmp); Can we rename tmp to something more meaningful? READ_U8_OR_RETURN_FALSE will print "tmp" if there's an error. Same for others. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:342: bool JpegParser::Parse(const uint8_t* buffer, Move this before ParseSOF to be consistent with the order in the header file. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:354: LOG(ERROR) << "Not a JPEG"; This is not an actual error. The caller should control the logging. s/LOG/DLOG/ https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:2: // Use of this source code is governed by a BSD-style license that can be Can you explain why we are not using ffmpeg/libavcodec/mjpeg_parser.c? Also it seems mjpeg_parser is not exposed to chrome. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:8: #include "base/big_endian.h" Move this to jpeg_parser.cc? https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:53: JpegParseResult(); Do we need the constructor? https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:74: // valid and supported JPEG baseline sequential process. If parsed "supported" sounds strange. s/supported JPEG baseline sequential process/JPEG baseline sequential process is present/? https://codereview.chromium.org/748023002/diff/120001/media/test/data/README File media/test/data/README (right): https://codereview.chromium.org/748023002/diff/120001/media/test/data/README#... media/test/data/README:162: // JPEG test files: s/JPEG/JPEG parser/
https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:6: #include "media/filters/jpeg_parser.h" On 2015/01/05 07:32:55, wuchengli wrote: > This should be moved to the first include. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... Done. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:82: DLOG(ERROR) << "Only support 8 bit precision, not " On 2015/01/05 07:32:55, wuchengli wrote: > nit: s/8 bit/8-bit/ Done. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:122: READ_U8_OR_RETURN_FALSE(&tmp); On 2015/01/05 07:32:55, wuchengli wrote: > Can we rename tmp to something more meaningful? READ_U8_OR_RETURN_FALSE will > print "tmp" if there's an error. Same for others. Done. Do you feel it make more sense now? https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:342: bool JpegParser::Parse(const uint8_t* buffer, On 2015/01/05 07:32:55, wuchengli wrote: > Move this before ParseSOF to be consistent with the order in the header file. Done. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:354: LOG(ERROR) << "Not a JPEG"; On 2015/01/05 07:32:55, wuchengli wrote: > This is not an actual error. The caller should control the logging. s/LOG/DLOG/ Done. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:2: // Use of this source code is governed by a BSD-style license that can be On 2015/01/05 07:32:55, wuchengli wrote: > Can you explain why we are not using ffmpeg/libavcodec/mjpeg_parser.c? Also it > seems mjpeg_parser is not exposed to chrome. ffmpeg/libavcodec/mjpeg_parser.c only scans for begin and end of frame. The real parsing is mixed with decoding in ffmpeg/libavcodec/mjpegdec.c . It didn't provide hook to let us get parsed result without actual decoding. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:8: #include "base/big_endian.h" On 2015/01/05 07:32:55, wuchengli wrote: > Move this to jpeg_parser.cc? Done. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:53: JpegParseResult(); On 2015/01/05 07:32:55, wuchengli wrote: > Do we need the constructor? gfx::Size is not plain data structure, thus it needs a constructor. https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:74: // valid and supported JPEG baseline sequential process. If parsed On 2015/01/05 07:32:55, wuchengli wrote: > "supported" sounds strange. s/supported JPEG baseline sequential process/JPEG > baseline sequential process is present/? Done. https://codereview.chromium.org/748023002/diff/120001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/748023002/diff/120001/media/media.gyp#newcode3 media/media.gyp:3: # found in the LICENSE file. On 2014/12/29 09:03:20, Owen Lin wrote: > How about the media/BUILD.gn? Should we update it as well? Done. https://codereview.chromium.org/748023002/diff/120001/media/test/data/README File media/test/data/README (right): https://codereview.chromium.org/748023002/diff/120001/media/test/data/README#... media/test/data/README:162: // JPEG test files: On 2015/01/05 07:32:55, wuchengli wrote: > s/JPEG/JPEG parser/ This will be used for decoder test as well.
kcwu@chromium.org changed reviewers: + xhwang@chromium.org
add owner review
LGTM https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/120001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:53: JpegParseResult(); On 2015/01/05 08:50:50, kcwu wrote: > On 2015/01/05 07:32:55, wuchengli wrote: > > Do we need the constructor? > > gfx::Size is not plain data structure, thus it needs a constructor. I wouldn't use gfx::Size because it brings ui/gfx dependency and it doesn't really gain anything. You can check with Pawel. https://codereview.chromium.org/748023002/diff/140001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/140001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:169: uint8_t table_class_and_table_id; nit: s/table_class_and_table_id/table_class_and_id/ should be enough.
Pawel, PTAL. https://codereview.chromium.org/748023002/diff/140001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/140001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:169: uint8_t table_class_and_table_id; On 2015/01/05 08:57:13, wuchengli wrote: > nit: s/table_class_and_table_id/table_class_and_id/ should be enough. Done.
https://codereview.chromium.org/748023002/diff/160001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/160001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:95: result->visible_size = gfx::Size(visible_width, visible_height); Use SetSize() here ?
https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_parser.h:57: const char* data; On 2014/12/27 13:27:35, kcwu wrote: > On 2014/12/26 04:47:40, Pawel Osciak wrote: > > const uint8_t* > > Done. I decided go back to const char*. Otherwise reinterpret_cast from "const char*" to "const uint8_t*" and later reinterpret_cast+const_cast to "char*" for use, is ugly. https://codereview.chromium.org/748023002/diff/160001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/160001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:95: result->visible_size = gfx::Size(visible_width, visible_height); On 2015/01/07 05:57:37, Owen Lin wrote: > Use SetSize() here ? gfx::Size change reverted
xhwang@ This CL is ready for owner review. Can you take a look? Thanks. https://codereview.chromium.org/748023002/diff/220001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/220001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:28: uint8_t value[64]; // only support 8 bits quantization table for baseline I think "baseline only supports 8 bits quantization table" is more clear that this is not a limitation of this parser.
https://codereview.chromium.org/748023002/diff/220001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/220001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:28: uint8_t value[64]; // only support 8 bits quantization table for baseline On 2015/01/12 05:23:16, wuchengli wrote: > I think "baseline only supports 8 bits quantization table" is more clear that > this is not a limitation of this parser. Done.
https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:34: #define IN_RANGE_OR_RETURN_FALSE(val, min, max) \ We don't like to overuse macros. Can we just have a range check function InRange(), and then in the caller, have something like: if (!InRange(...)) return false; ? https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:76: Can you move the definition of ParseSOI() closer to Parse()? IMHO the readability is better that way. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:79: JpegParseResult* result) { Here and below, can we make the output of these functions more specific. For example, this function only reads the |num_components|, |components|, |visible_height| and |visible_width|. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015? https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:14: static const size_t kJpegMaxHuffmanTableNum_baseline = 2; static not needed for consts. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:14: static const size_t kJpegMaxHuffmanTableNum_baseline = 2; s/_baseline/Baseline https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:63: // JpegParser parses JPEG header. It's not a full featured JPEG parser In this case, how about s/JpegParser/JpegHeaderParser? https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:67: class MEDIA_EXPORT JpegParser { It seems we only need a Parse() function which is static. If so, you don't need to use a class, you can just have a function declared here. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:77: static bool ParseSOF(const char* buffer, Can you hide all these functions in the .cc file? https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... File media/filters/jpeg_parser_unittest.cc (right): https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser_unittest.cc:31: // Verify selected fields empty line after this https://codereview.chromium.org/748023002/diff/240001/media/test/data/README File media/test/data/README (right): https://codereview.chromium.org/748023002/diff/240001/media/test/data/README#... media/test/data/README:163: pixel-1280x720.jpg - single MJEPG encoded frame of 1280x720, captured on Pixel. s/pixel/chromebook pixel? It's not obvious what "pixel" mean here.
https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 18:08:57, xhwang wrote: > 2015 Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:34: #define IN_RANGE_OR_RETURN_FALSE(val, min, max) \ On 2015/01/12 18:08:57, xhwang wrote: > We don't like to overuse macros. Can we just have a range check function > InRange(), and then in the caller, have something like: > > if (!InRange(...)) > return false; > > ? > Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:76: On 2015/01/12 18:08:57, xhwang wrote: > Can you move the definition of ParseSOI() closer to Parse()? IMHO the > readability is better that way. Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:79: JpegParseResult* result) { On 2015/01/12 18:08:58, xhwang wrote: > Here and below, can we make the output of these functions more specific. For > example, this function only reads the |num_components|, |components|, > |visible_height| and |visible_width|. Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 18:08:58, xhwang wrote: > 2015? Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:14: static const size_t kJpegMaxHuffmanTableNum_baseline = 2; On 2015/01/12 18:08:58, xhwang wrote: > static not needed for consts. Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:14: static const size_t kJpegMaxHuffmanTableNum_baseline = 2; On 2015/01/12 18:08:58, xhwang wrote: > s/_baseline/Baseline Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:63: // JpegParser parses JPEG header. It's not a full featured JPEG parser On 2015/01/12 18:08:58, xhwang wrote: > In this case, how about s/JpegParser/JpegHeaderParser? Now I named the function as ParseJpegPicture. Not naming it as ParseJpegHeader because for typical usage, the input is whole JPEG picture, not header only. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:67: class MEDIA_EXPORT JpegParser { On 2015/01/12 18:08:58, xhwang wrote: > It seems we only need a Parse() function which is static. If so, you don't need > to use a class, you can just have a function declared here. Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:77: static bool ParseSOF(const char* buffer, On 2015/01/12 18:08:58, xhwang wrote: > Can you hide all these functions in the .cc file? Done. https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... File media/filters/jpeg_parser_unittest.cc (right): https://codereview.chromium.org/748023002/diff/240001/media/filters/jpeg_pars... media/filters/jpeg_parser_unittest.cc:31: // Verify selected fields On 2015/01/12 18:08:58, xhwang wrote: > empty line after this Done.
Thanks! lgtm % nits https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:54: JpegFrameHeader* frame_header) { Have a comment somewhere in this file about how those output parameters are initialized to 0. Otherwise it's not clear. https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:75: bool ParseJpegPicture(const uint8_t* buffer, You need MEDIA_EXPORT to use it in the unittest.
Please update the CL description, as I believe this is no longer a vaapi-specific class.
https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... File media/filters/jpeg_parser.cc (right): https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... media/filters/jpeg_parser.cc:54: JpegFrameHeader* frame_header) { On 2015/01/13 17:07:14, xhwang wrote: > Have a comment somewhere in this file about how those output parameters are > initialized to 0. Otherwise it's not clear. Done. https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... File media/filters/jpeg_parser.h (right): https://codereview.chromium.org/748023002/diff/260001/media/filters/jpeg_pars... media/filters/jpeg_parser.h:75: bool ParseJpegPicture(const uint8_t* buffer, On 2015/01/13 17:07:14, xhwang wrote: > You need MEDIA_EXPORT to use it in the unittest. Done.
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/748023002/260001
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/748023002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_ng on tryserver.chromium.win (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/748023002/300001
The CQ bit was unchecked by kcwu@chromium.org
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/748023002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/ecfdf4bec75d0c0b4a8c52cb2655c051f6998963 Cr-Commit-Position: refs/heads/master@{#311444} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
