Bug 804387. Part 1.5: Clean up main-thread MediaStream event listeners. r=jesup

There is no need for these to be independent objects in general and we
don't need to addref/release them. We can just require the caller to
remove them before they die.
We can also save some refcount churn by having
DispatchToMainThreadAfterStreamStateUpdate take already_AddRefed.

--HG--
extra : rebase_source : 751114a1befd73b405dff3ee986496efb6f76201

content/html/content/src/nsHTMLMediaElement.cpp

@@ -2450,13 +2450,13 @@ public:
     } else {
       event = NS_NewRunnableMethod(this, &StreamListener::DoNotifyUnblocked);
     }
-    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event);
+    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event.forget());
   }
   virtual void NotifyFinished(MediaStreamGraph* aGraph)
   {
     nsCOMPtr<nsIRunnable> event =
       NS_NewRunnableMethod(this, &StreamListener::DoNotifyFinished);
-    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event);
+    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event.forget());
   }
   virtual void NotifyHasCurrentData(MediaStreamGraph* aGraph,
                                     bool aHasCurrentData)
@@ -2471,7 +2471,7 @@ public:
     if (aHasCurrentData) {
       nsCOMPtr<nsIRunnable> event =
         NS_NewRunnableMethod(this, &StreamListener::DoNotifyHaveCurrentData);
-      aGraph->DispatchToMainThreadAfterStreamStateUpdate(event);
+      aGraph->DispatchToMainThreadAfterStreamStateUpdate(event.forget());
     }
   }
   virtual void NotifyOutput(MediaStreamGraph* aGraph)
@@ -2482,7 +2482,7 @@ public:
     mPendingNotifyOutput = true;
     nsCOMPtr<nsIRunnable> event =
       NS_NewRunnableMethod(this, &StreamListener::DoNotifyOutput);
-    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event);
+    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event.forget());
   }
 
 private:

content/media/MediaDecoder.cpp

@@ -157,30 +157,36 @@ void MediaDecoder::ConnectDecodedStreamToOutputStream(OutputStreamData* aStream)
 }
 
 MediaDecoder::DecodedStreamData::DecodedStreamData(MediaDecoder* aDecoder,
-                                                       int64_t aInitialTime,
+                                                   int64_t aInitialTime,
-                                                       SourceMediaStream* aStream)
+                                                   SourceMediaStream* aStream)
   : mLastAudioPacketTime(-1),
     mLastAudioPacketEndTime(-1),
     mAudioFramesWritten(0),
     mInitialTime(aInitialTime),
     mNextVideoTime(aInitialTime),
+    mDecoder(aDecoder),
     mStreamInitialized(false),
     mHaveSentFinish(false),
     mHaveSentFinishAudio(false),
     mHaveSentFinishVideo(false),
     mStream(aStream),
-    mMainThreadListener(new DecodedStreamMainThreadListener(aDecoder)),
     mHaveBlockedForPlayState(false)
 {
-  mStream->AddMainThreadListener(mMainThreadListener);
+  mStream->AddMainThreadListener(this);
 }
 
 MediaDecoder::DecodedStreamData::~DecodedStreamData()
 {
-  mStream->RemoveMainThreadListener(mMainThreadListener);
+  mStream->RemoveMainThreadListener(this);
   mStream->Destroy();
 }
 
+void
+MediaDecoder::DecodedStreamData::NotifyMainThreadStateChanged()
+{
+  mDecoder->NotifyDecodedStreamMainThreadStateChanged();
+}
+
 void MediaDecoder::DestroyDecodedStream()
 {
   MOZ_ASSERT(NS_IsMainThread());

content/media/MediaDecoder.h

@@ -235,7 +235,6 @@ class MediaDecoder : public nsIObserver,
 {
 public:
   typedef mozilla::layers::Image Image;
-  class DecodedStreamMainThreadListener;
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIOBSERVER
@@ -340,7 +339,7 @@ public:
   // replaying after the input as ended. In the latter case, the new source is
   // not connected to streams created by captureStreamUntilEnded.
 
-  struct DecodedStreamData {
+  struct DecodedStreamData MOZ_FINAL : public MainThreadMediaStreamListener {
     DecodedStreamData(MediaDecoder* aDecoder,
                       int64_t aInitialTime, SourceMediaStream* aStream);
     ~DecodedStreamData();
@@ -358,6 +357,7 @@ public:
     // Therefore video packets starting at or after this time need to be copied
     // to the output stream.
     int64_t mNextVideoTime; // microseconds
+    MediaDecoder* mDecoder;
     // The last video image sent to the stream. Useful if we need to replicate
     // the image.
     nsRefPtr<Image> mLastVideoImage;
@@ -372,12 +372,11 @@ public:
     // The decoder is responsible for calling Destroy() on this stream.
     // Can be read from any thread.
     const nsRefPtr<SourceMediaStream> mStream;
-    // A listener object that receives notifications when mStream's
-    // main-thread-visible state changes. Used on the main thread only.
-    const nsRefPtr<DecodedStreamMainThreadListener> mMainThreadListener;
     // True when we've explicitly blocked this stream because we're
     // not in PLAY_STATE_PLAYING. Used on the main thread only.
     bool mHaveBlockedForPlayState;
+
+    virtual void NotifyMainThreadStateChanged() MOZ_OVERRIDE;
   };
   struct OutputStreamData {
     void Init(ProcessedMediaStream* aStream, bool aFinishWhenEnded)
@@ -421,16 +420,6 @@ public:
     GetReentrantMonitor().AssertCurrentThreadIn();
     return mDecodedStream;
   }
-  class DecodedStreamMainThreadListener : public MainThreadMediaStreamListener {
-  public:
-    DecodedStreamMainThreadListener(MediaDecoder* aDecoder)
-      : mDecoder(aDecoder) {}
-    virtual void NotifyMainThreadStateChanged()
-    {
-      mDecoder->NotifyDecodedStreamMainThreadStateChanged();
-    }
-    MediaDecoder* mDecoder;
-  };
 
   // Add an output stream. All decoder output will be sent to the stream.
   // The stream is initially blocked. The decoder is responsible for unblocking

content/media/MediaStreamGraph.h

@@ -172,19 +172,16 @@ public:
  * This callback is invoked on the main thread when the main-thread-visible
  * state of a stream has changed.
  *
- * These methods are called without the media graph monitor held, so
+ * These methods are called with the media graph monitor held, so
- * reentry into media graph methods is possible, although very much discouraged!
+ * reentry into general media graph methods is not possible.
  * You should do something non-blocking and non-reentrant (e.g. dispatch an
- * event) and return.
+ * event) and return. DispatchFromMainThreadAfterNextStreamStateUpdate
+ * would be a good choice.
  * The listener is allowed to synchronously remove itself from the stream, but
  * not add or remove any other listeners.
  */
 class MainThreadMediaStreamListener {
 public:
-  virtual ~MainThreadMediaStreamListener() {}
-
-  NS_INLINE_DECL_REFCOUNTING(MainThreadMediaStreamListener)
-
   virtual void NotifyMainThreadStateChanged() = 0;
 };
 
@@ -274,7 +271,12 @@ public:
     , mMainThreadDestroyed(false)
   {
   }
-  virtual ~MediaStream() {}
+  virtual ~MediaStream()
+  {
+    NS_ASSERTION(mMainThreadDestroyed, "Should have been destroyed already");
+    NS_ASSERTION(mMainThreadListeners.IsEmpty(),
+                 "All main thread listeners should have been removed");
+  }
 
   /**
    * Returns the graph that owns this stream.
@@ -303,11 +305,15 @@ public:
   // Events will be dispatched by calling methods of aListener.
   void AddListener(MediaStreamListener* aListener);
   void RemoveListener(MediaStreamListener* aListener);
+  // Events will be dispatched by calling methods of aListener. It is the
+  // responsibility of the caller to remove aListener before it is destroyed.
   void AddMainThreadListener(MainThreadMediaStreamListener* aListener)
   {
     NS_ASSERTION(NS_IsMainThread(), "Call only on main thread");
     mMainThreadListeners.AppendElement(aListener);
   }
+  // It's safe to call this even if aListener is not currently a listener;
+  // the call will be ignored.
   void RemoveMainThreadListener(MainThreadMediaStreamListener* aListener)
   {
     NS_ASSERTION(NS_IsMainThread(), "Call only on main thread");
@@ -424,7 +430,7 @@ protected:
   // API, minus the number of times it has been explicitly unblocked.
   TimeVarying<GraphTime,uint32_t> mExplicitBlockerCount;
   nsTArray<nsRefPtr<MediaStreamListener> > mListeners;
-  nsTArray<nsRefPtr<MainThreadMediaStreamListener> > mMainThreadListeners;
+  nsTArray<MainThreadMediaStreamListener*> mMainThreadListeners;
 
   // Precomputed blocking status (over GraphTime).
   // This is only valid between the graph's mCurrentTime and
@@ -838,9 +844,9 @@ public:
    * main-thread stream state has been next updated.
    * Should only be called during MediaStreamListener callbacks.
    */
-  void DispatchToMainThreadAfterStreamStateUpdate(nsIRunnable* aRunnable)
+  void DispatchToMainThreadAfterStreamStateUpdate(already_AddRefed<nsIRunnable> aRunnable)
   {
-    mPendingUpdateRunnables.AppendElement(aRunnable);
+    *mPendingUpdateRunnables.AppendElement() = aRunnable;
   }
 
 protected: