Add unittest for H264BitReader.

Add unittest for H264BitReader.

BUG=

TBR=jam@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149463

[xiaomings, 2012-07-30 20:51:24]

Added unittest to H264BitReader.

I also added a MultiStreamFileParsing into H264ParserTest, this briefly
enumerate through all .h264 files in the testing folder and try to parse them.
Let me know if I should remove it.

[Ami GONE FROM CHROMIUM, 2012-07-30 23:32:32]

https://chromiumcodereview.appspot.com/10834071/diff/2003/content/common/gpu/...
File content/common/gpu/media/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/2003/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:21: TEST(H264ParserTest,
StreamFileParsing) {
Is there any reason for this to still exist??

https://chromiumcodereview.appspot.com/10834071/diff/2003/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:133: ~H264BitReaderTest() {}
These two aren't adding value. 
In fact I don't actually see any reason to have this class at all.

[xiaomings, 2012-07-30 23:59:34]

Removed the newly added MutliFileStreamParser.
Remove the empty ctor/dtor.

https://chromiumcodereview.appspot.com/10834071/diff/2003/content/common/gpu/...
File content/common/gpu/media/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/2003/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:21: TEST(H264ParserTest,
StreamFileParsing) {
I will keep the original one and remove the one to test multiple files.
On 2012/07/30 23:32:32, Ami Fischman wrote:
> Is there any reason for this to still exist??

https://chromiumcodereview.appspot.com/10834071/diff/2003/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:133: ~H264BitReaderTest() {}
Removed.

On 2012/07/30 23:32:32, Ami Fischman wrote:
> These two aren't adding value. 
> In fact I don't actually see any reason to have this class at all.

[Ami GONE FROM CHROMIUM, 2012-07-31 17:24:14]

https://chromiumcodereview.appspot.com/10834071/diff/5004/content/common/gpu/...
File content/common/gpu/media/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/5004/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:75: class H264BitReaderTest :
public ::testing::Test {
AFAICT this class's sole purpose is to forward calls to reader_.  Why not just
declare the reader_ in each TEST() and elide this class entirely?

(in case you're unfamiliar with the difference between TEST_F and TEST, the
latter doesn't require an explicit testing::Test-derived base class).

[Ami GONE FROM CHROMIUM, 2012-07-31 17:24:47]

BTW I'm not familiar w/ the vagaries of RBSP so I'm going to leave verifying
correctness of the tests to posciak@.

[xiaomings, 2012-07-31 19:24:32]

https://chromiumcodereview.appspot.com/10834071/diff/5004/content/common/gpu/...
File content/common/gpu/media/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/5004/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:75: class H264BitReaderTest :
public ::testing::Test {
The class H264BitReader is a private nested class of H264Parser. So I have to
make the test class friend of H264Parser in order to access methods of
H264BitReader. If I use TEST(), the result class name is composed.
On 2012/07/31 17:24:15, Ami Fischman wrote:
> AFAICT this class's sole purpose is to forward calls to reader_.  Why not just
> declare the reader_ in each TEST() and elide this class entirely?
> 
> (in case you're unfamiliar with the difference between TEST_F and TEST, the
> latter doesn't require an explicit testing::Test-derived base class).

[Ami GONE FROM CHROMIUM, 2012-07-31 20:30:25]

https://chromiumcodereview.appspot.com/10834071/diff/5004/content/common/gpu/...
File content/common/gpu/media/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/5004/content/common/gpu/...
content/common/gpu/media/h264_parser_unittest.cc:75: class H264BitReaderTest :
public ::testing::Test {
On 2012/07/31 19:24:32, xiaomings wrote:
> The class H264BitReader is a private nested class of H264Parser. So I have to
> make the test class friend of H264Parser in order to access methods of
> H264BitReader. If I use TEST(), the result class name is composed.
> On 2012/07/31 17:24:15, Ami Fischman wrote:
> > AFAICT this class's sole purpose is to forward calls to reader_.  Why not
just
> > declare the reader_ in each TEST() and elide this class entirely?
> > 
> > (in case you're unfamiliar with the difference between TEST_F and TEST, the
> > latter doesn't require an explicit testing::Test-derived base class).
> 

Any reason not to extract the bitreader outside of the parser?  (possibly either
in the same file or in a new file trio)

[xiaomings, 2012-07-31 22:52:32]

Moved H264BitReader into separate files.

Add jam@ for the gypi file.

[Ami GONE FROM CHROMIUM, 2012-07-31 23:22:12]

LGTM, still modulo posciak's LGTM on the tests.
You can TBR=jam@chromium.org for the gypi changes once posciak LGTMs.

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.h (right):

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:5: // This file contains an
implementation of an H264 Annex-B video stream parser.
s/stream parser/bit reader/

[Pawel Osciak, 2012-08-01 02:04:37]

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader_unittest.cc:49:
EXPECT_FALSE(reader.Initialize(rbsp, 1));
Why should this be false?

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader_unittest.cc:50:
EXPECT_FALSE(reader.Initialize(rbsp, sizeof(rbsp)));
And this?

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader_unittest.cc:121:
EXPECT_FALSE(reader.HasMoreRBSPData());
This I'm assuming is where the current parser would fail (i.e. that would be
true). Technically, the upper layer now I think strips trailing zeros when it
chops into NALUs, but this should probably be also verified in the parser. I'm
not sure how happy I'd be with that being done in HasMoreRBSPData() though. But
that's for another CL.

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader_unittest.cc:133:
EXPECT_FALSE(reader.HasMoreRBSPData());
And here.

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader_unittest.cc:147:
EXPECT_FALSE(reader.HasMoreRBSPData());
Here as well.

[Pawel Osciak, 2012-08-01 03:59:41]

lgtm after some clarification in chat. We might modify Initialize() to strip
trailing zero bytes.

[commit-bot: I haz the power, 2012-08-01 06:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiaomings@google.com/10834071/5016

[commit-bot: I haz the power, 2012-08-01 06:09:53]

Presubmit check for 10834071-5016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 1.2s to calculate.

[xiaomings, 2012-08-01 06:12:04]

TBR=jam@chromium.org

[commit-bot: I haz the power, 2012-08-01 06:12:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiaomings@google.com/10834071/5016

[commit-bot: I haz the power, 2012-08-01 06:12:23]

Presubmit check for 10834071-5016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 1.1s to calculate.

[Ami GONE FROM CHROMIUM, 2012-08-01 14:50:00]

TBR belongs in cl description, not reviewlog.

[commit-bot: I haz the power, 2012-08-01 18:08:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiaomings@google.com/10834071/5016

[commit-bot: I haz the power, 2012-08-01 19:34:10]

Change committed as 149463

[Ami GONE FROM CHROMIUM, 2012-09-01 13:32:45]

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834071/diff/5016/content/common/gpu/...
content/common/gpu/media/h264_bit_reader_unittest.cc:49:
EXPECT_FALSE(reader.Initialize(rbsp, 1));
How did this test ever pass??  AFAICT from this CL (and to 154058, at least),
Initialize doesn't strip trailing zeros of any sort.  

Were these tests committed too soon?  Should they be reverted for now?