Send metadata change to renderer after decoder is drained

MediaSourcePlayer sends a metadata change to renderer whenever it receives a config change.
However, the decoder might still be in the draining process when this happens.
To fix this, we should wait for a OUTPUT_FORMAT_CHANGED status before sending out the IPC.

BUG=381975

Review URL: https://codereview.chromium.org/623363002

Cr-Commit-Position: refs/heads/master@{#299338}
(cherry picked from commit 07cdc5cc953f92e8c494375faf5e408f3e04ead9)

Conflicts:
	media/base/android/audio_decoder_job.h
	media/base/android/media_source_player_unittest.cc
	media/base/android/video_decoder_job.h

NOTRY=true
R=qinmin@chromium.org

Review URL: https://codereview.chromium.org/696453003

Cr-Commit-Position: refs/branch-heads/2171@{#332}
Cr-Branched-From: 267aeeb8d85c8503a7fd12bd14654b8ea78d3974-refs/heads/master@{#297060}

8 files changed, 106 insertions, 51 deletions

diff --git a/media/base/android/audio_decoder_job.cc b/media/base/android/audio_decoder_job.cc
index cb882ce..e376920 100644
--- a/media/base/android/audio_decoder_job.cc
+++ b/media/base/android/audio_decoder_job.cc
@@ -51,6 +51,17 @@ bool AudioDecoderJob::HasStream() const {
   return audio_codec_ != kUnknownAudioCodec;
 }
 
+void AudioDecoderJob::SetDemuxerConfigs(const DemuxerConfigs& configs) {
+  // TODO(qinmin): split DemuxerConfig for audio and video separately so we
+  // can simply store the stucture here.
+  audio_codec_ = configs.audio_codec;
+  num_channels_ = configs.audio_channels;
+  sampling_rate_ = configs.audio_sampling_rate;
+  set_is_content_encrypted(configs.is_audio_encrypted);
+  audio_extra_data_ = configs.audio_extra_data;
+  bytes_per_frame_ = kBytesPerAudioOutputSample * num_channels_;
+}
+
 void AudioDecoderJob::SetVolume(double volume) {
   volume_ = volume;
   SetVolumeInternal();
@@ -94,17 +105,6 @@ bool AudioDecoderJob::ComputeTimeToRender() const {
   return false;
 }
 
-void AudioDecoderJob::UpdateDemuxerConfigs(const DemuxerConfigs& configs) {
-  // TODO(qinmin): split DemuxerConfig for audio and video separately so we
-  // can simply store the stucture here.
-  audio_codec_ = configs.audio_codec;
-  num_channels_ = configs.audio_channels;
-  sampling_rate_ = configs.audio_sampling_rate;
-  set_is_content_encrypted(configs.is_audio_encrypted);
-  audio_extra_data_ = configs.audio_extra_data;
-  bytes_per_frame_ = kBytesPerAudioOutputSample * num_channels_;
-}
-
 bool AudioDecoderJob::AreDemuxerConfigsChanged(
     const DemuxerConfigs& configs) const {
   return audio_codec_ != configs.audio_codec ||
diff --git a/media/base/android/audio_decoder_job.h b/media/base/android/audio_decoder_job.h
index f3bb091..ecd3f21 100644
--- a/media/base/android/audio_decoder_job.h
+++ b/media/base/android/audio_decoder_job.h
@@ -29,6 +29,7 @@ class AudioDecoderJob : public MediaDecoderJob {
 
   // MediaDecoderJob implementation.
   virtual bool HasStream() const OVERRIDE;
+  virtual void SetDemuxerConfigs(const DemuxerConfigs& configs) OVERRIDE;
 
   // Sets the volume of the audio output.
   void SetVolume(double volume);
@@ -47,7 +48,6 @@ class AudioDecoderJob : public MediaDecoderJob {
   virtual bool ComputeTimeToRender() const OVERRIDE;
   virtual bool AreDemuxerConfigsChanged(
       const DemuxerConfigs& configs) const OVERRIDE;
-  virtual void UpdateDemuxerConfigs(const DemuxerConfigs& configs) OVERRIDE;
   virtual bool CreateMediaCodecBridgeInternal() OVERRIDE;
 
   // Helper method to set the audio output volume.
diff --git a/media/base/android/media_decoder_job.cc b/media/base/android/media_decoder_job.cc
index bceba43..b0aa1af 100644
--- a/media/base/android/media_decoder_job.cc
+++ b/media/base/android/media_decoder_job.cc
@@ -203,13 +203,6 @@ void MediaDecoderJob::ReleaseDecoderResources() {
   release_resources_pending_ = true;
 }
 
-bool MediaDecoderJob::SetDemuxerConfigs(const DemuxerConfigs& configs) {
-  bool config_changed = AreDemuxerConfigsChanged(configs);
-  if (config_changed)
-    UpdateDemuxerConfigs(configs);
-  return config_changed;
-}
-
 base::android::ScopedJavaLocalRef<jobject> MediaDecoderJob::GetMediaCrypto() {
   base::android::ScopedJavaLocalRef<jobject> media_crypto;
   if (drm_bridge_)
@@ -335,19 +328,18 @@ void MediaDecoderJob::DecodeCurrentAccessUnit(
     int index = CurrentReceivedDataChunkIndex();
     const DemuxerConfigs& configs = received_data_[index].demuxer_configs[0];
     bool reconfigure_needed = IsCodecReconfigureNeeded(configs);
-    // TODO(qinmin): |config_changed_cb_| should be run after draining finishes.
-    // http://crbug.com/381975.
-    if (SetDemuxerConfigs(configs))
-      config_changed_cb_.Run();
+    SetDemuxerConfigs(configs);
     if (!drain_decoder_) {
       // If we haven't decoded any data yet, just skip the current access unit
       // and request the MediaCodec to be recreated on next Decode().
       if (skip_eos_enqueue_ || !reconfigure_needed) {
         need_to_reconfig_decoder_job_ =
             need_to_reconfig_decoder_job_ || reconfigure_needed;
+        // Report MEDIA_CODEC_OK status so decoder will continue decoding and
+        // MEDIA_CODEC_OUTPUT_FORMAT_CHANGED status will come later.
         ui_task_runner_->PostTask(FROM_HERE, base::Bind(
             &MediaDecoderJob::OnDecodeCompleted, base::Unretained(this),
-            MEDIA_CODEC_OUTPUT_FORMAT_CHANGED, kNoTimestamp(), kNoTimestamp()));
+            MEDIA_CODEC_OK, kNoTimestamp(), kNoTimestamp()));
         return;
       }
       // Start draining the decoder so that all the remaining frames are
@@ -541,6 +533,9 @@ void MediaDecoderJob::OnDecodeCompleted(
     status = MEDIA_CODEC_OK;
   }
 
+  if (status == MEDIA_CODEC_OUTPUT_FORMAT_CHANGED && UpdateOutputFormat())
+    config_changed_cb_.Run();
+
   if (release_resources_pending_) {
     ReleaseMediaCodecBridge();
     release_resources_pending_ = false;
@@ -642,6 +637,10 @@ bool MediaDecoderJob::IsCodecReconfigureNeeded(
   return true;
 }
 
+bool MediaDecoderJob::UpdateOutputFormat() {
+  return false;
+}
+
 void MediaDecoderJob::ReleaseMediaCodecBridge() {
   if (!media_codec_bridge_)
     return;
diff --git a/media/base/android/media_decoder_job.h b/media/base/android/media_decoder_job.h
index 3b5da14..15d9635 100644
--- a/media/base/android/media_decoder_job.h
+++ b/media/base/android/media_decoder_job.h
@@ -84,9 +84,8 @@ class MediaDecoderJob {
   // Releases all the decoder resources as the current tab is going background.
   virtual void ReleaseDecoderResources();
 
-  // Sets the demuxer configs. Returns true if configs has changed, or false
-  // otherwise.
-  bool SetDemuxerConfigs(const DemuxerConfigs& configs);
+  // Sets the demuxer configs.
+  virtual void SetDemuxerConfigs(const DemuxerConfigs& configs) = 0;
 
   // Returns whether the decoder has finished decoding all the data.
   bool OutputEOSReached() const;
@@ -228,13 +227,14 @@ class MediaDecoderJob {
   virtual bool AreDemuxerConfigsChanged(
       const DemuxerConfigs& configs) const = 0;
 
-  // Updates the demuxer configs.
-  virtual void UpdateDemuxerConfigs(const DemuxerConfigs& configs) = 0;
-
   // Returns true if |media_codec_bridge_| needs to be reconfigured for the
   // new DemuxerConfigs, or false otherwise.
   virtual bool IsCodecReconfigureNeeded(const DemuxerConfigs& configs) const;
 
+  // Update the output format from the decoder, returns true if the output
+  // format changes, or false otherwise.
+  virtual bool UpdateOutputFormat();
+
   // Return the index to |received_data_| that is not currently being decoded.
   size_t inactive_demuxer_data_index() const {
     return 1 - current_demuxer_data_index_;
diff --git a/media/base/android/media_source_player.cc b/media/base/android/media_source_player.cc
index ac2df3b..4018451 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -150,11 +150,11 @@ bool MediaSourcePlayer::IsPlaying() {
 }
 
 int MediaSourcePlayer::GetVideoWidth() {
-  return video_decoder_job_->width();
+  return video_decoder_job_->output_width();
 }
 
 int MediaSourcePlayer::GetVideoHeight() {
-  return video_decoder_job_->height();
+  return video_decoder_job_->output_height();
 }
 
 void MediaSourcePlayer::SeekTo(base::TimeDelta timestamp) {
diff --git a/media/base/android/media_source_player_unittest.cc b/media/base/android/media_source_player_unittest.cc
index 9cb2b41..366ec8c 100644
--- a/media/base/android/media_source_player_unittest.cc
+++ b/media/base/android/media_source_player_unittest.cc
@@ -45,6 +45,7 @@ class MockMediaPlayerManager : public MediaPlayerManager {
       : message_loop_(message_loop),
         playback_completed_(false),
         num_resources_requested_(0),
+        num_metadata_changes_(0),
         timestamp_updated_(false) {}
   virtual ~MockMediaPlayerManager() {}
 
@@ -62,7 +63,9 @@ class MockMediaPlayerManager : public MediaPlayerManager {
   }
   virtual void OnMediaMetadataChanged(
       int player_id, base::TimeDelta duration, int width, int height,
-      bool success) OVERRIDE {}
+      bool success) OVERRIDE {
+    num_metadata_changes_++;
+  }
   virtual void OnPlaybackComplete(int player_id) OVERRIDE {
     playback_completed_ = true;
     if (message_loop_->is_running())
@@ -92,6 +95,10 @@ class MockMediaPlayerManager : public MediaPlayerManager {
     return num_resources_requested_;
   }
 
+  int num_metadata_changes() const {
+    return num_metadata_changes_;
+  }
+
   void OnMediaResourcesRequested(int player_id) {
     num_resources_requested_++;
   }
@@ -109,6 +116,8 @@ class MockMediaPlayerManager : public MediaPlayerManager {
   bool playback_completed_;
   // The number of resource requests this object has seen.
   int num_resources_requested_;
+  // The number of metadata changes reported by the player.
+  int num_metadata_changes_;
   // Playback timestamp was updated.
   bool timestamp_updated_;
 
@@ -272,7 +281,7 @@ class MediaSourcePlayerTest : public testing::Test {
     DemuxerConfigs configs;
     configs.video_codec = kCodecVP8;
     configs.video_size =
-        use_larger_size ? gfx::Size(640, 480) : gfx::Size(320, 240);
+        use_larger_size ? gfx::Size(640, 240) : gfx::Size(320, 240);
     configs.is_video_encrypted = false;
     configs.duration = kDefaultDuration;
     return configs;
@@ -2258,4 +2267,30 @@ TEST_F(MediaSourcePlayerTest, CurrentTimeKeepsIncreasingAfterConfigChange) {
   DecodeAudioDataUntilOutputBecomesAvailable();
 }
 
+TEST_F(MediaSourcePlayerTest, VideoMetadataChangeAfterConfigChange) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that after a config change, metadata change will be happen
+  // after decoder is drained.
+  StartConfigChange(false, true, 2, false);
+  EXPECT_EQ(1, manager_.num_metadata_changes());
+  EXPECT_FALSE(IsDrainingDecoder(false));
+
+  // Create video data with new resolutions.
+  DemuxerData data = CreateReadFromDemuxerAckForVideo();
+  AccessUnit unit;
+  unit.status = DemuxerStream::kOk;
+  scoped_refptr<DecoderBuffer> buffer = ReadTestDataFile("vp8-I-frame-640x240");
+  unit.data = std::vector<uint8>(
+      buffer->data(), buffer->data() + buffer->data_size());
+  data.access_units[0] = unit;
+
+  // Wait for the metadata change.
+  while(manager_.num_metadata_changes() == 1) {
+    player_.OnDemuxerDataAvailable(data);
+    WaitForVideoDecodeDone();
+  }
+  EXPECT_EQ(2, manager_.num_metadata_changes());
+}
+
 }  // namespace media
diff --git a/media/base/android/video_decoder_job.cc b/media/base/android/video_decoder_job.cc
index 1eb3723..bfa0dc3 100644
--- a/media/base/android/video_decoder_job.cc
+++ b/media/base/android/video_decoder_job.cc
@@ -33,8 +33,10 @@ VideoDecoderJob::VideoDecoderJob(
                       request_data_cb,
                       on_demuxer_config_changed_cb),
       video_codec_(kUnknownVideoCodec),
-      width_(0),
-      height_(0),
+      config_width_(0),
+      config_height_(0),
+      output_width_(0),
+      output_height_(0),
       request_resources_cb_(request_resources_cb),
       next_video_data_is_iframe_(true) {
 }
@@ -69,6 +71,17 @@ void VideoDecoderJob::ReleaseDecoderResources() {
   surface_ = gfx::ScopedJavaSurface();
 }
 
+void VideoDecoderJob::SetDemuxerConfigs(const DemuxerConfigs& configs) {
+  video_codec_ = configs.video_codec;
+  config_width_ = configs.video_size.width();
+  config_height_ = configs.video_size.height();
+  set_is_content_encrypted(configs.is_video_encrypted);
+  if (!media_codec_bridge_) {
+    output_width_ = config_width_;
+    output_height_ = config_height_;
+  }
+}
+
 void VideoDecoderJob::ReleaseOutputBuffer(
     int output_buffer_index,
     size_t size,
@@ -83,13 +96,6 @@ bool VideoDecoderJob::ComputeTimeToRender() const {
   return true;
 }
 
-void VideoDecoderJob::UpdateDemuxerConfigs(const DemuxerConfigs& configs) {
-  video_codec_ = configs.video_codec;
-  width_ = configs.video_size.width();
-  height_ = configs.video_size.height();
-  set_is_content_encrypted(configs.is_video_encrypted);
-}
-
 bool VideoDecoderJob::IsCodecReconfigureNeeded(
     const DemuxerConfigs& configs) const {
   if (!media_codec_bridge_)
@@ -114,8 +120,8 @@ bool VideoDecoderJob::AreDemuxerConfigsChanged(
     const DemuxerConfigs& configs) const {
   return video_codec_ != configs.video_codec ||
       is_content_encrypted() != configs.is_video_encrypted ||
-      width_ != configs.video_size.width() ||
-      height_ != configs.video_size.height();
+      config_width_ != configs.video_size.width() ||
+      config_height_ != configs.video_size.height();
 }
 
 bool VideoDecoderJob::CreateMediaCodecBridgeInternal() {
@@ -133,7 +139,7 @@ bool VideoDecoderJob::CreateMediaCodecBridgeInternal() {
       drm_bridge()->IsProtectedSurfaceRequired();
 
   media_codec_bridge_.reset(VideoCodecBridge::CreateDecoder(
-      video_codec_, is_secure, gfx::Size(width_, height_),
+      video_codec_, is_secure, gfx::Size(config_width_, config_height_),
       surface_.j_surface().obj(), GetMediaCrypto().obj()));
 
   if (!media_codec_bridge_)
@@ -147,6 +153,16 @@ void VideoDecoderJob::CurrentDataConsumed(bool is_config_change) {
   next_video_data_is_iframe_ = is_config_change;
 }
 
+bool VideoDecoderJob::UpdateOutputFormat() {
+  if (!media_codec_bridge_)
+    return false;
+  int prev_output_width = output_width_;
+  int prev_output_height = output_height_;
+  media_codec_bridge_->GetOutputFormat(&output_width_, &output_height_);
+  return (output_width_ != prev_output_width) ||
+      (output_height_ != prev_output_height);
+}
+
 bool VideoDecoderJob::IsProtectedSurfaceRequired() {
   return is_content_encrypted() && drm_bridge() &&
       drm_bridge()->IsProtectedSurfaceRequired();
diff --git a/media/base/android/video_decoder_job.h b/media/base/android/video_decoder_job.h
index ac3dc27..c6005fb 100644
--- a/media/base/android/video_decoder_job.h
+++ b/media/base/android/video_decoder_job.h
@@ -35,13 +35,14 @@ class VideoDecoderJob : public MediaDecoderJob {
   virtual bool HasStream() const OVERRIDE;
   virtual void Flush() OVERRIDE;
   virtual void ReleaseDecoderResources() OVERRIDE;
+  virtual void SetDemuxerConfigs(const DemuxerConfigs& configs) OVERRIDE;
 
   bool next_video_data_is_iframe() {
     return next_video_data_is_iframe_;
   }
 
-  int width() const { return width_; }
-  int height() const { return height_; }
+  int output_width() const { return output_width_; }
+  int output_height() const { return output_height_; }
 
  private:
   // MediaDecoderJob implementation.
@@ -52,21 +53,25 @@ class VideoDecoderJob : public MediaDecoderJob {
       base::TimeDelta current_presentation_timestamp,
       const ReleaseOutputCompletionCallback& callback) OVERRIDE;
   virtual bool ComputeTimeToRender() const OVERRIDE;
-  virtual void UpdateDemuxerConfigs(const DemuxerConfigs& configs) OVERRIDE;
   virtual bool IsCodecReconfigureNeeded(
       const DemuxerConfigs& configs) const OVERRIDE;
   virtual bool AreDemuxerConfigsChanged(
       const DemuxerConfigs& configs) const OVERRIDE;
   virtual bool CreateMediaCodecBridgeInternal() OVERRIDE;
   virtual void CurrentDataConsumed(bool is_config_change) OVERRIDE;
+  virtual bool UpdateOutputFormat() OVERRIDE;
 
   // Returns true if a protected surface is required for video playback.
   bool IsProtectedSurfaceRequired();
 
   // Video configs from the demuxer.
   VideoCodec video_codec_;
-  int width_;
-  int height_;
+  int config_width_;
+  int config_height_;
+
+  // Video output format.
+  int output_width_;
+  int output_height_;
 
   // The surface object currently owned by the player.
   gfx::ScopedJavaSurface surface_;