Add HE AAC support to ISO BMFF.

media/base/bit_reader.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// 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_H_
+#define MEDIA_BASE_BIT_READER_H_
+
+#include <sys/types.h>
+#include <algorithm>
+#include <climits>
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "media/base/media_export.h"
+
+
+namespace media {
+
+// A class to read bit streams.
+class MEDIA_EXPORT BitReader {
+ public:
+  // Initialize the reader to start reading at |data|, |size| being size
+  // of |data| in bytes.
+  BitReader(const uint8* data, off_t size);
+  ~BitReader();
+
+  // Read |num_bits| next bits from stream and return in |*out|, first bit
+  // from the stream starting at |num_bits| position in |*out|.
+  // |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.
+  template<typename T>
+  bool ReadBits(int num_bits, T *out) {

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

This inlines the entire method definition at every call-site, which slows down
compilation, linking, and runtime.

Instead you could have:
template<typename T>
bool ReadBits(int num_bits, T *out) {
  DCHECK(num_bits <= static_cast<int>(sizeof(T) * 8));
  uint64 val;
  ReadBitsInternal(num_bits, &val);
  *out = val;
}
where ReadBitsInternal is *not* a templated function, and defined in the .cc
file.

Or, you could overload and have 4 explicit members for uint8/16/32/64.

Or, best of all IMO, just hard-code that T==uint64 and let the call-sites deal
with it.

[xiaomings, 2012/07/19 00:16:32]

Use ReadInternal to avoid duplicate code.

+    DCHECK(num_bits <= static_cast<int>(sizeof(T) * 8));

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

Replace <= with _LE

[xiaomings, 2012/07/19 00:16:32]

Done.

+
+    *out = 0;
+
+    while (num_remaining_bits_in_curr_byte_ != 0 && num_bits != 0) {
+      int bits_to_take = std::min(num_remaining_bits_in_curr_byte_, num_bits);
+      *out = (*out << bits_to_take) +

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

This can left-shift by the width of *out, which is undefined behavior in C++.

[xiaomings, 2012/07/19 00:16:32]

Done.

+          (curr_byte_ >> (num_remaining_bits_in_curr_byte_ - bits_to_take));
+      num_bits -= bits_to_take;
+      num_remaining_bits_in_curr_byte_ -= bits_to_take;
+      curr_byte_ &= (1 << num_remaining_bits_in_curr_byte_) - 1;
+
+      if (num_remaining_bits_in_curr_byte_ == 0)
+        UpdateCurrByte();
+    }
+
+    if (num_bits == 0)
+      return true;
+
+    *out = 0;
+    num_remaining_bits_in_curr_byte_ = 0;

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

how can we get here and this isn't already the case?

[xiaomings, 2012/07/19 00:16:32]

Done.

+    bytes_left_ = 0;

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

ditto

[xiaomings, 2012/07/19 00:16:32]

Done.

+
+    return false;
+  }
+
+  // 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/SkipBits operations will always return false unless
+  // |num_bits| is 0.
+  bool SkipBits(int num_bits);

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

I question the value of this API.  Was is this more useful than using a
throw-away temporary at the callsite?

(I don't like it b/c it has such an interesting interface comment)

[xiaomings, 2012/07/19 00:16:32]

Done.

+
+  // Return the number of bits left in the stream.
+  off_t NumBitsLeft() const;

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

Like SkipBits, I suspect this not the API you want.
This is only better than ReadBits() if you have client logic that needs to
branch between two different subsequent ReadBits() calls depending on the answer
to this question.  But I don't think you have such client logic.

[xiaomings, 2012/07/19 00:16:32]

Done.

+
+ protected:

[Ami GONE FROM CHROMIUM, 2012/07/17 23:22:18]

Why protected?
(esp. given no virtuals in this class, and esp. no virtual dtor)

[xiaomings, 2012/07/19 00:16:32]

Done.

+  // Advance to the next byte, loading it into curr_byte_.
+  // If the num_remaining_bits_in_curr_byte_ is 0 after this function returns,
+  // the stream has reached the end.
+  void UpdateCurrByte();
+
+  // Pointer to the next unread (not in curr_byte_) byte in the stream.
+  const uint8* data_;
+
+  // Bytes left in the stream (without the curr_byte_).
+  off_t bytes_left_;
+
+  // Contents of the current byte; first unread bit starting at position
+  // 8 - num_remaining_bits_in_curr_byte_ from MSB.
+  uint8 curr_byte_;
+
+  // Number of bits remaining in curr_byte_
+  int num_remaining_bits_in_curr_byte_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BitReader);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_BIT_READER_H_