Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1258083004:
Add a Vp9Parser implementation (Closed)
DescriptionAdd a Vp9Parser implementation
Add a class for parsing VP9 bitstreams as described in VP9 draft spec and libvpx.
Also add tests for it to media_unittests.
TEST=media_unittests for Vp9Parser
BUG=509500
Committed: https://crrev.com/2c5ec1f7703801d794e8b50b1be5753daba187ff
Cr-Commit-Position: refs/heads/master@{#342792}
Patch Set 1 #Patch Set 2 : remove dependent files from this CL #
Total comments: 51
Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : nits #
Total comments: 42
Patch Set 6 : move ref slots back to Vp9Parser #
Total comments: 10
Patch Set 7 : #Patch Set 8 : rebase #Patch Set 9 : ReadBool, fix compile warning #
Messages
Total messages: 34 (9 generated)
kcwu@chromium.org changed reviewers: + owenlin@chromium.org, posciak@chromium.org, wuchengli@chromium.org
PTAL This CL depends on https://codereview.chromium.org/1269473002/ IVF parser, and https://codereview.chromium.org/1258083003/ VP9RawBitReader.
Pawel, Does Vp9Parser need to support out of order parsing? (Should it return succeeded or failed?) Because out-of-order interframe may reference to invalid previous frames. I am wondering if I return failed for such case, the decoder won't be able to tell it is a real failure or just an interframe (with invalid parsing result). I'm considering to change the API of parser.
https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:13: int GetMinLog2TileCols(int sb64_cols) { static? Also please document what this function is doing. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:21: int GetMaxLog2TileCols(int sb64_cols) { static? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:50: if (reader_.ReadLiteral(8 * 3) != kSyncCode) { General comment: ReadLiteral/ReadBit should have a way to fail, for example if we are out of data. It should probably return bool and fill argument passed to it. Please see vp8_parser.cc for example. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:52: return false; DVLOG please https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:89: // this bit is not specified in spec?? s/this/This/ s/??/?/ https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:110: for (int i = 0; i < kVp9RefsPerFrame; i++) { s/int/size_t/ Please use size_t in general for iterating over arrays. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:137: Vp9InterpFilter table[] = { const? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:141: return table[reader_.ReadLiteral(2)]; Is this different from return ReadLiteral(2) + 1 ? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:278: fhdr->loop_filter.filter_level = 0; It seems that sometimes we initialize members to 0 (or false), like here, sometimes not (segment->update_map/update_data for example). We should make this consistent, i.e. either depend on the memset to 0, or not (I'd lean towards depending on zeroing and not 0 or false-initializing). https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:284: if (reader_.IsOutOfBuffer()) { This shouldn't be needed once we make ReadBit/Literal test. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:313: if (fhdr->error_resilient_mode) Please add empty line above. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:360: const int kFrameContextLog2 = 2; Any reason to make this specifically into a constant? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:25: BT_601, We should assign explicit values since this is per spec. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:43: static const int kNumSegment = 8; kNumSegments https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:44: static const int kTreeProbs = kNumSegment - 1; kNumTreeProbs https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:45: static const int kPredictionProbs = 3; kNumPredictionProbs https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:89: KEYFRAME, Explicit values please. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:100: enum FrameType frame_type; s/enum//
posciak@chromium.org changed reviewers: + jimbankoski@chromium.org, vmr@chromium.org
kcwu@google.com changed reviewers: + kcwu@google.com
https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:52: return false; On 2015/07/30 08:27:38, Pawel Osciak wrote: > DVLOG please Only this one or all in this class? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:89: // this bit is not specified in spec?? On 2015/07/30 08:27:37, Pawel Osciak wrote: > s/this/This/ > s/??/?/ just keep note. I expect this will be removed before commit. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:141: return table[reader_.ReadLiteral(2)]; On 2015/07/30 08:27:38, Pawel Osciak wrote: > Is this different from return ReadLiteral(2) + 1 ? No difference. I just followed libvpx and feel it reasonable to have a mapping table. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:284: if (reader_.IsOutOfBuffer()) { On 2015/07/30 08:27:37, Pawel Osciak wrote: > This shouldn't be needed once we make ReadBit/Literal test. Since out of data is rare, I'd prefer checked only once here and not every read. It makes the code cleaner (and maybe faster). Macros looks ugly and make the generated code bloat. What do you think? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:360: const int kFrameContextLog2 = 2; On 2015/07/30 08:27:37, Pawel Osciak wrote: > Any reason to make this specifically into a constant? Do you mean why not use literal 2 directly?
https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:52: return false; On 2015/07/30 08:55:49, kcwu1 wrote: > On 2015/07/30 08:27:38, Pawel Osciak wrote: > > DVLOG please > > Only this one or all in this class? All please. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:141: return table[reader_.ReadLiteral(2)]; On 2015/07/30 08:55:49, kcwu1 wrote: > On 2015/07/30 08:27:38, Pawel Osciak wrote: > > Is this different from return ReadLiteral(2) + 1 ? > > No difference. I just followed libvpx and feel it reasonable to have a mapping > table. Acknowledged. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:284: if (reader_.IsOutOfBuffer()) { On 2015/07/30 08:55:49, kcwu1 wrote: > On 2015/07/30 08:27:37, Pawel Osciak wrote: > > This shouldn't be needed once we make ReadBit/Literal test. > > Since out of data is rare, I'd prefer checked only once here and not every read. > It makes the code cleaner (and maybe faster). Macros looks ugly and make the > generated code bloat. What do you think? We can't read beyond memory that we have available. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:360: const int kFrameContextLog2 = 2; On 2015/07/30 08:55:49, kcwu1 wrote: > On 2015/07/30 08:27:37, Pawel Osciak wrote: > > Any reason to make this specifically into a constant? > > Do you mean why not use literal 2 directly? Yes.
https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:284: if (reader_.IsOutOfBuffer()) { On 2015/07/30 08:59:24, Pawel Osciak wrote: > On 2015/07/30 08:55:49, kcwu1 wrote: > > On 2015/07/30 08:27:37, Pawel Osciak wrote: > > > This shouldn't be needed once we make ReadBit/Literal test. > > > > Since out of data is rare, I'd prefer checked only once here and not every > read. > > It makes the code cleaner (and maybe faster). Macros looks ugly and make the > > generated code bloat. What do you think? > > We can't read beyond memory that we have available. The reader returns 0 if out of buffer. The return value is garbage but guarantee it's safe.
On 2015/07/30 09:04:15, kcwu1 wrote: > https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... > File media/filters/vp9_parser.cc (right): > > https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... > media/filters/vp9_parser.cc:284: if (reader_.IsOutOfBuffer()) { > On 2015/07/30 08:59:24, Pawel Osciak wrote: > > On 2015/07/30 08:55:49, kcwu1 wrote: > > > On 2015/07/30 08:27:37, Pawel Osciak wrote: > > > > This shouldn't be needed once we make ReadBit/Literal test. > > > > > > Since out of data is rare, I'd prefer checked only once here and not every > > read. > > > It makes the code cleaner (and maybe faster). Macros looks ugly and make the > > > generated code bloat. What do you think? > > > > We can't read beyond memory that we have available. > > The reader returns 0 if out of buffer. The return value is garbage but guarantee > it's safe. Yes, but then we will not know that we failed...
https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:198: if (reader_.ReadBit()) { Please store this in frame header (as temporal_update). https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:374: fhdr->compressed_header = stream_ + reader_.GetBytesRead(); Could we also store the size of uncompressed header in frame header? https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:21: const int kVp9RefsPerFrame = 3; kVp9NumRefFrames https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:77: return base_qindex == 0 && y_dc_delta == 0 && uv_dc_delta == 0; Do we need && uv_ac_delta_q == 0 here as well?
https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:52: return false; On 2015/07/30 08:59:24, Pawel Osciak wrote: > On 2015/07/30 08:55:49, kcwu1 wrote: > > On 2015/07/30 08:27:38, Pawel Osciak wrote: > > > DVLOG please > > > > Only this one or all in this class? > > All please. Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:110: for (int i = 0; i < kVp9RefsPerFrame; i++) { On 2015/07/30 08:27:37, Pawel Osciak wrote: > s/int/size_t/ > > Please use size_t in general for iterating over arrays. Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:137: Vp9InterpFilter table[] = { On 2015/07/30 08:27:37, Pawel Osciak wrote: > const? Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:198: if (reader_.ReadBit()) { On 2015/07/30 11:52:31, Pawel Osciak wrote: > Please store this in frame header (as temporal_update). Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:278: fhdr->loop_filter.filter_level = 0; On 2015/07/30 08:27:37, Pawel Osciak wrote: > It seems that sometimes we initialize members to 0 (or false), like here, > sometimes not (segment->update_map/update_data for example). We should make this > consistent, i.e. either depend on the memset to 0, or not (I'd lean towards > depending on zeroing and not 0 or false-initializing). Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:313: if (fhdr->error_resilient_mode) On 2015/07/30 08:27:37, Pawel Osciak wrote: > Please add empty line above. Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:360: const int kFrameContextLog2 = 2; On 2015/07/30 08:59:24, Pawel Osciak wrote: > On 2015/07/30 08:55:49, kcwu1 wrote: > > On 2015/07/30 08:27:37, Pawel Osciak wrote: > > > Any reason to make this specifically into a constant? > > > > Do you mean why not use literal 2 directly? > > Yes. Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.cc:374: fhdr->compressed_header = stream_ + reader_.GetBytesRead(); On 2015/07/30 11:52:31, Pawel Osciak wrote: > Could we also store the size of uncompressed header in frame header? Done. And remove |compressed_header| since it is redundant. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:21: const int kVp9RefsPerFrame = 3; On 2015/07/30 11:52:31, Pawel Osciak wrote: > kVp9NumRefFrames Ok, I changed the name kVp9RefFrames -> kVp9NumRefFrames kVp9RefsPerFrame -> kVp9NumRefsPerFrame https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:25: BT_601, On 2015/07/30 08:27:38, Pawel Osciak wrote: > We should assign explicit values since this is per spec. Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:43: static const int kNumSegment = 8; On 2015/07/30 08:27:38, Pawel Osciak wrote: > kNumSegments Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:44: static const int kTreeProbs = kNumSegment - 1; On 2015/07/30 08:27:38, Pawel Osciak wrote: > kNumTreeProbs Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:45: static const int kPredictionProbs = 3; On 2015/07/30 08:27:38, Pawel Osciak wrote: > kNumPredictionProbs Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:77: return base_qindex == 0 && y_dc_delta == 0 && uv_dc_delta == 0; On 2015/07/30 11:52:31, Pawel Osciak wrote: > Do we need && uv_ac_delta_q == 0 here as well? Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:89: KEYFRAME, On 2015/07/30 08:27:38, Pawel Osciak wrote: > Explicit values please. Done. https://chromiumcodereview.appspot.com/1258083004/diff/20001/media/filters/vp... media/filters/vp9_parser.h:100: enum FrameType frame_type; On 2015/07/30 08:27:38, Pawel Osciak wrote: > s/enum// Done.
All comments are addressed. PTAL. https://codereview.chromium.org/1258083004/diff/20001/media/filters/vp9_parse... File media/filters/vp9_parser.cc (right): https://codereview.chromium.org/1258083004/diff/20001/media/filters/vp9_parse... media/filters/vp9_parser.cc:13: int GetMinLog2TileCols(int sb64_cols) { On 2015/07/30 08:27:37, Pawel Osciak wrote: > static? > Also please document what this function is doing. IIUC, no need to add static qualifier if it is already in unnamed namespace. For comments, I don't fully understand these functions and thus only leave reference to spec. https://codereview.chromium.org/1258083004/diff/20001/media/filters/vp9_parse... media/filters/vp9_parser.cc:21: int GetMaxLog2TileCols(int sb64_cols) { On 2015/07/30 08:27:37, Pawel Osciak wrote: > static? Acknowledged. https://codereview.chromium.org/1258083004/diff/20001/media/filters/vp9_parse... media/filters/vp9_parser.cc:50: if (reader_.ReadLiteral(8 * 3) != kSyncCode) { On 2015/07/30 08:27:37, Pawel Osciak wrote: > General comment: ReadLiteral/ReadBit should have a way to fail, for example if > we are out of data. It should probably return bool and fill argument passed to > it. Please see vp8_parser.cc for example. Per our chat. We agreed checking reader failure at the end of parsing is not a bad idea.
https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... media/filters/vp9_parser.cc:70: fhdr->bit_depth = 12; reader_.ReadBit() ? 12 : 10. https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... media/filters/vp9_parser.cc:203: segment->tree_probs[i] = reader_.ReadLiteral(8); use the conditional operator "?:" ? https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... media/filters/vp9_parser.cc:390: stream_ = stream; CHECK on stream_ and size_ ? https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... media/filters/vp9_parser.h:47: static const int kNumFeature = 4; kNumFeatures https://chromiumcodereview.appspot.com/1258083004/diff/60001/media/filters/vp... media/filters/vp9_parser.h:160: // |ref_slots| is the VP9 reference slots maintained by the caller. explain the return value
https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.cc:128: 1 <= fhdr->height && fhdr->height <= kMaxDimension)) { if (width < 1 || width > kMaxDimension || height < 1 || height > kMaxDimension) https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.cc:210: segment->temporal_update = reader_.ReadBit(); Empty line above please. https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.cc:394: return ParseUncompressedHeader(ref_slots, fhdr); Sorry, I think I introduced some confusion, but I think in the end we decided to keep refs context in the parser? Could we keep doing UpdateSlots() here please? https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.h:63: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. s/Member/Members/ s/and // https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.h:79: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. Ditto. https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.h:83: uv_ac_delta == 0; Is this a correct indent (not 4 spaces?). What does git cl format say? https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.h:149: // The parsing context for Vp9Parser to keep track references. s/track/track of/ https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.h:150: struct MEDIA_EXPORT Vp9ReferenceSlots { Do we need this 1-member structure? Perhaps move the slots array into Vp9Parser as ref_slots[] ? https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser.h:162: // Parses one frame and fill parsing result to |fhdr|. s/fill/fills/ Returns true on success, false otherwise. https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... File media/filters/vp9_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/80001/media/filters/vp... media/filters/vp9_parser_unittest.cc:19: stream_.reset(new base::MemoryMappedFile); s/base::MemoryMappedFile/base::MemoryMappedFile()/
https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... File media/filters/vp9_parser.cc (right): https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:68: if (fhdr->profile >= 2) { if (profile == 2 || profile == 3) { ... to make things break less if profile 4 would do something else in the future. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:99: // this bit is not specified in spec?? s/this bit is not specified in spec??// https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parser.h File media/filters/vp9_parser.h (right): https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:3: // found in the LICENSE file. It would be easier to understand this file, if there was short description of the targeted use case for the functionality described in this header and example showing the relationships between Vp9FrameHeader, Vp9ReferenceSlots and Vp9Parser, it would help potential users to understand the code quicker. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:43: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. There's no (explicit) Vp9FrameHeader ctor. Maybe you meant: "... 0-initialized by Vp9Parser::ParseFrame" If you meant by implicit ctor, I thought c++ does not mandate the zero-initialization even though most compilers might do it. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:63: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. ditto https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:79: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. ditto https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:149: // The parsing context for Vp9Parser to keep track references. What is the designed purpose for having the list of frame sizes maintained? How is this going to be used? https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:152: void Update(const Vp9FrameHeader& fhdr); You're not using Vp9ReferenceSlots::Update currently. This gets filled up by the Vp9Parser directly. Is there planned use for the function or is it just leftover? https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:164: // |ref_slots| is the VP9 reference slots maintained by the caller. Explain also the return value. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:171: uint8_t ReadProfile(); As far as I can tell all the private members are just implementation details for the user of the class and their understanding are not essential for the use of the class. Maybe hide them in a forward declared private class defined in the implementation file (PIMPL)?
https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parser.h File media/filters/vp9_parser.h (right): https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:43: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/02 23:43:50, Ville-Mikko Rautio wrote: > There's no (explicit) Vp9FrameHeader ctor. Maybe you meant: > > "... 0-initialized by Vp9Parser::ParseFrame" > > If you meant by implicit ctor, I thought c++ does not mandate the > zero-initialization even though most compilers might do it. Good catch, I think we were both thinking about the memset, not the constructor. But we should have a constructor instead of the memset, something like https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vp8_.... Here and in all the other structs.
https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parse... File media/filters/vp9_parser.cc (right): https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parse... media/filters/vp9_parser.cc:70: fhdr->bit_depth = 12; On 2015/07/31 13:34:00, Owen Lin wrote: > reader_.ReadBit() ? 12 : 10. Done. https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parse... media/filters/vp9_parser.cc:203: segment->tree_probs[i] = reader_.ReadLiteral(8); On 2015/07/31 13:34:00, Owen Lin wrote: > use the conditional operator "?:" ? Done. https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parse... media/filters/vp9_parser.cc:390: stream_ = stream; On 2015/07/31 13:34:00, Owen Lin wrote: > CHECK on stream_ and size_ ? I checked stream but not for size since it sounds "reasonable" to pass 0 to a parser (and failed at the beginning of parsing). https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parser.h File media/filters/vp9_parser.h (right): https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parse... media/filters/vp9_parser.h:47: static const int kNumFeature = 4; On 2015/07/31 13:34:00, Owen Lin wrote: > kNumFeatures Done. https://codereview.chromium.org/1258083004/diff/60001/media/filters/vp9_parse... media/filters/vp9_parser.h:160: // |ref_slots| is the VP9 reference slots maintained by the caller. On 2015/07/31 13:34:00, Owen Lin wrote: > explain the return value Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... File media/filters/vp9_parser.cc (right): https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:68: if (fhdr->profile >= 2) { On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > if (profile == 2 || profile == 3) { > > ... to make things break less if profile 4 would do something else in the > future. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:99: // this bit is not specified in spec?? On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > s/this bit is not specified in spec??// Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:128: 1 <= fhdr->height && fhdr->height <= kMaxDimension)) { On 2015/08/01 15:22:48, Pawel Osciak wrote: > if (width < 1 || width > kMaxDimension || height < 1 || height > kMaxDimension) Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:210: segment->temporal_update = reader_.ReadBit(); On 2015/08/01 15:22:48, Pawel Osciak wrote: > Empty line above please. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.cc:394: return ParseUncompressedHeader(ref_slots, fhdr); On 2015/08/01 15:22:48, Pawel Osciak wrote: > Sorry, I think I introduced some confusion, but I think in the end we decided to > keep refs context in the parser? Could we keep doing UpdateSlots() here please? Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parser.h File media/filters/vp9_parser.h (right): https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:3: // found in the LICENSE file. On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > It would be easier to understand this file, if there was short description of > the targeted use case for the functionality described in this header and example > showing the relationships between Vp9FrameHeader, Vp9ReferenceSlots and > Vp9Parser, it would help potential users to understand the code quicker. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:43: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/02 23:43:50, Ville-Mikko Rautio wrote: > There's no (explicit) Vp9FrameHeader ctor. Maybe you meant: > > "... 0-initialized by Vp9Parser::ParseFrame" > > If you meant by implicit ctor, I thought c++ does not mandate the > zero-initialization even though most compilers might do it. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:43: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/02 23:58:15, Pawel Osciak wrote: > On 2015/08/02 23:43:50, Ville-Mikko Rautio wrote: > > There's no (explicit) Vp9FrameHeader ctor. Maybe you meant: > > > > "... 0-initialized by Vp9Parser::ParseFrame" > > > > If you meant by implicit ctor, I thought c++ does not mandate the > > zero-initialization even though most compilers might do it. > > Good catch, I think we were both thinking about the memset, not the constructor. > But we should have a constructor instead of the memset, something like > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vp8_.... > > Here and in all the other structs. I'm not so comfortable to write memset(this). If all these structures memset themselves, there are redundant work since they are nested. If only Vp9FrameHeader memset(this) and others don't, then the others still relay on external memset (and they are inconsistent). I'd prefer memset by Vp9Parser. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:63: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/01 15:22:48, Pawel Osciak wrote: > s/Member/Members/ > s/and // Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:63: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > ditto Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:79: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/01 15:22:49, Pawel Osciak wrote: > Ditto. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:79: // Member of Vp9FrameHeader and will be 0-initialized in Vp9FrameHeader's ctor. On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > ditto Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:83: uv_ac_delta == 0; On 2015/08/01 15:22:48, Pawel Osciak wrote: > Is this a correct indent (not 4 spaces?). What does git cl format say? This is "git cl format" did. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:149: // The parsing context for Vp9Parser to keep track references. On 2015/08/01 15:22:48, Pawel Osciak wrote: > s/track/track of/ Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:149: // The parsing context for Vp9Parser to keep track references. On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > What is the designed purpose for having the list of frame sizes maintained? How > is this going to be used? The sizes are needed to parse next frames as ReadFrameSizeFromRefs. Isn't it? I revised to make it internal state of the parser. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:150: struct MEDIA_EXPORT Vp9ReferenceSlots { On 2015/08/01 15:22:48, Pawel Osciak wrote: > Do we need this 1-member structure? Perhaps move the slots array into Vp9Parser > as ref_slots[] ? I was under impression that it will be used by vp9 decoder, so I packed it into single object to make it "friendly". I will move it back to Vp9Parser since the parser will maintain it. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:152: void Update(const Vp9FrameHeader& fhdr); On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > You're not using Vp9ReferenceSlots::Update currently. This gets filled up by the > Vp9Parser directly. Is there planned use for the function or is it just > leftover? I was misunderstood it will be used by vp9 decoder. Revised. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:162: // Parses one frame and fill parsing result to |fhdr|. On 2015/08/01 15:22:49, Pawel Osciak wrote: > s/fill/fills/ > Returns true on success, false otherwise. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:164: // |ref_slots| is the VP9 reference slots maintained by the caller. On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > Explain also the return value. Done. https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser.h:171: uint8_t ReadProfile(); On 2015/08/02 23:43:49, Ville-Mikko Rautio wrote: > As far as I can tell all the private members are just implementation details for > the user of the class and their understanding are not essential for the use of > the class. Maybe hide them in a forward declared private class defined in the > implementation file (PIMPL)? Indeed they are implementation detail. However I'm not sure it worth to implement PIMPL pattern here. (which makes it not straightforward) https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... File media/filters/vp9_parser_unittest.cc (right): https://codereview.chromium.org/1258083004/diff/80001/media/filters/vp9_parse... media/filters/vp9_parser_unittest.cc:19: stream_.reset(new base::MemoryMappedFile); On 2015/08/01 15:22:49, Pawel Osciak wrote: > s/base::MemoryMappedFile/base::MemoryMappedFile()/ Done.
LGTM I still think Vp9Parser would be cleaner with PIMPL but I will leave it up to you.
lgtm % nits https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:116: if (fhdr->width < 1 || fhdr->width > kMaxDimension || fhdr->height < 1 || Oh sorry, we should say if fhdr->width == 0 not < 1 actually since it's a uint. Same for height. https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:118: DVLOG(1) << "The size of referenced frame is out of range: " s/referenced/reference/ https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:277: if (!reader_.IsValid()) { I'm wondering, why this check here specifically? We have one at the end of the method... https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:374: for (int i = 0; i < kVp9NumRefFrames; i++) { s/int/size_t/ https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.h:212: // The parsing context to keep track references. s/references/of references/
kcwu@chromium.org changed reviewers: + xhwang@chromium.org
@xhwang, owner review, please. Thanks https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:116: if (fhdr->width < 1 || fhdr->width > kMaxDimension || fhdr->height < 1 || On 2015/08/05 07:46:32, Pawel Osciak wrote: > Oh sorry, we should say if fhdr->width == 0 not < 1 actually since it's a uint. > Same for height. Done. https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:118: DVLOG(1) << "The size of referenced frame is out of range: " On 2015/08/05 07:46:32, Pawel Osciak wrote: > s/referenced/reference/ Done. https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:277: if (!reader_.IsValid()) { On 2015/08/05 07:46:32, Pawel Osciak wrote: > I'm wondering, why this check here specifically? We have one at the end of the > method... Here is a "return true". https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.cc:374: for (int i = 0; i < kVp9NumRefFrames; i++) { On 2015/08/05 07:46:32, Pawel Osciak wrote: > s/int/size_t/ Done. https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/1258083004/diff/100001/media/filters/v... media/filters/vp9_parser.h:212: // The parsing context to keep track references. On 2015/08/05 07:46:32, Pawel Osciak wrote: > s/references/of references/ Done.
rs lgtm
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmr@chromium.org, posciak@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1258083004/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258083004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258083004/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
The patchset sent to the CQ was uploaded after l-g-t-m from vmr@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1258083004/#ps160001 (title: "ReadBool, fix compile warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258083004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258083004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2c5ec1f7703801d794e8b50b1be5753daba187ff Cr-Commit-Position: refs/heads/master@{#342792} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
