Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
112343011:
Split the bit reader functionalities from the byte stream provider. (Closed)
|
Created:
7 years ago by damienv1 Modified:
6 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSplit the bit reader functionalities from the byte stream provider.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244959
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address comments from patch set #1. #Patch Set 3 : Address comments from patch set #1. #Patch Set 4 : Slight change of design. #Patch Set 5 : Add an H264 bit reader for proof of concept. #Patch Set 6 : Use a ByteStreamProvider interface instead of a callback. #
Total comments: 8
Patch Set 7 : Import the H264BitReader unit tests. #
Total comments: 10
Patch Set 8 : Split the H264 bit reader from the regular bit reader. #Patch Set 9 : Add unit test for the removal of the emulation prevention byte. #
Total comments: 73
Patch Set 10 : Address comments from patch set #9. #
Total comments: 3
Patch Set 11 : Fix compilation warning on windows & Add more unit tests #
Total comments: 26
Patch Set 12 : Address comments from patch set #11. #
Total comments: 2
Patch Set 13 : Modify ReadUE function prototype. #
Total comments: 70
Patch Set 14 : More descriptive comments. #Patch Set 15 : Remove bits_buffered from BitReaderCore. #Patch Set 16 : H264 bit reader removed. #Patch Set 17 : Rebase. #
Messages
Total messages: 32 (0 generated)
Provided the design looks good to you, I will then update the CL so that both BitReader and H264BitReader use BitReaderBase as their base class.
https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc#new... media/base/bit_reader.cc:19: #if 0 Forgot to remove the code between #if 0 / #endif. => Remove it. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.cc File media/base/bit_reader_base.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:116: int max_nbytes = 8; Should be sizeof(RegType) https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:137: if (nbits_ == 64 || nbits_next_ == 0) Should be sizeof(RegType) * CHAR_BIT
https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc#new... media/base/bit_reader.cc:58: return 8 * bytes_left_ + bits_available_in_registers(); Should be: bytes_left_ * CHAR_BIT https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h File media/base/bit_reader.h (left): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h#oldc... media/base/bit_reader.h:58: // Pointer to the next unread (not in curr_byte_) byte in the stream. Update the comment. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h#oldc... media/base/bit_reader.h:61: // Bytes left in the stream (without the curr_byte_). Ditto.
https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.cc File media/base/bit_reader_base.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:23: if (!Refill(1)) Could be only one condition. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:91: if (!Refill(num_bits)) { Could be only one condition. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:126: reg_next_ = base::ByteSwap(reg_next_); Should be NetToHost64 !
https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.cc File media/base/bit_reader_base.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:43: nbits_--; zero_count++;
This split doesn't seem right to me. Is there any reason not to implement the H264Parser in terms of BitReader? Why lean towards an "is-a" relationship instead of a "has-a"?
https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h File media/base/bit_reader.h (left): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h#oldc... media/base/bit_reader.h:11: #include "base/logging.h" Not needed anymore.
Next patch set just address these comments. I am going to think about a slightly different design. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc#new... media/base/bit_reader.cc:19: #if 0 On 2013/12/19 02:01:25, damienv1 wrote: > Forgot to remove the code between #if 0 / #endif. > => Remove it. Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.cc#new... media/base/bit_reader.cc:58: return 8 * bytes_left_ + bits_available_in_registers(); On 2013/12/19 03:21:08, damienv1 wrote: > Should be: bytes_left_ * CHAR_BIT Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h File media/base/bit_reader.h (left): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h#oldc... media/base/bit_reader.h:11: #include "base/logging.h" On 2013/12/19 17:57:23, damienv1 wrote: > Not needed anymore. Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h#oldc... media/base/bit_reader.h:58: // Pointer to the next unread (not in curr_byte_) byte in the stream. On 2013/12/19 03:21:08, damienv1 wrote: > Update the comment. Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader.h#oldc... media/base/bit_reader.h:61: // Bytes left in the stream (without the curr_byte_). On 2013/12/19 03:21:08, damienv1 wrote: > Ditto. Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.cc File media/base/bit_reader_base.cc (right): https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:23: if (!Refill(1)) On 2013/12/19 03:29:12, damienv1 wrote: > Could be only one condition. Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:43: nbits_--; On 2013/12/19 03:46:51, damienv1 wrote: > zero_count++; Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:91: if (!Refill(num_bits)) { On 2013/12/19 03:29:12, damienv1 wrote: > Could be only one condition. Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:116: int max_nbytes = 8; On 2013/12/19 02:01:25, damienv1 wrote: > Should be sizeof(RegType) Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:126: reg_next_ = base::ByteSwap(reg_next_); On 2013/12/19 03:29:12, damienv1 wrote: > Should be NetToHost64 ! Done. https://codereview.chromium.org/112343011/diff/1/media/base/bit_reader_base.c... media/base/bit_reader_base.cc:137: if (nbits_ == 64 || nbits_next_ == 0) On 2013/12/19 02:01:25, damienv1 wrote: > Should be sizeof(RegType) * CHAR_BIT Done.
I also did a rough speed test between the previous and new version of the bit reader. Time to read 128 MBytes by chunk of "k" bits: k=1 bit: prv=13.00s => new=6.04s (54% decrease) k=3 bits: prv=6.55s => new=4.94s (25% decrease) k=6 bits: prv=3.58s => new=2.85s (20% decrease) k=12 bits: prv=2.03s => new=1.87s (8% decrease) k=27 bits: prv=1.84s => new=1.07s (42% decrease)
https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:33: *flag = (static_cast<int64>(reg_) < 0); Should be static_cast<SignedRegType> https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:47: is_one = (static_cast<int64>(reg_) < 0); Should be static_cast<SignedRegType> https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:91: // Try to have at least 9 bits (strictly more than 1 byte). if (nbits_ < 9) https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:173: void BitReaderCore::RefillCurrentRegister() { Comment.
https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:33: *flag = (static_cast<int64>(reg_) < 0); On 2013/12/26 19:35:04, damienv1 wrote: > Should be static_cast<SignedRegType> Done. https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:47: is_one = (static_cast<int64>(reg_) < 0); On 2013/12/26 19:35:04, damienv1 wrote: > Should be static_cast<SignedRegType> Done. https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:91: // Try to have at least 9 bits (strictly more than 1 byte). On 2013/12/26 19:35:04, damienv1 wrote: > if (nbits_ < 9) Done. https://codereview.chromium.org/112343011/diff/110001/media/base/bit_reader_c... media/base/bit_reader_core.cc:173: void BitReaderCore::RefillCurrentRegister() { On 2013/12/26 19:35:04, damienv1 wrote: > Comment. Done.
So I think this is getting closer, but I still have a few comments about the
approach.
At this point it seems like BitReader is just a thin wrapper over BitReaderCore
which makes me believe that BitReaderCore could actually just become BitReader.
Then H264BitReader could look as follows.
class H264BitReader {
H264BitReader(data, size) : bit_reader_(new H264ByteStreamProvider(data,size))
{}
BitReader bit_reader_;
}
You could then forward whatever calls you needed.
I suppose you could also do the following if you wanted H264BitReader to always
have the same generic interface as BitReader.
class H264BitReader : public BitReader {
H264BitReader(data, size) : BitReader(new H264ByteStreamProvider(data,size))
{}
}
I'm not as big of a fan of this second route since it looks like it would cause
a lot of rework for the existing code that uses H264BitReader without any sort
of clear win.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.cc
File media/base/bit_reader.cc (right):
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.c...
media/base/bit_reader.cc:58: class H264ByteStreamProvider : public
BitReaderCore::ByteStreamProvider {
This should not be here. It should be with the H.264 code.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.c...
media/base/bit_reader.cc:151: return (byte_stream_provider_->GetBytesLeft() *
CHAR_BIT +
Why not push this part of the computation down into the BitReaderCore? It has a
reference to the provider.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h
File media/base/bit_reader.h (right):
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h...
media/base/bit_reader.h:22: bool use_h264_byte_provider = false);
I don't think the generic BitReader should have any knowledge of H.264. Having
this here basically defeats the purpose of factoring out the byte provider. It
seems like there should be a constructor that takes a provider and one that
doesn't and uses the linear provider by default.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h...
media/base/bit_reader.h:45: bool HasMoreRBSPData() {
This seems like a H.264 specific concept and shouldn't be in this generic
reader.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h...
media/base/bit_reader.h:50: scoped_ptr<BitReaderCore::ByteStreamProvider>
byte_stream_provider_;
Why does this object own the provider instead of the BitReaderCore?
Aaron, here is how I was seeing the design: 1) ByteStreamProvider together with the BitReaderCore is a pipe: e.g. ByteStreamProvider gets its data from an array of bytes and provide some bytes to the BitReaderCore which itself delivers some bits. In this model, there is no reason why the BitReaderCore would be the owner of the ByteStreamProvider (On top of that, we could imagine a class implementing the ByteStreamProvider but already being managed by a different object). In my model, the BitReader is the owner of this pipe. 2) You're right, it would be better to give an instance of the ByteStreamProvider to the BitReader constructor (instead of having this h264 flag). However, internal classes BitReaderCore::ByteStreamProvider cannot be forward declared, and I didn't really want to expose this interface which in theory could just be hidden from the perspective of the user of BitReader.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.c... media/base/bit_reader.cc:58: class H264ByteStreamProvider : public BitReaderCore::ByteStreamProvider { On 2013/12/27 20:35:21, acolwell wrote: > This should not be here. It should be with the H.264 code. Will do. https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.c... media/base/bit_reader.cc:151: return (byte_stream_provider_->GetBytesLeft() * CHAR_BIT + On 2013/12/27 20:35:21, acolwell wrote: > Why not push this part of the computation down into the BitReaderCore? It has a > reference to the provider. You're right. I initially didn't have GetBytesLeft in the ByteStreamProvider interface, that's why it was not part of the BitReaderCore. https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h... media/base/bit_reader.h:22: bool use_h264_byte_provider = false); On 2013/12/27 20:35:21, acolwell wrote: > I don't think the generic BitReader should have any knowledge of H.264. Having > this here basically defeats the purpose of factoring out the byte provider. It > seems like there should be a constructor that takes a provider and one that > doesn't and uses the linear provider by default. You're right, it would be better to give an instance of the ByteStreamProvider to the BitReader constructor (instead of having this h264 flag). However, internal classes BitReaderCore::ByteStreamProvider cannot be forward declared, and I didn't really want to expose this interface which in theory could just be hidden from the perspective of the user of BitReader. https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h... media/base/bit_reader.h:50: scoped_ptr<BitReaderCore::ByteStreamProvider> byte_stream_provider_; On 2013/12/27 20:35:21, acolwell wrote: > Why does this object own the provider instead of the BitReaderCore? The BitReader is the owner of the pipe "ByteStreamProvider --(bytes)--> BitReaderCore --(Bits)-->" I don't see any reason why the BitReaderCore would be the owner of the ByteStreamProvider.
https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/230001/media/base/bit_reader.h... media/base/bit_reader.h:45: bool HasMoreRBSPData() { On 2013/12/27 20:35:21, acolwell wrote: > This seems like a H.264 specific concept and shouldn't be in this generic > reader. Done.
On 2013/12/27 21:17:56, damienv1 wrote: > Aaron, here is how I was seeing the design: > 1) ByteStreamProvider together with the BitReaderCore is a pipe: e.g. > ByteStreamProvider gets its data from an array of bytes and provide some bytes > to the BitReaderCore which itself delivers some bits. > In this model, there is no reason why the BitReaderCore would be the owner of > the ByteStreamProvider (On top of that, we could imagine a class implementing > the ByteStreamProvider but already being managed by a different object). > In my model, the BitReader is the owner of this pipe. Ok. Makes sense to me. Thanks for the explanation. > > 2) You're right, it would be better to give an instance of the > ByteStreamProvider to the BitReader constructor (instead of having this h264 > flag). However, internal classes BitReaderCore::ByteStreamProvider cannot be > forward declared, and I didn't really want to expose this interface which in > theory could just be hidden from the perspective of the user of BitReader. I see. I think I understand the separation of roles now.
https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:31: explicit BitReaderCore(ByteStreamProvider* byte_stream_provider); Add a comment to say the lifetime of the ByteStreamProvider should be longer than the lifetime of the BitReaderCore. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:31: // Array used to return some bytes when emulation prevention bytes when *some* ... https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.h:40: int NumBitsLeft() const { This method should not be exposed. This is confusing as the number of bits truly depends on whether some start code emulation prevention bytes are detected. The user of this class should not have to rely on that method.
https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:11: class LinearByteStreamProvider : public BitReaderCore::ByteStreamProvider { Since nothing else can access this class, is there any reason not to just make this part of BitReader and have BitReader privately inherit from BitReaderCore::ByteStreamProvider? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:13: LinearByteStreamProvider(const uint8* data, off_t size); nit: s/off_t/size_t/ since it is a size? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:29: int bytes_left_; nit: This should match the type passed into the constructor. It doesn't matter to me which one you use, but you should be consistent and not rely on implicit casting. I'm slightly leaning towards int everywhere since your other methods appear to be using that. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:36: DCHECK(data_ != NULL && bytes_left_ > 0); nit: You should probably change this to DCHECK(data_); CHECK_GE(bytes_left_, 0); since a negative bytes_left_ could result in reading invalid memory in the code below. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:44: int nbytes = max_nbytes; nit: Add the following DCHECKS to clarify expectations. DCHECK_GT(min_nbytes, 0); DCHECK_GE(max_nbytes, min_nbytes); DCHECK(out); https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:61: : byte_stream_provider_(new LinearByteStreamProvider(data, size)), If you don't go with my suggestion above, you should consider just making byte_stream_provider_ a LinearByteStreamProvider instead a pointer to the abstract interface since there is no way to actually change byte_stream_provider_ to any other type and the lifetime is always equal to the BitReader itself. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h... media/base/bit_reader.h:28: bool ReadFlag(bool* flag) { Why is this being added? It doesn't appear to be used by anything. Please defer adding this until actual usage appears. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h... media/base/bit_reader.h:41: int GetBitCount() { ditto https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:33: *flag = (static_cast<SignedRegType>(reg_) < 0); nit: How about just using something like reg_ > kint64max instead of introducing an extra type here just to check for MSb? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:45: if (nbits_ == 0 && !Refill(1)) How about this instead to avoid code duplication. do { if (!ReadFlag(&is_one)) return false; zero_count++; } while (!is_one); https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:55: if (zero_count > 31) nit: Shouldn't we put this inside the loop so we can fail as soon as we would overflow? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:74: while (num_bits >= 32) { nit: Why not skip 64 bits at a time since dummy is 64 bits? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:87: return (byte_stream_provider_->GetBytesLeft() * CHAR_BIT + nit: Is using CHAR_BIT here and elsewhere really buying us anything? Why not just use 8 since we are making that assumption elsewhere in this file. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:160: if (window_size == 0) Shouldn't this be window_size < min_nbytes? IIUC that would prevent this code from doing work that will ultimately result in returning false anyways. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:171: return (nbits_ >= min_nbits); nit: if you adjust the condition above, I believe you can turn this condition into a DCHECK_GE and always return true. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:182: int free_nbits = static_cast<int>(sizeof(RegType) * CHAR_BIT) - nbits_; nit: Please make a constant for static_cast<int>(sizeof(RegType) * CHAR_BIT) and use it everywhere or just use 64. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:187: } else { nit: early return and drop else. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:25: virtual int GetBytes(int min_n, int max_n, const uint8** array) = 0; nit: Add text about the expected lifetime of |*array|. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:45: template<typename T> bool ReadBits(int num_bits, T *out) { nit: s/T *out/T* out/ https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:57: bool ReadUE(uint32* out); nit: s/ReadUE/ReadExpGolomb/ ? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:62: // SkipBits operations will always return false unless |num_bits| is 0. nit: ReadFlag & ReadUE should be added or the comment should be reworded to include all bit reading and skipping methods in general. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:72: bool HasMoreRBSPData(); This H.264 concept should not be here. Please move it to the H.264 code. IIUC you'll probably need to add a PeekBits() method here to implement it though. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:24: // Does not include bits held in the bit registers. nit: Remove this comment since this code shouldn't know about the bit registers in the core? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:28: // Does not include bits held in the bit registers. ditto? https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:50: int min_nbytes, int max_nbytes, const uint8** out) { nit: This implementation and the one used by BitReader don't appear to care about min_nbytes. Consider removing it. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:54: bool epb = false; nit:s/epb/copy_to_data_window/ or something similar so the variable name is more descriptive. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:73: prev_two_bytes_ = (prev_two_bytes_ << 8) | static_cast<int>(*data_); If you make prev_two_bytes_ unsigned does it remove the need for a cast? If so, please make the change to simplify the code here. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:85: return bytes_left_; This doesn't seem right. Shouldn't this be returning the number of unescaped bytes. I think you might have to do a prescan of the buffer to find the number of escape sequences and subtract that off. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.h:17: class MEDIA_EXPORT BitReaderH264 { Why create this? I thought you'd just be moving content/common/gpu/media/h264_bit_reader.h into media/base and updating its implementation to use BitReaderCore. That seems like it would be the path of least work. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.h:54: scoped_ptr<BitReaderCore::ByteStreamProvider> byte_stream_provider_; nit: Since this can't be changed and has the same lifetime as this object, consider just using H264ByteStreamProvider here or move the functionality into this class an privately derive from ByteStreamProvider.
Aaron, thanks for the comments. Will upload a new CL when these are addressed. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:13: LinearByteStreamProvider(const uint8* data, off_t size); On 2013/12/28 01:54:42, acolwell wrote: > nit: s/off_t/size_t/ since it is a size? Need to check what is best (the original code was using off_t which does not look the best choice to me). https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:29: int bytes_left_; On 2013/12/28 01:54:42, acolwell wrote: > nit: This should match the type passed into the constructor. It doesn't matter > to me which one you use, but you should be consistent and not rely on implicit > casting. I'm slightly leaning towards int everywhere since your other methods > appear to be using that. Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:36: DCHECK(data_ != NULL && bytes_left_ > 0); On 2013/12/28 01:54:42, acolwell wrote: > nit: You should probably change this to > DCHECK(data_); > CHECK_GE(bytes_left_, 0); > > since a negative bytes_left_ could result in reading invalid memory in the code > below. Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:44: int nbytes = max_nbytes; On 2013/12/28 01:54:42, acolwell wrote: > nit: Add the following DCHECKS to clarify expectations. > DCHECK_GT(min_nbytes, 0); > DCHECK_GE(max_nbytes, min_nbytes); > DCHECK(out); Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:61: : byte_stream_provider_(new LinearByteStreamProvider(data, size)), On 2013/12/28 01:54:42, acolwell wrote: > If you don't go with my suggestion above, you should consider just making > byte_stream_provider_ a LinearByteStreamProvider instead a pointer to the > abstract interface since there is no way to actually change > byte_stream_provider_ to any other type and the lifetime is always equal to the > BitReader itself. But in this case, I cannot have LinearByteStreamProvider in an anonymous namespace (need forward declaration in the header file). https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h... media/base/bit_reader.h:28: bool ReadFlag(bool* flag) { On 2013/12/28 01:54:42, acolwell wrote: > Why is this being added? It doesn't appear to be used by anything. Please defer > adding this until actual usage appears. If we don't expose ReadFlag, all the parsers will continue to use ReadBits with an int to read flags... (vicious circle). I can add some unittests to exercise ReadFlag. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h... media/base/bit_reader.h:41: int GetBitCount() { On 2013/12/28 01:54:42, acolwell wrote: > ditto GetBitCount will remove the need of both: - NumEmulationPreventionBytesRead - and NumBitsLeft which are only used in the following piece of code: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:33: *flag = (static_cast<SignedRegType>(reg_) < 0); On 2013/12/28 01:54:42, acolwell wrote: > nit: How about just using something like reg_ > kint64max instead of introducing > an extra type here just to check for MSb? The comparison with 0 should be more efficient. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:45: if (nbits_ == 0 && !Refill(1)) On 2013/12/28 01:54:42, acolwell wrote: > How about this instead to avoid code duplication. > > do { > if (!ReadFlag(&is_one)) > return false; > zero_count++; > } while (!is_one); Might probably be best (even though there is a slight overhead: bit_count_ is updated many times instead of once. I might just go awith a different solution based on PeekBits (and might move this method to H264 bit reader only). https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:74: while (num_bits >= 32) { On 2013/12/28 01:54:42, acolwell wrote: > nit: Why not skip 64 bits at a time since dummy is 64 bits? Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:160: if (window_size == 0) On 2013/12/28 01:54:42, acolwell wrote: > Shouldn't this be window_size < min_nbytes? IIUC that would prevent this code > from doing work that will ultimately result in returning false anyways. That's what I was doing originally, but Refill is currently used by HasMoreRBSPData which requires 9 bits (but if there is less, those bits should be *correctly* returned). PeekBits / ViewBits will make a similar assumption. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:171: return (nbits_ >= min_nbits); On 2013/12/28 01:54:42, acolwell wrote: > nit: if you adjust the condition above, I believe you can turn this condition > into a DCHECK_GE and always return true. See explanation above. Need the current behavior for PeekBits / HasMoreRBSPData. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:25: virtual int GetBytes(int min_n, int max_n, const uint8** array) = 0; On 2013/12/28 01:54:42, acolwell wrote: > nit: Add text about the expected lifetime of |*array|. Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:31: explicit BitReaderCore(ByteStreamProvider* byte_stream_provider); On 2013/12/28 01:32:25, damienv1 wrote: > Add a comment to say the lifetime of the ByteStreamProvider should be longer > than the lifetime of the BitReaderCore. Done. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:45: template<typename T> bool ReadBits(int num_bits, T *out) { On 2013/12/28 01:54:42, acolwell wrote: > nit: s/T *out/T* out/ Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:57: bool ReadUE(uint32* out); On 2013/12/28 01:54:42, acolwell wrote: > nit: s/ReadUE/ReadExpGolomb/ ? That was my original naming but I noticed the current H264 bit reader (in content/common/gpu) is using ReadUE instead (which is the abbreviation used in the H264 spec for Exp-Golomb values). https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:72: bool HasMoreRBSPData(); On 2013/12/28 01:54:42, acolwell wrote: > This H.264 concept should not be here. Please move it to the H.264 code. IIUC > you'll probably need to add a PeekBits() method here to implement it though. Agree, I actually already started to do that (using ViewBits but PeekBits might be a better name). https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:24: // Does not include bits held in the bit registers. On 2013/12/28 01:54:42, acolwell wrote: > nit: Remove this comment since this code shouldn't know about the bit registers > in the core? Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:28: // Does not include bits held in the bit registers. On 2013/12/28 01:54:42, acolwell wrote: > ditto? Will do. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:31: // Array used to return some bytes when emulation prevention bytes On 2013/12/28 01:32:25, damienv1 wrote: > when *some* ... Done. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:50: int min_nbytes, int max_nbytes, const uint8** out) { On 2013/12/28 01:54:42, acolwell wrote: > nit: This implementation and the one used by BitReader don't appear to care > about min_nbytes. Consider removing it. Should be able to remove it. Didn't do it right away since I didn't know if I would use it in a near future. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:73: prev_two_bytes_ = (prev_two_bytes_ << 8) | static_cast<int>(*data_); On 2013/12/28 01:54:42, acolwell wrote: > If you make prev_two_bytes_ unsigned does it remove the need for a cast? If so, > please make the change to simplify the code here. Will check. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:85: return bytes_left_; On 2013/12/28 01:54:42, acolwell wrote: > This doesn't seem right. Shouldn't this be returning the number of unescaped > bytes. I think you might have to do a prescan of the buffer to find the number > of escape sequences and subtract that off. I implemented this one to be inline with the current H264 bit reader. However, I agree. If we want to correctly implement it, we should remove start code emulation prevention bytes. Since this is costly, I would prefer having a LOG(FATAL) or sth similar since this method should not be needed when dealing with H264 streams. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.h:40: int NumBitsLeft() const { On 2013/12/28 01:32:25, damienv1 wrote: > This method should not be exposed. > This is confusing as the number of bits truly depends on whether some start code > emulation prevention bytes are detected. > > The user of this class should not have to rely on that method. Will do.
https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:11: class LinearByteStreamProvider : public BitReaderCore::ByteStreamProvider { On 2013/12/28 01:54:42, acolwell wrote: > Since nothing else can access this class, is there any reason not to just make > this part of BitReader and have BitReader privately inherit from > BitReaderCore::ByteStreamProvider? Done. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:13: LinearByteStreamProvider(const uint8* data, off_t size); On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > nit: s/off_t/size_t/ since it is a size? > > Need to check what is best (the original code was using off_t which does not > look the best choice to me). Now use "int" everywhere. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:61: : byte_stream_provider_(new LinearByteStreamProvider(data, size)), On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > If you don't go with my suggestion above, you should consider just making > > byte_stream_provider_ a LinearByteStreamProvider instead a pointer to the > > abstract interface since there is no way to actually change > > byte_stream_provider_ to any other type and the lifetime is always equal to > the > > BitReader itself. > > But in this case, I cannot have LinearByteStreamProvider in an anonymous > namespace (need forward declaration in the header file). Did the private inheritance. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:45: if (nbits_ == 0 && !Refill(1)) On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > How about this instead to avoid code duplication. > > > > do { > > if (!ReadFlag(&is_one)) > > return false; > > zero_count++; > > } while (!is_one); > > Might probably be best (even though there is a slight overhead: bit_count_ is > updated many times instead of once. > > I might just go awith a different solution based on PeekBits (and might move > this method to H264 bit reader only). Done the way you suggested but moved the code to the H264 bit reader only. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:55: if (zero_count > 31) On 2013/12/28 01:54:42, acolwell wrote: > nit: Shouldn't we put this inside the loop so we can fail as soon as we would > overflow? I modified the behavior, it's now up to the caller to decide what to do in case of an exp-golomb value overflow. The function sticks to parsing an exp-golomb value whatever the length is. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:182: int free_nbits = static_cast<int>(sizeof(RegType) * CHAR_BIT) - nbits_; On 2013/12/28 01:54:42, acolwell wrote: > nit: Please make a constant for static_cast<int>(sizeof(RegType) * CHAR_BIT) and > use it everywhere or just use 64. Done. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:187: } else { On 2013/12/28 01:54:42, acolwell wrote: > nit: early return and drop else. Having an early return implicity involves that one case corresponds to the exceptional behavior. Here, both cases are completely equivalent and both can occur at roughly the same rate. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:62: // SkipBits operations will always return false unless |num_bits| is 0. On 2013/12/28 01:54:42, acolwell wrote: > nit: ReadFlag & ReadUE should be added or the comment should be reworded to > include all bit reading and skipping methods in general. Done. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:54: bool epb = false; On 2013/12/28 01:54:42, acolwell wrote: > nit:s/epb/copy_to_data_window/ or something similar so the variable name is more > descriptive. Done. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.h:17: class MEDIA_EXPORT BitReaderH264 { On 2013/12/28 01:54:42, acolwell wrote: > Why create this? I thought you'd just be moving > content/common/gpu/media/h264_bit_reader.h into media/base and updating its > implementation to use BitReaderCore. That seems like it would be the path of > least work. I was planning to do another CL which: - remove the previous H264 bit reader - and then use this one instead. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_h... media/base/bit_reader_h264.h:54: scoped_ptr<BitReaderCore::ByteStreamProvider> byte_stream_provider_; On 2013/12/28 01:54:42, acolwell wrote: > nit: Since this can't be changed and has the same lifetime as this object, > consider just using H264ByteStreamProvider here or move the functionality into > this class an privately derive from ByteStreamProvider. Done: now derives from ByteStreamProvider.
https://codereview.chromium.org/112343011/diff/630001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/630001/media/base/bit_reader.h... media/base/bit_reader.h:16: : private BitReaderCore::ByteStreamProvider { Should be NON_EXPORTED_BASE(private BitReaderCore::ByteStreamProvider) https://codereview.chromium.org/112343011/diff/630001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/630001/media/base/bit_reader_h... media/base/bit_reader_h264.h:16: : private BitReaderCore::ByteStreamProvider { Should be NON_EXPORTED_BASE(private BitReaderCore::ByteStreamProvider)
https://codereview.chromium.org/112343011/diff/630001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/630001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:44: return false; return -1;
https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:37: if (zero_count > 31) { Equivalent but better would be: if (code_size > kMaxExpGolombCodeSize)
More comments on that CL ? Note: I can remove the H264 bit reader and add it later in a different CL if needed (when the H264 parser has been moved to /media).
Here's my next round of comments. It might be best to defer adding the H.264 reader to another CL that actually adds production code that uses it. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.cc File media/base/bit_reader.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.c... media/base/bit_reader.cc:61: : byte_stream_provider_(new LinearByteStreamProvider(data, size)), On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > If you don't go with my suggestion above, you should consider just making > > byte_stream_provider_ a LinearByteStreamProvider instead a pointer to the > > abstract interface since there is no way to actually change > > byte_stream_provider_ to any other type and the lifetime is always equal to > the > > BitReader itself. > > But in this case, I cannot have LinearByteStreamProvider in an anonymous > namespace (need forward declaration in the header file). That's ok. You can just make it a private inner class instead. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h... media/base/bit_reader.h:28: bool ReadFlag(bool* flag) { On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > Why is this being added? It doesn't appear to be used by anything. Please > defer > > adding this until actual usage appears. > > If we don't expose ReadFlag, all the parsers will continue to use ReadBits with > an int to read flags... (vicious circle). > I can add some unittests to exercise ReadFlag. I understand. I'd prefer you add it in a follow-up CL where you replace some of those int flag readers w/ ReadFlag() calls. That way we make sure production code is using this functionality. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader.h... media/base/bit_reader.h:41: int GetBitCount() { On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > ditto > > GetBitCount will remove the need of both: > - NumEmulationPreventionBytesRead > - and NumBitsLeft > which are only used in the following piece of code: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... Ok, but I'd prefer you defer the addition of the method until you actually replace that functionality. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.cc:33: *flag = (static_cast<SignedRegType>(reg_) < 0); On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > nit: How about just using something like reg_ > kint64max instead of > introducing > > an extra type here just to check for MSb? > > The comparison with 0 should be more efficient. I think we should let the compiler worry about that. I'm more concerned about readability and making it clear to the reader what is happening. https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/470001/media/base/bit_reader_c... media/base/bit_reader_core.h:57: bool ReadUE(uint32* out); On 2013/12/28 02:26:17, damienv1 wrote: > On 2013/12/28 01:54:42, acolwell wrote: > > nit: s/ReadUE/ReadExpGolomb/ ? > > That was my original naming but I noticed the current H264 bit reader (in > content/common/gpu) is using ReadUE instead (which is the abbreviation used in > the H264 spec for Exp-Golomb values). That is why I suggested ExpGolumb over UE because it is a more descriptive name and not tied to the H.264 spec. :) https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.cc:37: *flag = (static_cast<SignedRegType>(reg_) < 0); Please don't do this. It obscures what the actual code is doing. Just use (reg_ & (1 << (kRegWidthInBits - 1))) != 0 or some similar construct that makes it clear that you are just checking the MSb. Any reasonable compiler will properly recognize this as an MSb check and generate the best instructions for it. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.cc:120: int max_nbytes = sizeof(RegType); nit: How about just using sizeof(reg_next_) here since that's that you are moving the bits into anyways? https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.cc:132: nbits_next_ = window_size * CHAR_BIT; Please just use 8 instead of CHAR_BIT everywhere in this file. It is highly unlikely we'll build this code on a platform where CHAR_BIT is not 8 and I don't think this extra level of indirection helps make the code more readable. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:43: // T can especially not be a boolean since it generates Perhaps we should just define the following so this comment isn't necessary. bool ReadBits(int num_bits, bool* out) { DCHECK_EQ(num_bits, 1); return ReadFlag(out); } Either that or declare the bool specialization, but don't define it anywhere so that a link error leads people to ReadFlag(). https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:63: int PeekBitsMsbAligned(int num_bits, uint64* out); PeekBits should expose bits the same way that ReadBits() does. Having them shifted towards the MSb seem odd and feels out of place here. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:73: int GetBitCount() const; s/GetBitCount/bits_read/ since it is a simple accessor and make the name reflect what the comment says. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:79: typedef uint64 RegType; Please remove these types and just use uint64 below. The aren't really buying us anything since there are already 64-bit assumptions sprinkled around the code. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:40: return code_size; Why don't we just return -1 here? It seems like the caller is assuming that this should be a valid 32 bit ExpGolomb code. Anything bigger than this is likely a bitstream error. Why allow the code to progress here? https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:64: return (bit_reg << 1) != 0; Is this correct? This doesn't check that the MSb is set. This appears to check whether there are any non-zero bits after the next available, but it does not verify that the next bit is the last non-zero bit. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:108: NOTREACHED(); This is a code smell. If both providers can't properly implement this method then it should be removed from the provider interface. Perhaps bits_available() should be moved to BitReader if you don't want to support it for H.264 https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.h:36: int GetBitCount() { s/GetBitCount/bits_read/ for similar reasons as in the other file. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.h:43: // Note: if the number of bits is greater than |kMaxExpGolombCodeSize|, This seems like a weird API. Why not simply make this bool ReadUE(uint32* out); so it matches the style of the other methods? It would then just return false if a code that was too long was detected. Why do you need to know how many bits were actually used by the code?
Aaron, I will remove the H264 bit reader when there is no more comment on that CL. (I just want to make sure this CL is suitable for implementing an H264 bit reader, think of the H264 bit reader as an example that I will remove before merging). https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.cc:37: *flag = (static_cast<SignedRegType>(reg_) < 0); On 2014/01/06 22:44:06, acolwell wrote: > Please don't do this. It obscures what the actual code is doing. Just use (reg_ > & (1 << (kRegWidthInBits - 1))) != 0 or some similar construct that makes it > clear that you are just checking the MSb. Any reasonable compiler will properly > recognize this as an MSb check and generate the best instructions for it. Will do. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.cc:120: int max_nbytes = sizeof(RegType); On 2014/01/06 22:44:06, acolwell wrote: > nit: How about just using sizeof(reg_next_) here since that's that you are > moving the bits into anyways? Will do. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.cc:132: nbits_next_ = window_size * CHAR_BIT; On 2014/01/06 22:44:06, acolwell wrote: > Please just use 8 instead of CHAR_BIT everywhere in this file. It is highly > unlikely we'll build this code on a platform where CHAR_BIT is not 8 and I don't > think this extra level of indirection helps make the code more readable. I can do it although I think it's good programming practice to use this type of constant when available. I don't think this makes the code less readable in this specific case. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:43: // T can especially not be a boolean since it generates On 2014/01/06 22:44:06, acolwell wrote: > Perhaps we should just define the following so this comment isn't necessary. > > bool ReadBits(int num_bits, bool* out) { > DCHECK_EQ(num_bits, 1); > return ReadFlag(out); > } > > Either that or declare the bool specialization, but don't define it anywhere so > that a link error leads people to ReadFlag(). The first solution sounds good to me. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:63: int PeekBitsMsbAligned(int num_bits, uint64* out); On 2014/01/06 22:44:06, acolwell wrote: > PeekBits should expose bits the same way that ReadBits() does. Having them > shifted towards the MSb seem odd and feels out of place here. Most of the time, you use PeekBits to read multiple chunks of bits. And it's much more convenient to work with bits aligned on MSB. => I think it simplify code using PeekBits to get bits MSB aligned. What I can do is provide both PeekBits and PeekBitsMsbAligned in this case. Does it sound good to you ? https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:73: int GetBitCount() const; On 2014/01/06 22:44:06, acolwell wrote: > s/GetBitCount/bits_read/ since it is a simple accessor and make the name reflect > what the comment says. Will do. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_c... media/base/bit_reader_core.h:79: typedef uint64 RegType; On 2014/01/06 22:44:06, acolwell wrote: > Please remove these types and just use uint64 below. The aren't really buying us > anything since there are already 64-bit assumptions sprinkled around the code. OK, will do it in the end. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:37: if (zero_count > 31) { On 2014/01/02 22:56:23, damienv1 wrote: > Equivalent but better would be: > > if (code_size > kMaxExpGolombCodeSize) Done. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:40: return code_size; On 2014/01/06 22:44:06, acolwell wrote: > Why don't we just return -1 here? It seems like the caller is assuming that this > should be a valid 32 bit ExpGolomb code. Anything bigger than this is likely a > bitstream error. Why allow the code to progress here? My assumption here is that the bit reader should not be the one to decide whether this is a wrong code. Likely to be a bitstream error does not mean this is a actually a bitstream error. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:64: return (bit_reg << 1) != 0; On 2014/01/06 22:44:06, acolwell wrote: > Is this correct? This doesn't check that the MSb is set. This appears to check > whether there are any non-zero bits after the next available, but it does not > verify that the next bit is the last non-zero bit. * Case 1, "(bit_reg << 1) != 0": in this case, this is correct, we didn't reach the end of the RBSP data since at least the MSB is part of the RBSP data. * Case 2, "(bit_reg << 1) == 0": - implicitly, the MSB will be one: in this case, this is really the end of the RBSP data. - If the MSB was zero, either the application would have read too many bits (and read the stop bit) or the stream would be ill-formed (having the last 8 bits all set to 0). In either case, this can be considered as the end of the rbsp data. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:108: NOTREACHED(); On 2014/01/06 22:44:06, acolwell wrote: > This is a code smell. If both providers can't properly implement this method > then it should be removed from the provider interface. Perhaps bits_available() > should be moved to BitReader if you don't want to support it for H.264 Yes, I can do that. This is now possible since the BitReader is the one implementing the byte stream provider. https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.h:36: int GetBitCount() { On 2014/01/06 22:44:06, acolwell wrote: > s/GetBitCount/bits_read/ for similar reasons as in the other file. OK I can do it since this is the chromium style. However, I don't really like that convention because it makes the naming dependent on what you are doing in the function (let's say later, I change the code, and returning the number of bits read so far is not a simple accessor, we have to change the name and all the code using this class). https://codereview.chromium.org/112343011/diff/740001/media/base/bit_reader_h... media/base/bit_reader_h264.h:43: // Note: if the number of bits is greater than |kMaxExpGolombCodeSize|, On 2014/01/06 22:44:06, acolwell wrote: > This seems like a weird API. Why not simply make this bool ReadUE(uint32* out); > so it matches the style of the other methods? It would then just return false if > a code that was too long was detected. Why do you need to know how many bits > were actually used by the code? This allows the user of this class to differentiate between an "end of stream" error VS a code which is using too many bits. Another example: let's the user of this class just wants to skip an unsigned exp-golomb value. It does not care about the value. It we just return false, there is no way to check whether the error is the result of end of stream. The API is more flexible to me. As opposed to other functions which knows how many bits the code is, here we don't know in advance. This information might be useful.
lgtm % nits and assuming the H264 changes are removed and used in a follow-up CL that replaced the existing reader. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader.... media/base/bit_reader.h:37: return GetBytesLeft() * 8 + bit_reader_core_.bits_buffered(); nit: s/GetBytesLeft()/bytes_left_/ and remove the function since this appears to be the only call site. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. s/2013/2014 https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:37: *flag = (reg_ & (UINT64_C(1) << (kRegWidthInBits - 1))) != 0; nit: I believe this should be GG_UINT64_C based on other example I've seen in the codebase. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:125: if (window_size == 0) Add the following before this line just to allow us to detect rogue providers. DCHECK_GE(windows_size, 0); DCHECK_LE(windows_size, max_nbytes); https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:152: } else { nit: early return and drop else. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. s/2013/2014 https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:28: // Lifetime of |byte_stream_provider| should be longer than BitReaderCore. nit: s/should/must/ https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:49: // T can especially not be a boolean since it generates nit: I believe this comment is no longer necessary because of the bool overload above. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:64: // Bits returned in |*out| are written from MSB to LSB. nit: I think the from MSB to LSB might be a little confusing. How about something like: Bits returned in |*out| are shifted so the most significant bit contains the next bit that can be read from the stream. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:69: int PeekBitsMsbAligned(int num_bits, uint64* out); nit: You might also want to note the guarantees about the (64 - (return value)) bits. They are always guaranteed to be 0 right? https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. s/2013/2014 https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.cc:94: // If zero_count is greater than 31, the calculated value will overflow, nit: This comment appears to be out of sync w/ the code. kMaxExpGolombCodeSize is 63 https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.cc:103: uint32 base = (1 << zero_count) - 1; nit: Doesn't this take us into undefined territory since you can shift beyond 31 bits here. Is it even worth running this code if we know it will overflow an uint32 but not overflow a uint64? https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. s/2013/2014 https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.h:16: class MEDIA_EXPORT BitReaderH264 I'm assuming you're removing this before landing since it isn't used by production code yet. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_h264_unittest.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/2012/2014 https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_unittest.cc:67: TEST(BitReaderTest, BitCountTest) { nit: s/BitCount/BitsRead
https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... File media/base/bit_reader_core.cc (right): https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.cc:60: if (!ReadBitsInternal(kRegWidthInBits, &dummy)) What do you think of instead: if (num_bits >= kRegWidthInBits * 2) zero out both regs skip ((num_bits - existing bits in regs) % 8) bytes in provider read reminder and eliminate the loop and the inefficiency altogether? A SkipBytes/Seek method in provider could be useful. https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.cc:154: reg_next_ <<= free_nbits; Would this work instead of l:145-156? free_nbits = kRegWidthInBits - nbits_; bits_to_refill = min(free_nbits : nbits_next_); reg_ |= (reg_next_ >> bits_to_refill); reg_next_ <<= bits_to_refill; nbits_ += bits_to_refill; nbits_next_ -= bits_to_refill; https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... File media/base/bit_reader_core.h (right): https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:22: // - n must be less than or equal to |max_n|. Perhaps instead something like below would be easier to understand? "Set |*array| to point to a memory buffer containing at most max_n bytes and return the number of bytes available." Why go with max instead of min though? The buffer is owned by the provider so the provider is not limited by maximum size allocated by client... I feel the provider should be free to do whatever it wants as long it can fulfill client's request for a required number of bytes (i.e. minimum), or return error... https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:24: // Note: |*array| is ensured to be valid until the next call to GetBytes. s/is ensured to be/has to remain/ https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:29: explicit BitReaderCore(ByteStreamProvider* byte_stream_provider); Why not a scoped_ptr instead? I don't think anyone else can use the provider once it's passed to us and it would be owned solely by this class? https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:30: ~BitReaderCore(); virtual? https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:32: // Read a boolean from the stream and return in |*out|. To be super correct we should say: "Read one bit from the stream, convert it to bool and return in |*out|." "Read a bool" feels ambiguous since we are exposing a num_bits argument, but weakly DCHECK for it and then silently assume num_bits=1. https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:35: // on windows platforms. s/windows/Windows/ https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:36: bool ReadBits(int num_bits, bool* out) { Is the remark needed still since it's a specialization already (my templates knowledge is a bit rusty though)? But honestly I'm a bit uncomfortable with this. We have a num_bits argument and DCHECKs have a tendency not to be hit, people don't run in Debug that often. https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:42: // from the stream starting at |num_bits| position in |*out|. I feel this could be easier to understand (at least to me): "Read next |num_bits| from the stream and return in |*out| as a value of type T, with the first bit from the stream as the most significant bit of the resulting value." I had to consult the implementation to make sure our interpretation of what the "num_bits position" was the same though, there is quite a bit of MSB/LSB juggling around this class. https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:48: // integer type. I feel there should be a note what happens when T is signed and how that is influenced by num_bits. If we have T=int, and want an 8 bit value from the stream. Depending on the stream format, we could have some room for interpretation that that might be interpreted as a negative value (if MSBit is 1) or not, but the returned value will always be positive? https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:52: template<typename T> bool ReadBits(int num_bits, T* out) { I feel all num_bits should be unsigned in this CL... https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:60: // Read a boolean. I'd suggest "Read one next bit and convert to bool". https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:69: int PeekBitsMsbAligned(int num_bits, uint64* out); Nit again, but it feels a bit asymmetric to have ReadBits return bool success, and PeakBits return number of bits... https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:81: // Returns the number of bits buffered in BitReaderCore. Why is this useful for clients? Isn't this an implementation detail of BRC? https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:100: // Number of valid bits left in |reg|. s/reg/reg_ https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:102: int nbits_; Why do we need both bits_read_ and nbits_ ? https://chromiumcodereview.appspot.com/112343011/diff/1030001/media/base/bit_... media/base/bit_reader_core.h:105: // Number of valid bits left in |reg_next_|. s/valid/unconsumed/
https://codereview.chromium.org/112343011/diff/910001/media/base/bit_reader_h... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/910001/media/base/bit_reader_h... media/base/bit_reader_h264.cc:23: int BitReaderH264::ReadUE(uint32* out) { ReadUEInternal https://codereview.chromium.org/112343011/diff/910001/media/base/bit_reader_h... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/910001/media/base/bit_reader_h... media/base/bit_reader_h264.h:45: int ReadUE(uint32* out); bool ReadUE(uint32* out, int* code_size = NULL); https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader.h File media/base/bit_reader.h (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader.... media/base/bit_reader.h:37: return GetBytesLeft() * 8 + bit_reader_core_.bits_buffered(); On 2014/01/11 00:24:37, acolwell wrote: > nit: s/GetBytesLeft()/bytes_left_/ and remove the function since this appears to > be the only call site. Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/11 00:24:37, acolwell wrote: > s/2013/2014 Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:37: *flag = (reg_ & (UINT64_C(1) << (kRegWidthInBits - 1))) != 0; On 2014/01/11 00:24:37, acolwell wrote: > nit: I believe this should be GG_UINT64_C based on other example I've seen in > the codebase. Thanks, that's correct (I forgot Chromium did port these macros). https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:60: if (!ReadBitsInternal(kRegWidthInBits, &dummy)) On 2014/01/11 02:30:49, Pawel Osciak wrote: > What do you think of instead: > if (num_bits >= kRegWidthInBits * 2) > zero out both regs > skip ((num_bits - existing bits in regs) % 8) bytes in provider > read reminder > > and eliminate the loop and the inefficiency altogether? > > A SkipBytes/Seek method in provider could be useful. I agree this is more efficient. However, I would rather leave it as an optimization that can be done in a different CL if there are some real use cases of large skips. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:125: if (window_size == 0) On 2014/01/11 00:24:37, acolwell wrote: > Add the following before this line just to allow us to detect rogue providers. > > DCHECK_GE(windows_size, 0); > DCHECK_LE(windows_size, max_nbytes); Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:152: } else { On 2014/01/11 00:24:37, acolwell wrote: > nit: early return and drop else. Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.cc:154: reg_next_ <<= free_nbits; On 2014/01/11 02:30:49, Pawel Osciak wrote: > Would this work instead of l:145-156? > > free_nbits = kRegWidthInBits - nbits_; > bits_to_refill = min(free_nbits : nbits_next_); > > reg_ |= (reg_next_ >> bits_to_refill); > reg_next_ <<= bits_to_refill; > > nbits_ += bits_to_refill; > nbits_next_ -= bits_to_refill; > If |bits_to_refill| is equal to 64, then it does not work: if a is a 64 bit integer, (a << 64) or (a >> 64) is not defined by the C/C++ standard and does not return 0 as it could have been expected. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_core.h (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/11 00:24:37, acolwell wrote: > s/2013/2014 Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:22: // - n must be less than or equal to |max_n|. On 2014/01/11 02:30:49, Pawel Osciak wrote: > Perhaps instead something like below would be easier to understand? > > "Set |*array| to point to a memory buffer containing at most max_n bytes and > return the number of bytes available." > > Why go with max instead of min though? > > The buffer is owned by the provider so the provider is not limited by maximum > size allocated by client... I feel the provider should be free to do whatever it > wants as long it can fulfill client's request for a required number of bytes > (i.e. minimum), or return error... Done. BitReaderCore has a maximum capacity in terms of bits. So when it needs to get some bytes, it specify what is the max number of bytes it can consume. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:24: // Note: |*array| is ensured to be valid until the next call to GetBytes. On 2014/01/11 02:30:49, Pawel Osciak wrote: > s/is ensured to be/has to remain/ Will fix the description. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:28: // Lifetime of |byte_stream_provider| should be longer than BitReaderCore. On 2014/01/11 00:24:37, acolwell wrote: > nit: s/should/must/ Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:29: explicit BitReaderCore(ByteStreamProvider* byte_stream_provider); On 2014/01/11 02:30:49, Pawel Osciak wrote: > Why not a scoped_ptr instead? I don't think anyone else can use the provider > once it's passed to us and it would be owned solely by this class? Here is one of my previous comment that should answer your question: "ByteStreamProvider together with the BitReaderCore is a pipe: e.g. ByteStreamProvider gets its data from an array of bytes and provide some bytes to the BitReaderCore which itself delivers some bits. In this model, there is no reason why the BitReaderCore would be the owner of the ByteStreamProvider (On top of that, we could imagine a class implementing the ByteStreamProvider but already being managed by a different object). In my model, the BitReader is the owner of this pipe." https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:30: ~BitReaderCore(); On 2014/01/11 02:30:49, Pawel Osciak wrote: > virtual? BitReaderCore is not meant to be subclassed so no need of virtual. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:32: // Read a boolean from the stream and return in |*out|. On 2014/01/11 02:30:49, Pawel Osciak wrote: > To be super correct we should say: > > "Read one bit from the stream, convert it to bool and return in |*out|." > > "Read a bool" feels ambiguous since we are exposing a num_bits argument, but > weakly DCHECK for it and then silently assume num_bits=1. I'll try to make the description more accurate. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:35: // on windows platforms. On 2014/01/11 02:30:49, Pawel Osciak wrote: > s/windows/Windows/ Will do. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:36: bool ReadBits(int num_bits, bool* out) { On 2014/01/11 02:30:49, Pawel Osciak wrote: > Is the remark needed still since it's a specialization already (my templates > knowledge is a bit rusty though)? > > But honestly I'm a bit uncomfortable with this. We have a num_bits argument and > DCHECKs have a tendency not to be hit, people don't run in Debug that often. - I think it's less error prone than what we used to use before that CL where every flag was declared as an "int" and we use to do: int flag; ReadBits(1, &flag); Now at least in debug mode, we can catch the issue which was not the case before. - Running it only once in debug mode should catch most of the issue since reading a flag means the field is of constant length (and it should be one) and so the DCHECK will be triggered from the 1st run. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:42: // from the stream starting at |num_bits| position in |*out|. On 2014/01/11 02:30:49, Pawel Osciak wrote: > I feel this could be easier to understand (at least to me): > > "Read next |num_bits| from the stream and return in |*out| as a value of type T, > with the first bit from the stream as the most significant bit of the resulting > value." > > I had to consult the implementation to make sure our interpretation of what the > "num_bits position" was the same though, there is quite a bit of MSB/LSB > juggling around this class. This description was the original description of BitReader::ReadBits. I didn't change it. It's also clear that "the first bit of the stream is set at |num_bits| position in |*out|". Let me make the comment more detailed. I will add that all the other bits are set to 0. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:48: // integer type. On 2014/01/11 02:30:49, Pawel Osciak wrote: > I feel there should be a note what happens when T is signed and how that is > influenced by num_bits. > > If we have T=int, and want an 8 bit value from the stream. Depending on the > stream format, we could have some room for interpretation that that might be > interpreted as a negative value (if MSBit is 1) or not, but the returned value > will always be positive? Added a note in the comment to make it clear that when |T| is a signed type, the first bit is *NOT* the sign of the resulting value. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:49: // T can especially not be a boolean since it generates On 2014/01/11 00:24:37, acolwell wrote: > nit: I believe this comment is no longer necessary because of the bool overload > above. Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:52: template<typename T> bool ReadBits(int num_bits, T* out) { On 2014/01/11 02:30:49, Pawel Osciak wrote: > I feel all num_bits should be unsigned in this CL... From the Chromium coding style, I feel the other way. "Do not use unsigned types to mean "this value should never be < 0". For that, use assertions or run-time checks (as appropriate)." https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:60: // Read a boolean. On 2014/01/11 02:30:49, Pawel Osciak wrote: > I'd suggest "Read one next bit and convert to bool". Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:64: // Bits returned in |*out| are written from MSB to LSB. On 2014/01/11 00:24:37, acolwell wrote: > nit: I think the from MSB to LSB might be a little confusing. How about > something like: > Bits returned in |*out| are shifted so the most significant bit contains the > next bit that can be read from the stream. Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:69: int PeekBitsMsbAligned(int num_bits, uint64* out); On 2014/01/11 00:24:37, acolwell wrote: > nit: You might also want to note the guarantees about the (64 - (return value)) > bits. They are always guaranteed to be 0 right? Rephrased what is the meaning of |num_bits| in this function. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:69: int PeekBitsMsbAligned(int num_bits, uint64* out); On 2014/01/11 02:30:49, Pawel Osciak wrote: > Nit again, but it feels a bit asymmetric to have ReadBits return bool success, > and PeakBits return number of bits... It's asymmetric because they don't offer the same features. PeekBits is basically always successful. As an example, PeekBits can be used to read variable length codes where you know what is the maximum code size. In this case, returning a number of bits that is lower is not an error. Same applies to the way it is used by the H264 bit reader (to check the end of the RBSP). https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:81: // Returns the number of bits buffered in BitReaderCore. On 2014/01/11 02:30:49, Pawel Osciak wrote: > Why is this useful for clients? Isn't this an implementation detail of BRC? I rewrote BitReader::bits_available so that it does not use BitReaderCore::bits_buffered anymore. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:100: // Number of valid bits left in |reg|. On 2014/01/11 02:30:49, Pawel Osciak wrote: > s/reg/reg_ Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:102: int nbits_; On 2014/01/11 02:30:49, Pawel Osciak wrote: > Why do we need both bits_read_ and nbits_ ? |bits_read| is the number of bits read so far (it keeps accumulating). On the other hand, |nbits_| is the number of valid bits left in |reg_|. There is no relationship between the 2 of them. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_core.h:105: // Number of valid bits left in |reg_next_|. On 2014/01/11 02:30:49, Pawel Osciak wrote: > s/valid/unconsumed/ Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_h264.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/11 00:24:37, acolwell wrote: > s/2013/2014 Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.cc:94: // If zero_count is greater than 31, the calculated value will overflow, On 2014/01/11 00:24:37, acolwell wrote: > nit: This comment appears to be out of sync w/ the code. kMaxExpGolombCodeSize > is 63 Updated the comment based on the exp-golomb code size. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.cc:103: uint32 base = (1 << zero_count) - 1; On 2014/01/11 00:24:37, acolwell wrote: > nit: Doesn't this take us into undefined territory since you can shift beyond 31 > bits here. Is it even worth running this code if we know it will overflow an > uint32 but not overflow a uint64? The code size is basically twice longer that the size in bits of the final value so a 32 bit integer can receive a 63 bit exp-golomb value. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_h264.h (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/11 00:24:37, acolwell wrote: > s/2013/2014 Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264.h:16: class MEDIA_EXPORT BitReaderH264 On 2014/01/11 00:24:37, acolwell wrote: > I'm assuming you're removing this before landing since it isn't used by > production code yet. Correct. Will fix the nits and remove it after that (will be part of a different CL). https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_h264_unittest.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_h264_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/01/11 00:24:37, acolwell wrote: > s/2012/2014 Done. https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/112343011/diff/1030001/media/base/bit_reader_... media/base/bit_reader_unittest.cc:67: TEST(BitReaderTest, BitCountTest) { On 2014/01/11 00:24:37, acolwell wrote: > nit: s/BitCount/BitsRead Done.
If no more feedback, I will commit tomorrow morning based on Aaron's LGTM. Thanks for the review !
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/112343011/1420001
Message was sent while issue was closed.
Change committed as 244959 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
