Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133993002:
Parse VP9 compressed header (Closed)
|
Created:
4 years, 5 months ago by kcwu Modified:
4 years, 4 months ago CC:
DaleCurtis, chromium-reviews, feature-media-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement VP9 compressed header parsing
Follow VP9 draft spec 0.6
- Rename variables according to spec
- More verification for parsed values
- Add spec section numbers as comments
Refactoring the parser for compressed header parsing
- split uncompressed parser to separate file
Unittest:
- Fix frame counting for superframe
TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\*
BUG=none
Committed: https://crrev.com/e5a9a629b414a491ec071e5edbc800f376042733
Cr-Commit-Position: refs/heads/master@{#412442}
Patch Set 1 : Implement VP9 compressed header parsing #
Total comments: 95
Patch Set 2 : fix parse which discard frame while awaiting context update #
Total comments: 19
Patch Set 3 : address Pawel's comments #
Total comments: 6
Patch Set 4 : address Pawel's comments #Patch Set 5 : address Pawel's comments #Patch Set 6 : rebase only #Patch Set 7 : (rebase only) #Patch Set 8 : fix compile #Patch Set 9 : nits #Patch Set 10 : reset curr_frame_info_ when Reset #
Messages
Total messages: 60 (44 generated)
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Parse VP9 compressed header ========== to ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verificaiton for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unitest: - Add bear-vp9.ivf and dump of context values - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none ==========
kcwu@chromium.org changed reviewers: + posciak@chromium.org
Comments for everything apart from media/filters/vp9_uncompressed_header_parser.cc, which I'm still reviewing. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_bool_decoder.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:14: const int kCountToShiftTo128[256] = { Please add a comment what this is. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:29: Vp9BoolDecoder::Vp9BoolDecoder() : valid_(true) {} We could perhaps use class member initialization in the class body syntax instead. Should we also initialize other members? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:39: if (size < 1) { Perhaps we should check this before BitReader initialization? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:48: Fill(); I think ReadLiteral() will Fill() by itself, so perhaps no need to have this here? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:62: int bits_left = reader_->bits_available(); Should all of these be size_t ? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:82: if (!valid_) Should we be checking for valid_ here and in ReadLiteral and returning false, if the callers of ReadBool and ReadLiteral do not distinguish between an error and 0? Perhaps rather the caller of Read*() client should be checking this? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:87: if (!valid_) Perhaps Fill() should just return a bool? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:91: unsigned int split = (bool_range_ * prob + (256 - prob)) >> kBoolSize; Would it be safer to use a fixed-length type? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:122: int x = 0; Should this be a fixed-size type and also unsigned? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:123: for (int i = 0; i < bits; i++) s/int/size_t/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.cc:136: // should have enough bits to fill bool range, this should never happend s/happend/happen./ ? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_bool_decoder.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.h:48: using BigBool = size_t; Is this assuming size_t is 64bit? This may not always be the case. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.h:49: const int kBoolSize = 8; Could you please document what these are? We should perhaps use size_t for sizes. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.h:50: const int kBigBoolSize = sizeof(BigBool) * 8; Should this be called kBigBoolBitSize or something similar? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_bool_decoder.h:63: int count_to_fill_; Should this be a size_t? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_compressed_header_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_compressed_header_parser.cc:7: #include "base/logging.h" Nit: header out of order. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_compressed_header_parser.cc:14: int inv_recenter_nonneg(int v, int m) { I know this is to match the spec, but I think we should use CamelCase, and perhaps just keep this name in the comment? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_compressed_header_parser.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_compressed_header_parser.h:16: bool Parse(const uint8_t* stream, off_t frame_size, Vp9FrameHeader* fhdr); Please document. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_compressed_header_parser.h:48: #endif // MEDIA_FILTERS_VP9_UNCOMPRESSED_HEADER_PARSER_H_ s/UNCOM/COM/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:122: need_update_ = false; Should we also zero out frame_context_ ? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:139: // Verify values from driver explicitly. s/driver/client/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:152: // ContextRefreshCallback. Because we overwrite the value of context here and When would we not need a pending update? Is it possible to have a stream that sets need_update_, but then does not use the context anymore to decode further frames? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:192: base::Callback<void(const Vp9FrameContext&)>* context_refresh_cb) { ContextRefreshCallback* ? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:213: // TODO(kcwu): remove this debug info Marking with a comment so we don't forget to remove. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:229: for (int i = fhdr->uncompressed_header_size; i < frame_info.size; i++) { s/int/size_t/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:257: << static_cast<int>(fhdr->frame_context_idx) << " to update"; Is the cast needed? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:489: Vp9SegmentationParams& segmentation = context_.segmentation; Please move down to before the fist use site. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:33: const int kVp9NumFrameContext = 4; size_t ? s/Context/Contexts/ ? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:35: typedef uint8_t Vp9Prob; Nit: perhaps "using" syntax? Perhaps move to into the context class? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:56: enum Vp9FrameType { Perhaps RefType? We already have FrameType in header, so it'd be a bit confusing, and spec uses FrameType for keyframe/non-keyframe. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:64: enum Vp9TxMode { Perhaps move to Vp9Compressed header? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:73: enum Vp9TxSize { Perhaps we should move this to ReadCoefProbs()? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:136: // calculated from above fields Nit: to match the style, please capitalize first letters and add dots at end of sentences. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:238: uint8_t frame_context_idx_backup; Perhaps frame_context_idx and frame_context_idx_to_save_probs? We should probably add a comment saying "frame_context_idx_to_save_probs is to be used by save_probs() only, and frame_context_idx otherwise". https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:268: using ContextRefreshCallback = base::Callback<void(const Vp9FrameContext&)>; Please document. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:275: bool need_update() const { return need_update_; } s/need/needs/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:278: void Reset(); Please document methods. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:310: // More fields for consistency checking. I think color_space we also use to copy to frame_hdr? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:329: explicit Vp9Parser(bool parsing_compressed_header); Please document, maybe referring to ParseNextFrame() doc. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:340: // state. If |parsing_compressed_header|, this function also fills s/parsing_compressed_header/parsing_compressed_header_/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser_unittest.cc:9: // The syntax of these context dump is described as following. For every s/following/follows/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser_unittest.cc:10: // frames, there are corresponding data in context file, s/frames/frame/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser_unittest.cc:62: return static_cast<bool>(should_update); return should_update == 1? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser_unittest.cc:119: // Allow to parse double frames in order to detect extra frames parsed. s/double/twice as many/ s/extra/any extra/ https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_uncompressed_header_parser.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_uncompressed_header_parser.h:35: // Raw bits decoder for uncompressed frame header. s/decoder/reader/ ? https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/media.gyp... media/media.gyp:571: 'filters/vp8_parser.cc', Should we be adding the other parser files here as well?
Done with UncompressedHeaderParser and diff between CR1 and CR2. https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_parser.cc:198: // |curr_frame_header_| and awaiting context update. "and we are awaiting context update to proceed with compressed header parsing." https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_parser.cc:212: DCHECK(!frames_.empty()); Perhaps we could use frames_.empty() instead of return value from ParseSuperFrame() as an indicator of ParseSuperFrame() failure instead? ParseSuperFrame() could push into a temporary frames deque and std::swap() it with frames_ only on return true? https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_parser.cc:239: static_cast<size_t>(curr_frame_info_.size)) { Perhaps checked cast? https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... File media/filters/vp9_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_parser_unittest.cc:134: void DumpContext(const char* fmt, int idx, const Vp9FrameContext& context) { Do we want to keep this? https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... File media/filters/vp9_uncompressed_header_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:870: VLOG(1) << "tile_cols_log2 should be <= 6"; s/VLOG/DVLOG/? https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:994: // TODO(kcwu): how about color_range? Do we want to leave this TODO? https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:1000: // Some fields are not specified for inter-frame in header, so copy s/Some/Below/ https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:1050: arraysize(context_->frame_context_managers)); maybe static assert on (arraysize() == 1 << 2)?
https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... File media/filters/vp9_bool_decoder.cc (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:14: const int kCountToShiftTo128[256] = { On 2016/08/04 10:20:18, Pawel Osciak wrote: > Please add a comment what this is. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:29: Vp9BoolDecoder::Vp9BoolDecoder() : valid_(true) {} On 2016/08/04 10:20:17, Pawel Osciak wrote: > We could perhaps use class member initialization in the class body syntax > instead. > > Should we also initialize other members? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:39: if (size < 1) { On 2016/08/04 10:20:17, Pawel Osciak wrote: > Perhaps we should check this before BitReader initialization? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:48: Fill(); On 2016/08/04 10:20:17, Pawel Osciak wrote: > I think ReadLiteral() will Fill() by itself, so perhaps no need to have this > here? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:62: int bits_left = reader_->bits_available(); On 2016/08/04 10:20:18, Pawel Osciak wrote: > Should all of these be size_t ? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:82: if (!valid_) On 2016/08/04 10:20:18, Pawel Osciak wrote: > Should we be checking for valid_ here and in ReadLiteral and returning false, if > the callers of ReadBool and ReadLiteral do not distinguish between an error and > 0? Perhaps rather the caller of Read*() client should be checking this? This is intended behavior. So the caller doesn't need to check error everywhere. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:87: if (!valid_) On 2016/08/04 10:20:18, Pawel Osciak wrote: > Perhaps Fill() should just return a bool? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:91: unsigned int split = (bool_range_ * prob + (256 - prob)) >> kBoolSize; On 2016/08/04 10:20:17, Pawel Osciak wrote: > Would it be safer to use a fixed-length type? |bool_range| is less than 256, thus |split| is less than 256 as well. |int| should be enough. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:122: int x = 0; On 2016/08/04 10:20:17, Pawel Osciak wrote: > Should this be a fixed-size type and also unsigned? I'm not sure what size it should be. Just use arbitrary uint8_t. (current max(bits)=7) https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:123: for (int i = 0; i < bits; i++) On 2016/08/04 10:20:17, Pawel Osciak wrote: > s/int/size_t/ Acknowledged. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.cc:136: // should have enough bits to fill bool range, this should never happend On 2016/08/04 10:20:17, Pawel Osciak wrote: > s/happend/happen./ ? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... File media/filters/vp9_bool_decoder.h (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.h:48: using BigBool = size_t; On 2016/08/04 10:20:18, Pawel Osciak wrote: > Is this assuming size_t is 64bit? This may not always be the case. Not necessary. Any integer type >= 8bits is enough. For extra bits than 8bits, it will be used for pre-fill optimization. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.h:49: const int kBoolSize = 8; On 2016/08/04 10:20:18, Pawel Osciak wrote: > Could you please document what these are? > > We should perhaps use size_t for sizes. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.h:50: const int kBigBoolSize = sizeof(BigBool) * 8; On 2016/08/04 10:20:18, Pawel Osciak wrote: > Should this be called kBigBoolBitSize or something similar? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_bool... media/filters/vp9_bool_decoder.h:63: int count_to_fill_; On 2016/08/04 10:20:18, Pawel Osciak wrote: > Should this be a size_t? No, it may be negative. Comment updated. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_comp... File media/filters/vp9_compressed_header_parser.cc (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_comp... media/filters/vp9_compressed_header_parser.cc:7: #include "base/logging.h" On 2016/08/04 10:20:18, Pawel Osciak wrote: > Nit: header out of order. vp9_compressed_header_parser.h first because it's corresponding header of this .cc. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_comp... media/filters/vp9_compressed_header_parser.cc:14: int inv_recenter_nonneg(int v, int m) { On 2016/08/04 10:20:18, Pawel Osciak wrote: > I know this is to match the spec, but I think we should use CamelCase, and > perhaps just keep this name in the comment? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_comp... File media/filters/vp9_compressed_header_parser.h (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_comp... media/filters/vp9_compressed_header_parser.h:16: bool Parse(const uint8_t* stream, off_t frame_size, Vp9FrameHeader* fhdr); On 2016/08/04 10:20:18, Pawel Osciak wrote: > Please document. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_comp... media/filters/vp9_compressed_header_parser.h:48: #endif // MEDIA_FILTERS_VP9_UNCOMPRESSED_HEADER_PARSER_H_ On 2016/08/04 10:20:18, Pawel Osciak wrote: > s/UNCOM/COM/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... File media/filters/vp9_parser.cc (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:122: need_update_ = false; On 2016/08/04 10:20:19, Pawel Osciak wrote: > Should we also zero out frame_context_ ? It's inteded not do so. It's not small (2k) and is already guard by DCHECKs. wdyt? https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:139: // Verify values from driver explicitly. On 2016/08/04 10:20:19, Pawel Osciak wrote: > s/driver/client/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:152: // ContextRefreshCallback. Because we overwrite the value of context here and On 2016/08/04 10:20:18, Pawel Osciak wrote: > When would we not need a pending update? Is it possible to have a stream that > sets need_update_, but then does not use the context anymore to decode further > frames? Theoretically it is possible. I don't know how often it is, though. If fhdr->IsIntra() and (fhdr->IsKeyframe() || fhdr->error_resilient_mode || fhdr->reset_frame_context == 3), the parser will reset all global contexts to default value. I don't know whether encoder will set previous frame's refresh_frame_context to false or not for such case. BTW, I revised Update() function, split it into UpdateFromClient() for case of client update. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:192: base::Callback<void(const Vp9FrameContext&)>* context_refresh_cb) { On 2016/08/04 10:20:18, Pawel Osciak wrote: > ContextRefreshCallback* ? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:213: // TODO(kcwu): remove this debug info On 2016/08/04 10:20:19, Pawel Osciak wrote: > Marking with a comment so we don't forget to remove. Removed. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:229: for (int i = fhdr->uncompressed_header_size; i < frame_info.size; i++) { On 2016/08/04 10:20:19, Pawel Osciak wrote: > s/int/size_t/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:257: << static_cast<int>(fhdr->frame_context_idx) << " to update"; On 2016/08/04 10:20:19, Pawel Osciak wrote: > Is the cast needed? Yes, otherwise it is uint8_t and DVLOG() will print it as 'unsigned char'. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.cc:489: Vp9SegmentationParams& segmentation = context_.segmentation; On 2016/08/04 10:20:19, Pawel Osciak wrote: > Please move down to before the fist use site. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... File media/filters/vp9_parser.h (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:33: const int kVp9NumFrameContext = 4; On 2016/08/04 10:20:19, Pawel Osciak wrote: > size_t ? > > s/Context/Contexts/ ? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:35: typedef uint8_t Vp9Prob; On 2016/08/04 10:20:19, Pawel Osciak wrote: > Nit: perhaps "using" syntax? > Perhaps move to into the context class? It's not only used in context class. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:56: enum Vp9FrameType { On 2016/08/04 10:20:19, Pawel Osciak wrote: > Perhaps RefType? We already have FrameType in header, so it'd be a bit > confusing, and spec uses FrameType for keyframe/non-keyframe. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:64: enum Vp9TxMode { On 2016/08/04 10:20:19, Pawel Osciak wrote: > Perhaps move to Vp9Compressed header? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:73: enum Vp9TxSize { On 2016/08/04 10:20:19, Pawel Osciak wrote: > Perhaps we should move this to ReadCoefProbs()? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:136: // calculated from above fields On 2016/08/04 10:20:19, Pawel Osciak wrote: > Nit: to match the style, please capitalize first letters and add dots at end of > sentences. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:238: uint8_t frame_context_idx_backup; On 2016/08/04 10:20:19, Pawel Osciak wrote: > Perhaps frame_context_idx and frame_context_idx_to_save_probs? > > We should probably add a comment saying "frame_context_idx_to_save_probs is to > be used by save_probs() only, and frame_context_idx otherwise". Done. However, it seem look odd for vaapi_vda. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:268: using ContextRefreshCallback = base::Callback<void(const Vp9FrameContext&)>; On 2016/08/04 10:20:19, Pawel Osciak wrote: > Please document. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:275: bool need_update() const { return need_update_; } On 2016/08/04 10:20:19, Pawel Osciak wrote: > s/need/needs/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:278: void Reset(); On 2016/08/04 10:20:19, Pawel Osciak wrote: > Please document methods. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:310: // More fields for consistency checking. On 2016/08/04 10:20:19, Pawel Osciak wrote: > I think color_space we also use to copy to frame_hdr? Yes, but it is not mentioned in spec 8.10. I list them separately according to 8.10. Perhaps the spec forgot to mention it? https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:329: explicit Vp9Parser(bool parsing_compressed_header); On 2016/08/04 10:20:19, Pawel Osciak wrote: > Please document, maybe referring to ParseNextFrame() doc. Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser.h:340: // state. If |parsing_compressed_header|, this function also fills On 2016/08/04 10:20:19, Pawel Osciak wrote: > s/parsing_compressed_header/parsing_compressed_header_/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... File media/filters/vp9_parser_unittest.cc (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser_unittest.cc:9: // The syntax of these context dump is described as following. For every On 2016/08/04 10:20:20, Pawel Osciak wrote: > s/following/follows/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser_unittest.cc:10: // frames, there are corresponding data in context file, On 2016/08/04 10:20:20, Pawel Osciak wrote: > s/frames/frame/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser_unittest.cc:62: return static_cast<bool>(should_update); On 2016/08/04 10:20:20, Pawel Osciak wrote: > return should_update == 1? Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_pars... media/filters/vp9_parser_unittest.cc:119: // Allow to parse double frames in order to detect extra frames parsed. On 2016/08/04 10:20:20, Pawel Osciak wrote: > s/double/twice as many/ > > s/extra/any extra/ Done. https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_unco... File media/filters/vp9_uncompressed_header_parser.h (right): https://codereview.chromium.org/2133993002/diff/120001/media/filters/vp9_unco... media/filters/vp9_uncompressed_header_parser.h:35: // Raw bits decoder for uncompressed frame header. On 2016/08/04 10:20:20, Pawel Osciak wrote: > s/decoder/reader/ ? Done. https://codereview.chromium.org/2133993002/diff/120001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/2133993002/diff/120001/media/media.gyp#newcod... media/media.gyp:571: 'filters/vp8_parser.cc', On 2016/08/04 10:20:20, Pawel Osciak wrote: > Should we be adding the other parser files here as well? Done. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_pars... File media/filters/vp9_parser.cc (right): https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_pars... media/filters/vp9_parser.cc:198: // |curr_frame_header_| and awaiting context update. On 2016/08/05 10:09:39, Pawel Osciak wrote: > "and we are awaiting context update to proceed with compressed header parsing." Done. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_pars... media/filters/vp9_parser.cc:212: DCHECK(!frames_.empty()); On 2016/08/05 10:09:39, Pawel Osciak wrote: > Perhaps we could use frames_.empty() instead of return value from > ParseSuperFrame() as an indicator of ParseSuperFrame() failure instead? > > ParseSuperFrame() could push into a temporary frames deque and std::swap() it > with frames_ only on return true? I make ParseSuperFrame() return std::deque. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_pars... media/filters/vp9_parser.cc:239: static_cast<size_t>(curr_frame_info_.size)) { On 2016/08/05 10:09:39, Pawel Osciak wrote: > Perhaps checked cast? Done. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_pars... File media/filters/vp9_parser_unittest.cc (right): https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_pars... media/filters/vp9_parser_unittest.cc:134: void DumpContext(const char* fmt, int idx, const Vp9FrameContext& context) { On 2016/08/05 10:09:39, Pawel Osciak wrote: > Do we want to keep this? Done. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_unco... File media/filters/vp9_uncompressed_header_parser.cc (right): https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_unco... media/filters/vp9_uncompressed_header_parser.cc:870: VLOG(1) << "tile_cols_log2 should be <= 6"; On 2016/08/05 10:09:39, Pawel Osciak wrote: > s/VLOG/DVLOG/? Done. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_unco... media/filters/vp9_uncompressed_header_parser.cc:994: // TODO(kcwu): how about color_range? On 2016/08/05 10:09:39, Pawel Osciak wrote: > Do we want to leave this TODO? removed https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_unco... media/filters/vp9_uncompressed_header_parser.cc:1000: // Some fields are not specified for inter-frame in header, so copy On 2016/08/05 10:09:39, Pawel Osciak wrote: > s/Some/Below/ Done. https://codereview.chromium.org/2133993002/diff/140001/media/filters/vp9_unco... media/filters/vp9_uncompressed_header_parser.cc:1050: arraysize(context_->frame_context_managers)); On 2016/08/05 10:09:39, Pawel Osciak wrote: > maybe static assert on (arraysize() == 1 << 2)? Do you mean "== 1 << 2" literally? It looks ugly.
https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:122: need_update_ = false; On 2016/08/05 11:38:47, kcwu wrote: > On 2016/08/04 10:20:19, Pawel Osciak wrote: > > Should we also zero out frame_context_ ? > > It's inteded not do so. It's not small (2k) and is already guard by DCHECKs. > wdyt? Acknowledged. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.cc:152: // ContextRefreshCallback. Because we overwrite the value of context here and On 2016/08/05 11:38:47, kcwu wrote: > On 2016/08/04 10:20:18, Pawel Osciak wrote: > > When would we not need a pending update? Is it possible to have a stream that > > sets need_update_, but then does not use the context anymore to decode further > > frames? > > Theoretically it is possible. I don't know how often it is, though. > > If fhdr->IsIntra() and (fhdr->IsKeyframe() || fhdr->error_resilient_mode || > fhdr->reset_frame_context == 3), the parser will reset all global contexts to > default value. I don't know whether encoder will set previous frame's > refresh_frame_context to false or not for such case. > > BTW, I revised Update() function, split it into UpdateFromClient() for case of > client update. Acknowledged. https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/120001/media/filters/v... media/filters/vp9_parser.h:310: // More fields for consistency checking. On 2016/08/05 11:38:48, kcwu wrote: > On 2016/08/04 10:20:19, Pawel Osciak wrote: > > I think color_space we also use to copy to frame_hdr? > > Yes, but it is not mentioned in spec 8.10. I list them separately according to > 8.10. Perhaps the spec forgot to mention it? > Acknowledged. https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... File media/filters/vp9_uncompressed_header_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:1050: arraysize(context_->frame_context_managers)); On 2016/08/05 11:38:48, kcwu wrote: > On 2016/08/05 10:09:39, Pawel Osciak wrote: > > maybe static assert on (arraysize() == 1 << 2)? > > Do you mean "== 1 << 2" literally? It looks ugly. Anything is ok, my point being this could perhaps be a compile-time check, given the fact that frame_context_idx cannot be >3? https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... File media/filters/vp9_bool_decoder.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... media/filters/vp9_bool_decoder.cc:69: if (bits_left < static_cast<size_t>(count_to_fill_)) { Hmm, ok, I admit I'm not seeing any good ways to avoid the casts. I'm really sorry, perhaps we should go back to ints after all in this case (count and sizes for bools). https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... media/filters/vp9_parser.cc:40: : initialized_(false), Perhaps we could initialize in class body? https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... media/filters/vp9_parser.h:261: // the client.. If context update is needed after decoding a frame, the client must execute this callback, passing the updated context state.
https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... File media/filters/vp9_uncompressed_header_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:1050: arraysize(context_->frame_context_managers)); On 2016/08/08 08:13:37, Pawel Osciak wrote: > On 2016/08/05 11:38:48, kcwu wrote: > > On 2016/08/05 10:09:39, Pawel Osciak wrote: > > > maybe static assert on (arraysize() == 1 << 2)? > > > > Do you mean "== 1 << 2" literally? It looks ugly. > > Anything is ok, my point being this could perhaps be a compile-time check, given > the fact that frame_context_idx cannot be >3? I don't like magic number and prefer to keep it as now. https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... File media/filters/vp9_bool_decoder.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... media/filters/vp9_bool_decoder.cc:69: if (bits_left < static_cast<size_t>(count_to_fill_)) { On 2016/08/08 08:13:37, Pawel Osciak wrote: > Hmm, ok, I admit I'm not seeing any good ways to avoid the casts. I'm really > sorry, perhaps we should go back to ints after all in this case (count and sizes > for bools). Done. https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... File media/filters/vp9_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... media/filters/vp9_parser.cc:40: : initialized_(false), On 2016/08/08 08:13:37, Pawel Osciak wrote: > Perhaps we could initialize in class body? Done. https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... File media/filters/vp9_parser.h (right): https://chromiumcodereview.appspot.com/2133993002/diff/160001/media/filters/v... media/filters/vp9_parser.h:261: // the client.. On 2016/08/08 08:13:37, Pawel Osciak wrote: > If context update is needed after decoding a frame, the client must execute this > callback, passing the updated context state. Done.
https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... File media/filters/vp9_uncompressed_header_parser.cc (right): https://chromiumcodereview.appspot.com/2133993002/diff/140001/media/filters/v... media/filters/vp9_uncompressed_header_parser.cc:1050: arraysize(context_->frame_context_managers)); On 2016/08/09 04:29:43, kcwu wrote: > On 2016/08/08 08:13:37, Pawel Osciak wrote: > > On 2016/08/05 11:38:48, kcwu wrote: > > > On 2016/08/05 10:09:39, Pawel Osciak wrote: > > > > maybe static assert on (arraysize() == 1 << 2)? > > > > > > Do you mean "== 1 << 2" literally? It looks ugly. > > > > Anything is ok, my point being this could perhaps be a compile-time check, > given > > the fact that frame_context_idx cannot be >3? > > I don't like magic number and prefer to keep it as now. I changed to use static_assert.
lgtm
The CQ bit was checked by posciak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by kcwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kcwu@chromium.org changed reviewers: + dalecurtis@chromium.org
Hi DaleCurtis, Please OWNERS review, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verificaiton for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unitest: - Add bear-vp9.ivf and dump of context values - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none ========== to ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verification for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unittest: - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none ==========
The CQ bit was checked by kcwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by kcwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kcwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:260001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dalecurtis@chromium.org changed reviewers: + fgalligan@chromium.org, sandersd@chromium.org
+fgalligan for vp9 review -- I don't know this spec well enough to review and will be traveling tomorrow. +sandersd for media/ OWNERS review once vpx folk are happy. At a glance this seems like there should be unittests and fuzzer tests added for each individual component; i.e. the bool decoder, compressed header parser, and uncompressed header parser.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
Hi Frank, Do you have time to take a look this CL?
Yeah I will today... Or at least someone on the team. I just got back from vacation. On Tue, Aug 16, 2016 at 12:09 AM, <kcwu@chromium.org> wrote: > Hi Frank, > Do you have time to take a look this CL? > > > https://codereview.chromium.org/2133993002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/16 16:49:58, chromium-reviews wrote: > Yeah I will today... Or at least someone on the team. I just got back from > vacation. > > On Tue, Aug 16, 2016 at 12:09 AM, <mailto:kcwu@chromium.org> wrote: > > > Hi Frank, > > Do you have time to take a look this CL? > > > > > > https://codereview.chromium.org/2133993002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. lgtm but long term I don't think it is a great idea to split the code like this. I think we should add the functionality you need to libvpx.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org, fgalligan@google.com
lgtm
The CQ bit was checked by posciak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2133993002/#ps320001 (title: "reset curr_frame_info_ when Reset")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verification for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unittest: - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none ========== to ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verification for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unittest: - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none ==========
Message was sent while issue was closed.
Committed patchset #10 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verification for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unittest: - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none ========== to ========== Implement VP9 compressed header parsing Follow VP9 draft spec 0.6 - Rename variables according to spec - More verification for parsed values - Add spec section numbers as comments Refactoring the parser for compressed header parsing - split uncompressed parser to separate file Unittest: - Fix frame counting for superframe TEST=vp9 vda still works; media_unittests --gtest_filter=Vp9ParserTest.\* BUG=none Committed: https://crrev.com/e5a9a629b414a491ec071e5edbc800f376042733 Cr-Commit-Position: refs/heads/master@{#412442} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e5a9a629b414a491ec071e5edbc800f376042733 Cr-Commit-Position: refs/heads/master@{#412442} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
