WebmMuxer-MediaRecorderHandler: thread hopping and data ownership

content/renderer/media/media_recorder_handler.cc

Index: content/renderer/media/media_recorder_handler.cc
diff --git a/content/renderer/media/media_recorder_handler.cc b/content/renderer/media/media_recorder_handler.cc
index 5934ef9fd11ee121a05e63fa12dcb506736b2ce8..cb5843d4bd16fe56b8560138a804b421e647e9fc 100644
--- a/content/renderer/media/media_recorder_handler.cc
+++ b/content/renderer/media/media_recorder_handler.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/strings/stringprintf.h"
 #include "content/renderer/media/video_track_recorder.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/capture/webm_muxer.h"
@@ -18,6 +19,11 @@ namespace content {
 MediaRecorderHandler::MediaRecorderHandler()
     : recording_(false), client_(nullptr), weak_factory_(this) {
   DVLOG(3) << __FUNCTION__;
+  webm_muxer_ =
+      new media::WebmMuxer(base::MessageLoop::current()->task_runner(),
+                           media::BindToCurrentLoop(
+                               base::Bind(&MediaRecorderHandler::WriteData,
+                               weak_factory_.GetWeakPtr())));
 }
 
 MediaRecorderHandler::~MediaRecorderHandler() {
@@ -60,9 +66,6 @@ bool MediaRecorderHandler::start(int timeslice) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!recording_);
   DCHECK(!media_stream_.isNull());
-
-  webm_muxer_.reset(new media::WebmMuxer(media::BindToCurrentLoop(base::Bind(
-      &MediaRecorderHandler::WriteData, weak_factory_.GetWeakPtr()))));
   DCHECK(webm_muxer_);
 
   blink::WebVector<blink::WebMediaStreamTrack> video_tracks;
@@ -84,8 +87,7 @@ bool MediaRecorderHandler::start(int timeslice) {
     return false;
 
   const VideoTrackRecorder::OnEncodedVideoCB on_encoded_video_cb =
-      base::Bind(&media::WebmMuxer::OnEncodedVideo,
-                 base::Unretained(webm_muxer_.get()));
+      base::Bind(&media::WebmMuxer::OnEncodedVideo, webm_muxer_);

[miu, 2015/09/21 19:39:43]

This does not call for ref-counting WebmMuxer.  Why not:

  media::BindToCurrentLoop(
      base::Bind(&MediaRecorderHandler::OnEncodedVideo,
                 weak_factory_.GetWeakPtr()))

...and then...

void MediaRecorderHandler::OnEncodedVideo(...) {
  DCHECK(main_render_thread_checker_.CalledOnValidThread());
  if (webm_muxer_)
    webm_muxer_->OnEncodedVideo(...);
}

[mcasas, 2015/09/21 20:22:18]

Yes, that's another possibility, essentially bouncing on 
MediaRecorderHandler to keep WebmMuxer not ref-counted,
and I considered this indeed.

But then we'd be using MediaRecorderHandler::weak_factory_
for PostTasks being used from Render IO thread, right?
Note that VideoTrackRecorder lives on and sends frames from 
the Renderer IO thread, and thus 
base::Bind(&MediaRecorderHandler::OnEncodedVideo,
                 weak_factory_.GetWeakPtr())
would be executed on IO thread, whereas |weak_factory_|
can only be invalidated on Main Render Thread. And we're
back to the situation before

(Note: |weak_factory_| is there for doing the jump between
WebmMuxer and its callback MediaRecorderHandler::WriteData(),
all of those on Main Render thread.

IOW, since we have
- creation of stuff on thread A
- bindings for frame callbacks on thread B
- a final hoop from thread B to thread A
someone has to be RefCountedThreadSafe so that frames
in flight are correctly processed while destruction is happening,
and that happens to be the place where the jump from A->B is 
done, currently WebmMuxer. Or do you propose to have a larger 
change, move WebmMuxer to Main Render thread, and make 
MediaRecorderHandler fully ref counted?

[miu, 2015/09/21 20:57:12]

That's why, in my example, I wrapped the base::Bind() in a
media::BindToCurrentLoop().  This means:

1. The render IO thread runs the callback provided by BindToCurrentLoop().  This
causes a task with the inner callback to be posted to the main thread.

2. The main thread checks the weak pointer is valid and, if so, calls
MediaRecorderHander::OnEncodedVideo().

3. Profit.

 
   video_recorders_.push_back(new VideoTrackRecorder(video_track,
                                                     on_encoded_video_cb));
@@ -100,7 +102,6 @@ void MediaRecorderHandler::stop() {
 
   recording_ = false;
   video_recorders_.clear();
-  webm_muxer_.reset(NULL);
 }
 
 void MediaRecorderHandler::pause() {
@@ -117,9 +118,9 @@ void MediaRecorderHandler::resume() {
   NOTIMPLEMENTED();
 }
 
-void MediaRecorderHandler::WriteData(const base::StringPiece& data) {
+void MediaRecorderHandler::WriteData(scoped_ptr<std::string> data) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
-  client_->writeData(data.data(), data.size(), false  /* lastInSlice */);
+  client_->writeData(data->data(), data->length(), false  /* lastInSlice */);
 }
 
 void MediaRecorderHandler::OnVideoFrameForTesting(