Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1314473008:
H264 bitstream parser. (Closed)
|
Created:
5 years, 3 months ago by pbos-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionH264 bitstream parser.
Parsing the encoded bitstream is required for doing downscaling
decisions based on average encoded QP to improve perceived quality.
BUG=webrtc:4968
R=noahric@chromium.org, stefan@webrtc.org
TBR=pthatcher@webrtc.org
Committed: https://crrev.com/8c266e6baff043a1fa5c9134f46042908a376d5b
Cr-Commit-Position: refs/heads/master@{#10051}
Patch Set 1 #Patch Set 2 : remove DCHECK_GT #
Total comments: 9
Patch Set 3 : fix signed golomb #Patch Set 4 : pic_order_cnt_type_ == 0 check #Patch Set 5 : cleanup #Patch Set 6 : win64 build warnings #
Total comments: 18
Patch Set 7 : feedback #Patch Set 8 : add missing vector include #Patch Set 9 : fix index error #
Total comments: 16
Patch Set 10 : feedback #Patch Set 11 : make pps/sps/qp parsing failable #Patch Set 12 : add basic unittests #
Total comments: 6
Messages
Total messages: 40 (7 generated)
Noah can you help me read this? I've most parsing in place but it's currently failing to extract QP properly (or hardware bitstreams are severely broken, not likely). I saw parsed QPs of like -1403 or something very negative, which makes me think I'm misparsing parts of the spec. I also have some TODOs in some places where I'm not sure I'm doing things right at all. Please take a look at those and tell me whether I'm likely to need to implement them as well. Do we need to protect against parsing reading certain NALUs btw? This CL is in a WIP state, but the parser is reviewable and likely contains bugs since I had to copy stuff off a spec. manually.
remove DCHECK_GT
On 2015/09/03 13:30:14, pbos-webrtc wrote: > remove DCHECK_GT I have a local test program that feeds a bitstream into this, couldn't upload it as it's 3.2M with the bitstream hard-coded: ParseSlice(0x424010, 17): [103.. ParseSlice(0x424021, 8): [104.. pic_init_qp: 20 ParseSlice(0x424029, 7023): [101.. Slice QP: 20 ParseSlice(0x425b98, 3128): [65.. Slice QP: 20 ParseSlice(0x4267d0, 1536): [65.. Slice QP: 20 ParseSlice(0x426dd0, 1262): [65.. Slice QP: 19 ParseSlice(0x4272be, 921): [65.. Slice QP: 19 .. and following QPs (a lot) are 19. These mean that as pic_init_qp is 20, the slice QP deltas have only been either 0 or -1 which sounds unlikely to me (unless the hardware encoder puts out a pretty-much-fixed slice QP delta). I think I'm mis-parsing parts before this.
Stopped at what I think is the parser error you're hitting. Also another important one in bitbuffer that will probably cause you to report incorrect values. https://codereview.webrtc.org/1314473008/diff/10001/webrtc/base/bitbuffer.cc File webrtc/base/bitbuffer.cc (right): https://codereview.webrtc.org/1314473008/diff/10001/webrtc/base/bitbuffer.cc#... webrtc/base/bitbuffer.cc:193: if (!ReadExponentialGolomb(&unsigned_val)) Brackets to match the rest of the file :-p https://codereview.webrtc.org/1314473008/diff/10001/webrtc/base/bitbuffer.cc#... webrtc/base/bitbuffer.cc:195: if ((unsigned_val & 1) == 0) This if just falls through and overwrites val, so that can't work correctly. if/else? Also it'd be helpful to have a short comment explaining what the sequence is, e.g. // Signed exponential golomb values are just the unsigned values // mapped to the sequence 0, 1, -1, 2, -2, etc. in order. (plus a unit test, for at least a small set of expected values) https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:323: // pic_order_cnt_lsb: u(v) This needs to be behind a pic_order_cnt_type == 0 check (probably the cause of the parser failure, since most streams in the wild will have pic_order_cnt_type == 2). https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:507: if (head[0]) { There's a much faster way to loop this (I did an experiment a few months ago to find a fast/readable one): uint8_t* end = buffer + buffer_size - kNaluHeaderSize; for (uint8_t* head = buffer; head < end;) { if (head[3] > 1) { head += 4; } else if (head[3] == 1 && head[2] == 0 && head[1] == 0 && head[0] == 0) { // THIS IS A NALU: head - buffer head += 4; } else { head++; } } } Or you could steal this version, which returns a vector of indices: // Returns a vector of the NALU start sequences (0 0 0 1) in the given buffer. std::vector<int> FindNaluStartSequences(const uint8_t* buffer, size_t buffer_size) { std::vector<int> sequences; // This is sorta like Boyer-Moore, but with only the first optimization step: // given a 4-byte sequence we're looking at, if the 4th byte isn't 1 or 0, // skip ahead to the next 4-byte sequence. 0s and 1s are relatively rare, so // this will skip the majority of reads/checks. const uint8_t* end = buffer + buffer_size - 4; for (const uint8_t* head = buffer; head < end;) { if (head[3] > 1) { head += 4; } else if (head[3] == 1 && head[2] == 0 && head[1] == 0 && head[0] == 0) { sequences.push_back(head - buffer); head += 4; } else { head++; } } return sequences; }
https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:327: if (bottom_field_pic_order_in_frame_present_flag_ && field_pic_flag == 0) { (This is also part of pic_order_cnt_type == 0)
fix signed golomb
pic_order_cnt_type_ == 0 check
https://codereview.webrtc.org/1314473008/diff/10001/webrtc/base/bitbuffer.cc File webrtc/base/bitbuffer.cc (right): https://codereview.webrtc.org/1314473008/diff/10001/webrtc/base/bitbuffer.cc#... webrtc/base/bitbuffer.cc:193: if (!ReadExponentialGolomb(&unsigned_val)) On 2015/09/03 17:48:58, noahric wrote: > Brackets to match the rest of the file :-p Done. https://codereview.webrtc.org/1314473008/diff/10001/webrtc/base/bitbuffer.cc#... webrtc/base/bitbuffer.cc:195: if ((unsigned_val & 1) == 0) On 2015/09/03 17:48:58, noahric wrote: > This if just falls through and overwrites val, so that can't work correctly. > if/else? > > Also it'd be helpful to have a short comment explaining what the sequence is, > e.g. > // Signed exponential golomb values are just the unsigned values > // mapped to the sequence 0, 1, -1, 2, -2, etc. in order. > > (plus a unit test, for at least a small set of expected values) Done, which also uncovered additional bugs. That are now fixed. Thanks. :') https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:323: // pic_order_cnt_lsb: u(v) On 2015/09/03 17:48:58, noahric wrote: > This needs to be behind a pic_order_cnt_type == 0 check (probably the cause of > the parser failure, since most streams in the wild will have pic_order_cnt_type > == 2). This gives me more reasonable values. Thanks a lot! :D https://codereview.webrtc.org/1314473008/diff/10001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:327: if (bottom_field_pic_order_in_frame_present_flag_ && field_pic_flag == 0) { On 2015/09/03 17:49:37, noahric wrote: > (This is also part of pic_order_cnt_type == 0) Done.
pbos@webrtc.org changed reviewers: + pthatcher@webrtc.org, stefan@webrtc.org
OK, this should be ready for review. I think it's best reviewed by comparing http://www.itu.int/rec/T-REC-H.264 to the comments inside this function and that we're reading the corresponding number of bits (or golomb coded values). It does work for Android HW encoders on at least 2 devices, so some parsing should be correct. Producing a minimal frame for unittesting feels like more effort than it's worth, and it won't contain all possible ways of encoding H.264 down to the slice QP. noahric@ for general correctness review, pthatcher@ for webrtc/base ownership and stefan@ for webrtc/modules/rtp_rtcp ownership. PTAL :)
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314473008/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314473008/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/3845) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9262)
win64 build warnings
Did one pass. Still need to side-by-side it with the standard, I'll try to get to that after some afternoon meetings :) https://codereview.webrtc.org/1314473008/diff/60001/webrtc/base/bitbuffer.h File webrtc/base/bitbuffer.h (right): https://codereview.webrtc.org/1314473008/diff/60001/webrtc/base/bitbuffer.h#n... webrtc/base/bitbuffer.h:63: bool ReadSignedExponentialGolomb(int32_t* val); // Reads the signed exponential golomb encoded value at the current offset. // Signed values are calculated as <explanation, sequence example, etc.> https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:26: // The IDR type. The NALU type. (only 5 is an IDR) https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:29: static const uint8_t kNaluIdr = 0x5; either 5 or make them all 0xn, for consistency. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:34: // static const uint8_t kSliceTypeI = 2; Consider removing the commented out lines, since they are unused. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:177: // We're starting a new stream, so reset picture type rewriting values. Delete this comment. Alternatively, update it to: 1) Clear the stored pps fields 2) (optionally) Assert that the sps fields are set, since an SPS should always precede a PPS. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:193: // trivial to parse? Yeah, it't not true anymore. There are a few differences, but not a ton, so maybe TODO to implement CABAC support. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:421: // TODO(pbos): Do we need support for pred_weight_table()? Potentially for P slices. Not B slices, since those are disabled everywhere. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:505: while (head < end - 4) { You'll still definitely want to replace this. From my comment on an older patchset: There's a much faster way to loop this (I did an experiment a few months ago to find a fast/readable one): uint8_t* end = buffer + buffer_size - kNaluHeaderSize; for (uint8_t* head = buffer; head < end;) { if (head[3] > 1) { head += 4; } else if (head[3] == 1 && head[2] == 0 && head[1] == 0 && head[0] == 0) { // THIS IS A NALU: head - buffer head += 4; } else { head++; } } } Or you could steal this version, which returns a vector of indices: // Returns a vector of the NALU start sequences (0 0 0 1) in the given buffer. std::vector<int> FindNaluStartSequences(const uint8_t* buffer, size_t buffer_size) { std::vector<int> sequences; // This is sorta like Boyer-Moore, but with only the first optimization step: // given a 4-byte sequence we're looking at, if the 4th byte isn't 1 or 0, // skip ahead to the next 4-byte sequence. 0s and 1s are relatively rare, so // this will skip the majority of reads/checks. const uint8_t* end = buffer + buffer_size - 4; for (const uint8_t* head = buffer; head < end;) { if (head[3] > 1) { head += 4; } else if (head[3] == 1 && head[2] == 0 && head[1] == 0 && head[0] == 0) { sequences.push_back(head - buffer); head += 4; } else { head++; } } return sequences; } https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h (right): https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h:23: class H264BitstreamParser { Comments, pretty please :)
feedback
PTAL, be extra careful with the vector part. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/base/bitbuffer.h File webrtc/base/bitbuffer.h (right): https://codereview.webrtc.org/1314473008/diff/60001/webrtc/base/bitbuffer.h#n... webrtc/base/bitbuffer.h:63: bool ReadSignedExponentialGolomb(int32_t* val); On 2015/09/09 20:51:42, noahric wrote: > // Reads the signed exponential golomb encoded value at the current offset. > // Signed values are calculated as <explanation, sequence example, etc.> Moved comment here, done. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:26: // The IDR type. On 2015/09/09 20:51:42, noahric wrote: > The NALU type. > > (only 5 is an IDR) Done. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:29: static const uint8_t kNaluIdr = 0x5; On 2015/09/09 20:51:42, noahric wrote: > either 5 or make them all 0xn, for consistency. Done. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:34: // static const uint8_t kSliceTypeI = 2; On 2015/09/09 20:51:42, noahric wrote: > Consider removing the commented out lines, since they are unused. Done. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:177: // We're starting a new stream, so reset picture type rewriting values. On 2015/09/09 20:51:42, noahric wrote: > Delete this comment. Alternatively, update it to: > 1) Clear the stored pps fields > 2) (optionally) Assert that the sps fields are set, since an SPS should always > precede a PPS. Resetting. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:193: // trivial to parse? On 2015/09/09 20:51:42, noahric wrote: > Yeah, it't not true anymore. There are a few differences, but not a ton, so > maybe TODO to implement CABAC support. TODO done. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:421: // TODO(pbos): Do we need support for pred_weight_table()? On 2015/09/09 20:51:42, noahric wrote: > Potentially for P slices. Not B slices, since those are disabled everywhere. Do you think we can punt adding support for this? https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:505: while (head < end - 4) { On 2015/09/09 20:51:42, noahric wrote: > You'll still definitely want to replace this. From my comment on an older > patchset: > > There's a much faster way to loop this (I did an experiment a few months ago to > find a fast/readable one): > > uint8_t* end = buffer + buffer_size - kNaluHeaderSize; > for (uint8_t* head = buffer; head < end;) { > if (head[3] > 1) { > head += 4; > } else if (head[3] == 1 && head[2] == 0 && head[1] == 0 && head[0] == 0) { > // THIS IS A NALU: head - buffer > head += 4; > } else { > head++; > } > } > } > > Or you could steal this version, which returns a vector of indices: > // Returns a vector of the NALU start sequences (0 0 0 1) in the given buffer. > std::vector<int> FindNaluStartSequences(const uint8_t* buffer, > size_t buffer_size) { > std::vector<int> sequences; > // This is sorta like Boyer-Moore, but with only the first optimization step: > // given a 4-byte sequence we're looking at, if the 4th byte isn't 1 or 0, > // skip ahead to the next 4-byte sequence. 0s and 1s are relatively rare, so > // this will skip the majority of reads/checks. > const uint8_t* end = buffer + buffer_size - 4; > for (const uint8_t* head = buffer; head < end;) { > if (head[3] > 1) { > head += 4; > } else if (head[3] == 1 && head[2] == 0 && head[1] == 0 && head[0] == 0) { > sequences.push_back(head - buffer); > head += 4; > } else { > head++; > } > } > > return sequences; > } Done. https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h (right): https://codereview.webrtc.org/1314473008/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h:23: class H264BitstreamParser { On 2015/09/09 20:51:42, noahric wrote: > Comments, pretty please :) Done.
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314473008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314473008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/4057) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/849)
add missing vector include
fix index error
Ok, vector part had an index error. I've tested this locally with pending QP scaling fixes and it still seems to work.
LGTM with some minor style comments. Went through against the standard line-by-line and I didn't see anything amiss. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:298: // deblocking_filter_control_present_flag: u(1) You can combine these two if you want (doesn't make a big difference) https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:327: // slice_type's 5..9 range is used to indicate things.that all slices of a I think "things." is probably a mispaste? https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:387: if (num_ref_idx_active_override_flag) { != 0, for consistency (or you could strip the rest if you prefer; I wrote them with originally because I still have the old libjingle style guide embedded in my brain, which is "all integral comparisons should be explicit", and that only bools and pointers do implicit foo and !foo). https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:397: // if (nal_unit_type == 20 || nal_unit_type == 21) Worth adding CHECKS for these? You're right, there should never be MVC, but just in case. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:404: // verify that they are both the same. Heh, yeah, I was just gonna comment on that being weird :) I'd guess it's because these are % comparisons (in the standard) in the others are == comparisons (in the standard). https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:455: // if ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || Worth adding CHECKS for these? https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:459: if (nal_ref_idc) { nal_ref_idc != 0 https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h (right): https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h:35: int GetLastSliceQp() const; Consider making this do something expected if there is no last slice qp (say if it feeds in an sps/pps without having the IDR and then asks for the last slice qp).
feedback
make pps/sps/qp parsing failable
PTAL stefan@, take a special look at the diff after Noah reviewed. :) https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:298: // deblocking_filter_control_present_flag: u(1) On 2015/09/22 20:47:19, noahric wrote: > You can combine these two if you want (doesn't make a big difference) Done. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:327: // slice_type's 5..9 range is used to indicate things.that all slices of a On 2015/09/22 20:47:19, noahric wrote: > I think "things." is probably a mispaste? Done. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:387: if (num_ref_idx_active_override_flag) { On 2015/09/22 20:47:19, noahric wrote: > != 0, for consistency (or you could strip the rest if you prefer; I wrote them > with originally because I still have the old libjingle style guide embedded in > my brain, which is "all integral comparisons should be explicit", and that only > bools and pointers do implicit foo and !foo). Done. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:397: // if (nal_unit_type == 20 || nal_unit_type == 21) On 2015/09/22 20:47:19, noahric wrote: > Worth adding CHECKS for these? You're right, there should never be MVC, but just > in case. Done. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:404: // verify that they are both the same. On 2015/09/22 20:47:19, noahric wrote: > Heh, yeah, I was just gonna comment on that being weird :) I'd guess it's > because these are % comparisons (in the standard) in the others are == > comparisons (in the standard). Acknowledged. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:455: // if ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || On 2015/09/22 20:47:19, noahric wrote: > Worth adding CHECKS for these? Done. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:459: if (nal_ref_idc) { On 2015/09/22 20:47:19, noahric wrote: > nal_ref_idc != 0 Done. https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h (right): https://codereview.webrtc.org/1314473008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.h:35: int GetLastSliceQp() const; On 2015/09/22 20:47:19, noahric wrote: > Consider making this do something expected if there is no last slice qp (say if > it feeds in an sps/pps without having the IDR and then asks for the last slice > qp). Done.
add basic unittests
PTAL, I added a simple unittest just to make sure some things stay working if we touch this file. It doesn't contain more complex things like different slice types etc, but it provides some protection for things breaking.
https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:467: // TODO(pbos): Do we need support for pred_weight_table()? I would guess not, I don't think it's part of the base profile? https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:562: *qp = 26 + pps_.pic_init_qp_minus26 + last_slice_qp_delta_; Why is it the last_slice_qp_delta_ we want to use and not, e.g., an average of all slice qps? Seems a bit arbitrary. Couldn't we in that case just parse the first and return after?
https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:467: // TODO(pbos): Do we need support for pred_weight_table()? On 2015/09/23 21:14:39, stefan-webrtc (holmer) wrote: > I would guess not, I don't think it's part of the base profile? Then if any chip implements past the base profile we might? Saving the CHECK probably makes sense. https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:562: *qp = 26 + pps_.pic_init_qp_minus26 + last_slice_qp_delta_; On 2015/09/23 21:14:38, stefan-webrtc (holmer) wrote: > Why is it the last_slice_qp_delta_ we want to use and not, e.g., an average of > all slice qps? Seems a bit arbitrary. Couldn't we in that case just parse the > first and return after? Easier to not have to know which slices are part of a single frame and aggregate across those. From what I've heard most (or all) HW encoders do single picture slices.
lgtm https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc (right): https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:467: // TODO(pbos): Do we need support for pred_weight_table()? On 2015/09/24 11:38:23, pbos-webrtc wrote: > On 2015/09/23 21:14:39, stefan-webrtc (holmer) wrote: > > I would guess not, I don't think it's part of the base profile? > > Then if any chip implements past the base profile we might? Saving the CHECK > probably makes sense. I don't think there are any plans to support profiles > baseline. https://codereview.webrtc.org/1314473008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/h264_bitstream_parser.cc:562: *qp = 26 + pps_.pic_init_qp_minus26 + last_slice_qp_delta_; On 2015/09/24 11:38:23, pbos-webrtc wrote: > On 2015/09/23 21:14:38, stefan-webrtc (holmer) wrote: > > Why is it the last_slice_qp_delta_ we want to use and not, e.g., an average of > > all slice qps? Seems a bit arbitrary. Couldn't we in that case just parse the > > first and return after? > > Easier to not have to know which slices are part of a single frame and aggregate > across those. From what I've heard most (or all) HW encoders do single picture > slices. It's true that it probably doesn't matter in most cases. I was mostly confused why we pick the last and not the first, but as you explained offline, you have written this parser with no assumption that you are parsing a single frame, and you only want the latest qp.
TBR=pthatcher@ for webrtc/base, I consider that part reviewed by Noah. :)
The CQ bit was checked by pbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from noahric@chromium.org Link to the patchset: https://codereview.webrtc.org/1314473008/#ps180001 (title: "add basic unittests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314473008/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314473008/180001
Message was sent while issue was closed.
Committed patchset #12 (id:180001) manually as 8c266e6baff043a1fa5c9134f46042908a376d5b (presubmit successful).
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/8c266e6baff043a1fa5c9134f46042908a376d5b Cr-Commit-Position: refs/heads/master@{#10051} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
