MediaStream: Adding VideoTrackRecorder class and unittests

content/renderer/media/video_track_recorder.cc

Index: content/renderer/media/video_track_recorder.cc
diff --git a/content/renderer/media/video_track_recorder.cc b/content/renderer/media/video_track_recorder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6ee57c91f8cf73e17709daea277400ad64052045
--- /dev/null
+++ b/content/renderer/media/video_track_recorder.cc
@@ -0,0 +1,292 @@
+// 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/video_track_recorder.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/threading/thread.h"
+#include "base/time/time.h"
+#include "base/trace_event/trace_event.h"
+#include "content/child/child_process.h"
+#include "media/base/bind_to_current_loop.h"
+#include "media/base/video_frame.h"
+
+extern "C" {
+// VPX_CODEC_DISABLE_COMPAT excludes parts of the libvpx API that provide
+// backwards compatibility for legacy applications using the library.
+#define VPX_CODEC_DISABLE_COMPAT 1
+#include "third_party/libvpx/source/libvpx/vpx/vp8cx.h"
+#include "third_party/libvpx/source/libvpx/vpx/vpx_encoder.h"
+}
+
+using media::VideoFrame;
+using media::VideoFrameMetadata;
+
+namespace content {
+
+namespace {
+const vpx_codec_flags_t kNoFlags = 0;
+}  // anonymous namespace
+
+// Inner class encapsulating all libvpx interactions and the encoding+delivery
+// of received frames. This class is:
+// - created and destroyed on its parent's thread (usually the main render
+// thread),
+// - receives VideoFrames and Run()s the callbacks on another thread (supposedly
+// the render IO thread), which is cached on first frame arrival,
+// - uses an internal |encoding_thread_| for libvpx interactions, notably for
+// encoding (which might take some time).
+// Only VP8 is supported for the time being.
+class VideoTrackRecorder::VpxEncoder
+    : public base::RefCountedThreadSafe<VpxEncoder> {
+ public:
+  VpxEncoder(const OnEncodedVideoCB& on_encoded_video_callback);

[miu, 2015/08/18 02:08:35]

Need explicit keyword here.

[mcasas, 2015/08/19 00:16:30]

Done.

+
+  void StartFrameEncode(const scoped_refptr<VideoFrame>& frame,
+                        const base::TimeTicks& capture_timestamp);
+
+ private:
+  friend class base::RefCountedThreadSafe<VpxEncoder>;
+  virtual ~VpxEncoder();
+
+  void EncodeOnEncodingThread(const scoped_refptr<VideoFrame>& frame,
+                              const base::TimeTicks& capture_timestamp);
+
+  void OnFrameEncodeCompleted(const scoped_refptr<VideoFrame>& frame,
+                              scoped_ptr<std::string> data,
+                              const base::TimeTicks& capture_timestamp,
+                              bool keyframe);
+
+  void ConfigureVp8Encoding(const gfx::Size& size);
+
+  // Returns true if |codec_config_| has been filled in at least once.
+  bool IsInitialized() const;
+
+  // Estimate the frame duration from |frame| and |last_frame_timestamp_|.
+  base::TimeDelta CalculateFrameDuration(
+      const scoped_refptr<VideoFrame>& frame);
+
+  // Used to check that we are destroyed on the same thread we were created.
+  base::ThreadChecker main_render_thread_checker_;
+
+  // Task runner where frames to encode and reply callbacks must happen.
+  scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_;
+
+  // This callback should be exercised on IO thread.
+  const OnEncodedVideoCB on_encoded_video_callback_;
+
+  // Thread for encoding. Active as long as VpxEncoder exists. All variables
+  // below this are used in this thread.
+  base::Thread encoding_thread_;
+  // VP8 internal objects: configuration, encoder and Vpx Image wrapper.
+  vpx_codec_enc_cfg_t codec_config_;
+  vpx_codec_ctx_t encoder_;
+
+  // The |VideoFrame::timestamp()| of the last encoded frame.  This is used to
+  // predict the duration of the next frame.
+  base::TimeDelta last_frame_timestamp_;
+};
+
+VideoTrackRecorder::VpxEncoder::VpxEncoder(
+    const OnEncodedVideoCB& on_encoded_video_callback)
+    : on_encoded_video_callback_(on_encoded_video_callback),
+      encoding_thread_("EncodingThread") {
+  DCHECK(!on_encoded_video_callback_.is_null());
+
+  codec_config_.g_timebase.den = 0;  // Not initialized.
+
+  DCHECK(!encoding_thread_.IsRunning());
+  encoding_thread_.Start();
+}
+
+void VideoTrackRecorder::VpxEncoder::StartFrameEncode(
+    const scoped_refptr<VideoFrame>& frame,
+    const base::TimeTicks& capture_timestamp) {
+  // Cache the thread sending frames on first frame arrival.
+  if (!origin_task_runner_.get())
+    origin_task_runner_ = base::MessageLoop::current()->task_runner();
+  DCHECK(origin_task_runner_->BelongsToCurrentThread());
+
+  encoding_thread_.task_runner()->PostTask(FROM_HERE,
+      base::Bind(&VpxEncoder::EncodeOnEncodingThread,
+                 this, frame, capture_timestamp));
+}
+
+VideoTrackRecorder::VpxEncoder::~VpxEncoder() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());

[miu, 2015/08/18 02:08:35]

This class is ref-counted.  Therefore, there's no guarantee this dtor will be
run on the main_render_thread_.  Perhaps you should remove the ref-counting? 
You may need to use WeakPtrs for posting tasks to the encoding_thread_.

[mcasas, 2015/08/19 00:16:30]

Several references are held to VpxEncoder:
1. VideoTrackRecorder has one, on Main Render thread.
2. the VideoCaptureDeliverFrameCB in l.278 holds another
3. VpxEncoder internal PostTask()'s, one for jumping to
 EncodeOnEncodingThread and one for jumping back to
 whichever thread called it (render IO thread).

I think the problem comes from 2., which is the thread
we don't control and that could keep VpxEncoder alive
beyond VTR and its own PostTask. 

So I propose a similar solution to the other MSVSinks,
that is, adding an intermediate step in VTR, namely an
OnVideoFrame() which will forward to VpxEncoder
if some marker does not prevent that from happening.
I'm using |track_.is_null()| as the marker. WDYT?

(CastVideoSink [1] is such a VideoSink, uses a bool |sink_added_|
as marker)
(WebRtcVideoTrackAdapter has a similar approach
but is more complex due to jingle interactions).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me...

[miu, 2015/08/19 19:43:45]

It looks like #3 is the only remaining issue.  And, you don't need ref-counting
for that.  See comment below.

[mcasas, 2015/08/20 00:37:04]

Done.

+  DCHECK(encoding_thread_.IsRunning());
+  encoding_thread_.Stop();
+}

[miu, 2015/08/18 02:08:35]

You need to call vpx_codec_destroy() if the encoder was initialized.  I'm not
sure whether you'll need to either:

1. Block the current thread with a base::WaitableEvent, and hop over to the
encoding_thread_ to call vpx_codec_destroy()

2. After encoding_thread_.Stop(), call vpx_codec_destroy().

#2 is much simpler, but #1 may be necessary if libvpx needs to join on the
threads it creates for itself internally.

[mcasas, 2015/08/19 00:16:30]

I've been digging down the vpx_codec_destroy() f pointers and
vp8e_destroy() [1] doesn't seem to be doing anything specific
about the threads, so +1 for 2.

tomfinegan@ WDYT?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx...

[Tom Finegan, 2015/08/19 04:20:10]

2 is fine unless you're going to end up calling vpx_codec_destroy() while your
encode thread is already waiting on a call to vpx_codec_encode() or
vpx_codec_get_cx_data(), then I'm not sure-- I suspect you'd end up stuck
waiting on a join when vpx_thread cleans up its workers. If your encoding thread
has already been joined, and you're certain a thread isn't already waiting on
the encoder, 2 is safe.

Is the concern a stall while inside vpx_codec_destroy() while waiting on it to
shut down?

[miu, 2015/08/19 19:43:45]

My concern is that the thread calling vpx_codec_enc_init() and
vpx_codec_encode() may cause libvpx to spawn one or more threads to speed up
encoding.  My understanding is that these threads remain alive between
invocations of vpx_codec_encode(), as it would be silly to spawn *and* join on
them for each call to vpx_codec_encode().  Thus, I'm assuming
vpx_codec_destroy() is where the threads are shut down.

The issue: Unless libvpx does something weird, shouldn't these encode threads be
joined with the thread that originally spawned them?

[Tom Finegan, 2015/08/19 20:04:11]

The main thread waits for all workers to complete before returning from an
encode call. There is no internal event loop running inside libvpx. 

vpx_codec_destroy will join _and_ destroy the threads. Note, this behavior is
implemented differently in VP8 and VP9.

For VP8, see:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx...

For VP9, see:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx...
For VP9 Libvpx spawns workers on a per tile basis. Number of workers is tiles -
1:

- main thread (yours) gives tiles - 1 tiles to workers (one to each) for
processing
- main thread works on remaining tile, then waits for workers to finish

This is when libvpx returns from vpx_codec_encode(). The threads are idle when
your main thread is not executing within the vpx_codec_encode() call.

The basic rule (threads idle when not encoding) holds for VP8, although the
count is not tile based and the implementation is different because of
differences between the codecs... Anyway, this is going pretty far back, but
iirc it's macro block row based with a sync range (basically how many rows
belong to each worker, and where they must align).

+
+void VideoTrackRecorder::VpxEncoder::EncodeOnEncodingThread(
+    const scoped_refptr<VideoFrame>& frame,
+    const base::TimeTicks& capture_timestamp) {
+  TRACE_EVENT0("video",
+               "VideoTrackRecorder::VpxEncoder::EncodeOnEncodingThread");
+  DCHECK(encoding_thread_.task_runner()->BelongsToCurrentThread());
+
+  const gfx::Size frame_size = frame->visible_rect().size();
+  if (!IsInitialized() ||
+      gfx::Size(codec_config_.g_w, codec_config_.g_h) != frame_size) {
+    ConfigureVp8Encoding(frame_size);
+  }
+
+  vpx_image_t vpx_image;
+  vpx_image_t* const result = vpx_img_wrap(&vpx_image,
+                                           VPX_IMG_FMT_I420,
+                                           frame_size.width(),
+                                           frame_size.height(),
+                                           1  /* align */,
+                                           frame->data(VideoFrame::kYPlane));
+  DCHECK_EQ(result, &vpx_image);
+  vpx_image.planes[VPX_PLANE_Y] = frame->visible_data(VideoFrame::kYPlane);
+  vpx_image.planes[VPX_PLANE_U] = frame->visible_data(VideoFrame::kUPlane);
+  vpx_image.planes[VPX_PLANE_V] = frame->visible_data(VideoFrame::kVPlane);
+  vpx_image.stride[VPX_PLANE_Y] = frame->stride(VideoFrame::kYPlane);
+  vpx_image.stride[VPX_PLANE_U] = frame->stride(VideoFrame::kUPlane);
+  vpx_image.stride[VPX_PLANE_V] = frame->stride(VideoFrame::kVPlane);
+
+  const base::TimeDelta duration = CalculateFrameDuration(frame);
+  // Encode the frame.  The presentation time stamp argument here is fixed to
+  // zero to force the encoder to base its single-frame bandwidth calculations
+  // entirely on |predicted_frame_duration|.
+  const vpx_codec_err_t ret = vpx_codec_encode(&encoder_,
+                                               &vpx_image,
+                                               0  /* pts */,
+                                               duration.InMicroseconds(),
+                                               kNoFlags,
+                                               VPX_DL_REALTIME);
+  DCHECK_EQ(ret, VPX_CODEC_OK) << vpx_codec_err_to_string(ret) << ", #"
+                               << vpx_codec_error(&encoder_) << " -"
+                               << vpx_codec_error_detail(&encoder_);
+
+  scoped_ptr<std::string> data(new std::string);
+  bool keyframe = false;
+  vpx_codec_iter_t iter = NULL;
+  const vpx_codec_cx_pkt_t* pkt = NULL;
+  while ((pkt = vpx_codec_get_cx_data(&encoder_, &iter)) != NULL) {
+    if (pkt->kind != VPX_CODEC_CX_FRAME_PKT)
+      continue;
+    data->assign(static_cast<char*>(pkt->data.frame.buf), pkt->data.frame.sz);
+    keyframe = (pkt->data.frame.flags & VPX_FRAME_IS_KEY) != 0;
+    break;
+  }
+  origin_task_runner_->PostTask(FROM_HERE,
+      base::Bind(&VpxEncoder::OnFrameEncodeCompleted,
+                 this,
+                 frame,
+                 base::Passed(&data),
+                 capture_timestamp,
+                 keyframe));
+}
+
+void VideoTrackRecorder::VpxEncoder::OnFrameEncodeCompleted(
+    const scoped_refptr<VideoFrame>& frame,
+    scoped_ptr<std::string> data,
+    const base::TimeTicks& capture_timestamp,
+    bool keyframe) {
+  DVLOG(1) << (keyframe ? "" : "non ") << "keyframe "
+           << capture_timestamp << " ms - " << data->length() << "B ";
+  DCHECK(origin_task_runner_->BelongsToCurrentThread());
+  on_encoded_video_callback_.Run(frame,
+                                 base::StringPiece(*data),
+                                 capture_timestamp,
+                                 keyframe);
+}
+
+void VideoTrackRecorder::VpxEncoder::ConfigureVp8Encoding(
+    const gfx::Size& size) {
+  if (IsInitialized()) {
+    // TODO(mcasas): Workaround for certain bug.

[miu, 2015/08/18 02:08:35]

What bug?  ;)

[mcasas, 2015/08/19 00:16:30]

Oops, sorry, both this TODO and the commented out l.206
was during experiments to get the ut working, and forgot to
update them to reflect [1]. Pardon!

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/...

+    DVLOG(1) << "Destroying/Re-Creating encoder for larger frame size: "
+             << gfx::Size(codec_config_.g_w, codec_config_.g_h).ToString()
+             << " --> " << size.ToString();
+    //vpx_codec_destroy(&encoder_);

[miu, 2015/08/18 02:08:35]

Is this supposed to be commented out?

[mcasas, 2015/08/19 00:16:29]

Acknowledged.

+  }
+  const vpx_codec_iface_t* interface = vpx_codec_vp8_cx();
+  vpx_codec_enc_config_default(interface, &codec_config_, 0 /* reserved */);
+
+  // Adjust default bit rate to account for the actual size.
+  codec_config_.rc_target_bitrate = size.width() * size.height() *

[miu, 2015/08/18 02:08:35]

nit: size.GetArea() would be cleaner instead of width*height.

[mcasas, 2015/08/19 00:16:29]

Done.

+                                    codec_config_.rc_target_bitrate /

[miu, 2015/08/18 02:08:35]

IMO, I'd be a bit nervous about the default codec values changing out from
underneath you.  Either DCHECK() the assumptions, or define constants in this
file.

FWICT, this math does reasonable results based on the current libvpx defaults:

for 1280x720: 1280*720*256/320/240 => 3072
for 1920x1080: 1920*1080*256/320/240 => 6912 (maybe a little high, but ok)

[mcasas, 2015/08/19 00:16:30]

Done.

+                                    codec_config_.g_w / codec_config_.g_h;
+  DCHECK(size.width());
+  DCHECK(size.height());
+  codec_config_.g_w = size.width();
+  codec_config_.g_h = size.height();
+  codec_config_.g_pass = VPX_RC_ONE_PASS;
+
+  // Timebase is the smallest interval used by the stream, can be set to the
+  // frame rate or just to milliseconds.
+  codec_config_.g_timebase.num = 1;
+  codec_config_.g_timebase.den = base::Time::kMillisecondsPerSecond;

[miu, 2015/08/18 02:08:35]

Microseconds please.  Milliseconds is for people who want jittery playback.  ;-)

BTW--You're already calling vpx_encode() correctly (using
duration.InMicroseconds()).

[mcasas, 2015/08/19 00:16:30]

Done.

+
+  // Let the encoder decide where to place the Keyframes, between min and max.
+  // In VPX_KF_AUTO mode libvpx will sometimes emit keyframes regardless of min/
+  // max distance out of necessity. Due to http://crbug.com/440223, decoding

[miu, 2015/08/18 02:08:35]

Are you sure this is the same problem?  If you use this to record to a webm file
and then try to play the webm file in a normal <VIDEO src="..."> element, what
happens?

[mcasas, 2015/08/19 00:16:30]

I don't know. I assumed so due to the similar configuration etc.
In any case l.231-233 need to be configured, so these values
looked OK to me. 

tomfinegan@ WDYT?

[Tom Finegan, 2015/08/19 04:20:10]

What's the question? Whether or not a file with 30000 consecutive non-keyframes
will still cause the problem noted in the bug? According to the comments in the
bug (#1, actually), this only effect a media source decode, so no, I don't think
a <VIDEO src=...> element is going to be effected unless its hooked up for
playback via MSE. I suspect the problem remains-- I'm not aware of an upstream
bug. Want to file one? I didn't have time to read through everything in there:
sort of in a rush... not certain if this is a bug in libvpx or within the MSE
stuff. Anyway, tracker is here: https://code.google.com/p/webm/issues/list :)

[mcasas, 2015/08/20 00:37:04]

Acknowledged.

+  // fails after 30,000 non-key frames, so force an "unnecessary" key-frame
+  // every 10,000 frames.
+  codec_config_.kf_mode = VPX_KF_AUTO;
+  codec_config_.kf_min_dist = 10000;

[miu, 2015/08/18 02:08:35]

If this really is needed, I'd suggest setting the min and max to a wider range
and let the encoder decide where it's best to generate a KF.  Suggestion: min=0,
max=30000

[mcasas, 2015/08/19 00:16:29]

Done.

+  codec_config_.kf_max_dist = 10000;
+
+  // Number of frames to consume before producing output.
+  codec_config_.g_lag_in_frames = 0;
+

[miu, 2015/08/18 02:08:35]

You may also want to set codec_config_.g_threads.  Otherwise, IIRC, you get one
(which on most systems probably won't be enough to keep up in realtime).

Possible formula:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me...

[mcasas, 2015/08/19 00:16:30]

Done.

+  const vpx_codec_err_t ret = vpx_codec_enc_init(&encoder_, interface,
+                                                 &codec_config_, kNoFlags);
+  DCHECK_EQ(VPX_CODEC_OK, ret);
+}
+
+bool VideoTrackRecorder::VpxEncoder::IsInitialized() const {
+  DCHECK(encoding_thread_.task_runner()->BelongsToCurrentThread());
+  return codec_config_.g_timebase.den != 0;
+}
+
+base::TimeDelta VideoTrackRecorder::VpxEncoder::CalculateFrameDuration(
+    const scoped_refptr<VideoFrame>& frame) {
+  DCHECK(encoding_thread_.task_runner()->BelongsToCurrentThread());
+
+  base::TimeDelta predicted_frame_duration;
+  if (!frame->metadata()->GetTimeDelta(VideoFrameMetadata::FRAME_DURATION,
+                                       &predicted_frame_duration) ||
+      predicted_frame_duration <= base::TimeDelta()) {
+    // The source of the video frame did not provide the frame duration.  Use
+    // the actual amount of time between the current and previous frame as a
+    // prediction for the next frame's duration.
+    // TODO(mcasas): This duration estimation could lead to artifacts if the
+    // cadence of the received stream is compromised (e.g. camera freeze, pause,
+    // remote packet loss).  Investigate using GetFrameRate() in this case.
+    predicted_frame_duration = frame->timestamp() - last_frame_timestamp_;
+  }
+  last_frame_timestamp_ = frame->timestamp();
+  const base::TimeDelta kMinFrameDuration =
+      base::TimeDelta::FromMilliseconds(1);
+  return std::max(predicted_frame_duration, kMinFrameDuration);

[miu, 2015/08/18 02:08:35]

Suggest you upper-bound this as well.  It's possible there will be minutes or
hours between two frames (e.g., static screen capture, screen change after a
long resting period).  Something like base::TimeDelta::FromSecondsD(1.0 / 8)
should work well.

[mcasas, 2015/08/19 00:16:29]

Done.

+}
+
+VideoTrackRecorder::VideoTrackRecorder(
+    const blink::WebMediaStreamTrack& track,
+    const OnEncodedVideoCB& on_encoded_video_callback)
+    : track_(track),
+      encoder_(new VpxEncoder(on_encoded_video_callback)) {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  DCHECK(track.extraData());
+  AddToVideoTrack(this,
+      base::Bind(&VpxEncoder::StartFrameEncode, encoder_), track_);
+}
+
+VideoTrackRecorder::~VideoTrackRecorder() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  RemoveFromVideoTrack(this, track_);
+}
+
+void VideoTrackRecorder::StartFrameEncodeForTesting(
+    const scoped_refptr<VideoFrame>& frame,
+    const base::TimeTicks& capture_timestamp) {
+  encoder_->StartFrameEncode(frame, capture_timestamp);
+}
+
+}  // namespace content