Move H264Parser and H264BitReader to media/filters.

Move H264Parser and H264BitReader to media/filters.

In preparation for wider usage in media code, move the H.264 parser and
reader classes to media/filters from GPU-accelerated media locations, and
into the media namespace. These classes have only been, up to now, used
by hardware decode accelerators.

Also, move their unittests along and make them a part of media_unittests,
demoting H264ParserUnittest from being a standalone executable. While it
was designed to be run on a set of streams from commandline, it should make
more sense to run it more regularly (if only on just one stream), together
with media tests, to prevent basic regressions, if it's to be used more
widely than only in CrOS hardware acceleration classes.

BUG=None
TEST=Build VAVDA and vdaunittest for x86,
build EVDA, vdaunittest and veaunittest for ARM.
Also build and run media_unittests for both.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244132

[Ami GONE FROM CHROMIUM, 2013-12-19 19:01:19]

LGTM assuming the new files in media/ are straight-up copies.

[Pawel Osciak, 2013-12-20 00:32:01]

On 2013/12/19 19:01:19, Ami Fischman (OOO Dec23-Jan5) wrote:
> LGTM assuming the new files in media/ are straight-up copies.

Yes, apart from:
- namespace content -> namespace media, CONTENT_EXPORT -> MEDIA_EXPORT and a few
git cl format fixes.
- parser unittest, as mentioned in the cl message, is now a part of
media_unittest and is not a standalone executable anymore

PS#2 is reverting a few git cl format fixes that I didn't like.

[Pawel Osciak, 2013-12-20 00:36:09]

Adding random sample of OWNERS for:
content/content_common.gypi
content/content_tests.gypi

avi@, piman@, darin@: would appreciate a singoff on the above please. Thanks.

[Avi (use Gerrit), 2013-12-20 01:30:39]

All red in content? LGTM

[Pawel Osciak, 2013-12-24 07:23:00]

PS3: Use PathService::Get(base::DIR_SOURCE_ROOT...) for test stream path
acquisition.

Propagating LGTMs.

[commit-bot: I haz the power, 2013-12-24 07:23:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/posciak@chromium.org/119153002/110001

[commit-bot: I haz the power, 2013-12-24 13:45:28]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-12-25 01:01:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/posciak@chromium.org/119153002/110001

[commit-bot: I haz the power, 2013-12-25 06:50:06]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Pawel Osciak, 2013-12-25 08:47:23]

So the H264ParserTest in media_unittests now uses
content/common/gpu/testdata/test-25fps.h264, also added it to isolate.

Media OWNERS: since this CL is not time critical, I'll hold off CQing this until
one more LGTM saying we are ok with the above.

Thanks.

[Pawel Osciak, 2013-12-25 08:51:51]

+maruel: please *.isolate.

[Ami GONE FROM CHROMIUM, 2014-01-01 03:50:28]

On 2013/12/25 08:47:23, posciak wrote:
> So the H264ParserTest in media_unittests now uses
> content/common/gpu/testdata/test-25fps.h264, also added it to isolate.
> 
> Media OWNERS: since this CL is not time critical, I'll hold off CQing this
until
> one more LGTM saying we are ok with the above.
> 
> Thanks.

Why not instead move the .h264 file to media/ ?

[Pawel Osciak, 2014-01-02 06:32:23]

On 2014/01/01 03:50:28, Ami Fischman (OOO Dec23-Jan5) wrote:
> On 2013/12/25 08:47:23, posciak wrote:
> > So the H264ParserTest in media_unittests now uses
> > content/common/gpu/testdata/test-25fps.h264, also added it to isolate.
> > 
> > Media OWNERS: since this CL is not time critical, I'll hold off CQing this
> until
> > one more LGTM saying we are ok with the above.
> > 
> > Thanks.
> 
> Why not instead move the .h264 file to media/ ?

Do you prefer it in media/, with vdatest using it from that location as well?

[Ami GONE FROM CHROMIUM, 2014-01-02 06:43:07]

I prefer data dependencies to honor DEPS files ;)
On Jan 1, 2014 8:32 PM, <posciak@chromium.org> wrote:

> On 2014/01/01 03:50:28, Ami Fischman (OOO Dec23-Jan5) wrote:
>
>> On 2013/12/25 08:47:23, posciak wrote:
>> > So the H264ParserTest in media_unittests now uses
>> > content/common/gpu/testdata/test-25fps.h264, also added it to isolate.
>> >
>> > Media OWNERS: since this CL is not time critical, I'll hold off CQing
>> this
>> until
>> > one more LGTM saying we are ok with the above.
>> >
>> > Thanks.
>>
>
>  Why not instead move the .h264 file to media/ ?
>>
>
> Do you prefer it in media/, with vdatest using it from that location as
> well?
>
> https://chromiumcodereview.appspot.com/119153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[M-A Ruel, 2014-01-02 13:28:30]

On 2014/01/02 06:43:07, Ami Fischman (OOO Dec23-Jan5) wrote:
> I prefer data dependencies to honor DEPS files ;)

It's an historical reason that all test data files are mixed up together. So yes
having a media specific test directory would be nice. But I'll leave it to Ami
to enforce. The .isolate file technically lgtm.

Note that I'm still OOO up to Jan 7th.

[Pawel Osciak, 2014-01-06 05:49:06]

On 2014/01/02 06:43:07, Ami Fischman wrote:
> I prefer data dependencies to honor DEPS files ;)

Please tell me what I should do, and I'll do it :) I'm not sure which
dependencies we are talking about...

> On Jan 1, 2014 8:32 PM, <mailto:posciak@chromium.org> wrote:
> 
> > On 2014/01/01 03:50:28, Ami Fischman (OOO Dec23-Jan5) wrote:
> >
> >> On 2013/12/25 08:47:23, posciak wrote:
> >> > So the H264ParserTest in media_unittests now uses
> >> > content/common/gpu/testdata/test-25fps.h264, also added it to isolate.
> >> >
> >> > Media OWNERS: since this CL is not time critical, I'll hold off CQing
> >> this
> >> until
> >> > one more LGTM saying we are ok with the above.
> >> >
> >> > Thanks.
> >>
> >
> >  Why not instead move the .h264 file to media/ ?
> >>
> >
> > Do you prefer it in media/, with vdatest using it from that location as
> > well?
> >
> > https://chromiumcodereview.appspot.com/119153002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-01-06 06:00:36]

Data goes in media/, content test depends on it from that location.  Same
thing I proposed above and you asked whether that's what I want, in our
last two emails in this thread.
On Jan 5, 2014 9:49 PM, <posciak@chromium.org> wrote:

> On 2014/01/02 06:43:07, Ami Fischman wrote:
>
>> I prefer data dependencies to honor DEPS files ;)
>>
>
> Please tell me what I should do, and I'll do it :) I'm not sure which
> dependencies we are talking about...
>
>  On Jan 1, 2014 8:32 PM, <mailto:posciak@chromium.org> wrote:
>>
>
>  > On 2014/01/01 03:50:28, Ami Fischman (OOO Dec23-Jan5) wrote:
>> >
>> >> On 2013/12/25 08:47:23, posciak wrote:
>> >> > So the H264ParserTest in media_unittests now uses
>> >> > content/common/gpu/testdata/test-25fps.h264, also added it to
>> isolate.
>> >> >
>> >> > Media OWNERS: since this CL is not time critical, I'll hold off CQing
>> >> this
>> >> until
>> >> > one more LGTM saying we are ok with the above.
>> >> >
>> >> > Thanks.
>> >>
>> >
>> >  Why not instead move the .h264 file to media/ ?
>> >>
>> >
>> > Do you prefer it in media/, with vdatest using it from that location as
>> > well?
>> >
>> > https://chromiumcodereview.appspot.com/119153002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://chromiumcodereview.appspot.com/119153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Pawel Osciak, 2014-01-06 07:33:01]

On 2014/01/06 06:00:36, Ami Fischman wrote:
> Data goes in media/, content test depends on it from that location.  Same
> thing I proposed above and you asked whether that's what I want, in our
> last two emails in this thread.

Right. I just didn't understand what exactly you meant by DEPS dependencies.
AFAICT, no DEPS file refers to gpu/testdata...

[Ami GONE FROM CHROMIUM, 2014-01-06 16:26:34]

On Sun, Jan 5, 2014 at 11:33 PM, <posciak@chromium.org> wrote:

> On 2014/01/06 06:00:36, Ami Fischman wrote:
>
>> Data goes in media/, content test depends on it from that location.  Same
>> thing I proposed above and you asked whether that's what I want, in our
>> last two emails in this thread.
>
> Right. I just didn't understand what exactly you meant by DEPS
> dependencies.
> AFAICT, no DEPS file refers to gpu/testdata...
>

Correct.  I was making an argument by analogy: for "code" we use DEPS to
enforce that content/ must depend on media/ but that the reverse mustn't
happen; therefore I'd like to have the same direction of dependencies
(content depends on media, media never depends on content) for _data_ as
well.


> https://chromiumcodereview.appspot.com/119153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scherkus (not reviewing), 2014-01-06 19:59:39]

On 2014/01/06 16:26:34, Ami Fischman wrote:
> On Sun, Jan 5, 2014 at 11:33 PM, <mailto:posciak@chromium.org> wrote:
> 
> > On 2014/01/06 06:00:36, Ami Fischman wrote:
> >
> >> Data goes in media/, content test depends on it from that location.  Same
> >> thing I proposed above and you asked whether that's what I want, in our
> >> last two emails in this thread.
> >
> > Right. I just didn't understand what exactly you meant by DEPS
> > dependencies.
> > AFAICT, no DEPS file refers to gpu/testdata...
> >
> 
> Correct.  I was making an argument by analogy: for "code" we use DEPS to
> enforce that content/ must depend on media/ but that the reverse mustn't
> happen; therefore I'd like to have the same direction of dependencies
> (content depends on media, media never depends on content) for _data_ as
> well.

+1 -- media test code shouldn't depend on content test data

[scherkus (not reviewing), 2014-01-06 20:10:11]

(also some nits)

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
File media/filters/h264_bit_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
media/filters/h264_bit_reader_unittest.cc:8: using media::H264BitReader;
unless there's some strong reason not to, we typically put tests in the media
namespace so you shouldn't need this

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
File media/filters/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
media/filters/h264_parser_unittest.cc:14: using media::H264Parser;
ditto

[piman, 2014-01-08 04:25:48]

lgtm

[Pawel Osciak, 2014-01-08 07:30:33]

We have more test streams in content/common/gpu/testdata and I think it's not
the best idea to have the VDA h264 stream in media/ and vp8 one in content/, for
example. A lot of our tests depend on these streams. If anyone feels otherwise
(this will grow media/test/data by 2.3MB), please shout.

So in this CL I'm just *copying* test-25fps.h264 into media/test/data for now
and let's make the move a separate CL (submitted crbug.com/332315 for it).

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
File media/filters/h264_bit_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
media/filters/h264_bit_reader_unittest.cc:8: using media::H264BitReader;
On 2014/01/06 20:10:12, scherkus wrote:
> unless there's some strong reason not to, we typically put tests in the media
> namespace so you shouldn't need this

Done.

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
File media/filters/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/119153002/diff/600001/media/filters/h2...
media/filters/h264_parser_unittest.cc:14: using media::H264Parser;
On 2014/01/06 20:10:12, scherkus wrote:
> ditto

Done.

[Ami GONE FROM CHROMIUM, 2014-01-08 21:02:22]

On Tue, Jan 7, 2014 at 11:30 PM, <posciak@chromium.org> wrote:

> So in this CL I'm just *copying* test-25fps.h264 into media/test/data for
> now
> and let's make the move a separate CL (submitted crbug.com/332315 for it).
>

FTR I'm ok with this.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-10 07:27:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/posciak@chromium.org/119153002/1490003

[commit-bot: I haz the power, 2014-01-10 09:11:30]

Change committed as 244132