Enable PipelineIntegrationTests in mojo!

Enable PipelineIntegrationTests in mojo!

This hooks up our pipeline integration tests into mojo and removes
the existing primitive pipeline test.

There's a bit of nastiness with mojo not setting up the media library
or command line, probably something we'll want in its own module later.

The following tests had to be disabled because the actual rendering
happens in a separate process and required information is unavailable:
- Any tests which perform hash checks.
- Any tests which exercise EME.
- Any tests which look at the video frame formats.

There also seems to be a couple intermittent crashes which need to be
debugged in follow up investigations:
- Sometimes all tests complete successfully but a "Segmentation fault
(core dumped)" shows after printing the summary.  No core is actually
written and it won't happen under gdb thus far :-/
- Once the WriteDataRaw() call in MojoDemuxerStreamImpl() failed with
MOJO_RESULT_FAILED_PRECONDITION for reasons unknown.

BUG=440850
TEST=all enabled tests pass!

Committed: https://crrev.com/65590011ef779b4bdaf64c87a358f809a19c1367
Cr-Commit-Position: refs/heads/master@{#310036}

[DaleCurtis, 2014-12-13 02:30:02]

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

[DaleCurtis, 2014-12-13 02:30:02]

Here's the outcome of trying to enable the tests. There's a fair bit of
investigative work left to do this more cleanly. In any case, here's an early
peek!

[xhwang, 2014-12-15 22:22:39]

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

[xhwang, 2014-12-15 22:22:40]

Nice job! The overall approach looks pretty clean. I only have some ideas for
discussion.

How about moving pipeline_integration_test* to media/test folder? Also, I
vaguely remember scherkus@ talked about moving all pipeline* to somewhere like
meida/pipeline. Having pipeline.* in media/base seems not ideal. (+scherkus for
ideas)

Also you can have something like DISABLE_FOR_MOJO_RENDERER macro so that some
tests will be disabled when MOJO_RENDERER is defined. This will help us check
this CL in and make it clear which functionality needs more work to get it
working.

https://codereview.chromium.org/806443002/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/806443002/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:621: #endif
Neat! 

I thought about putting mojo related code in media/mojo folder. But now I am not
sure whether it's worth the effort. Here's the rough idea just FYI:
- Have PipelineIntegrationTest defined in two files:
media/filters/pipeline_integration_test_host.h and
media/mojo/services/pipeline_integration_test_host.h
- Have an ifdef in this file to include different header files.

[scherkus (not reviewing), 2014-12-15 22:37:58]

yeah we need a media/test/ folder for our test-related harnesses

also pipeline has no business being in media/base/ :(

it's fine as is for now ... perhaps around the holidays someone can fix it

[DaleCurtis, 2014-12-15 23:08:55]

Are you also onboard with moving the mojo apptest to media/tests? Otherwise we
just have the same linking issues with different folder names. If so I'll go
ahead and move those in another CL and then rebase this one.

I've started a thread on mojo-dev about the extra message loop. It seems we
might be able to remove it with some strategery. I'm still investigating.

[scherkus (not reviewing), 2014-12-15 23:09:44]

On 2014/12/15 23:08:55, DaleCurtis wrote:
> Are you also onboard with moving the mojo apptest to media/tests? Otherwise we
> just have the same linking issues with different folder names. If so I'll go
> ahead and move those in another CL and then rebase this one.
> 
> I've started a thread on mojo-dev about the extra message loop. It seems we
> might be able to remove it with some strategery. I'm still investigating.

I'm onboard with you two making a decision. You know much more about writing
mojo apps and tests than I do!

[xhwang, 2014-12-15 23:31:20]

On 2014/12/15 23:08:55, DaleCurtis wrote:
> Are you also onboard with moving the mojo apptest to media/tests? 

I think with this CL, we can drop the existing mojo app test. That test is
pretty much empty, and the pipeline integration test is providing a really good
test coverage over the mojo renderer path. WDYT?

> Otherwise we
> just have the same linking issues with different folder names. If so I'll go
> ahead and move those in another CL and then rebase this one.

Go ahead with the move CL!
 
> I've started a thread on mojo-dev about the extra message loop. It seems we
> might be able to remove it with some strategery. I'm still investigating.

I saw it. Nice discussion!

[DaleCurtis, 2014-12-16 01:10:49]

Cleaned up and reworked to avoid these issues:
- mojo seems to generate its own MessageLoop which we can't nest
  the default test loop under, so there's hairiness around trying
  to use that loop.
- GN makes it weird to link these files in from another target,
  really I need to convert the pipeline stuff to a source_set or
  whatever the current hotness is.
- Likewise including the filters/ dir constitutes a DEPS violation
  since we don't have a DEPS entry for the mojo code in filters/

I'm still not sure why the commandline and media library aren't being
initialized, so I'm looking into a clean way of resolving that.

[DaleCurtis, 2014-12-16 01:11:08]

Requires https://codereview.chromium.org/792463007/ to land.

[DaleCurtis, 2014-12-17 01:01:14]

Patchset #3 (id:40001) has been deleted

[DaleCurtis, 2014-12-17 01:09:35]

Okay this is ready for review.  There are a couple things that need to be
investigated further, but overall this is in good enough shape to land before
the holidays.

Especially since nothing is actually running this for continuous integration.

[DaleCurtis, 2014-12-17 01:10:11]

dalecurtis@chromium.org changed reviewers:
+ jamesr@chromium.org

[DaleCurtis, 2014-12-17 01:10:11]

+jamesr for the DEPS change to include mojo/public in media/test.

[jamesr, 2014-12-17 01:20:01]

DEPS lgtm

[xhwang, 2014-12-17 05:25:03]

Great! Nice to see most tests are actually passing (well, after your previous
fixes) :)

Just have a few comments.

https://codereview.chromium.org/806443002/diff/60001/media/mojo/services/BUIL...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/806443002/diff/60001/media/mojo/services/BUIL...
media/mojo/services/BUILD.gn:114: #   out/Debug/mojo_shell mojo://media_test
OOC, is mojo:media_test not working, or is mojo://* recommended?

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:33: #define DISABLE_HASH_CHECKS 1
How about having a EXPECT_HASH or CHECK_HASH macro? By default, it's the same as
EXPECT_EQ; for mojo, it's a noop. Then you don't need to ifdef every hash check.

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:34: #define DISABLE_FORMAT_CHECKS 1
ditto

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:42: #define DISABLE_TEXT_TRACK_TESTS 1
ditto

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:589: virtual bool
ShouldCreateDefaultRunLoop() override { return false; }
s/virtual//

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:591: virtual void SetUp() override {
s/virtual//

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
File media/test/pipeline_integration_test_base.h (right):

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test_base.h:133: virtual scoped_ptr<Renderer>
CreateRenderer();
FYI, It'll be nice to replace this with RendererFactory. Currently there are a
lot of duplication between this function and DefaultRendererFactory.

[DaleCurtis, 2014-12-17 21:40:50]

Still waiting for a mojo roll before this can hit the try bots unfortunately.

https://codereview.chromium.org/806443002/diff/60001/media/mojo/services/BUIL...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/806443002/diff/60001/media/mojo/services/BUIL...
media/mojo/services/BUILD.gn:114: #   out/Debug/mojo_shell mojo://media_test
On 2014/12/17 05:25:03, xhwang wrote:
> OOC, is mojo:media_test not working, or is mojo://* recommended?

It works. I thought a thread encouraged the :// behavior, but it seems that's
less clear now:

https://groups.google.com/a/chromium.org/forum/#!searchin/mojo-dev/PSA/mojo-d...

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:33: #define DISABLE_HASH_CHECKS 1
On 2014/12/17 05:25:03, xhwang wrote:
> How about having a EXPECT_HASH or CHECK_HASH macro? By default, it's the same
as
> EXPECT_EQ; for mojo, it's a noop. Then you don't need to ifdef every hash
check.

This makes sense.

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:34: #define DISABLE_FORMAT_CHECKS 1
On 2014/12/17 05:25:03, xhwang wrote:
> ditto

As does this.

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:42: #define DISABLE_TEXT_TRACK_TESTS 1
On 2014/12/17 05:25:03, xhwang wrote:
> ditto

This doesn't make sense though, since it's an entire test that needs to be
disabled.

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:589: virtual bool
ShouldCreateDefaultRunLoop() override { return false; }
On 2014/12/17 05:25:03, xhwang wrote:
> s/virtual//

Done.

https://codereview.chromium.org/806443002/diff/60001/media/test/pipeline_inte...
media/test/pipeline_integration_test.cc:591: virtual void SetUp() override {
On 2014/12/17 05:25:03, xhwang wrote:
> s/virtual//

Done.

[xhwang, 2014-12-17 21:53:39]

LGTM!

https://codereview.chromium.org/806443002/diff/60001/media/mojo/services/BUIL...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/806443002/diff/60001/media/mojo/services/BUIL...
media/mojo/services/BUILD.gn:114: #   out/Debug/mojo_shell mojo://media_test
On 2014/12/17 21:40:49, DaleCurtis wrote:
> On 2014/12/17 05:25:03, xhwang wrote:
> > OOC, is mojo:media_test not working, or is mojo://* recommended?
> 
> It works. I thought a thread encouraged the :// behavior, but it seems that's
> less clear now:
> 
>
https://groups.google.com/a/chromium.org/forum/#!searchin/mojo-dev/PSA/mojo-d...

jamesr: Do you know what's the recommended style now?

[jamesr, 2014-12-17 21:54:52]

I *think* it's mojo:foobar, not mojo://foobar

[DaleCurtis, 2014-12-17 23:10:48]

Okay, I've removed the :\\ comment. Once mojo rolls I'll CQ this.

[DaleCurtis, 2014-12-18 22:31:01]

No mojo roll yet and I'm heading OOO now, so this will have to wait until the
new year.

[DaleCurtis, 2015-01-06 00:53:13]

Patch applied cleanly locally, so bombs away.

[DaleCurtis, 2015-01-06 00:53:18]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-01-06 00:54:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/806443002/100001

[DaleCurtis, 2015-01-06 00:54:36]

The CQ bit was unchecked by dalecurtis@chromium.org

[DaleCurtis, 2015-01-06 00:56:55]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-01-06 00:58:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/806443002/120001

[commit-bot: I haz the power, 2015-01-06 01:06:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-06 01:06:28]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2015-01-06 01:11:33]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-01-06 01:12:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/806443002/140001

[commit-bot: I haz the power, 2015-01-06 02:03:24]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-01-06 02:04:16]

Patchset 7 (id:??) landed as
https://crrev.com/65590011ef779b4bdaf64c87a358f809a19c1367
Cr-Commit-Position: refs/heads/master@{#310036}