MediaStream: Adding VideoTrackRecorder class and unittests

content/renderer/media/video_track_recorder.h

Index: content/renderer/media/video_track_recorder.h
diff --git a/content/renderer/media/video_track_recorder.h b/content/renderer/media/video_track_recorder.h
new file mode 100644
index 0000000000000000000000000000000000000000..97cd8b8a31e93a55828db943d07f137e00097e8e
--- /dev/null
+++ b/content/renderer/media/video_track_recorder.h
@@ -0,0 +1,61 @@
+// 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.
+
+#ifndef CONTENT_RENDERER_MEDIA_VIDEO_TRACK_RECORDER_H_
+#define CONTENT_RENDERER_MEDIA_VIDEO_TRACK_RECORDER_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/strings/string_piece.h"
+#include "base/threading/thread_checker.h"
+#include "content/public/renderer/media_stream_video_sink.h"
+#include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
+#include "ui/gfx/geometry/size.h"
+
+namespace content {
+
+// VideoTrackRecorder is a MediaStreamVideoSink that encodes the video frames
+// received from a Stream Video Track. The class is constructed and used on a
+// single thread, namely the main Render thread. It has an internal VpxEncoder
+// living on Render IO thread and using an encoding thread.

[miu, 2015/07/27 22:30:09]

1. This should not be done on the IO thread.

2. Instead of naming the IO thread as the thread where things must run, why not
just specify that users of this class need to provide a thread for encoding? 
Meaning, you can provide a task runner to the ctor (just like you are doing
now), but call it encode_task_runner.  Said another way: It looks like you
should just change the naming for all variables and comments from "IO thread" to
"encode thread."

[mcasas, 2015/07/31 11:26:27]

What I meant is that callbacks to the "internal VpxEncoder" happen
on IO thread. This "internal VpxEncoder" has a worker thread inside.
There are, thus, 3 threads involved in this party (!).

I changed VpxEncoder to be more lenient on the threads: now it caches
the VideoFrame delivery thread (which should be the IO thread) and
reuses it for callback execution -- instead of getting it on constructor. 

+class CONTENT_EXPORT VideoTrackRecorder
+    : NON_EXPORTED_BASE(public MediaStreamVideoSink) {
+ public:
+  using OnFirstFrameCB = base::Callback<uint64_t(const gfx::Size& frame_size,
+                                                 double frame_rate)>;
+
+  using OnEncodedVideoCB = base::Callback<void(uint64_t track_number,

[miu, 2015/07/27 22:30:09]

|track_number| does not belong in this API nor this class.  You should curry the
value by binding the track number in a callback that becomes an
OnEncodedVideoCB.

[mcasas, 2015/07/31 11:26:27]

I'm not sure I understand. Do you mean OnFirstFrameCB should
return a OnEncodedVideoCB() with the first parameter bound, or
something else?

[miu, 2015/08/04 04:06:30]

Sorry.  I wasn't very clear here.  Basically, I think you're letting the details
of your WebM muxer have too much influence on the interface in this class.  In
particular, VideoTrackRecorder doesn't care about |track_number| at all.

My suggestion: Get rid of OnFirstFrameCB, and define OnEncodedVideoCB as:

  using OnEncodedVideoCB = base::Callback<void(
      const scoped_refptr<media::VideoFrame>& source_frame,
      const base::StringPiece& encoded_data,
      base::TimeTicks reference_timestamp,
      bool is_key_frame)>;

Then, your client code would do something like:

class MyClient {
 public:
  MyClient(io_task_runner, const blink::WebMediaStreamTrack& track)
    : webm_muxer_track_weak_factory_(this) {
      recorder_.reset(new VideoTrackRecorder(io_task_runner, track,
          base::Bind(&MyClient::OnEncodedFrame,
                     weak_factory_.GetWeakPtr()));
  }

 private:
  void OnEncodedFrame(const scoped_refptr<media::VideoFrame>& source_frame,
                      const base::StringPiece& encoded_data,
                      base::TimeTicks reference_timestamp,
                      bool is_key_frame) {
    if (!muxer_) {  // This is the first frame.
      muxer_.reset(new WebmMuxer());
      double frame_rate = 0.0;
      source_frame->metadata()->GetDouble(FRAME_RATE, &frame_rate);
      track_number_ = muxer_->AddVideoTrack(source_frame->visible_rect().size(),
                                            frame_rate);
      first_frame_timestamp_ = reference_timestamp;
    }

    muxer_->OnEncodedVideo(track_number_, encoded_data,
                           reference_timestamp - first_frame_timestamp_,
                           is_key_frame);
  }
  
  scoped_ptr<VideoTrackRecorder> recorder_;

  scoped_ptr<WebmMuxer> muxer_;
  uint64_t track_number_;
  base::TimeTicks first_frame_timestamp_;

  base::WeakPtrFactory<MyClient> weak_factory_;
};

[mcasas, 2015/08/11 09:35:17]

I agree that the |track_index| should not be surfaced
to the client. Patchset 5 follows your advice with a
twist: the OnFirstFrameCB returns and
OnEncodedVideoCB with the first parameter bound,
since the WebmMuxer does need this |track_index|
to differentiate between multiple clients. But the index
is bound where it belongs, i.e. in the CB object and 
not in the VideoTrackRecorder.

+                                               const base::StringPiece& data,
+                                               base::TimeDelta timestamp,
+                                               bool keyframe)>;
+
+  VideoTrackRecorder(
+      const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
+      const blink::WebMediaStreamTrack& track,
+      const OnFirstFrameCB& on_first_frame_cb,
+      const OnEncodedVideoCB& on_encoded_video_cb);
+  ~VideoTrackRecorder() override;
+
+ private:
+  friend class VideoTrackRecorderTest;
+
+  // Used to ping |encoder_->EncodeOnIo()| for testing.
+  void EncodeOnIoForTesting(const scoped_refptr<media::VideoFrame>& frame,
+                            const base::TimeTicks& estimated_capture_time);
+
+  // Used to check that we are destroyed on the same thread we were created.
+  base::ThreadChecker main_render_thread_checker_;
+
+  // We need to hold on to the Blink track to remove ourselves on dtor.
+  const blink::WebMediaStreamTrack track_;
+
+  // Forward declaration and member of an inner class to encode using VPx.
+  class VpxEncoder;
+  const scoped_refptr<VpxEncoder> encoder_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoTrackRecorder);
+};
+
+}  // namespace content
+#endif  // CONTENT_RENDERER_MEDIA_VIDEO_TRACK_RECORDER_H_