WebmMuxer-MediaRecorderHandler: thread hopping and data ownership

media/capture/webm_muxer.cc

Index: media/capture/webm_muxer.cc
diff --git a/media/capture/webm_muxer.cc b/media/capture/webm_muxer.cc
index b913fa7501416818c31a857084d225fa66e740d1..71bdaa18c06df28d856d1c45dfb36559f148f9cd 100644
--- a/media/capture/webm_muxer.cc
+++ b/media/capture/webm_muxer.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "media/base/limits.h"
 #include "media/base/video_frame.h"
+#include "ui/gfx/geometry/size.h"
 
 namespace media {
 
@@ -34,16 +35,17 @@ WebmMuxer::WebmMuxer(const WriteDataCB& write_data_callback)
 }
 
 WebmMuxer::~WebmMuxer() {
-  // No need to segment_.Finalize() since is not Seekable(), i.e. a live stream,
-  // but is good practice.
+  // No need to segment_.Finalize() since is not Seekable(), i.e. a live
+  // stream, but is a good practice.
+  DCHECK(thread_checker_.CalledOnValidThread());
   segment_.Finalize();
 }
 
 void WebmMuxer::OnEncodedVideo(const scoped_refptr<VideoFrame>& video_frame,
-                               const base::StringPiece& encoded_data,
+                               scoped_ptr<std::string> encoded_data,
                                base::TimeTicks timestamp,
                                bool is_key_frame) {
-  DVLOG(1) << __FUNCTION__ << " - " << encoded_data.size() << "B";
+  DVLOG(1) << __FUNCTION__ << " - " << encoded_data->size() << "B";
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!track_index_) {
     // |track_index_|, cannot be zero (!), initialize WebmMuxer in that case.
@@ -52,8 +54,8 @@ void WebmMuxer::OnEncodedVideo(const scoped_refptr<VideoFrame>& video_frame,
                   GetFrameRate(video_frame));
     first_frame_timestamp_ = timestamp;
   }
-  segment_.AddFrame(reinterpret_cast<const uint8_t*>(encoded_data.data()),
-                    encoded_data.size(),
+  segment_.AddFrame(reinterpret_cast<const uint8_t*>(encoded_data->data()),
+                    encoded_data->size(),
                     track_index_,
                     (timestamp - first_frame_timestamp_).InMicroseconds() *
                         base::Time::kNanosecondsPerMicrosecond,
@@ -97,8 +99,8 @@ void WebmMuxer::AddVideoTrack(const gfx::Size& frame_size, double frame_rate) {
 mkvmuxer::int32 WebmMuxer::Write(const void* buf, mkvmuxer::uint32 len) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(buf);
-  write_data_callback_.Run(base::StringPiece(reinterpret_cast<const char*>(buf),
-                                             len));
+  write_data_callback_.Run(scoped_ptr<std::string>(

[miu, 2015/09/22 04:11:08]

The extra copy here is unnecessary since the call is synchronous and not a task
posted across threads.  You should just go back to passing the StringPiece.

[mcasas, 2015/09/22 16:58:41]

Done.

FWIW I find StringPiece semantics a bit confusing
since they relate to String, but StringPiece does not
copy contents :?

[miu, 2015/09/22 19:10:06]

The purpose of StringPiece is to reference substrings instead of making and
passing a copy.

Docs:
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

[mcasas, 2015/09/22 20:20:04]

Heh thanks for the reference.

What I meant is that base::StringPiece and std::string are similar,
since they share e.g. the constructor with a char* and a length, 
but base::StringPiece does not copy the contents pointed by
the said first argument. Perhaps I'd like to see it called, e.g.
StringPieceRef or similar, to indicate that StringPiece is not a
string itself, but a reference to one :) 
IOW the documentation is nice, but having a name that reveals
intent is much better.

+      new std::string(reinterpret_cast<const char*>(buf), len)));
   position_ += len;
   return 0;
 }