Parse VP9 compressed header

media/filters/vp9_bool_decoder.cc

+// Copyright 2016 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.
+
+#include "media/filters/vp9_bool_decoder.h"
+
+#include "base/logging.h"
+#include "media/base/bit_reader.h"
+
+namespace media {
+
+namespace {
+
+const int kCountToShiftTo128[256] = {

[Pawel Osciak, 2016/08/04 10:20:18]

Please add a comment what this is.

[kcwu, 2016/08/05 11:38:46]

Done.

+    0, 7, 6, 6, 5, 5, 5, 5, 4, 4, 4, 4, 4, 4, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3,
+    3, 3, 3, 3, 3, 3, 3, 3, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
+    2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1, 1, 1,
+    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+    1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+};
+}  // namespace
+
+Vp9BoolDecoder::Vp9BoolDecoder() : valid_(true) {}

[Pawel Osciak, 2016/08/04 10:20:17]

We could perhaps use class member initialization in the class body syntax
instead.

Should we also initialize other members?

[kcwu, 2016/08/05 11:38:46]

Done.

+
+Vp9BoolDecoder::~Vp9BoolDecoder() {}
+
+// 9.2.1 Initialization process for Boolean decoder
+bool Vp9BoolDecoder::Initialize(const uint8_t* data, size_t size) {
+  DCHECK(data);
+  reader_.reset(new BitReader(data, size));
+  valid_ = true;
+
+  if (size < 1) {

[Pawel Osciak, 2016/08/04 10:20:17]

Perhaps we should check this before BitReader initialization?

[kcwu, 2016/08/05 11:38:46]

Done.

+    DVLOG(1) << "input size of bool decoder shall be at least 1";
+    valid_ = false;
+    return false;
+  }
+
+  bool_value_ = 0;
+  count_to_fill_ = 8;
+  bool_range_ = 255;
+  Fill();

[Pawel Osciak, 2016/08/04 10:20:17]

I think ReadLiteral() will Fill() by itself, so perhaps no need to have this
here?

[kcwu, 2016/08/05 11:38:46]

Done.

+  if (ReadLiteral(1) != 0) {
+    DVLOG(1) << "marker bit should be 0";
+    valid_ = false;
+    return false;
+  }
+  return true;
+}
+
+// Fill at least |count_to_fill_| bits and prefill remain bits of |bool_value_|
+// if data is enough.
+void Vp9BoolDecoder::Fill() {
+  DCHECK_GE(count_to_fill_, 0);
+
+  int bits_left = reader_->bits_available();

[Pawel Osciak, 2016/08/04 10:20:18]

Should all of these be size_t ?

[kcwu, 2016/08/05 11:38:46]

Done.

+  if (bits_left < count_to_fill_) {
+    valid_ = false;
+    DVLOG(1) << "Vp9BoolDecoder reads beyond the end of stream";
+    return;
+  }
+
+  DCHECK_LE(count_to_fill_, kBoolSize);
+  int max_bits_to_read = kBigBoolSize - kBoolSize + count_to_fill_;
+  int bits_to_read = std::min(max_bits_to_read, bits_left);
+
+  BigBool data;
+  reader_->ReadBits(bits_to_read, &data);
+  bool_value_ |= data << (max_bits_to_read - bits_to_read);
+  count_to_fill_ -= bits_to_read;
+}
+
+// 9.2.2 Boolean decoding process
+bool Vp9BoolDecoder::ReadBool(int prob) {
+  DCHECK(reader_);
+  if (!valid_)

[Pawel Osciak, 2016/08/04 10:20:18]

Should we be checking for valid_ here and in ReadLiteral and returning false, if
the callers of ReadBool and ReadLiteral do not distinguish between an error and
0? Perhaps rather the caller of Read*() client should be checking this?

[kcwu, 2016/08/05 11:38:46]

This is intended behavior. So the caller doesn't need to check error everywhere.

+    return false;
+
+  if (count_to_fill_ > 0) {
+    Fill();
+    if (!valid_)

[Pawel Osciak, 2016/08/04 10:20:18]

Perhaps Fill() should just return a bool?

[kcwu, 2016/08/05 11:38:46]

Done.

+      return false;
+  }
+
+  unsigned int split = (bool_range_ * prob + (256 - prob)) >> kBoolSize;

[Pawel Osciak, 2016/08/04 10:20:17]

Would it be safer to use a fixed-length type?

[kcwu, 2016/08/05 11:38:46]

|bool_range| is less than 256, thus |split| is less than 256 as well.
|int| should be enough.

+  BigBool big_split = static_cast<BigBool>(split) << (kBigBoolSize - kBoolSize);
+
+  bool bit;
+  if (bool_value_ < big_split) {
+    bool_range_ = split;
+    bit = false;
+  } else {
+    bool_range_ -= split;
+    bool_value_ -= big_split;
+    bit = true;
+  }
+
+  // Need to fill |count| bits next time in order to make |bool_range_| >=
+  // 128.
+  DCHECK_LT(bool_range_, arraysize(kCountToShiftTo128));
+  DCHECK_GT(bool_range_, 0u);
+  int count = kCountToShiftTo128[bool_range_];
+  bool_range_ <<= count;
+  bool_value_ <<= count;
+  count_to_fill_ += count;
+
+  return bit;
+}
+
+// 9.2.4 Parsing process for read_literal
+int Vp9BoolDecoder::ReadLiteral(int bits) {
+  DCHECK(reader_);
+  if (!valid_)
+    return 0;
+
+  int x = 0;

[Pawel Osciak, 2016/08/04 10:20:17]

Should this be a fixed-size type and also unsigned?

[kcwu, 2016/08/05 11:38:46]

I'm not sure what size it should be. Just use arbitrary uint8_t.
(current max(bits)=7)
 

+  for (int i = 0; i < bits; i++)

[Pawel Osciak, 2016/08/04 10:20:17]

s/int/size_t/

[kcwu, 2016/08/05 11:38:47]

Acknowledged.

+    x = 2 * x + ReadBool(128);
+
+  return x;
+}
+
+bool Vp9BoolDecoder::ConsumePaddingBits() {
+  DCHECK(reader_);
+  int data;
+
+  if (count_to_fill_ > reader_->bits_available()) {
+    // 9.2.2 Boolean decoding process
+    // Although we actually don't used the value, spec says the bitstream
+    // should have enough bits to fill bool range, this should never happend

[Pawel Osciak, 2016/08/04 10:20:17]

s/happend/happen./ ?

[kcwu, 2016/08/05 11:38:46]

Done.

+    DVLOG(2) << "not enough bits in bitstream to fill bool range";
+    return false;
+  }
+
+  if (bool_value_ != 0) {
+    DVLOG(1) << "prefilled padding bits are not zero";
+    return false;
+  }
+  while (reader_->bits_available() > 0) {
+    int size_to_read =
+        std::min(reader_->bits_available(), static_cast<int>(sizeof(data) * 8));
+    reader_->ReadBits(size_to_read, &data);
+    if (data != 0) {
+      DVLOG(1) << "padding bits are not zero";
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace media