MediaStream Recorder: Support VP9 encoder

MediaStream Recorder: Support VP9 encoder

This CL adds support for VP9 codec when recording
MediaStream(s). It's a simple change but spreads a bit
in many classes due essentially to passing the VP9
indication. Unittests are adapted and parameterized.

BUG=262211
TEST=All Unit Tests passing, and [1] with this patch
and "Enable experimental Web Platform features"
enabled in chrome://flags.

[1] https://rawgit.com/Miguelao/demos/master/mediarecorder9.html

Committed: https://crrev.com/7f0786af5f5f03501be8fd56498a0ee6bba5a8a2
Cr-Commit-Position: refs/heads/master@{#353117}

[mcasas, 2015-10-07 00:35:29]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-10-07 00:54:17]

Patchset #1 (id:20001) has been deleted

[mcasas, 2015-10-07 00:55:30]

mcasas@chromium.org changed reviewers:
+ miu@chromium.org

[mcasas, 2015-10-07 00:55:31]

miu@ PTAL

Let me direct your reviewer gaze to:
void VideoTrackRecorder::VpxEncoder::ConfigureEncoding()
where the VP9 configuration happens :)

[miu, 2015-10-07 18:16:36]

Looks good, just some comments on minor things.

BTW--Have you considered what will happen if the encoder cannot keep up in
realtime with the incoming video frames?  Should you add frame-dropping logic
(in a later CL)?  Or, is there a finite buffer pool managed upstream of this to
restrict the maximum number of frames in-flight?  In any case, I'd suggest you
test realtime VP8/9 recording sessions where the data volume is too large for
the encoders to operate in real-time.

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.h (right):

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
content/renderer/media/media_recorder_handler.h:73: bool use_vp9_;
Comment that false means VP8.

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:78: VpxEncoder(const
OnEncodedVideoCB& on_encoded_video_callback, bool use_vp9);
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.

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:78: VpxEncoder(const
OnEncodedVideoCB& on_encoded_video_callback, bool use_vp9);
style nit: Callback should be the last argument.

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:260:
codec_config_.g_lag_in_frames = 0;
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?

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:266: DCHECK_EQ(VPX_VBR,
codec_config_.rc_end_usage);
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?

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
File content/renderer/media/video_track_recorder.h (right):

https://codereview.chromium.org/1384483005/diff/60001/content/renderer/media/...
content/renderer/media/video_track_recorder.h:37: bool use_vp9);
style nit: Callback argument should be last.

https://codereview.chromium.org/1384483005/diff/60001/media/capture/webm_muxer.h
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1384483005/diff/60001/media/capture/webm_muxe...
media/capture/webm_muxer.h:43: WebmMuxer(bool use_vp9, const WriteDataCB&
write_data_callback);
Please add a comment that VP8 will be used if |use_vp9| is false.

Alternatively, consider using a "const char* codec_id" argument instead, where
|codec_id| must match one of the constants in mkvmuxer.hpp
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...).

https://codereview.chromium.org/1384483005/diff/60001/media/capture/webm_muxe...
media/capture/webm_muxer.h:72: const bool use_vp9_;
ditto: comment that meaning of "false" means VP8

[mcasas, 2015-10-07 18:54:17]

Patchset #1 (id:40001) has been deleted

[mcasas, 2015-10-07 19:34:50]

Patchset #2 (id:80001) has been deleted

[mcasas, 2015-10-07 20:48:26]

Patchset #2 (id:100001) has been deleted

[mcasas, 2015-10-07 21:01:15]

miu@ PTAL

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
File content/renderer/media/media_recorder_handler.h (right):

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
content/renderer/media/media_recorder_handler.h:73: bool use_vp9_;
On 2015/10/07 18:16:35, miu wrote:
> Comment that false means VP8.

Done.

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
File content/renderer/media/video_track_recorder.cc (right):

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
content/renderer/media/video_track_recorder.cc:78: VpxEncoder(const
OnEncodedVideoCB& on_encoded_video_callback, bool use_vp9);
On 2015/10/07 18:16:35, miu wrote:
> style nit: Callback should be the last argument.

Done.

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
content/renderer/media/video_track_recorder.cc:78: VpxEncoder(const
OnEncodedVideoCB& on_encoded_video_callback, bool use_vp9);
On 2015/10/07 18:16:35, miu wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
content/renderer/media/video_track_recorder.cc:260:
codec_config_.g_lag_in_frames = 0;
On 2015/10/07 18:16:35, miu wrote:
> 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?

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...

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
content/renderer/media/video_track_recorder.cc:266: DCHECK_EQ(VPX_VBR,
codec_config_.rc_end_usage);
On 2015/10/07 18:16:35, miu wrote:
> 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?

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.

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
File content/renderer/media/video_track_recorder.h (right):

https://chromiumcodereview.appspot.com/1384483005/diff/60001/content/renderer...
content/renderer/media/video_track_recorder.h:37: bool use_vp9);
On 2015/10/07 18:16:35, miu wrote:
> style nit: Callback argument should be last.

Done.

https://chromiumcodereview.appspot.com/1384483005/diff/60001/media/capture/we...
File media/capture/webm_muxer.h (right):

https://chromiumcodereview.appspot.com/1384483005/diff/60001/media/capture/we...
media/capture/webm_muxer.h:43: WebmMuxer(bool use_vp9, const WriteDataCB&
write_data_callback);
On 2015/10/07 18:16:35, miu wrote:
> Please add a comment that VP8 will be used if |use_vp9| is false.
> 

Done.

> Alternatively, consider using a "const char* codec_id" argument instead, where
> |codec_id| must match one of the constants in mkvmuxer.hpp
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...).

Wouldn't that surface implementation details? IOW the owner/ctor of this
class will have to know a detail of how libwebm identifies codecs.

https://chromiumcodereview.appspot.com/1384483005/diff/60001/media/capture/we...
media/capture/webm_muxer.h:72: const bool use_vp9_;
On 2015/10/07 18:16:35, miu wrote:
> ditto: comment that meaning of "false" means VP8

Done.

[miu, 2015-10-07 21:56:56]

lgtm

https://codereview.chromium.org/1384483005/diff/60001/media/capture/webm_muxer.h
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1384483005/diff/60001/media/capture/webm_muxe...
media/capture/webm_muxer.h:43: WebmMuxer(bool use_vp9, const WriteDataCB&
write_data_callback);
On 2015/10/07 21:01:15, mcasas wrote:
> On 2015/10/07 18:16:35, miu wrote:
> > Alternatively, consider using a "const char* codec_id" argument instead,
where
> > |codec_id| must match one of the constants in mkvmuxer.hpp
> 
> Wouldn't that surface implementation details? IOW the owner/ctor of this
> class will have to know a detail of how libwebm identifies codecs.

Yeah, that's a good point.  I suppose I'm fine with leaving it the way it is.

https://codereview.chromium.org/1384483005/diff/120001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1384483005/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:267: // VP8 usually produces
frames instantaneously.
nit: s/usually/always/

[mcasas, 2015-10-07 22:22:07]

mcasas@chromium.org changed reviewers:
+ sandersd@chromium.org

[mcasas, 2015-10-07 22:22:07]

sandersd@ Owners RS/PTAL at
  media/capture/webm_muxer*

(yeah, I know, but dalecurtis@ is OOO)

[sandersd (OOO until July 31), 2015-10-08 00:11:04]

https://codereview.chromium.org/1384483005/diff/120001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1384483005/diff/120001/media/capture/webm_mux...
media/capture/webm_muxer.h:44: WebmMuxer(bool use_vp9, const WriteDataCB&
write_data_callback);
I would vastly prefer an enum over a bool for this. Ideally it would be passed
when the track is added, but I won't demand that now.

[mcasas, 2015-10-08 18:06:16]

Patchset #3 (id:140001) has been deleted

[mcasas, 2015-10-08 18:08:33]

Patchset #3 (id:160001) has been deleted

[mcasas, 2015-10-08 18:08:46]

sandersd@ PTAL

https://codereview.chromium.org/1384483005/diff/120001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1384483005/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:267: // VP8 usually produces
frames instantaneously.
On 2015/10/07 21:56:56, miu wrote:
> nit: s/usually/always/

Done.

https://codereview.chromium.org/1384483005/diff/120001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1384483005/diff/120001/media/capture/webm_mux...
media/capture/webm_muxer.h:44: WebmMuxer(bool use_vp9, const WriteDataCB&
write_data_callback);
On 2015/10/08 00:11:04, sandersd wrote:
> I would vastly prefer an enum over a bool for this. Ideally it would be passed
> when the track is added, but I won't demand that now.

Done.

[sandersd (OOO until July 31), 2015-10-08 18:23:09]

media/ lgtm.

[mcasas, 2015-10-08 18:33:30]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-10-08 18:33:31]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1384483005/#ps180001
(title: "sandersd@ comments")

[commit-bot: I haz the power, 2015-10-08 18:37:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1384483005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1384483005/180001

[commit-bot: I haz the power, 2015-10-08 19:12:58]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2015-10-08 19:13:42]

Patchset 3 (id:??) landed as
https://crrev.com/7f0786af5f5f03501be8fd56498a0ee6bba5a8a2
Cr-Commit-Position: refs/heads/master@{#353117}