Directly call ChunkDemuxer::Initialize completion callback.

media/filters/chunk_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 "media/filters/chunk_demuxer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
@@ skipping 225 matching lines @@
   EXPECT_EQ(status, DemuxerStream::kOk);
   EXPECT_TRUE(buffer->end_of_stream());
   *called = true;
 }
 
 static void OnSeekDone_OKExpected(bool* called, PipelineStatus status) {
   EXPECT_EQ(status, PIPELINE_OK);
   *called = true;
 }
 
+static void OnPipelineStatus_NOTREACHED(PipelineStatus status) {
+  NOTREACHED();
+}
+
 class ChunkDemuxerTest : public ::testing::Test {
  protected:
   enum CodecsIndex {
     AUDIO,
     VIDEO,
     MAX_CODECS_INDEX
   };
 
   // Default cluster to append first for simple tests.
   std::unique_ptr<Cluster> kDefaultFirstCluster() {
@@ skipping 523 matching lines @@
   void AppendGarbage() {
     // Fill up an array with gibberish.
     int garbage_cluster_size = 10;
     std::unique_ptr<uint8_t[]> garbage_cluster(
         new uint8_t[garbage_cluster_size]);
     for (int i = 0; i < garbage_cluster_size; ++i)
       garbage_cluster[i] = i;
     ASSERT_FALSE(AppendData(garbage_cluster.get(), garbage_cluster_size));
   }
 
-  void InitDoneCalled(PipelineStatus expected_status,
-                      PipelineStatus status) {
-    EXPECT_EQ(status, expected_status);
-  }
+  MOCK_METHOD1(DemuxerInitialized, void(PipelineStatus));

[alokp, 2016/09/16 22:10:16]

Converted InitDoneCalled into a mock method so that the sequence of calls can be
verified with testing::InSequence.

Without the changes in ChunkDemuxer, Init and ErrorWhileParsingClusterAfterInit
tests will fail.

+
+  void InitDoneCB(PipelineStatus status) { DemuxerInitialized(status); }
 
   PipelineStatusCB CreateInitDoneCB(const base::TimeDelta& expected_duration,
                                     PipelineStatus expected_status) {
     if (expected_duration != kNoTimestamp)
       EXPECT_CALL(host_, SetDuration(expected_duration));
     return CreateInitDoneCB(expected_status);
   }
 
   PipelineStatusCB CreateInitDoneCB(PipelineStatus expected_status) {
-    return base::Bind(&ChunkDemuxerTest::InitDoneCalled,
-                      base::Unretained(this),
-                      expected_status);
+    EXPECT_CALL(*this, DemuxerInitialized(expected_status));
+    return base::Bind(&ChunkDemuxerTest::InitDoneCB, base::Unretained(this));
   }
 
   enum StreamFlags {
     HAS_AUDIO = 1 << 0,
     HAS_VIDEO = 1 << 1,
     HAS_TEXT = 1 << 2,
     USE_ALTERNATE_AUDIO_TRACK_ID = 1 << 3,
     USE_ALTERNATE_VIDEO_TRACK_ID = 1 << 4,
     USE_ALTERNATE_TEXT_TRACK_ID = 1 << 5,
   };
@@ skipping 854 matching lines @@
 
   CheckExpectedBuffers(audio_stream, "23K 46K 69K");
   CheckExpectedBuffers(video_stream, "30K 90K");
   CheckExpectedBuffers(text_stream, "25K 40K 80K 90K");
 }
 
 // Make sure that the demuxer reports an error if Shutdown()
 // is called before all the initialization segments are appended.
 TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppended) {
   EXPECT_CALL(*this, DemuxerOpened());
-  demuxer_->Initialize(
-      &host_, CreateInitDoneCB(
-          kDefaultDuration(), DEMUXER_ERROR_COULD_NOT_OPEN), true);
+  demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true);

[alokp, 2016/09/16 22:10:16]

init_cb should not be called here since the demuxer is shutdown before it is
initialized (which is considered completed only after initialization segment is
fully parsed). This is verified by NOTREACHED.

[xhwang, 2016/09/16 23:41:47]

hmm, we typically don't like outstanding callbacks and require all callbacks to
be fired. So if the demuxer is destroyed before it finishes demuxer init, it
should fire the callback with an error status, rather than just hold the
callback.

[alokp, 2016/09/17 00:06:05]

This patch does not change this behavior. ChunkDemuxer never fired the init_cb
if destroyed before init completes. I had to change the completion callback here
because CreateInitDoneCB now expects InitDoneCB to be called, which ChunkDemuxer
does not. So I made it explicit. If we want to change this behavior, it should
be done in a separate patch.

[xhwang, 2016/09/17 03:58:24]

The demuxer API is a bit old and it's not very clear about whether the callback
should always be fired. I agree such changes (if any) don't belong to this CL.
But then I guess for a demuxer implementation, it's legit to either fire the
callback, or not fire the callback, when it's destructed before init is
finished. With that, I wonder what's the value of asserting that the callback
will not be called in these tests? I kinda feel we are testing implementation
detail, not the API.

Can we just use Bind(&DemuxerInitialized) here but don't expect it to be called?
If we "regress" that the callback is actually fired, we'll get a warning about
uninterested call, but will not fail.

[alokp, 2016/09/17 05:43:14]

I see. I misunderstood your previous comment. I agree about not over-testing.
Done.

 
   EXPECT_EQ(AddId("audio", HAS_AUDIO), ChunkDemuxer::kOk);
   EXPECT_EQ(AddId("video", HAS_VIDEO), ChunkDemuxer::kOk);
 
   ExpectInitMediaLogs(HAS_AUDIO);
   EXPECT_CALL(*this, InitSegmentReceivedMock(_));
   ASSERT_TRUE(AppendInitSegmentWithSourceId("audio", HAS_AUDIO));
 
   ShutdownDemuxer();
 }
 
 TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppendedText) {
   EXPECT_CALL(*this, DemuxerOpened());
-  demuxer_->Initialize(
-      &host_, CreateInitDoneCB(
-          kDefaultDuration(), DEMUXER_ERROR_COULD_NOT_OPEN), true);
+  demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true);

[alokp, 2016/09/16 22:10:16]

ditto

 
   EXPECT_EQ(AddId("audio", HAS_AUDIO), ChunkDemuxer::kOk);
   EXPECT_EQ(AddId("video_and_text", HAS_VIDEO), ChunkDemuxer::kOk);
 
   EXPECT_CALL(host_, AddTextStream(_, _))
       .Times(Exactly(1));
 
   ExpectInitMediaLogs(HAS_VIDEO);
   EXPECT_CALL(*this, InitSegmentReceivedMock(_));
   ASSERT_TRUE(
@@ skipping 50 matching lines @@
 
   ASSERT_TRUE(AppendCluster(kDefaultSecondCluster()));
 
   base::RunLoop().RunUntilIdle();
 
   Checkpoint(2);
 }
 
 // Test that parsing errors are handled for clusters appended after init.
 TEST_F(ChunkDemuxerTest, ErrorWhileParsingClusterAfterInit) {
+  InSequence s;

[alokp, 2016/09/16 22:10:16]

Added to verify that init_cb is called before OnDemuxerError. If we add
InSequence without making the changes in DemuxerError, this test will fail.

   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
-  ASSERT_TRUE(AppendCluster(kDefaultFirstCluster()));
 
   EXPECT_MEDIA_LOG(StreamParsingFailed());
   EXPECT_CALL(host_, OnDemuxerError(CHUNK_DEMUXER_ERROR_APPEND_FAILED));
   AppendGarbage();
 }
 
 // Test the case where a Seek() is requested while the parser
 // is in the middle of cluster. This is to verify that the parser
 // does not reset itself on a seek.
 TEST_F(ChunkDemuxerTest, SeekWhileParsingCluster) {
@@ skipping 1434 matching lines @@
   EXPECT_EQ(DemuxerStream::kOk, status);
   EXPECT_EQ(kLastAudioTimestamp, last_timestamp);
 
   ReadUntilNotOkOrEndOfStream(DemuxerStream::VIDEO, &status, &last_timestamp);
   EXPECT_EQ(DemuxerStream::kOk, status);
   EXPECT_EQ(kLastVideoTimestamp, last_timestamp);
 }
 
 TEST_F(ChunkDemuxerTest, GetBufferedRangesBeforeInitSegment) {
   EXPECT_CALL(*this, DemuxerOpened());
-  demuxer_->Initialize(&host_, CreateInitDoneCB(PIPELINE_OK), true);
+  demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true);
   ASSERT_EQ(AddId("audio", HAS_AUDIO), ChunkDemuxer::kOk);
   ASSERT_EQ(AddId("video", HAS_VIDEO), ChunkDemuxer::kOk);
 
   CheckExpectedRanges("audio", "{ }");
   CheckExpectedRanges("video", "{ }");
 }
 
 // Test that Seek() completes successfully when the first cluster
 // arrives.
 TEST_F(ChunkDemuxerTest, EndOfStreamDuringSeek) {
@@ skipping 1503 matching lines @@
   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
   DemuxerStream* audio_stream = demuxer_->GetStream(DemuxerStream::AUDIO);
   EXPECT_NE(nullptr, audio_stream);
   CheckStreamStatusNotifications(audio_stream);
   DemuxerStream* video_stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   EXPECT_NE(nullptr, video_stream);
   CheckStreamStatusNotifications(video_stream);
 }
 
 }  // namespace media