WebmMuxer-MediaRecorderHandler: thread hopping and data ownership

content/renderer/media/media_recorder_handler.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/media_recorder_handler.h"
 
 #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"
 #include "third_party/WebKit/public/platform/WebMediaRecorderHandlerClient.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 
 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() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   // Send a |last_in_slice| to our |client_|.
   if (client_)
     client_->writeData(nullptr, 0u, true);
 }
 
 bool MediaRecorderHandler::canSupportMimeType(
@@ skipping 22 matching lines @@
 bool MediaRecorderHandler::start() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!recording_);
   return start(0);
 }
 
 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;
   media_stream_.videoTracks(video_tracks);
 
   if (video_tracks.isEmpty()) {
     // TODO(mcasas): Add audio_tracks and update the code in this function
     // correspondingly, see http://crbug.com/528519. As of now, only video
     // tracks are supported.
     LOG(WARNING) << "Recording no video tracks is not implemented";
     return false;
   }
   // TODO(mcasas): The muxer API supports only one video track. Extend it to
   // several video tracks, see http://crbug.com/528523.
   LOG_IF(WARNING, video_tracks.size() > 1u) << "Recording multiple video"
       << " tracks is not implemented.  Only recording first video track.";
   const blink::WebMediaStreamTrack& video_track = video_tracks[0];
   if (video_track.isNull())
     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));
 
   recording_ = true;
   return true;
 }
 
 void MediaRecorderHandler::stop() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(recording_);
 
   recording_ = false;
   video_recorders_.clear();
-  webm_muxer_.reset(NULL);
 }
 
 void MediaRecorderHandler::pause() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(recording_);
   recording_ = false;
   NOTIMPLEMENTED();
 }
 
 void MediaRecorderHandler::resume() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!recording_);
   recording_ = true;
   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(
     const scoped_refptr<media::VideoFrame>& frame,
     const base::TimeTicks& timestamp) {
   for (auto* recorder : video_recorders_)
     recorder->OnVideoFrameForTesting(frame, timestamp);
 }
 
 }  // namespace content