Add VP9 raw bits reader

Add VP9 raw bits reader

TEST=media_unittests
BUG=509500
Committed: https://crrev.com/0cc8eab5c0edb6438345dd1cb8483373923c3951
Cr-Commit-Position: refs/heads/master@{#342607}

[kcwu, 2015-07-29 13:18:23]

kcwu@chromium.org changed reviewers:
+ owenlin@chromium.org, posciak@chromium.org, wuchengli@chromium.org

[kcwu, 2015-07-29 13:18:59]

This CL depends on https://codereview.chromium.org/1269473002/ IVF parser.

[kcwu, 2015-07-29 13:20:17]

sorry, please ignore my comment #3, which is not for this CL.

[kcwu, 2015-07-30 04:33:49]

Please hold on the review of this CL. I will keep the interface and rewrite it
using media::BitReader.

[kcwu, 2015-07-30 12:42:32]

PTAL

[Owen Lin, 2015-07-31 04:25:32]

A general question, do we need this class, can't we just use BitReader? 
May be with a helper function to read signed literal.

https://codereview.chromium.org/1258083003/diff/40001/media/filters/vp9_raw_b...
File media/filters/vp9_raw_bits_reader.h (right):

https://codereview.chromium.org/1258083003/diff/40001/media/filters/vp9_raw_b...
media/filters/vp9_raw_bits_reader.h:32: // Returns how many bytes read.
Can you explain what returns when only part of a byte is read?

[kcwu, 2015-07-31 09:38:59]

On 2015/07/31 04:25:32, Owen Lin wrote:
> A general question, do we need this class, can't we just use BitReader? 
> May be with a helper function to read signed literal.
Since Vp9Parser is already big and the role of Vp9RawBitsReader is clear, I
still prefer a separate class.

[kcwu, 2015-07-31 09:39:54]

https://codereview.chromium.org/1258083003/diff/40001/media/filters/vp9_raw_b...
File media/filters/vp9_raw_bits_reader.h (right):

https://codereview.chromium.org/1258083003/diff/40001/media/filters/vp9_raw_b...
media/filters/vp9_raw_bits_reader.h:32: // Returns how many bytes read.
On 2015/07/31 04:25:32, Owen Lin wrote:
> Can you explain what returns when only part of a byte is read?

Done.

[Owen Lin, 2015-07-31 13:46:12]

lgtm

[Owen Lin, 2015-07-31 13:46:14]

lgtm

lgtm

[Owen Lin, 2015-08-01 11:35:37]

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
File media/filters/vp9_raw_bits_reader.cc (right):

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
media/filters/vp9_raw_bits_reader.cc:34: out_of_buffer_ =
!reader_->ReadBits(bits, &value);
Use out_of_buffer |= !reader_->ReadBits ...
It looks more robust. Same for other places.

[Pawel Osciak, 2015-08-02 10:46:11]

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/BUILD.gn
File media/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/BUILD.gn#n...
media/BUILD.gn:223: "filters/vp9_raw_bits_reader.cc",
Perhaps we should call it vp9_bit_reader to match h264_bit_reader?

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/filters/vp...
File media/filters/vp9_raw_bits_reader.cc (right):

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/filters/vp...
media/filters/vp9_raw_bits_reader.cc:34: out_of_buffer_ =
!reader_->ReadBits(bits, &value);
On 2015/08/01 11:35:37, Owen Lin wrote:
> Use out_of_buffer |= !reader_->ReadBits ...
> It looks more robust. Same for other places.

Good idea, and you could also instead of out_of_buffer have the opposite, e.g.
valid_ member. Then you wouldn't have to negate each read:

class Vp9BitReader {
  bool valid_;
};

and then every time:

valid_ = valid_ && reader_->ReadBits(...);

And I'd replace OutOfBuffer() with IsValid() { return valid_; }

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/filters/vp...
File media/filters/vp9_raw_bits_reader.h (right):

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/filters/vp...
media/filters/vp9_raw_bits_reader.h:32: // Returns how many bytes read. Partial
bytes will be counted as one byte.
s/how many bytes read/how many bytes were read since the last Initialize() call/

https://chromiumcodereview.appspot.com/1258083003/diff/80001/media/filters/vp...
media/filters/vp9_raw_bits_reader.h:33: // For example, it will return 1 if 3
bits read.
s/read/were read/

[Pawel Osciak, 2015-08-02 23:58:42]

posciak@chromium.org changed reviewers:
+ vmr@chromium.org

[kcwu, 2015-08-03 07:10:14]

https://codereview.chromium.org/1258083003/diff/80001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1258083003/diff/80001/media/BUILD.gn#newcode223
media/BUILD.gn:223: "filters/vp9_raw_bits_reader.cc",
On 2015/08/02 10:46:11, Pawel Osciak wrote:
> Perhaps we should call it vp9_bit_reader to match h264_bit_reader?

I'd prefer longer name to prevent confusing with VP9's entropy bit reader.

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
File media/filters/vp9_raw_bits_reader.cc (right):

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
media/filters/vp9_raw_bits_reader.cc:34: out_of_buffer_ =
!reader_->ReadBits(bits, &value);
On 2015/08/02 10:46:11, Pawel Osciak wrote:
> On 2015/08/01 11:35:37, Owen Lin wrote:
> > Use out_of_buffer |= !reader_->ReadBits ...
> > It looks more robust. Same for other places.
> 
> Good idea, and you could also instead of out_of_buffer have the opposite, e.g.
> valid_ member. Then you wouldn't have to negate each read:
> 
> class Vp9BitReader {
>   bool valid_;
> };
> 
> and then every time:
> 
> valid_ = valid_ && reader_->ReadBits(...);
> 
> And I'd replace OutOfBuffer() with IsValid() { return valid_; }

Done.

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
File media/filters/vp9_raw_bits_reader.h (right):

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
media/filters/vp9_raw_bits_reader.h:32: // Returns how many bytes read. Partial
bytes will be counted as one byte.
On 2015/08/02 10:46:11, Pawel Osciak wrote:
> s/how many bytes read/how many bytes were read since the last Initialize()
call/

Done.

https://codereview.chromium.org/1258083003/diff/80001/media/filters/vp9_raw_b...
media/filters/vp9_raw_bits_reader.h:33: // For example, it will return 1 if 3
bits read.
On 2015/08/02 10:46:11, Pawel Osciak wrote:
> s/read/were read/

Done.

[Pawel Osciak, 2015-08-05 07:08:43]

lgtm

[Pawel Osciak, 2015-08-05 07:08:57]

oh sorry, lgtm with nits.

https://chromiumcodereview.appspot.com/1258083003/diff/100001/media/filters/v...
File media/filters/vp9_raw_bits_reader.h (right):

https://chromiumcodereview.appspot.com/1258083003/diff/100001/media/filters/v...
media/filters/vp9_raw_bits_reader.h:29: // Returns true if last reads don't go
beyond the end of buffer.
This is not only the last read, but any read since last Initialize().

Returns true if any of the reads since the last Initialize() call has gone
beyond the end of available data.

https://chromiumcodereview.appspot.com/1258083003/diff/100001/media/filters/v...
media/filters/vp9_raw_bits_reader.h:52: // Indicates error status that the
caller reads beyond the end of input
This comment needs an update.

[kcwu, 2015-08-05 07:34:44]

https://codereview.chromium.org/1258083003/diff/100001/media/filters/vp9_raw_...
File media/filters/vp9_raw_bits_reader.h (right):

https://codereview.chromium.org/1258083003/diff/100001/media/filters/vp9_raw_...
media/filters/vp9_raw_bits_reader.h:29: // Returns true if last reads don't go
beyond the end of buffer.
On 2015/08/05 07:08:57, Pawel Osciak wrote:
> This is not only the last read, but any read since last Initialize().
> 
> Returns true if any of the reads since the last Initialize() call has gone
> beyond the end of available data.

Done.
As your suggestion, just s/any/none/

https://codereview.chromium.org/1258083003/diff/100001/media/filters/vp9_raw_...
media/filters/vp9_raw_bits_reader.h:52: // Indicates error status that the
caller reads beyond the end of input
On 2015/08/05 07:08:57, Pawel Osciak wrote:
> This comment needs an update.

Done.

[Pawel Osciak, 2015-08-05 07:37:15]

On 2015/08/05 07:34:44, kcwu wrote:
>
https://codereview.chromium.org/1258083003/diff/100001/media/filters/vp9_raw_...
> File media/filters/vp9_raw_bits_reader.h (right):
> 
>
https://codereview.chromium.org/1258083003/diff/100001/media/filters/vp9_raw_...
> media/filters/vp9_raw_bits_reader.h:29: // Returns true if last reads don't go
> beyond the end of buffer.
> On 2015/08/05 07:08:57, Pawel Osciak wrote:
> > This is not only the last read, but any read since last Initialize().
> > 
> > Returns true if any of the reads since the last Initialize() call has gone
> > beyond the end of available data.
> 
> Done.
> As your suggestion, just s/any/none/
> 
>
https://codereview.chromium.org/1258083003/diff/100001/media/filters/vp9_raw_...
> media/filters/vp9_raw_bits_reader.h:52: // Indicates error status that the
> caller reads beyond the end of input
> On 2015/08/05 07:08:57, Pawel Osciak wrote:
> > This comment needs an update.
> 
> Done.

lgtm

[kcwu, 2015-08-05 07:40:50]

@xhwang, owner review, please.
Thanks

[kcwu, 2015-08-05 07:41:11]

kcwu@chromium.org changed reviewers:
+ xhwang@chromium.org

[kcwu, 2015-08-05 07:41:12]

@xhwang, owner review, please.
Thanks

[xhwang, 2015-08-07 21:03:08]

lgtm with nits

https://codereview.chromium.org/1258083003/diff/120001/media/filters/vp9_raw_...
File media/filters/vp9_raw_bits_reader.h (right):

https://codereview.chromium.org/1258083003/diff/120001/media/filters/vp9_raw_...
media/filters/vp9_raw_bits_reader.h:44: int ReadLiteral(int bits);
I guess bits must be <=32 ? How about the sign bit? Could the result be
negative?

[kcwu, 2015-08-10 10:54:06]

https://codereview.chromium.org/1258083003/diff/120001/media/filters/vp9_raw_...
File media/filters/vp9_raw_bits_reader.h (right):

https://codereview.chromium.org/1258083003/diff/120001/media/filters/vp9_raw_...
media/filters/vp9_raw_bits_reader.h:44: int ReadLiteral(int bits);
On 2015/08/07 21:03:07, xhwang wrote:
> I guess bits must be <=32 ? How about the sign bit? Could the result be
> negative?

The max number in spec is 16. I added DCHECK to ensure it's range.

[kcwu, 2015-08-10 10:54:11]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2015-08-10 10:54:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
owenlin@chromium.org, xhwang@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1258083003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-10 10:54:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1258083003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1258083003/160001

[commit-bot: I haz the power, 2015-08-10 12:00:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-10 12:01:02]

Patchset 9 (id:??) landed as
https://crrev.com/0cc8eab5c0edb6438345dd1cb8483373923c3951
Cr-Commit-Position: refs/heads/master@{#342607}