MediaStream Recorder: Support VP9 encoder

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
index 52368b386050c4829e92b9fea755ddec32d54f9a..b5dfc5df3257b188d143b0f3e774e3f498bb7e94 100644
--- a/content/renderer/media/video_track_recorder.cc
+++ b/content/renderer/media/video_track_recorder.cc
@@ -75,7 +75,7 @@ class VideoTrackRecorder::VpxEncoder final
   static void ShutdownEncoder(scoped_ptr<base::Thread> encoding_thread,
                               ScopedVpxCodecCtxPtr encoder);
 
-  explicit VpxEncoder(const OnEncodedVideoCB& on_encoded_video_callback);
+  VpxEncoder(const OnEncodedVideoCB& on_encoded_video_callback, bool use_vp9);

[miu, 2015/10/07 18:16:35]

style nit: Callback should be the last argument.

[miu, 2015/10/07 18:16:35]

Suggestion: Up to you, since this requires a little more coding, but have you
considered making VpxEncoder an abstract class and then creating two subclasses:
Vp8Encoder and Vp9Encoder.  The subclasses would implement their own
ConfigureEncoding() override.

[mcasas, 2015/10/07 21:01:14]

Yeah I thought about it but since the differences are so minimal
I thought it easier to leave it like that. Unless we start configuring
them very differently, or add a different (family of) codecs.

[mcasas, 2015/10/07 21:01:14]

Done.

 
   void StartFrameEncode(const scoped_refptr<VideoFrame>& frame,
                         base::TimeTicks capture_timestamp);
@@ -87,7 +87,7 @@ class VideoTrackRecorder::VpxEncoder final
   void EncodeOnEncodingThread(const scoped_refptr<VideoFrame>& frame,
                               base::TimeTicks capture_timestamp);
 
-  void ConfigureVp8Encoding(const gfx::Size& size);
+  void ConfigureEncoding(const gfx::Size& size);
 
   // Returns true if |codec_config_| has been filled in at least once.
   bool IsInitialized() const;
@@ -96,6 +96,9 @@ class VideoTrackRecorder::VpxEncoder final
   base::TimeDelta CalculateFrameDuration(
       const scoped_refptr<VideoFrame>& frame);
 
+  // Force usage of VP9 for encoding, instead of VP8 which is the default.
+  const bool use_vp9_;
+
   // Used to shutdown properly on the same thread we were created.
   const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 
@@ -131,8 +134,10 @@ void VideoTrackRecorder::VpxEncoder::ShutdownEncoder(
 }
 
 VideoTrackRecorder::VpxEncoder::VpxEncoder(
-    const OnEncodedVideoCB& on_encoded_video_callback)
-    : main_task_runner_(base::MessageLoop::current()->task_runner()),
+    const OnEncodedVideoCB& on_encoded_video_callback,
+    bool use_vp9)
+    : use_vp9_(use_vp9),
+      main_task_runner_(base::MessageLoop::current()->task_runner()),
       on_encoded_video_callback_(on_encoded_video_callback),
       encoding_thread_(new base::Thread("EncodingThread")) {
   DCHECK(!on_encoded_video_callback_.is_null());
@@ -173,7 +178,7 @@ void VideoTrackRecorder::VpxEncoder::EncodeOnEncodingThread(
   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);
+    ConfigureEncoding(frame_size);
   }
 
   vpx_image_t vpx_image;
@@ -225,19 +230,19 @@ void VideoTrackRecorder::VpxEncoder::EncodeOnEncodingThread(
                  keyframe));
 }
 
-void VideoTrackRecorder::VpxEncoder::ConfigureVp8Encoding(
-    const gfx::Size& size) {
+void VideoTrackRecorder::VpxEncoder::ConfigureEncoding(const gfx::Size& size) {
   if (IsInitialized()) {
     // TODO(mcasas) VP8 quirk/optimisation: If the new |size| is strictly less-
     // than-or-equal than the old size, in terms of area, the existing encoder
     // instance could be reused after changing |codec_config_.{g_w,g_h}|.
     DVLOG(1) << "Destroying/Re-Creating encoder for new frame size: "
              << gfx::Size(codec_config_.g_w, codec_config_.g_h).ToString()
-             << " --> " << size.ToString();
+             << " --> " << size.ToString() << (use_vp9_ ? " vp9" : " vp8");
     encoder_.reset();
   }
 
-  const vpx_codec_iface_t* interface = vpx_codec_vp8_cx();
+  const vpx_codec_iface_t* interface =
+      use_vp9_ ? vpx_codec_vp9_cx() : vpx_codec_vp8_cx();
   const vpx_codec_err_t result = vpx_codec_enc_config_default(interface,
                                                               &codec_config_,
                                                               0 /* reserved */);
@@ -248,7 +253,18 @@ void VideoTrackRecorder::VpxEncoder::ConfigureVp8Encoding(
   DCHECK_EQ(240u, codec_config_.g_h);
   DCHECK_EQ(256u, codec_config_.rc_target_bitrate);
   codec_config_.rc_target_bitrate = size.GetArea() *
-     codec_config_.rc_target_bitrate / codec_config_.g_w / codec_config_.g_h;
+                                    codec_config_.rc_target_bitrate /
+                                    codec_config_.g_w / codec_config_.g_h;
+  if (use_vp9_) {
+    // Number of frames to consume before producing output.
+    codec_config_.g_lag_in_frames = 0;

[miu, 2015/10/07 18:16:35]

This should be set for VP8 as well.  I'm assuming it was already a default value
for VP8, and that's why things have worked without explicitly setting it?

[mcasas, 2015/10/07 21:01:14]

Yeah, VP8 says 0 [1], VP9 says 25 [2]. I'll add a DCHECK for VP8 to 
make this evident.

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

+
+    // DCHECK that the profile selected by default is I420 (magic number 0).
+    DCHECK_EQ(0u, codec_config_.g_profile);
+
+    // DCHECK that VP9 configuration is Variable BitRate.
+    DCHECK_EQ(VPX_VBR, codec_config_.rc_end_usage);

[miu, 2015/10/07 18:16:35]

Could you add to your comment here why VP8 is CBR and VP9 is VBR?  Or are both
supposed to be VBR?  What is your intention?

[mcasas, 2015/10/07 21:01:14]

Both are VBR by default and that's how it should be
IMHO, so they can accommodate large scene changes
without having to worry about the bitrate -- remember 
that this is a recording API, and is not oriented to 
put the encoded frames on the (transmission) line.

I'll move the DCHECK outside to make this evident as well.

+  }
 
   DCHECK(size.width());
   DCHECK(size.height());
@@ -319,9 +335,10 @@ base::TimeDelta VideoTrackRecorder::VpxEncoder::CalculateFrameDuration(
 
 VideoTrackRecorder::VideoTrackRecorder(
     const blink::WebMediaStreamTrack& track,
-    const OnEncodedVideoCB& on_encoded_video_callback)
+    const OnEncodedVideoCB& on_encoded_video_callback,
+    bool use_vp9)
     : track_(track),
-      encoder_(new VpxEncoder(on_encoded_video_callback)) {
+      encoder_(new VpxEncoder(on_encoded_video_callback, use_vp9)) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!track_.isNull());
   DCHECK(track_.extraData());