content: pepper: VideoEncoder: add software encoder support

content: pepper: VideoEncoder: add software encoder support

BUG=455409
TEST=run content_browsertests --gtest_filter=OutOfProcessPPAPITest.VideoEncoder

Committed: https://crrev.com/7918cb6a78193249a30188d8d8fa6ab333aedc0f
Cr-Commit-Position: refs/heads/master@{#321790}

[llandwerlin-old, 2015-02-25 12:30:37]

lionel.g.landwerlin@intel.com changed reviewers:
+ bbudge@chromium.org, sievers@chromium.org

[llandwerlin-old, 2015-02-25 12:30:38]

bbudge@, please review this change to add software VP8 encoder support to
PPB_VideoEncoder.

sievers@, just need an owner for build system stuff, but feel free to comment on
the entire CL.

Thanks!

[bbudge, 2015-02-25 17:58:22]

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_video_encoder_host.cc:295: acceleration ==
PP_HARDWAREACCELERATION_WITHFALLBACK) {
nit: The acceleration enum contains every possibility, so it's stable and a
simpler test is sufficient:
if (acceleration != PP_HARDWAREACCELERATION_ONLY)

Similarly above:
if (acceleration != PP_HARDWAREACCELERATION_NONE)

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_video_encoder_host.cc:550: encoder_.reset(new
VideoEncoderShim(this));
DCHECK(!encoder_);
Or simply inline this method at its call site.

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_video_encoder_host.cc:677: DCHECK_LT(buffer_id,
static_cast<int32>(shm_buffers_.size()));
since buffer_id is signed, you should check that it's >= 0.

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
content/renderer/pepper/video_encoder_shim.cc:65:
scoped_ptr<media::cast::Vp8Encoder> encoder_;
Could we make this SoftwareVideoEncoder in case other codecs are supported in
the future?

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
File content/renderer/pepper/video_encoder_shim.h (right):

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
content/renderer/pepper/video_encoder_shim.h:27: // This class is a shim to wrap
a media::cast::Vp8Encoder so that it can be used
Why only VP8 and not any SoftwareVideoEncoder?

[llandwerlin-old, 2015-02-26 13:02:14]

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_video_encoder_host.cc:295: acceleration ==
PP_HARDWAREACCELERATION_WITHFALLBACK) {
On 2015/02/25 17:58:22, bbudge wrote:
> nit: The acceleration enum contains every possibility, so it's stable and a
> simpler test is sufficient:
> if (acceleration != PP_HARDWAREACCELERATION_ONLY)
> 
> Similarly above:
> if (acceleration != PP_HARDWAREACCELERATION_NONE)

Done.

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_video_encoder_host.cc:550: encoder_.reset(new
VideoEncoderShim(this));
On 2015/02/25 17:58:22, bbudge wrote:
> DCHECK(!encoder_);
> Or simply inline this method at its call site. 

Done.

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_video_encoder_host.cc:677: DCHECK_LT(buffer_id,
static_cast<int32>(shm_buffers_.size()));
On 2015/02/25 17:58:22, bbudge wrote:
> since buffer_id is signed, you should check that it's >= 0.

Done.

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
content/renderer/pepper/video_encoder_shim.cc:65:
scoped_ptr<media::cast::Vp8Encoder> encoder_;
On 2015/02/25 17:58:22, bbudge wrote:
> Could we make this SoftwareVideoEncoder in case other codecs are supported in
> the future?

Done.

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
File content/renderer/pepper/video_encoder_shim.h (right):

https://codereview.chromium.org/956893002/diff/1/content/renderer/pepper/vide...
content/renderer/pepper/video_encoder_shim.h:27: // This class is a shim to wrap
a media::cast::Vp8Encoder so that it can be used
On 2015/02/25 17:58:22, bbudge wrote:
> Why only VP8 and not any SoftwareVideoEncoder?

That's the only encoder I could find at the moment. Updating the comment.

[bbudge, 2015-03-02 20:01:07]

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_encoder_host.cc:169:
PP_HardwareAcceleration demand) {
Now that we know how merging supported profiles will work with software
fallback, we should fix the supported profile struct to use PP_Bool for hardware
/ software rather than the enum. If you prefer to do this in another CL, leave a
TODO here to fix this.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:23: const int32_t kMaxHeight =
1080;
Needs a comment explaining these constants.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:42: struct Frame {
Suggestion: s/Frame/PendingEncode
So you don't have frame.frame below.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:63:
scoped_refptr<base::SingleThreadTaskRunner> shim_task_runner_;
Name suggestion: task_runner_ or media_task_runner_. This isn't really specific
to the shim.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:71: const
base::WeakPtr<VideoEncoderShim>& proxy)
s/proxy/shim
And above in class decl.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:86: 3;  //
media::cast::Vp8Encoder::kNumberOfVp8VideoBuffers;
This comment confused me, since it refers to a private enum value. It would be
better to refer the reader to the comments on the media::cast::VideoSenderConfig
struct. Looking at the actual source in vp8_encoder.cc, I don't think it holds
more than 1 video frame at a time, so you could return 1 to the shim below.
Perhaps defining some constants at the top of this file and commenting them
would help here.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:94: 3, input_visible_size, 4 *
1000000));
4 * 1000000 should be a constant with a comment.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:125: return;
I don't think you need this 'if' - the 'while' handles this case too.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:136: encoder_->Encode(frame.frame,
base::TimeTicks::Now(), encoded_frame.get());
nit: .get() isn't necessary.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:150: // freed on the right thread.
comment formatting is a little weird. Maybe put it above the PostTask call?

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:156: // VideoEncoderShim :
nit: remove

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:178: // TODO(llandwerlin): find
correct values from libvpx.
Move TODO to definitions at top of file.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:197: return false;
Is it possible that other formats could work? I see that vp8_encoder.cc sets
this format.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:202: return false;
Is this input validation needed? Doesn't the host check against invalid
profiles?
(Except for the I420 check.)

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_encoder_shim.h (right):

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.h:38: GetSupportedProfiles()
override;
nit: indent, or is this how the formatter handles this case?

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.h:67: // Task doing the encoding.
nit: s/Task/TaskRunner or Task runner

[llandwerlin-old, 2015-03-03 15:23:28]

I also added modification to the example to allow the user to select the desired
profile.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_encoder_host.cc:169:
PP_HardwareAcceleration demand) {
On 2015/03/02 20:01:06, bbudge wrote:
> Now that we know how merging supported profiles will work with software
> fallback, we should fix the supported profile struct to use PP_Bool for
hardware
> / software rather than the enum. If you prefer to do this in another CL, leave
a
> TODO here to fix this.

Leaving this for another CL.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:23: const int32_t kMaxHeight =
1080;
On 2015/03/02 20:01:06, bbudge wrote:
> Needs a comment explaining these constants.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:42: struct Frame {
On 2015/03/02 20:01:06, bbudge wrote:
> Suggestion: s/Frame/PendingEncode
> So you don't have frame.frame below.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:63:
scoped_refptr<base::SingleThreadTaskRunner> shim_task_runner_;
On 2015/03/02 20:01:06, bbudge wrote:
> Name suggestion: task_runner_ or media_task_runner_. This isn't really
specific
> to the shim.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:71: const
base::WeakPtr<VideoEncoderShim>& proxy)
On 2015/03/02 20:01:06, bbudge wrote:
> s/proxy/shim
> And above in class decl.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:86: 3;  //
media::cast::Vp8Encoder::kNumberOfVp8VideoBuffers;
On 2015/03/02 20:01:06, bbudge wrote:
> This comment confused me, since it refers to a private enum value. It would be
> better to refer the reader to the comments on the
media::cast::VideoSenderConfig
> struct. Looking at the actual source in vp8_encoder.cc, I don't think it holds
> more than 1 video frame at a time, so you could return 1 to the shim below.
> Perhaps defining some constants at the top of this file and commenting them
> would help here.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:94: 3, input_visible_size, 4 *
1000000));
On 2015/03/02 20:01:06, bbudge wrote:
> 4 * 1000000 should be a constant with a comment.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:125: return;
On 2015/03/02 20:01:06, bbudge wrote:
> I don't think you need this 'if' - the 'while' handles this case too.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:136: encoder_->Encode(frame.frame,
base::TimeTicks::Now(), encoded_frame.get());
On 2015/03/02 20:01:06, bbudge wrote:
> nit: .get() isn't necessary.

That doesn't compile on GCC : no known conversion for argument 3 from
'scoped_ptr<media::cast::EncodedFrame>' to 'media::cast::EncodedFrame*

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:150: // freed on the right thread.
On 2015/03/02 20:01:06, bbudge wrote:
> comment formatting is a little weird. Maybe put it above the PostTask call?

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:156: // VideoEncoderShim :
On 2015/03/02 20:01:06, bbudge wrote:
> nit: remove

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:178: // TODO(llandwerlin): find
correct values from libvpx.
On 2015/03/02 20:01:06, bbudge wrote:
> Move TODO to definitions at top of file.

Done.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:197: return false;
On 2015/03/02 20:01:06, bbudge wrote:
> Is it possible that other formats could work? I see that vp8_encoder.cc sets
> this format.

Looking at the VPX code, it depends on the codec and profile.
VP8 only accepts I420 and YV12.
VP9 always accepts I420 and YV12 and for some profiles, I422/I444/I440.

I put I420 as a restriction because it seems to be the norm all over the
chromium code base (from the capture to the encoders).

I avoided using libvpx directly because it's been used in several other places
through the chromium code base. Do you think it would make sense to drop the
usage of media/cast/sender/ and directly rely on libvpx?
I think this would give us VP9 support.

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:202: return false;
On 2015/03/02 20:01:06, bbudge wrote:
> Is this input validation needed? Doesn't the host check against invalid
> profiles?
> (Except for the I420 check.)

Fair, removing.

[bbudge, 2015-03-05 01:43:59]

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:197: return false;
On 2015/03/03 15:23:28, llandwerlin wrote:
> On 2015/03/02 20:01:06, bbudge wrote:
> > Is it possible that other formats could work? I see that vp8_encoder.cc sets
> > this format.
> 
> Looking at the VPX code, it depends on the codec and profile.
> VP8 only accepts I420 and YV12.
> VP9 always accepts I420 and YV12 and for some profiles, I422/I444/I440.
> 
> I put I420 as a restriction because it seems to be the norm all over the
> chromium code base (from the capture to the encoders).
> 
> I avoided using libvpx directly because it's been used in several other places
> through the chromium code base. Do you think it would make sense to drop the
> usage of media/cast/sender/ and directly rely on libvpx?
> I think this would give us VP9 support.

It would be nice to have more profiles. I don't know much about the video stack,
so I'm going to ask those folks what approach they recommend.

[llandwerlin-old, 2015-03-10 11:07:26]

On 2015/03/05 01:43:59, bbudge wrote:
>
https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
> File content/renderer/pepper/video_encoder_shim.cc (right):
> 
>
https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
> content/renderer/pepper/video_encoder_shim.cc:197: return false;
> On 2015/03/03 15:23:28, llandwerlin wrote:
> > On 2015/03/02 20:01:06, bbudge wrote:
> > > Is it possible that other formats could work? I see that vp8_encoder.cc
sets
> > > this format.
> > 
> > Looking at the VPX code, it depends on the codec and profile.
> > VP8 only accepts I420 and YV12.
> > VP9 always accepts I420 and YV12 and for some profiles, I422/I444/I440.
> > 
> > I put I420 as a restriction because it seems to be the norm all over the
> > chromium code base (from the capture to the encoders).
> > 
> > I avoided using libvpx directly because it's been used in several other
places
> > through the chromium code base. Do you think it would make sense to drop the
> > usage of media/cast/sender/ and directly rely on libvpx?
> > I think this would give us VP9 support.
> 
> It would be nice to have more profiles. I don't know much about the video
stack,
> so I'm going to ask those folks what approach they recommend.

Any news?

[bbudge, 2015-03-11 13:28:39]

On 2015/03/10 11:07:26, llandwerlin wrote:
> On 2015/03/05 01:43:59, bbudge wrote:
> >
>
https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
> > File content/renderer/pepper/video_encoder_shim.cc (right):
> > 
> >
>
https://codereview.chromium.org/956893002/diff/20001/content/renderer/pepper/...
> > content/renderer/pepper/video_encoder_shim.cc:197: return false;
> > On 2015/03/03 15:23:28, llandwerlin wrote:
> > > On 2015/03/02 20:01:06, bbudge wrote:
> > > > Is it possible that other formats could work? I see that vp8_encoder.cc
> sets
> > > > this format.
> > > 
> > > Looking at the VPX code, it depends on the codec and profile.
> > > VP8 only accepts I420 and YV12.
> > > VP9 always accepts I420 and YV12 and for some profiles, I422/I444/I440.
> > > 
> > > I put I420 as a restriction because it seems to be the norm all over the
> > > chromium code base (from the capture to the encoders).
> > > 
> > > I avoided using libvpx directly because it's been used in several other
> places
> > > through the chromium code base. Do you think it would make sense to drop
the
> > > usage of media/cast/sender/ and directly rely on libvpx?
> > > I think this would give us VP9 support.
> > 
> > It would be nice to have more profiles. I don't know much about the video
> stack,
> > so I'm going to ask those folks what approach they recommend.
> 
> Any news?

Not yet, I'll ping them.

[bbudge, 2015-03-16 18:27:48]

Since we're not getting much guidance, let's land this with VP8 support only.
I made a few comments, otherwise, LGTM.

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_encoder_host.cc:470: profile,
PP_HARDWAREACCELERATION_NONE));
use brackets here please.

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:30: const uint32_t
kBitstreamBufferSize = 4000000;
nit: not really megabits (Mbs) or even megabytes (MBs). Is this constant
determined by something else?

https://codereview.chromium.org/956893002/diff/40001/ppapi/examples/video_enc...
File ppapi/examples/video_encode/video_encode.cc (right):

https://codereview.chromium.org/956893002/diff/40001/ppapi/examples/video_enc...
ppapi/examples/video_encode/video_encode.cc:46: class IVFWriter {
Since this is an example, some general comments about what this class is for
would be helpful.

[llandwerlin-old, 2015-03-16 19:08:10]

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_video_encoder_host.cc:470: profile,
PP_HARDWAREACCELERATION_NONE));
On 2015/03/16 18:27:48, bbudge wrote:
> use brackets here please.

Done.

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/956893002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/video_encoder_shim.cc:30: const uint32_t
kBitstreamBufferSize = 4000000;
On 2015/03/16 18:27:48, bbudge wrote:
> nit: not really megabits (Mbs) or even megabytes (MBs). Is this constant
> determined by something else?

Thanks, I think that's a bit too much for the average case.
Moving to the same limit as the hardware encoder :

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

https://codereview.chromium.org/956893002/diff/40001/ppapi/examples/video_enc...
File ppapi/examples/video_encode/video_encode.cc (right):

https://codereview.chromium.org/956893002/diff/40001/ppapi/examples/video_enc...
ppapi/examples/video_encode/video_encode.cc:46: class IVFWriter {
On 2015/03/16 18:27:48, bbudge wrote:
> Since this is an example, some general comments about what this class is for
> would be helpful.

Done.

[llandwerlin-old, 2015-03-20 12:05:34]

sievers@ : Could you give an LGTM on the gn build file if this change is okay
with you? Thanks.

[no sievers, 2015-03-20 18:21:16]

lgtm

[llandwerlin-old, 2015-03-23 15:23:18]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[llandwerlin-old, 2015-03-23 15:23:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/956893002/#ps330001
(title: "Another android dep fix")

[commit-bot: I haz the power, 2015-03-23 15:23:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/956893002/330001

[commit-bot: I haz the power, 2015-03-23 16:35:57]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2015-03-23 16:36:46]

Patchset 18 (id:??) landed as
https://crrev.com/7918cb6a78193249a30188d8d8fa6ab333aedc0f
Cr-Commit-Position: refs/heads/master@{#321790}