Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891803003:
Add vp9 QP parser. (Closed)
DescriptionAdd vp9 QP parser.
BUG=webrtc:7662
Review-Url: https://codereview.webrtc.org/2891803003
Cr-Commit-Position: refs/heads/master@{#18260}
Committed: https://chromium.googlesource.com/external/webrtc/+/20acdf24437319dbd2832a4468b6ec606b25c07c
Patch Set 1 #Patch Set 2 : Clean up comments and prints. #Patch Set 3 : Fix compile warnings. #
Total comments: 25
Patch Set 4 : Refactor. Wrap bit reader in class. use rtc::BitBuffer. #Patch Set 5 : Fix compile warning on win. #
Total comments: 17
Patch Set 6 : Refactor. #
Total comments: 22
Patch Set 7 : Refactor. #
Messages
Total messages: 42 (31 generated)
Description was changed from ========== Add vp9 QP parser. BUG=webrtc:7662 ========== to ========== Add vp9 QP parser. BUG=webrtc:7662 ==========
The CQ bit was checked by jianj@google.com 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.webrtc.org/...
jianj@chromium.org changed reviewers: + marpan@webrtc.org
The CQ bit was checked by jianj@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jianj@chromium.org changed reviewers: + brandtr@webrtc.org, mflodman@webrtc.org
jianj@chromium.org changed reviewers: + jianj@chromium.org
Some initial comments. I haven't had a chance to look into the actual parsing code yet. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/BUILD.gn (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/BUILD.gn:125: "utility/vp9_uncomp_header_parser.cc", I'd prefer the full word in the file name: "vp9_uncompressed_header_parser.cc". https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:195: printf("vp8 parser qp %d\n", base_q0); Remove. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:11: General comment: have you looked into using the rtc::BitBuffer here? It might simplify things. https://cs.chromium.org/chromium/src/third_party/webrtc/base/bitbuffer.h?l=28... https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:24: static void VP9InitBitReader(VP9BitReader *const br, const uint8_t *const start, To me, it seems that VP9BitReader could be a full-blown C++ class? https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:25: const uint8_t *const end) { const uint8_t*, here and in other signatures. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:80: // subsampling x Comments: start with capital letter and end with period. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:15: #include <stdio.h> Do you need this? https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:21: typedef struct VP9BitReader VP9BitReader; Don't think you would need this in C++. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:23: const uint8_t *buf_; Although the style guide allows it, we typically put the star next to the type in WebRTC. Also, we typically don't use underscores for field names in structs. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:30: bool GetQp(const uint8_t *buf, size_t length, int *qp); const uint8_t* https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/DEPS (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/DEPS:7: "+webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h", To reduce the number of OWNERs required, you can remove the changes to the android/ files. Adding the header can then be done when support is actually added to the Android encoder implementation. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:70: DEFINE_string(codec, "VP9", "Video codec to use."); Is this an intentional change?
The CQ bit was checked by jianj@google.com 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.webrtc.org/...
https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/BUILD.gn (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/BUILD.gn:125: "utility/vp9_uncomp_header_parser.cc", On 2017/05/19 14:10:26, brandtr wrote: > I'd prefer the full word in the file name: "vp9_uncompressed_header_parser.cc". Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:195: printf("vp8 parser qp %d\n", base_q0); On 2017/05/19 14:10:27, brandtr wrote: > Remove. Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:11: On 2017/05/19 14:10:27, brandtr wrote: > General comment: have you looked into using the rtc::BitBuffer here? It might > simplify things. > https://cs.chromium.org/chromium/src/third_party/webrtc/base/bitbuffer.h?l=28... Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:24: static void VP9InitBitReader(VP9BitReader *const br, const uint8_t *const start, On 2017/05/19 14:10:27, brandtr wrote: > To me, it seems that VP9BitReader could be a full-blown C++ class? Done. BTW, the same thing could be done to vp8_header_parser, too. I will upload a new CL to make two qp parsers consistent. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:25: const uint8_t *const end) { On 2017/05/19 14:10:27, brandtr wrote: > const uint8_t*, here and in other signatures. Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.cc:80: // subsampling x On 2017/05/19 14:10:27, brandtr wrote: > Comments: start with capital letter and end with period. Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:15: #include <stdio.h> On 2017/05/19 14:10:27, brandtr wrote: > Do you need this? size_t will need it. I changed it to <stddef.h> https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:21: typedef struct VP9BitReader VP9BitReader; On 2017/05/19 14:10:27, brandtr wrote: > Don't think you would need this in C++. Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:23: const uint8_t *buf_; On 2017/05/19 14:10:27, brandtr wrote: > Although the style guide allows it, we typically put the star next to the type > in WebRTC. > > Also, we typically don't use underscores for field names in structs. Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h:30: bool GetQp(const uint8_t *buf, size_t length, int *qp); On 2017/05/19 14:10:27, brandtr wrote: > const uint8_t* Done. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/DEPS (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/DEPS:7: "+webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h", On 2017/05/19 14:10:27, brandtr wrote: > To reduce the number of OWNERs required, you can remove the changes to the > android/ files. Adding the header can then be done when support is actually > added to the Android encoder implementation. I'm not quite clear about this, though I removed this rule currently. Is the qp parser also used in other platforms besides android? https://codereview.webrtc.org/2891803003/diff/40001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:70: DEFINE_string(codec, "VP9", "Video codec to use."); On 2017/05/19 14:10:27, brandtr wrote: > Is this an intentional change? Done. It's just for test purpose. I forgot to change it back.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/19071)
The CQ bit was checked by jianj@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Some further comments. Would it be possible to add some simple unit test, or would the minimal bitstream be too long/complicated for that? I'm not familiar with the VP9 bitstream, so it would be great if Marco could look into that in more detail. https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/DEPS (right): https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/DEPS:7: "+webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h", On 2017/05/19 21:20:57, jianj1 wrote: > On 2017/05/19 14:10:27, brandtr wrote: > > To reduce the number of OWNERs required, you can remove the changes to the > > android/ files. Adding the header can then be done when support is actually > > added to the Android encoder implementation. > > I'm not quite clear about this, though I removed this rule currently. Is the qp > parser also used in other platforms besides android? No, it will only be used by Android. But since this CL does not add it to Android, you will require fewer OWNER approvals by not changing anything outside the video_coding/ folder :) https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:22: static uint8_t VP9ReadProfile(VP9BitReader* const br) { I think you can drop the const here, and in the signatures below. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:35: // bitdepth Nit: start comments with capital and end with period. Here and below. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:74: uint32_t width = br->GetValue(16); I think you could drop the comments, or drop the variables altogether but keep the comments? https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:138: // VP9InitBitReader(&br, buf, buf + length); Remove? https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:156: br.GetValue(3); Is this row needed when you return below? https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:209: for (size_t i = 0; i < kVp9NumRefsPerFrame; i++) { Maybe move this to its own function too? https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h (right): https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:26: VP9BitReader(const uint8_t *buffer, size_t length_) const uint8_t* https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:55: bool GetQp(const uint8_t *buf, size_t length, int *qp); const uint8_t*
On 2017/05/22 18:34:57, brandtr wrote: > Thanks! Some further comments. > > Would it be possible to add some simple unit test, or would the minimal > bitstream be too long/complicated for that? > > I'm not familiar with the VP9 bitstream, so it would be great if Marco could > look into that in more detail. > > https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... > File webrtc/sdk/android/src/jni/DEPS (right): > > https://codereview.webrtc.org/2891803003/diff/40001/webrtc/sdk/android/src/jn... > webrtc/sdk/android/src/jni/DEPS:7: > "+webrtc/modules/video_coding/utility/vp9_uncomp_header_parser.h", > On 2017/05/19 21:20:57, jianj1 wrote: > > On 2017/05/19 14:10:27, brandtr wrote: > > > To reduce the number of OWNERs required, you can remove the changes to the > > > android/ files. Adding the header can then be done when support is actually > > > added to the Android encoder implementation. > > > > I'm not quite clear about this, though I removed this rule currently. Is the > qp > > parser also used in other platforms besides android? > > No, it will only be used by Android. But since this CL does not add it to > Android, you will require fewer OWNER approvals by not changing anything outside > the video_coding/ folder :) > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc > (right): > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:22: static > uint8_t VP9ReadProfile(VP9BitReader* const br) { > I think you can drop the const here, and in the signatures below. > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:35: // > bitdepth > Nit: start comments with capital and end with period. Here and below. > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:74: > uint32_t width = br->GetValue(16); > I think you could drop the comments, or drop the variables altogether but keep > the comments? > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:138: // > VP9InitBitReader(&br, buf, buf + length); > Remove? > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:156: > br.GetValue(3); > Is this row needed when you return below? > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:209: for > (size_t i = 0; i < kVp9NumRefsPerFrame; i++) { > Maybe move this to its own function too? > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h > (right): > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:26: > VP9BitReader(const uint8_t *buffer, size_t length_) > const uint8_t* > > https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... > webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:55: bool > GetQp(const uint8_t *buf, size_t length, int *qp); > const uint8_t* Thanks for detailed comments! Do we have a test for vp8 parser? I found one for vp8 qp parser: vp8_qp_parser_fuzzer.cc. I don't know if that's just what's needed for vp9 or we need some more comprehensive tests.
https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:22: static uint8_t VP9ReadProfile(VP9BitReader* const br) { On 2017/05/22 18:34:56, brandtr wrote: > I think you can drop the const here, and in the signatures below. Done. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:35: // bitdepth On 2017/05/22 18:34:56, brandtr wrote: > Nit: start comments with capital and end with period. Here and below. Done. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:74: uint32_t width = br->GetValue(16); On 2017/05/22 18:34:56, brandtr wrote: > I think you could drop the comments, or drop the variables altogether but keep > the comments? Done. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:138: // VP9InitBitReader(&br, buf, buf + length); On 2017/05/22 18:34:56, brandtr wrote: > Remove? Done. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:156: br.GetValue(3); On 2017/05/22 18:34:56, brandtr wrote: > Is this row needed when you return below? Done. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:209: for (size_t i = 0; i < kVp9NumRefsPerFrame; i++) { On 2017/05/22 18:34:56, brandtr wrote: > Maybe move this to its own function too? Done. It's probably better to keep it here since the logic is the same as the bitstream manual. It would be easier to maintain and look back in the future. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h (right): https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:26: VP9BitReader(const uint8_t *buffer, size_t length_) On 2017/05/22 18:34:56, brandtr wrote: > const uint8_t* Done. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:55: bool GetQp(const uint8_t *buf, size_t length, int *qp); On 2017/05/22 18:34:56, brandtr wrote: > const uint8_t* Done.
The CQ bit was checked by jianj@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (with some style nits), but please consider adding a unit test. If you don't want to add a bitstream constant, you could use the VP9 encoder, and verify that the encoded QP is the same as the parsed QP. You could add a test that is similar to this one, for example: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... Except instead of using a decoder, you would use this QP parser. It would be good if Marco went over the parsing code in more detail, because I'm not familiar with the VP9 bitstream definition. https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:209: for (size_t i = 0; i < kVp9NumRefsPerFrame; i++) { On 2017/05/22 21:18:57, jianj1 wrote: > On 2017/05/22 18:34:56, brandtr wrote: > > Maybe move this to its own function too? > > Done. > > It's probably better to keep it here since the logic is the same as the > bitstream manual. It would be easier to maintain and look back in the future. Makes sense. Thanks. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:22: static uint8_t VP9ReadProfile(VP9BitReader *br) { Style nit: VP9ReadProfile(VP9BitReader* br) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:33: static bool VP9ReadColorConfig(VP9BitReader *const br, const uint8_t profile) { We generally don't make the parameters const, since they are passed by value. VP9ReadColorConfig(VP9BitReader* br, uint8_t profile) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:72: static void VP9ReadFrameSize(VP9BitReader *const br) { VP9ReadFrameSize(VP9BitReader* br) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:79: static void VP9ReadRenderSize(VP9BitReader *const br) { VP9ReadRenderSize(VP9BitReader* br) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:89: static void VP9ReadFrameSizeFromRefs(VP9BitReader *const br) { VP9ReadFrameSizeFromRefs(VP9BitReader* br) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:104: static void VP9ReadInterpolationFilter(VP9BitReader *const br) { VP9ReadInterpolationFilter(VP9BitReader* br) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:111: static void VP9ReadLoopfilter(VP9BitReader *const br) { VP9ReadLoopfilter(VP9BitReader* br) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:132: bool GetQp(const uint8_t *buf, size_t length, int *qp) { GetQp(const uint8_t* buf, size_t length, int* qp) https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h (right): https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:34: LOG(LS_WARNING) << "Failed to get QP. Reached EOF."; Since this is a general bit reader, maybe make the error message more general? Something like "Failed to get bit. Reached EOF."? https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:43: LOG(LS_WARNING) << "Failed to get QP. Reached EOF."; And here. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:55: bool GetQp(const uint8_t* buf, size_t length, int *qp); int* qp
I think it's better to add the unit test in another separate CL. I'll get that after this CL is submitted. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc (right): https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:22: static uint8_t VP9ReadProfile(VP9BitReader *br) { On 2017/05/23 08:59:02, brandtr wrote: > Style nit: VP9ReadProfile(VP9BitReader* br) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:33: static bool VP9ReadColorConfig(VP9BitReader *const br, const uint8_t profile) { On 2017/05/23 08:59:02, brandtr wrote: > We generally don't make the parameters const, since they are passed by value. > > VP9ReadColorConfig(VP9BitReader* br, uint8_t profile) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:72: static void VP9ReadFrameSize(VP9BitReader *const br) { On 2017/05/23 08:59:02, brandtr wrote: > VP9ReadFrameSize(VP9BitReader* br) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:79: static void VP9ReadRenderSize(VP9BitReader *const br) { On 2017/05/23 08:59:02, brandtr wrote: > VP9ReadRenderSize(VP9BitReader* br) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:89: static void VP9ReadFrameSizeFromRefs(VP9BitReader *const br) { On 2017/05/23 08:59:02, brandtr wrote: > VP9ReadFrameSizeFromRefs(VP9BitReader* br) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:104: static void VP9ReadInterpolationFilter(VP9BitReader *const br) { On 2017/05/23 08:59:02, brandtr wrote: > VP9ReadInterpolationFilter(VP9BitReader* br) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:111: static void VP9ReadLoopfilter(VP9BitReader *const br) { On 2017/05/23 08:59:02, brandtr wrote: > VP9ReadLoopfilter(VP9BitReader* br) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.cc:132: bool GetQp(const uint8_t *buf, size_t length, int *qp) { On 2017/05/23 08:59:02, brandtr wrote: > GetQp(const uint8_t* buf, size_t length, int* qp) Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h (right): https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:34: LOG(LS_WARNING) << "Failed to get QP. Reached EOF."; On 2017/05/23 08:59:02, brandtr wrote: > Since this is a general bit reader, maybe make the error message more general? > Something like "Failed to get bit. Reached EOF."? Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:43: LOG(LS_WARNING) << "Failed to get QP. Reached EOF."; On 2017/05/23 08:59:02, brandtr wrote: > And here. Done. https://codereview.webrtc.org/2891803003/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp9_uncompressed_header_parser.h:55: bool GetQp(const uint8_t* buf, size_t length, int *qp); On 2017/05/23 08:59:02, brandtr wrote: > int* qp Done.
The CQ bit was checked by jianj@google.com 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.webrtc.org/...
marpan@google.com changed reviewers: + marpan@google.com
lgtm for parser code
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 jianj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/2891803003/#ps120001 (title: "Refactor.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495645093895670,
"parent_rev": "a0c5d40a9e96302fb338b1b327f37be86924b993", "commit_rev":
"20acdf24437319dbd2832a4468b6ec606b25c07c"}
Message was sent while issue was closed.
Description was changed from ========== Add vp9 QP parser. BUG=webrtc:7662 ========== to ========== Add vp9 QP parser. BUG=webrtc:7662 Review-Url: https://codereview.webrtc.org/2891803003 Cr-Commit-Position: refs/heads/master@{#18260} Committed: https://chromium.googlesource.com/external/webrtc/+/20acdf24437319dbd2832a446... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/20acdf24437319dbd2832a446... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
