Bug 1000841 - remove SetActive() from MediaDecoderReader. Its user needs to know when to call SetIdle() only. r=cpearce

content/media/MediaDecoderReader.h

@@ -78,16 +78,17 @@ public:
                         int64_t aEndTime,
                         int64_t aCurrentTime) = 0;
 
-  // Called to move the reader into idle/active state. When the reader is
+  // Called to move the reader into idle state. When the reader is
   // created it is assumed to be active (i.e. not idle). When the media
   // element is paused and we don't need to decode any more data, the state
   // machine calls SetIdle() to inform the reader that its decoder won't be
-  // needed for a while. When we need to decode data again, the state machine
-  // calls SetActive() to activate the decoder. The reader can use these
-  // notifications to enter/exit a low power state when the decoder isn't
-  // needed, if desired. This is most useful on mobile.
+  // needed for a while. The reader can use these notifications to enter
+  // a low power state when the decoder isn't needed, if desired.
+  // This is most useful on mobile.
+  // Note: DecodeVideoFrame, DecodeAudioData, ReadMetadata and Seek should
+  // activate the decoder if necessary. The state machine only needs to know
+  // when to call SetIdle().
   virtual void SetIdle() { }
-  virtual void SetActive() { }
 
   // Tell the reader that the data decoded are not for direct playback, so it
   // can accept more files, in particular those which have more channels than

content/media/MediaDecoderStateMachine.cpp

@@ -194,7 +194,6 @@ MediaDecoderStateMachine::MediaDecoderStateMachine(MediaDecoder* aDecoder,
   mAmpleAudioThresholdUsecs(AMPLE_AUDIO_USECS),
   mDispatchedAudioDecodeTask(false),
   mDispatchedVideoDecodeTask(false),
-  mIsReaderIdle(false),
   mAudioCaptured(false),
   mTransportSeekable(true),
   mMediaSeekable(true),
@@ -573,7 +572,6 @@ MediaDecoderStateMachine::DecodeVideo()
     mDispatchedVideoDecodeTask = false;
     return;
   }
-  EnsureActive();
 
   // We don't want to consider skipping to the next keyframe if we've
   // only just started up the decode loop, so wait until we've decoded
@@ -667,7 +665,6 @@ MediaDecoderStateMachine::DecodeAudio()
     mDispatchedAudioDecodeTask = false;
     return;
   }
-  EnsureActive();
 
   // We don't want to consider skipping to the next keyframe if we've
   // only just started up the decode loop, so wait until we've decoded
@@ -1499,21 +1496,6 @@ MediaDecoderStateMachine::EnqueueDecodeMetadataTask()
   return NS_OK;
 }
 
-void
-MediaDecoderStateMachine::EnsureActive()
-{
-  AssertCurrentThreadInMonitor();
-  MOZ_ASSERT(OnDecodeThread());
-  if (!mIsReaderIdle) {
-    return;
-  }
-  mIsReaderIdle = false;
-  {
-    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
-    SetReaderActive();
-  }
-}
-
 void
 MediaDecoderStateMachine::SetReaderIdle()
 {
@@ -1529,14 +1511,6 @@ MediaDecoderStateMachine::SetReaderIdle()
   mReader->SetIdle();
 }
 
-void
-MediaDecoderStateMachine::SetReaderActive()
-{
-  DECODER_LOG(PR_LOG_DEBUG, "SetReaderActive()");
-  MOZ_ASSERT(OnDecodeThread());
-  mReader->SetActive();
-}
-
 void
 MediaDecoderStateMachine::DispatchDecodeTasksIfNeeded()
 {
@@ -1575,19 +1549,13 @@ MediaDecoderStateMachine::DispatchDecodeTasksIfNeeded()
     EnsureVideoDecodeTaskQueued();
   }
 
-  if (mIsReaderIdle == needIdle) {
-    return;
-  }
-  mIsReaderIdle = needIdle;
-  RefPtr<nsIRunnable> event;
-  if (mIsReaderIdle) {
-    event = NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetReaderIdle);
-  } else {
-    event = NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetReaderActive);
-  }
-  if (NS_FAILED(mDecodeTaskQueue->Dispatch(event.forget())) &&
-      mState != DECODER_STATE_SHUTDOWN) {
-    NS_WARNING("Failed to dispatch event to set decoder idle state");
+  if (needIdle) {
+    RefPtr<nsIRunnable> event = NS_NewRunnableMethod(
+        this, &MediaDecoderStateMachine::SetReaderIdle);
+    nsresult rv = mDecodeTaskQueue->Dispatch(event.forget());
+    if (NS_FAILED(rv) && mState != DECODER_STATE_SHUTDOWN) {
+      NS_WARNING("Failed to dispatch event to set decoder idle state");
+    }
   }
 }
 
@@ -1828,7 +1796,6 @@ nsresult MediaDecoderStateMachine::DecodeMetadata()
   if (mState != DECODER_STATE_DECODING_METADATA) {
     return NS_ERROR_FAILURE;
   }
-  EnsureActive();
 
   nsresult res;
   MediaInfo info;
@@ -1930,7 +1897,6 @@ void MediaDecoderStateMachine::DecodeSeek()
   if (mState != DECODER_STATE_SEEKING) {
     return;
   }
-  EnsureActive();
 
   // During the seek, don't have a lock on the decoder state,
   // otherwise long seek operations can block the main thread.

content/media/MediaDecoderStateMachine.h

@@ -565,11 +565,9 @@ protected:
   // The decoder monitor must be held.
   nsresult EnqueueDecodeSeekTask();
 
-  // Calls the reader's SetIdle(), with aIsIdle as parameter. This is only
-  // called in a task dispatched to the decode task queue, don't call it
-  // directly.
+  // Calls the reader's SetIdle(). This is only called in a task dispatched to
+  // the decode task queue, don't call it directly.
   void SetReaderIdle();
-  void SetReaderActive();
 
   // Re-evaluates the state and determines whether we need to dispatch
   // events to run the decode, or if not whether we should set the reader
@@ -577,11 +575,6 @@ protected:
   // The decoder monitor must be held.
   void DispatchDecodeTasksIfNeeded();
 
-  // Called before we do anything on the decode task queue to set the reader
-  // as not idle if it was idle. This is called before we decode, seek, or
-  // decode metadata (in case we were dormant or awaiting resources).
-  void EnsureActive();
-
   // Queries our state to see whether the decode has finished for all streams.
   // If so, we move into DECODER_STATE_COMPLETED and schedule the state machine
   // to run.
@@ -851,12 +844,6 @@ protected:
   // the video decode.
   bool mDispatchedVideoDecodeTask;
 
-  // True when the reader is initialized, but has been ordered "idle" by the
-  // state machine. This happens when the MediaQueue's of decoded data are
-  // "full" and playback is paused. The reader may choose to use the idle
-  // notification to enter a low power state.
-  bool mIsReaderIdle;
-
   // If the video decode is falling behind the audio, we'll start dropping the
   // inter-frames up until the next keyframe which is at or before the current
   // playback position. skipToNextKeyframe is true if we're currently

content/media/mediasource/MediaSourceDecoder.cpp

@@ -147,7 +147,6 @@ private:
         mActiveVideoDecoder = i;
         MSE_DEBUG("%p MSR::DecodeVF switching to %d", this, mActiveVideoDecoder);
 
-        GetVideoReader()->SetActive();
         return true;
       }
     }
@@ -326,7 +325,6 @@ MediaSourceReader::CallDecoderInitialization()
     MediaDecoderReader* reader = decoder->GetReader();
     MSE_DEBUG("%p: Initializating subdecoder %p reader %p", this, decoder.get(), reader);
 
-    reader->SetActive();
     MediaInfo mi;
     nsAutoPtr<MetadataTags> tags; // TODO: Handle metadata.
     nsresult rv;
@@ -456,8 +454,6 @@ MediaSourceReader::ReadMetadata(MediaInfo* aInfo, MetadataTags** aTags)
   for (uint32_t i = 0; i < mDecoders.Length(); ++i) {
     MediaDecoderReader* reader = mDecoders[i]->GetReader();
 
-    reader->SetActive(); // XXX check where this should be called
-
     MediaInfo mi = reader->GetMediaInfo();
 
     if (mi.HasVideo() && !mInfo.HasVideo()) {

content/media/omx/MediaOmxReader.cpp

@@ -47,9 +47,6 @@ MediaOmxReader::MediaOmxReader(AbstractMediaDecoder *aDecoder)
   , mVideoSeekTimeUs(-1)
   , mAudioSeekTimeUs(-1)
   , mSkipCount(0)
-#ifdef DEBUG
-  , mIsActive(true)
-#endif
 {
 #ifdef PR_LOGGING
   if (!gMediaDecoderLog) {
@@ -135,7 +132,7 @@ nsresult MediaOmxReader::ReadMetadata(MediaInfo* aInfo,
                                       MetadataTags** aTags)
 {
   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
-  MOZ_ASSERT(mIsActive);
+  EnsureActive();
 
   *aTags = nullptr;
 
@@ -211,7 +208,8 @@ nsresult MediaOmxReader::ReadMetadata(MediaInfo* aInfo,
 bool MediaOmxReader::DecodeVideoFrame(bool &aKeyframeSkip,
                                       int64_t aTimeThreshold)
 {
-  MOZ_ASSERT(mIsActive);
+  NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
+  EnsureActive();
 
   // Record number of frames decoded and parsed. Automatically update the
   // stats counters using the AutoNotifyDecoded stack-based class.
@@ -341,7 +339,7 @@ void MediaOmxReader::NotifyDataArrived(const char* aBuffer, uint32_t aLength, in
 bool MediaOmxReader::DecodeAudioData()
 {
   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
-  MOZ_ASSERT(mIsActive);
+  EnsureActive();
 
   // This is the approximate byte position in the stream.
   int64_t pos = mDecoder->GetResource()->Tell();
@@ -375,7 +373,7 @@ bool MediaOmxReader::DecodeAudioData()
 nsresult MediaOmxReader::Seek(int64_t aTarget, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime)
 {
   NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
-  MOZ_ASSERT(mIsActive);
+  EnsureActive();
 
   ResetDecode();
   VideoFrameContainer* container = mDecoder->GetVideoFrameContainer();
@@ -410,19 +408,13 @@ static uint64_t BytesToTime(int64_t offset, uint64_t length, uint64_t durationUs
 }
 
 void MediaOmxReader::SetIdle() {
-#ifdef DEBUG
-  mIsActive = false;
-#endif
   if (!mOmxDecoder.get()) {
     return;
   }
   mOmxDecoder->Pause();
 }
 
-void MediaOmxReader::SetActive() {
-#ifdef DEBUG
-  mIsActive = true;
-#endif
+void MediaOmxReader::EnsureActive() {
   if (!mOmxDecoder.get()) {
     return;
   }

content/media/omx/MediaOmxReader.h

@@ -49,6 +49,10 @@ protected:
   // information used for creating OMX decoder such as video/audio codec.
   virtual nsresult InitOmxDecoder();
 
+  // Called inside DecodeVideoFrame, DecodeAudioData, ReadMetadata and Seek
+  // to activate the decoder automatically.
+  virtual void EnsureActive();
+
 public:
   MediaOmxReader(AbstractMediaDecoder* aDecoder);
   ~MediaOmxReader();
@@ -83,7 +87,6 @@ public:
   virtual nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime);
 
   virtual void SetIdle() MOZ_OVERRIDE;
-  virtual void SetActive() MOZ_OVERRIDE;
 
   void SetAudioChannel(dom::AudioChannel aAudioChannel) {
     mAudioChannel = aAudioChannel;
@@ -99,12 +102,6 @@ public:
   // ANDROID_VERSION < 19
   void CheckAudioOffload();
 #endif
-
-private:
-  // This flag is true when SetActive() has been called without a matching
-  // SetIdle(). This is used to sanity check the SetIdle/SetActive calls, to
-  // ensure SetActive has been called before a decode call.
-  DebugOnly<bool> mIsActive;
 };
 
 } // namespace mozilla

content/media/omx/RtspOmxReader.cpp

@@ -299,18 +299,6 @@ nsresult RtspOmxReader::Seek(int64_t aTime, int64_t aStartTime,
   return MediaOmxReader::Seek(aTime, aStartTime, aEndTime, aCurrentTime);
 }
 
-nsresult
-RtspOmxReader::ReadMetadata(MediaInfo* aInfo,
-                            MetadataTags** aTags)
-{
-  SetActive();
-
-  nsresult rv = MediaOmxReader::ReadMetadata(aInfo, aTags);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  return NS_OK;
-}
-
 void RtspOmxReader::SetIdle() {
   // Call parent class to set OMXCodec idle.
   MediaOmxReader::SetIdle();
@@ -326,7 +314,7 @@ void RtspOmxReader::SetIdle() {
   }
 }
 
-void RtspOmxReader::SetActive() {
+void RtspOmxReader::EnsureActive() {
   // Need to start RTSP streaming OMXCodec decoding.
   if (mRtspResource) {
     nsIStreamingProtocolController* controller =
@@ -338,7 +326,7 @@ void RtspOmxReader::SetActive() {
   }
 
   // Call parent class to set OMXCodec active.
-  MediaOmxReader::SetActive();
+  MediaOmxReader::EnsureActive();
 }
 
 } // namespace mozilla

content/media/omx/RtspOmxReader.h

@@ -28,6 +28,7 @@ class RtspOmxReader : public MediaOmxReader
 protected:
   // Provide a Rtsp extractor.
   nsresult InitOmxDecoder() MOZ_FINAL MOZ_OVERRIDE;
+  virtual void EnsureActive() MOZ_OVERRIDE;
 
 public:
   RtspOmxReader(AbstractMediaDecoder* aDecoder)
@@ -44,9 +45,6 @@ public:
     MOZ_COUNT_DTOR(RtspOmxReader);
   }
 
-  virtual nsresult ReadMetadata(MediaInfo* aInfo,
-                                MetadataTags** aTags) MOZ_OVERRIDE;
-
   // Implement a time-based seek instead of byte-based..
   virtual nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime,
                         int64_t aCurrentTime) MOZ_FINAL MOZ_OVERRIDE;
@@ -66,7 +64,6 @@ public:
   }
 
   virtual void SetIdle() MOZ_OVERRIDE;
-  virtual void SetActive() MOZ_OVERRIDE;
 
 private:
   // A pointer to RtspMediaResource for calling the Rtsp specific function.