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 226 matching lines @@
   EXPECT_TRUE(buffer->end_of_stream());
   *called = true;
 }
 
 static void OnSeekDone_OKExpected(bool* called, PipelineStatus status) {
   EXPECT_EQ(status, PIPELINE_OK);
   *called = true;
 }
 
 class ChunkDemuxerTest : public ::testing::Test {
+ public:
+  // Public method becuase test cases use it directly.

[wolenetz, 2016/09/27 21:01:14]

nit: spelling

[alokp, 2016/09/28 17:22:45]

Done.

+  MOCK_METHOD1(DemuxerInitialized, void(PipelineStatus));
+
  protected:
   enum CodecsIndex {
     AUDIO,
     VIDEO,
     MAX_CODECS_INDEX
   };
 
   // Default cluster to append first for simple tests.
   std::unique_ptr<Cluster> kDefaultFirstCluster() {
     return GenerateCluster(0, 4);
@@ skipping 522 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);
-  }
-
   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));

[wolenetz, 2016/10/05 23:41:24]

Good :)

+    return base::Bind(&ChunkDemuxerTest::DemuxerInitialized,
+                      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 850 matching lines @@
   AppendMuxedCluster(MuxedStreamInfo(kAudioTrackNum, "46K 69K", 23),
                      MuxedStreamInfo(kVideoTrackNum, "60 90K", 30),
                      MuxedStreamInfo(kTextTrackNum, "80K 90K"));
   CheckExpectedRanges("{ [23,92) }");
 
   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()

[wolenetz, 2016/09/27 21:01:14]

Where is the error verified now?
(The err is because ShutDown() occurred before *both* audio and video init
segments appended to the distinct sourceIDs setup at the beginning of the test.)

[alokp, 2016/09/28 17:22:45]

The error was never being verified (silently) because the init_cb does not get
called if Shutdown occurs before Demuxer is initialized. ChunkDemuxer::Shutdown
or the destructor, does not do anything with init_cb. Do you want to handle it?
I would be happy to make that change but I think it should be done in a separate
patch.

[wolenetz, 2016/10/03 21:16:23]

Hmm. If there is a demuxer error that isn't reported to UMA because demuxer was
shutdown before the init completed, that sounds like an error we'd really like
to count as such. If the error does *not* get reported due to the SHutdown here,
then please fix the comment, add a crbug and TODO to get such reporting fixed
and verified later.  Thanks for digging into this!

[alokp, 2016/10/04 20:48:40]

Is shutting down before appending initialization segment an error? AFAICT this
test does not append any invalid segment, so it should not trigger any
initialization error. It just shuts down the demuxer before appending the video
segment.

[wolenetz, 2016/10/05 23:41:23]

Ok. Thanks for clarifying.

This test's comment is "make sure the demuxer reports an error". But you're
totally right: it's not an error to shutdown the demuxer before any/all
initialization segments have been appended. So I think the test comment should
at least be removed.

[alokp, 2016/10/06 05:49:17]

Done.

 // 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(&ChunkDemuxerTest::DemuxerInitialized,
+                                          base::Unretained(this)),
+                       true);
 
   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(&ChunkDemuxerTest::DemuxerInitialized,

[wolenetz, 2016/09/27 21:01:14]

Ditto.

[alokp, 2016/09/28 17:22:45]

ditto.

[wolenetz, 2016/10/03 21:16:23]

ditto :)

[wolenetz, 2016/10/05 23:41:23]

ignore

[alokp, 2016/10/06 05:49:17]

Acknowledged.

+                                          base::Unretained(this)),
+                       true);
 
   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;
   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(&ChunkDemuxerTest::DemuxerInitialized,

[wolenetz, 2016/09/27 21:01:14]

Where is *lack* of error on CDT::DemuxerInitialized now verified?

[alokp, 2016/09/28 17:22:45]

This again was silently not getting verified because the test completes before
Demuxer initializes. I found about this after I added EXPECT_CALL in
CreateInitDoneCB, which was not getting satisfied.

[wolenetz, 2016/10/03 21:16:23]

I wonder if this could be fixed with a test class destructor calling
RunUntilIdle() on the media thread. I would like positive verification of
expected initialization results, or at least lack of any error being dropped too
early by early test completion.

[alokp, 2016/10/04 20:48:40]

RunUntilIdle does not help here because init_cb is not even posted. The test
exits while demuxer was waiting for initialization segments to be appended.

out/Debug/media_unittests
--gtest_filter=ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment
--vmodule=*/media/*=4

[ RUN      ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment
[66548:66548:1004/134303:7701287375808:VERBOSE1:chunk_demuxer.cc(430)] Init()
[66548:66548:1004/134303:7701287375868:VERBOSE1:chunk_demuxer.cc(1045)]
ChunkDemuxer::ChangeState_Locked() : 0 -> 1
[66548:66548:1004/134303:7701287375948:VERBOSE1:chunk_demuxer.cc(582)] AddId
id=audio mime_type=audio/webm codecs=vorbis
[66548:66548:1004/134303:7701287376374:VERBOSE2:frame_processor.cc(169)]
FrameProcessor()
[66548:66548:1004/134303:7701287376450:VERBOSE1:webm_stream_parser.cc(121)]
ChangeState() : 0 -> 1
[66548:66548:1004/134303:7701287376502:VERBOSE1:chunk_demuxer.cc(582)] AddId
id=video mime_type=video/webm codecs=vp8
[66548:66548:1004/134303:7701287376567:VERBOSE2:frame_processor.cc(169)]
FrameProcessor()
[66548:66548:1004/134303:7701287376632:VERBOSE1:webm_stream_parser.cc(121)]
ChangeState() : 0 -> 1
[66548:66548:1004/134303:7701287376733:VERBOSE1:chunk_demuxer.cc(1021)]
Shutdown()
[66548:66548:1004/134303:7701287376768:VERBOSE1:chunk_demuxer.cc(1045)]
ChunkDemuxer::ChangeState_Locked() : 1 -> 5
[66548:66548:1004/134303:7701287376804:VERBOSE2:frame_processor.cc(174)]
~FrameProcessor()
[66548:66548:1004/134303:7701287376852:VERBOSE2:frame_processor.cc(174)]
~FrameProcessor()
../../media/filters/chunk_demuxer_unittest.cc:699: Failure
Actual function call count doesn't match EXPECT_CALL(*this,
DemuxerInitialized(expected_status))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment (2 ms)
[----------] 1 test from ChunkDemuxerTest (2 ms total)

[wolenetz, 2016/10/05 23:41:24]

I see. My comment was that RunUntilIdle() might let unexpected posted error cb
to fire and get caught by strictmock, but that's reasonably out of scope of this
CL. (I wasn't asking for EXPECT_CALL(init_cb_ called with some status) to be
added back to this test, since that cb shouldn't get called until after any/all
expected (1 per each AddId) initial init segments are successfully appended (or
some error), unlike what I think the most recent previous reply on this comment
was doing.)

[alokp, 2016/10/06 05:49:17]

Acknowledged.

+                                          base::Unretained(this)),
+                       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