Run ac3, eac3 and hevc unit tests in all configs

media/filters/ffmpeg_demuxer_unittest.cc

Index: media/filters/ffmpeg_demuxer_unittest.cc
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index e22e4545916e8f4cd413beedf0308150ac6ead0f..25acaa32e81a249c71e85417d593570078f320d8 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -98,21 +98,16 @@ class FFmpegDemuxerTest : public testing::Test {
 
   MOCK_METHOD1(CheckPoint, void(int v));
 
-  void InitializeDemuxerWithTimelineOffset(bool enable_text,
-                                           base::Time timeline_offset) {
-    EXPECT_CALL(host_, SetDuration(_));
+  void InitializeDemuxer(
+      bool enable_text = false,

[ddorwin, 2016/02/19 20:51:10]

bools are always a problem because there is no context for what it means at the
call site. I see you have commented on the meaning in some cases, which is
helpful.

[servolk, 2016/02/19 21:32:01]

Acknowledged.

+      media::PipelineStatus demuxer_init_expected_status = PIPELINE_OK,

[ddorwin, 2016/02/19 20:51:10]

Looking at the uses and considering the bool issue above and potential issues
with default parameters, it seems that separate functions would be more
readable, potentially with less text. Specific suggestions below.

Then this would be a private function without default parameters that each of
those call.

[servolk, 2016/02/19 21:32:01]

Done.

+      base::Time timeline_offset = base::Time()) {
+    if (demuxer_init_expected_status == PIPELINE_OK)
+      EXPECT_CALL(host_, SetDuration(_));
     WaitableMessageLoopEvent event;
     demuxer_->Initialize(&host_, event.GetPipelineStatusCB(), enable_text);
     demuxer_->timeline_offset_ = timeline_offset;
-    event.RunAndWaitForStatus(PIPELINE_OK);
-  }
-
-  void InitializeDemuxerText(bool enable_text) {
-    InitializeDemuxerWithTimelineOffset(enable_text, base::Time());
-  }
-
-  void InitializeDemuxer() {
-    InitializeDemuxerText(false);
+    event.RunAndWaitForStatus(demuxer_init_expected_status);
   }
 
   MOCK_METHOD2(OnReadDoneCalled, void(int, int64_t));
@@ -357,7 +352,7 @@ TEST_F(FFmpegDemuxerTest, Initialize_MultitrackText) {
   DemuxerStream* text_stream = NULL;
   EXPECT_CALL(host_, AddTextStream(_, _))
       .WillOnce(SaveArg<0>(&text_stream));
-  InitializeDemuxerText(true);
+  InitializeDemuxer(/*enable_text=*/true);

[ddorwin, 2016/02/19 20:51:09]

InitializeDemuxerWithText();

[servolk, 2016/02/19 21:32:01]

Done.

   ASSERT_TRUE(text_stream);
   EXPECT_EQ(DemuxerStream::TEXT, text_stream->type());
 
@@ -430,7 +425,7 @@ TEST_F(FFmpegDemuxerTest, Read_Text) {
   DemuxerStream* text_stream = NULL;
   EXPECT_CALL(host_, AddTextStream(_, _))
       .WillOnce(SaveArg<0>(&text_stream));
-  InitializeDemuxerText(true);
+  InitializeDemuxer(/*enable_text=*/true);
   ASSERT_TRUE(text_stream);
   EXPECT_EQ(DemuxerStream::TEXT, text_stream->type());
 
@@ -452,8 +447,8 @@ TEST_F(FFmpegDemuxerTest, Read_VideoPositiveStartTime) {
 
   // Test the start time is the first timestamp of the video and audio stream.
   CreateDemuxer("nonzero-start-time.webm");
-  InitializeDemuxerWithTimelineOffset(
-      false, base::Time::FromJsTime(kTimelineOffsetMs));
+  InitializeDemuxer(false, PIPELINE_OK,

[ddorwin, 2016/02/19 20:51:10]

InitializeDemuxer(base::Time::FromJsTime(kTimelineOffsetMs));

InitializeDemuxer always expects success and never enables text.

[servolk, 2016/02/19 21:32:01]

Given that the timeline_offset parameter is used/needed by only one test case,
I'd rather have a separate InitializeDemuxerWithTimelineOffset overload, to keep
all the other places simple with parameterless invocations of InitDemuxer. WDYT?

[ddorwin, 2016/02/24 00:28:00]

Acknowledged.

+                    base::Time::FromJsTime(kTimelineOffsetMs));
 
   // Attempt a read from the video stream and run the message loop until done.
   DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
@@ -689,7 +684,7 @@ TEST_F(FFmpegDemuxerTest, Read_EndOfStreamText) {
   DemuxerStream* text_stream = NULL;
   EXPECT_CALL(host_, AddTextStream(_, _))
       .WillOnce(SaveArg<0>(&text_stream));
-  InitializeDemuxerText(true);
+  InitializeDemuxer(/*enable_text=*/true);
   ASSERT_TRUE(text_stream);
   EXPECT_EQ(DemuxerStream::TEXT, text_stream->type());
 
@@ -787,7 +782,7 @@ TEST_F(FFmpegDemuxerTest, SeekText) {
   DemuxerStream* text_stream = NULL;
   EXPECT_CALL(host_, AddTextStream(_, _))
       .WillOnce(SaveArg<0>(&text_stream));
-  InitializeDemuxerText(true);
+  InitializeDemuxer(/*enable_text=*/true);
   ASSERT_TRUE(text_stream);
   EXPECT_EQ(DemuxerStream::TEXT, text_stream->type());
 
@@ -1115,11 +1110,9 @@ TEST_F(FFmpegDemuxerTest, NaturalSizeWithPASP) {
   EXPECT_EQ(gfx::Size(639, 360), video_config.natural_size());
 }
 
-#endif
-
-#if BUILDFLAG(ENABLE_HEVC_DEMUXING)
 TEST_F(FFmpegDemuxerTest, HEVC_in_MP4_container) {
   CreateDemuxer("bear-hevc-frag.mp4");
+#if BUILDFLAG(ENABLE_HEVC_DEMUXING)
   InitializeDemuxer();
 
   DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
@@ -1130,12 +1123,14 @@ TEST_F(FFmpegDemuxerTest, HEVC_in_MP4_container) {
 
   video->Read(NewReadCB(FROM_HERE, 1042, 200200, false));
   message_loop_.Run();
-}
+#else
+  InitializeDemuxer(false, DEMUXER_ERROR_NO_SUPPORTED_STREAMS);

[ddorwin, 2016/02/19 20:51:09]

InitializeDemuxerAndExpectUnsupportedFormat()
OR InitializeDemuxerAndExpectError(DEMUXER_ERROR_NO_SUPPORTED_STREAMS)

[servolk, 2016/02/19 21:32:01]

Done.

 #endif
+}
 
-#if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
 TEST_F(FFmpegDemuxerTest, Read_AC3_Audio) {
   CreateDemuxer("bear-ac3-only-frag.mp4");
+#if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
   InitializeDemuxer();
 
   // Attempt a read from the audio stream and run the message loop until done.
@@ -1147,10 +1142,14 @@ TEST_F(FFmpegDemuxerTest, Read_AC3_Audio) {
 
   audio->Read(NewReadCB(FROM_HERE, 836, 34830, true));
   message_loop_.Run();
+#else
+  InitializeDemuxer(false, DEMUXER_ERROR_NO_SUPPORTED_STREAMS);
+#endif
 }
 
 TEST_F(FFmpegDemuxerTest, Read_EAC3_Audio) {
   CreateDemuxer("bear-eac3-only-frag.mp4");
+#if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
   InitializeDemuxer();
 
   // Attempt a read from the audio stream and run the message loop until done.
@@ -1162,7 +1161,11 @@ TEST_F(FFmpegDemuxerTest, Read_EAC3_Audio) {
 
   audio->Read(NewReadCB(FROM_HERE, 872, 34830, true));
   message_loop_.Run();
+#else
+  InitializeDemuxer(false, DEMUXER_ERROR_NO_SUPPORTED_STREAMS);
+#endif
 }
-#endif  // ENABLE_AC3_EAC3_AUDIO_DEMUXING
+
+#endif

[ddorwin, 2016/02/19 20:51:09]

// defined(USE_PROPRIETARY_CODECS)

This is a long block, so we should include it in this case. I know it wasn't
there before. :)

[ddorwin, 2016/02/19 20:51:10]

Is there a reason not to run these tests in all cases? Do they fail differently?
(Ideally, we'd do the same thing as this CL to the tests before 1112.)

[servolk, 2016/02/19 21:32:01]

Done.

[servolk, 2016/02/19 21:32:01]

Well, some earlier patchsets of this CL (e.g. PS #5) placed these tests outside
of the USE_PROPRIETARY_CODECS section. This failed on some trybots, e.g.
linux_chromium_chromeos_asan_rel_ng
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
(other trybot failure results got deleted already, but they all looked like this
one). From the failure logs it looks like proprietary media files from
media/test/data were not present (since we got PipelineStatus == 12 ==
DEMUXER_ERROR_COULD_NOT_OPEN, instead of expected 14 ==
DEMUXER_ERROR_NO_SUPPORTED_STREAMS). So if we want to enable these tests on all
trybots, first we'll need to ensure that test files with proprietary codecs are
present on all trybots, even on those that build with
USE_PROPRIETARY_CODECS=false. I'm not sure if we want to do that.

[ddorwin, 2016/02/24 00:28:01]

Acknowledged.

 
 }  // namespace media