Run ac3, eac3 and hevc unit tests in all configs

media/filters/ffmpeg_demuxer_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <deque>
 #include <string>
@@ skipping 80 matching lines @@
     Demuxer::EncryptedMediaInitDataCB encrypted_media_init_data_cb = base::Bind(
         &FFmpegDemuxerTest::OnEncryptedMediaInitData, base::Unretained(this));
 
     demuxer_.reset(new FFmpegDemuxer(
         message_loop_.task_runner(), data_source_.get(),
         encrypted_media_init_data_cb, new MediaLog()));
   }
 
   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));
 
   struct ReadExpectation {
     ReadExpectation(size_t size,
                     int64_t timestamp_us,
                     const base::TimeDelta& discard_front_padding,
                     bool is_key_frame)
         : size(size),
@@ skipping 224 matching lines @@
 TEST_F(FFmpegDemuxerTest, Initialize_MultitrackText) {
   // Open a file containing the following streams:
   //   Stream #0: Video (VP8)
   //   Stream #1: Audio (Vorbis)
   //   Stream #2: Text (WebVTT)
 
   CreateDemuxer("bear-vp8-webvtt.webm");
   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());
 
   // Video stream should be VP8.
   DemuxerStream* stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   ASSERT_TRUE(stream);
   EXPECT_EQ(DemuxerStream::VIDEO, stream->type());
   EXPECT_EQ(kCodecVP8, stream->video_decoder_config().codec());
 
   // Audio stream should be Vorbis.
@@ skipping 52 matching lines @@
 
   EXPECT_EQ(323, demuxer_->GetMemoryUsage());
 }
 
 TEST_F(FFmpegDemuxerTest, Read_Text) {
   // We test that on a successful text packet read.
   CreateDemuxer("bear-vp8-webvtt.webm");
   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());
 
   text_stream->Read(NewReadCB(FROM_HERE, 31, 0, true));
   message_loop_.Run();
 
   text_stream->Read(NewReadCB(FROM_HERE, 19, 500000, true));
   message_loop_.Run();
 }
 
 TEST_F(FFmpegDemuxerTest, SeekInitialized_NoVideoStartTime) {
   CreateDemuxer("audio-start-time-only.webm");
   InitializeDemuxer();
   EXPECT_EQ(0, preferred_seeking_stream_index());
 }
 
 TEST_F(FFmpegDemuxerTest, Read_VideoPositiveStartTime) {
   const int64_t kTimelineOffsetMs = 1352550896000LL;
 
   // 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);
   DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
   const base::TimeDelta video_start_time =
       base::TimeDelta::FromMicroseconds(400000);
   const base::TimeDelta audio_start_time =
       base::TimeDelta::FromMicroseconds(396000);
 
@@ skipping 215 matching lines @@
   InitializeDemuxer();
   ReadUntilEndOfStream(demuxer_->GetStream(DemuxerStream::AUDIO));
 }
 
 TEST_F(FFmpegDemuxerTest, Read_EndOfStreamText) {
   // Verify that end of stream buffers are created.
   CreateDemuxer("bear-vp8-webvtt.webm");
   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());
 
   bool got_eos_buffer = false;
   const int kMaxBuffers = 10;
   for (int i = 0; !got_eos_buffer && i < kMaxBuffers; i++) {
     text_stream->Read(base::Bind(&EosOnReadDone, &got_eos_buffer));
     message_loop_.Run();
   }
 
@@ skipping 77 matching lines @@
   message_loop_.Run();
 }
 
 TEST_F(FFmpegDemuxerTest, SeekText) {
   // We're testing that the demuxer frees all queued packets when it receives
   // a Seek().
   CreateDemuxer("bear-vp8-webvtt.webm");
   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());
 
   // Get our streams.
   DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
   DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
   ASSERT_TRUE(video);
   ASSERT_TRUE(audio);
 
   // Read a text packet and release it.
@@ skipping 307 matching lines @@
   CreateDemuxer("bear-640x360-non_square_pixel-with_pasp.mp4");
   InitializeDemuxer();
 
   DemuxerStream* stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   ASSERT_TRUE(stream);
 
   const VideoDecoderConfig& video_config = stream->video_decoder_config();
   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);
   ASSERT_TRUE(video);
 
   video->Read(NewReadCB(FROM_HERE, 3569, 66733, true));
   message_loop_.Run();
 
   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
 }
-#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.
   DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
   // Read the first two frames and check that we are getting expected data
   audio->Read(NewReadCB(FROM_HERE, 834, 0, true));
   message_loop_.Run();
 
   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.
   DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
   // Read the first two frames and check that we are getting expected data
   audio->Read(NewReadCB(FROM_HERE, 870, 0, true));
   message_loop_.Run();
 
   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