Remove the IPC to request DemuxerConfigs when config changes

media/base/android/media_source_player_unittest.cc

Index: media/base/android/media_source_player_unittest.cc
diff --git a/media/base/android/media_source_player_unittest.cc b/media/base/android/media_source_player_unittest.cc
index c730c84792b3c4d056f5a6becd2854b0841b57c2..a9c3572efa2a26f996a0d9eaa2218a0a365a86d9 100644
--- a/media/base/android/media_source_player_unittest.cc
+++ b/media/base/android/media_source_player_unittest.cc
@@ -135,14 +135,10 @@ class MockDemuxerAndroid : public DemuxerAndroid {
       : message_loop_(message_loop),
         num_data_requests_(0),
         num_seek_requests_(0),
-        num_browser_seek_requests_(0),
-        num_config_requests_(0) {}
+        num_browser_seek_requests_(0) {}
   virtual ~MockDemuxerAndroid() {}
 
   virtual void Initialize(DemuxerAndroidClient* client) OVERRIDE {}
-  virtual void RequestDemuxerConfigs() OVERRIDE {
-    num_config_requests_++;
-  }
   virtual void RequestDemuxerData(DemuxerStream::Type type) OVERRIDE {
     num_data_requests_++;
     if (message_loop_->is_running())
@@ -158,7 +154,6 @@ class MockDemuxerAndroid : public DemuxerAndroid {
   int num_data_requests() const { return num_data_requests_; }
   int num_seek_requests() const { return num_seek_requests_; }
   int num_browser_seek_requests() const { return num_browser_seek_requests_; }
-  int num_config_requests() const { return num_config_requests_; }
 
  private:
   base::MessageLoop* message_loop_;
@@ -172,9 +167,6 @@ class MockDemuxerAndroid : public DemuxerAndroid {
   // The number of browser seek requests this object has seen.
   int num_browser_seek_requests_;
 
-  // The number of demuxer config requests this object has seen.
-  int num_config_requests_;
-
   DISALLOW_COPY_AND_ASSIGN(MockDemuxerAndroid);
 };
 
@@ -529,6 +521,8 @@ class MediaSourcePlayerTest : public testing::Test {
       data.access_units[i] = CreateAccessUnitWithData(is_audio, i);
 
     data.access_units[config_unit_index].status = DemuxerStream::kConfigChanged;
+    data.demuxer_configs.resize(1);
+    data.demuxer_configs[0] = CreateDemuxerConfigs(is_audio, !is_audio);
     return data;
   }
 
@@ -603,8 +597,6 @@ class MediaSourcePlayerTest : public testing::Test {
   void StartConfigChange(bool is_audio,
                          bool config_unit_in_prefetch,
                          int config_unit_index) {
-    int expected_num_config_requests = demuxer_->num_config_requests();
-
     EXPECT_FALSE(GetMediaDecoderJob(is_audio));
     if (is_audio) {
       StartAudioDecoderJob(true);
@@ -629,19 +621,15 @@ class MediaSourcePlayerTest : public testing::Test {
       EXPECT_EQ(expected_num_data_requests, demuxer_->num_data_requests());
     }
 
-    EXPECT_EQ(expected_num_config_requests, demuxer_->num_config_requests());
-
     // Feed and decode access units with data for any units prior to
     // |config_unit_index|, and a |kConfigChanged| unit at that index.
     // Player should prepare to reconfigure the decoder job, and should request
     // new demuxer configs.
     player_.OnDemuxerDataAvailable(
         CreateReadFromDemuxerAckWithConfigChanged(is_audio, config_unit_index));
-    WaitForDecodeDone(is_audio, !is_audio);
-
-    expected_num_config_requests++;
-    EXPECT_EQ(expected_num_data_requests, demuxer_->num_data_requests());
-    EXPECT_EQ(expected_num_config_requests, demuxer_->num_config_requests());
+    // Run until decoder starts to request new data.
+    while (demuxer_->num_data_requests() == expected_num_data_requests)
+      message_loop_.RunUntilIdle();
   }
 
   void CreateNextTextureAndSetVideoSurface() {
@@ -1199,15 +1187,11 @@ TEST_F(MediaSourcePlayerTest, AV_PlaybackCompletionAcrossConfigChange) {
   Start(CreateAudioVideoDemuxerConfigs(), true);
 
   player_.OnDemuxerDataAvailable(CreateEOSAck(true));  // Audio EOS
-  EXPECT_EQ(0, demuxer_->num_config_requests());
   player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckWithConfigChanged(
       false, 0));  // Video |kConfigChanged| as first unit.
 
   WaitForAudioVideoDecodeDone();
 
-  EXPECT_EQ(1, demuxer_->num_config_requests());
-  EXPECT_EQ(2, demuxer_->num_data_requests());
-  player_.OnDemuxerConfigsAvailable(CreateAudioVideoDemuxerConfigs());
   EXPECT_EQ(3, demuxer_->num_data_requests());
 
   // At no time after completing audio EOS decode, above, should the
@@ -1227,17 +1211,11 @@ TEST_F(MediaSourcePlayerTest, VA_PlaybackCompletionAcrossConfigChange) {
   Start(CreateAudioVideoDemuxerConfigs(), true);
 
   player_.OnDemuxerDataAvailable(CreateEOSAck(false));  // Video EOS
-  EXPECT_EQ(0, demuxer_->num_config_requests());
   player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckWithConfigChanged(
       true, 0));  // Audio |kConfigChanged| as first unit.
 
   WaitForAudioVideoDecodeDone();
 
-  // TODO(wolenetz/qinmin): Prevent redundant demuxer config request and change
-  // expectation to 1 here. See http://crbug.com/325528.
-  EXPECT_EQ(2, demuxer_->num_config_requests());
-  EXPECT_EQ(2, demuxer_->num_data_requests());
-  player_.OnDemuxerConfigsAvailable(CreateAudioVideoDemuxerConfigs());
   EXPECT_EQ(3, demuxer_->num_data_requests());
 
   // At no time after completing video EOS decode, above, should the
@@ -1592,9 +1570,7 @@ TEST_F(MediaSourcePlayerTest, PrerollContinuesAcrossConfigChange) {
   // sending an AU with |kConfigChanged|. Player should prepare to reconfigure
   // the audio decoder job, and should request new demuxer configs.
   DemuxerData data = CreateReadFromDemuxerAckWithConfigChanged(true, 0);
-  EXPECT_EQ(0, demuxer_->num_config_requests());
   player_.OnDemuxerDataAvailable(data);
-  EXPECT_EQ(1, demuxer_->num_config_requests());
 
   // Simulate arrival of new configs.
   player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs(kCodecVorbis));
@@ -1617,28 +1593,16 @@ TEST_F(MediaSourcePlayerTest, SimultaneousAudioVideoConfigChange) {
   // Simulate audio |kConfigChanged| prefetched as standalone access unit.
   player_.OnDemuxerDataAvailable(
       CreateReadFromDemuxerAckWithConfigChanged(true, 0));
-  EXPECT_EQ(0, demuxer_->num_config_requests());  // No OnPrefetchDone() yet.
 
   // Simulate video |kConfigChanged| prefetched as standalone access unit.
   player_.OnDemuxerDataAvailable(
       CreateReadFromDemuxerAckWithConfigChanged(false, 0));
-  EXPECT_EQ(1, demuxer_->num_config_requests());  // OnPrefetchDone() occurred.
-  EXPECT_EQ(2, demuxer_->num_data_requests());  // No more data requested yet.
+  EXPECT_EQ(4, demuxer_->num_data_requests());  // No more data requested yet.

[wolenetz, 2014/05/02 22:25:30]

nit: remove the incorrect(?) comment

[qinmin, 2014/05/05 20:52:19]

Done.

 
   // No job re-creation should occur until the requested configs arrive.

[wolenetz, 2014/05/02 22:25:30]

nit: adjust the wording of comment: Both jobs should have been reconfigured by
now.

[qinmin, 2014/05/05 20:52:19]

Done.

-  EXPECT_EQ(first_audio_job, GetMediaDecoderJob(true));
-  EXPECT_EQ(first_video_job, GetMediaDecoderJob(false));
-
-  player_.OnDemuxerConfigsAvailable(CreateAudioVideoDemuxerConfigs());
-  EXPECT_EQ(4, demuxer_->num_data_requests());
-  MediaDecoderJob* second_audio_job = GetMediaDecoderJob(true);
-  MediaDecoderJob* second_video_job = GetMediaDecoderJob(false);
-  EXPECT_NE(first_audio_job, second_audio_job);
-  EXPECT_NE(first_video_job, second_video_job);
-  EXPECT_TRUE(second_audio_job && second_video_job);
-
-  // Confirm no further demuxer configs requested.
-  EXPECT_EQ(1, demuxer_->num_config_requests());
+  EXPECT_NE(first_audio_job, GetMediaDecoderJob(true));

[wolenetz, 2014/05/02 22:25:30]

nit: these pointer checks can be flaky. I think we already have a bug for this
-- reference it here, too?

[qinmin, 2014/05/05 20:52:19]

Done.

+  EXPECT_NE(first_video_job, GetMediaDecoderJob(false));
+  EXPECT_TRUE(GetMediaDecoderJob(true) && GetMediaDecoderJob(false));
 }
 
 TEST_F(MediaSourcePlayerTest, DemuxerConfigRequestedIfInPrefetchUnit0) {
@@ -1703,90 +1667,27 @@ TEST_F(MediaSourcePlayerTest, BrowserSeek_PrerollAfterBrowserSeek) {
 TEST_F(MediaSourcePlayerTest, VideoDemuxerConfigChange) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
-  // Test that video config change notification results in request for demuxer
-  // configuration, and that a video decoder job results without any browser
-  // seek necessary once the new demuxer config arrives.
+  // Test that video config change notification results in creating a new
+  // video decoder job results without any browser seek.
   StartConfigChange(false, true, 1);
-  MediaDecoderJob* first_job = GetMediaDecoderJob(false);
-  EXPECT_TRUE(first_job);
-  EXPECT_EQ(1, demuxer_->num_data_requests());
-  EXPECT_EQ(1, demuxer_->num_config_requests());
-
-  // Simulate arrival of new configs.
-  player_.OnDemuxerConfigsAvailable(CreateVideoDemuxerConfigs());
-
-  // New video decoder job should have been created and configured, without any
-  // browser seek.
-  MediaDecoderJob* second_job = GetMediaDecoderJob(false);
-  EXPECT_TRUE(second_job);
-  EXPECT_NE(first_job, second_job);
+  EXPECT_TRUE(GetMediaDecoderJob(false));
   EXPECT_EQ(2, demuxer_->num_data_requests());
-  EXPECT_EQ(1, demuxer_->num_config_requests());
   EXPECT_EQ(0, demuxer_->num_seek_requests());
 }
 
-TEST_F(MediaSourcePlayerTest, VideoConfigChangeContinuesAcrossSeek) {
+TEST_F(MediaSourcePlayerTest, NewSurfaceAfterChangingConfigs) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
-  // Test if a demuxer config request is pending (due to previously receiving
-  // |kConfigChanged|), and a seek request arrives prior to demuxer configs,
-  // then seek is processed first, followed by the decoder config change.
-  // This assumes the demuxer sends |kConfigChanged| read response prior to
-  // canceling any reads pending seek; no |kAborted| is involved in this test.
+  // Test that no seek results from a SetVideoSurface() that occurs after
+  // the player processes new demuxer configs. This test may be good to keep
+  // beyond browser seek hack.
   StartConfigChange(false, false, 1);
-  MediaDecoderJob* first_job = GetMediaDecoderJob(false);
-  EXPECT_TRUE(first_job);
-  EXPECT_EQ(1, demuxer_->num_config_requests());
-  EXPECT_EQ(2, demuxer_->num_data_requests());
-  EXPECT_EQ(0, demuxer_->num_seek_requests());
-
-  player_.SeekTo(base::TimeDelta::FromMilliseconds(100));
-
-  // Verify that the seek is requested immediately.
-  EXPECT_EQ(1, demuxer_->num_seek_requests());
-
-  // Simulate unlikely delayed arrival of the demuxer configs, completing the
-  // config change.
-  // TODO(wolenetz): Is it even possible for requested demuxer configs to be
-  // delayed until after a SeekTo request arrives?
-  player_.OnDemuxerConfigsAvailable(CreateVideoDemuxerConfigs());
-
-  MediaDecoderJob* second_job = GetMediaDecoderJob(false);
-  EXPECT_NE(first_job, second_job);
-  EXPECT_TRUE(second_job);
-
-  // Send back the seek done notification. This should finish the seek and
-  // trigger the player to request more data.
-  EXPECT_EQ(2, demuxer_->num_data_requests());
-  player_.OnDemuxerSeekDone(kNoTimestamp());
+  EXPECT_TRUE(GetMediaDecoderJob(false));
   EXPECT_EQ(3, demuxer_->num_data_requests());
-}
-
-TEST_F(MediaSourcePlayerTest, NewSurfaceWhileChangingConfigs) {
-  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
-
-  // Test that no seek or duplicated demuxer config request results from a
-  // SetVideoSurface() that occurs while the player is expecting new demuxer
-  // configs. This test may be good to keep beyond browser seek hack.
-  StartConfigChange(false, false, 1);
-  MediaDecoderJob* first_job = GetMediaDecoderJob(false);
-  EXPECT_TRUE(first_job);
-  EXPECT_EQ(1, demuxer_->num_config_requests());
-  EXPECT_EQ(2, demuxer_->num_data_requests());
 
   CreateNextTextureAndSetVideoSurface();
-
-  // Surface change processing (including decoder job re-creation) should
-  // not occur until the pending video config change is completed.
-  EXPECT_EQ(first_job, GetMediaDecoderJob(false));
-
-  player_.OnDemuxerConfigsAvailable(CreateVideoDemuxerConfigs());
-  MediaDecoderJob* second_job = GetMediaDecoderJob(false);
-  EXPECT_NE(first_job, second_job);
-  EXPECT_TRUE(second_job);
-
+  EXPECT_TRUE(GetMediaDecoderJob(false));
   EXPECT_EQ(3, demuxer_->num_data_requests());
-  EXPECT_EQ(1, demuxer_->num_config_requests());
   EXPECT_EQ(0, demuxer_->num_seek_requests());
 }
 
@@ -1975,19 +1876,15 @@ TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenConfigsAvailable) {
   StartConfigChange(true, true, 0);
   ReleasePlayer();
 
-  player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs(kCodecVorbis));
   EXPECT_FALSE(GetMediaDecoderJob(true));
   EXPECT_FALSE(player_.IsPlaying());
-  EXPECT_EQ(1, demuxer_->num_data_requests());
+  EXPECT_EQ(2, demuxer_->num_data_requests());
 
   // Player should resume upon Start(), even without further configs supplied.
   player_.Start();
   EXPECT_TRUE(GetMediaDecoderJob(true));
   EXPECT_TRUE(player_.IsPlaying());
   EXPECT_EQ(2, demuxer_->num_data_requests());
-
-  // No further config request should have occurred since StartConfigChange().
-  EXPECT_EQ(1, demuxer_->num_config_requests());
 }
 
 TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenStart) {

[wolenetz, 2014/05/02 22:25:30]

This test is now entirely a subset of the previous test. Remove this one?

[qinmin, 2014/05/05 20:52:19]

merged with the above one
On 2014/05/02 22:25:30, wolenetz wrote:

@@ -2001,15 +1898,8 @@ TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenStart) {
 
   player_.Start();
   EXPECT_TRUE(player_.IsPlaying());
-  EXPECT_FALSE(GetMediaDecoderJob(true));
-  EXPECT_EQ(1, demuxer_->num_data_requests());
-
-  player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs(kCodecVorbis));
   EXPECT_TRUE(GetMediaDecoderJob(true));
   EXPECT_EQ(2, demuxer_->num_data_requests());
-
-  // No further config request should have occurred since StartConfigChange().
-  EXPECT_EQ(1, demuxer_->num_config_requests());
 }
 
 TEST_F(MediaSourcePlayerTest, BrowserSeek_ThenReleaseThenDemuxerSeekDone) {
@@ -2116,9 +2006,6 @@ TEST_F(MediaSourcePlayerTest, CurrentTimeKeepsIncreasingAfterConfigChange) {
   DemuxerData data = CreateReadFromDemuxerAckWithConfigChanged(true, 0);
   player_.OnDemuxerDataAvailable(data);
   WaitForAudioDecodeDone();
-
-  // Simulate arrival of new configs.
-  player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs(kCodecVorbis));
   DecodeAudioDataUntilOutputBecomesAvailable();
 }