Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader_core.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

s/2013/2014

[damienv1, 2014/01/13 22:41:06]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_BASE_BIT_READER_CORE_H_
+#define MEDIA_BASE_BIT_READER_CORE_H_
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+class MEDIA_EXPORT BitReaderCore {
+ public:
+  class ByteStreamProvider {
+   public:
+    ByteStreamProvider();
+    virtual ~ByteStreamProvider();
+
+    // Request n number of bytes where:
+    // - n must be less than or equal to |max_n|.

[Pawel Osciak, 2014/01/11 02:30:49]

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...

[damienv1, 2014/01/13 22:41:06]

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.

+    // Return the actual number of bytes available in |*array|.
+    // Note: |*array| is ensured to be valid until the next call to GetBytes.

[Pawel Osciak, 2014/01/11 02:30:49]

s/is ensured to be/has to remain/

[damienv1, 2014/01/13 22:41:06]

Will fix the description.

+    virtual int GetBytes(int max_n, const uint8** array) = 0;
+  };
+
+  // Lifetime of |byte_stream_provider| should be longer than BitReaderCore.

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

nit: s/should/must/

[damienv1, 2014/01/13 22:41:06]

Done.

+  explicit BitReaderCore(ByteStreamProvider* byte_stream_provider);

[Pawel Osciak, 2014/01/11 02:30:49]

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?

[damienv1, 2014/01/13 22:41:06]

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."

+  ~BitReaderCore();

[Pawel Osciak, 2014/01/11 02:30:49]

virtual?

[damienv1, 2014/01/13 22:41:06]

BitReaderCore is not meant to be subclassed so no need of virtual.

+
+  // Read a boolean from the stream and return in |*out|.

[Pawel Osciak, 2014/01/11 02:30:49]

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.

[damienv1, 2014/01/13 22:41:06]

I'll try to make the description more accurate.

+  // Remark: do not use the template version for reading a bool
+  // since it generates some optimization warnings during compilation
+  // on windows platforms.

[Pawel Osciak, 2014/01/11 02:30:49]

s/windows/Windows/

[damienv1, 2014/01/13 22:41:06]

Will do.

+  bool ReadBits(int num_bits, bool* out) {

[Pawel Osciak, 2014/01/11 02:30:49]

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.

[damienv1, 2014/01/13 22:41:06]

- 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.

+    DCHECK_EQ(num_bits, 1);
+    return ReadFlag(out);
+  }
+
+  // Read |num_bits| next bits from stream and return in |*out|, first bit
+  // from the stream starting at |num_bits| position in |*out|.

[Pawel Osciak, 2014/01/11 02:30:49]

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.

[damienv1, 2014/01/13 22:41:06]

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.

+  // |num_bits| cannot be larger than the bits the type can hold.
+  // Return false if the given number of bits cannot be read (not enough
+  // bits in the stream), true otherwise. When return false, the stream will
+  // enter a state where further ReadBits/SkipBits operations will always
+  // return false unless |num_bits| is 0. The type |T| has to be a primitive
+  // integer type.

[Pawel Osciak, 2014/01/11 02:30:49]

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?

[damienv1, 2014/01/13 22:41:06]

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.

+  // T can especially not be a boolean since it generates

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

nit: I believe this comment is no longer necessary because of the bool overload
above.

[damienv1, 2014/01/13 22:41:06]

Done.

+  // some optimization warnings during compilation on windows platforms,
+  // use ReadFlag instead.
+  template<typename T> bool ReadBits(int num_bits, T* out) {

[Pawel Osciak, 2014/01/11 02:30:49]

I feel all num_bits should be unsigned in this CL...

[damienv1, 2014/01/13 22:41:06]

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)."

+    DCHECK_LE(num_bits, static_cast<int>(sizeof(T) * 8));
+    uint64 temp;
+    bool ret = ReadBitsInternal(num_bits, &temp);
+    *out = static_cast<T>(temp);
+    return ret;
+  }
+
+  // Read a boolean.

[Pawel Osciak, 2014/01/11 02:30:49]

I'd suggest "Read one next bit and convert to bool".

[damienv1, 2014/01/13 22:41:06]

Done.

+  bool ReadFlag(bool* flag);
+
+  // Retrieve some bits without actually consuming them.
+  // Bits returned in |*out| are written from MSB to LSB.

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

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.

[damienv1, 2014/01/13 22:41:06]

Done.

+  // Return the number of bits written in |out|.
+  // Note: if the number of remaining bits is less than |num_bits|,
+  // only the remaining bits are returned and in this case, the number
+  // of bits returned is less than |num_bits|.
+  int PeekBitsMsbAligned(int num_bits, uint64* out);

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

nit: You might also want to note the guarantees about the (64 - (return value))
bits. They are always guaranteed to be 0 right?

[Pawel Osciak, 2014/01/11 02:30:49]

Nit again, but it feels a bit asymmetric to have ReadBits return bool success,
and PeakBits return number of bits...

[damienv1, 2014/01/13 22:41:06]

Rephrased what is the meaning of |num_bits| in this function.

[damienv1, 2014/01/13 22:41:06]

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).

+
+  // Skip |num_bits| next bits from stream. Return false if the given number of
+  // bits cannot be skipped (not enough bits in the stream), true otherwise.
+  // When return false, the stream will enter a state where further
+  // ReadBits/ReadFlag/SkipBits operations
+  // will always return false unless |num_bits| is 0.
+  bool SkipBits(int num_bits);
+
+  // Returns the number of bits read so far.
+  int bits_read() const;
+
+  // Returns the number of bits buffered in BitReaderCore.

[Pawel Osciak, 2014/01/11 02:30:49]

Why is this useful for clients? Isn't this an implementation detail of BRC?

[damienv1, 2014/01/13 22:41:06]

I rewrote BitReader::bits_available so that it does not use
BitReaderCore::bits_buffered anymore.

+  int bits_buffered() const;
+
+ private:
+  // Help function used by ReadBits to avoid inlining the bit reading logic.
+  bool ReadBitsInternal(int num_bits, uint64* out);
+
+  // Refill bit registers to have at least |min_nbits| bits available.
+  // Return true if the mininimum bit count condition is met after the refill.
+  bool Refill(int min_nbits);
+
+  // Refill the current bit register from the next bit register.
+  void RefillCurrentRegister();
+
+  ByteStreamProvider* const byte_stream_provider_;
+
+  // Number of bits read so far.
+  int bits_read_;
+
+  // Number of valid bits left in |reg|.

[Pawel Osciak, 2014/01/11 02:30:49]

s/reg/reg_

[damienv1, 2014/01/13 22:41:06]

Done.

+  // Note: bits are consumed from MSB to LSB.
+  int nbits_;

[Pawel Osciak, 2014/01/11 02:30:49]

Why do we need both bits_read_ and nbits_ ?

[damienv1, 2014/01/13 22:41:06]

|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.

+  uint64 reg_;
+
+  // Number of valid bits left in |reg_next_|.

[Pawel Osciak, 2014/01/11 02:30:49]

s/valid/unconsumed/

[damienv1, 2014/01/13 22:41:06]

Done.

+  // Note: bits are consumed from MSB to LSB.
+  int nbits_next_;
+  uint64 reg_next_;
+
+  DISALLOW_COPY_AND_ASSIGN(BitReaderCore);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_BIT_READER_CORE_H_