pepper: add software vp9 encoder support to PPB_VideoEncoder

pepper: add software vp9 encoder support to PPB_VideoEncoder

BUG=455409
TEST=run the video_encode NaCl SDK example using the vp9 encoder

Committed: https://crrev.com/dec70f66db4941ce6e8d37ff33a5750a8f55af2c
Cr-Commit-Position: refs/heads/master@{#341970}

[llandwerlin-old, 2015-08-04 17:06:50]

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

[llandwerlin-old, 2015-08-04 17:06:51]

bbudge@: PTAL, thanks.

sbc@: Need an owner for the native_client_sdk directory. This is the same change
as in ppapi/example/, thanks.

[Sam Clegg, 2015-08-04 17:42:10]

lgtm

[bbudge, 2015-08-05 02:07:45]

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

https://codereview.chromium.org/1132833002/diff/40001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:41: class CodecConfig {
This seems like an awful lot of machinery just to set different parameters and
call an extra VP9 initialization function. Is there some reason you can't use
the mode and a switch statement in the EncoderImpl code?

https://codereview.chromium.org/1132833002/diff/40001/ppapi/tests/test_video_...
File ppapi/tests/test_video_encoder.cc (right):

https://codereview.chromium.org/1132833002/diff/40001/ppapi/tests/test_video_...
ppapi/tests/test_video_encoder.cc:106: }
Can you add a test for VP9? You could macro-ize the above code with a codec
parameter to reduce duplication.

[llandwerlin-old, 2015-08-05 14:55:17]

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

https://codereview.chromium.org/1132833002/diff/40001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:41: class CodecConfig {
On 2015/08/05 02:07:45, bbudge wrote:
> This seems like an awful lot of machinery just to set different parameters and
> call an extra VP9 initialization function. Is there some reason you can't use
> the mode and a switch statement in the EncoderImpl code?

Went back to the previous iteration just holding initialization parameters in a
structure.
Hopefully that's acceptable.

https://codereview.chromium.org/1132833002/diff/40001/ppapi/tests/test_video_...
File ppapi/tests/test_video_encoder.cc (right):

https://codereview.chromium.org/1132833002/diff/40001/ppapi/tests/test_video_...
ppapi/tests/test_video_encoder.cc:106: }
On 2015/08/05 02:07:45, bbudge wrote:
> Can you add a test for VP9? You could macro-ize the above code with a codec
> parameter to reduce duplication.

Done, also split the tests into different functions.

[bbudge, 2015-08-05 17:04:41]

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

https://codereview.chromium.org/1132833002/diff/60001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:39: // Magic encoder constants for
VP9 adaptive quantization strategy.
nit: s/constants/constant
Or do you want to move more magic numbers here? I think it's a good idea, so you
can add some commentary on why the particular values are chosen.

https://codereview.chromium.org/1132833002/diff/60001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:44: struct CodecDescription {
This class just holds parameter values, and some state about whether the codec
is valid. I think you could eliminate this struct, define some kConstants for
the magic numbers, check for a valid codec value in
VideoEncoderShim::Initialize, and switch in EncoderImpl::Initialize on that.

https://codereview.chromium.org/1132833002/diff/60001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:163: const CodecDescription&
codec_descr,
nit: desc is a more common abbreviation for description or descriptor in
chromium. Also to be consistent with codec_desc in Initialize.

[llandwerlin-old, 2015-08-05 17:35:55]

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

https://codereview.chromium.org/1132833002/diff/60001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:39: // Magic encoder constants for
VP9 adaptive quantization strategy.
On 2015/08/05 17:04:41, bbudge wrote:
> nit: s/constants/constant
> Or do you want to move more magic numbers here? I think it's a good idea, so
you
> can add some commentary on why the particular values are chosen.

Done.

https://codereview.chromium.org/1132833002/diff/60001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:44: struct CodecDescription {
On 2015/08/05 17:04:41, bbudge wrote:
> This class just holds parameter values, and some state about whether the codec
> is valid. I think you could eliminate this struct, define some kConstants for
> the magic numbers, check for a valid codec value in
> VideoEncoderShim::Initialize, and switch in EncoderImpl::Initialize on that.

Replaced with a function that does the switch on return values.
A switch in EncoderImpl::Initialize doesn't feel pratical as there is a fair
amount of error checking there.

https://codereview.chromium.org/1132833002/diff/60001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:163: const CodecDescription&
codec_descr,
On 2015/08/05 17:04:41, bbudge wrote:
> nit: desc is a more common abbreviation for description or descriptor in
> chromium. Also to be consistent with codec_desc in Initialize.

Done.

[bbudge, 2015-08-05 17:42:57]

LGTM w/some comments.
Thanks for doing this!

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

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:43: // -8..8).
If you can, explain why how these values were chosen for anyone trying to tune
this in the future.

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:47: // Default quantizer min/max
values.
Ditto. The comment doesn't add anything.

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:53: // VP9 adaptive quantization
strategy (live video conferencing).
This comment is good.

[bbudge, 2015-08-05 17:44:20]

One more comment.

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

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:74: default:
zero vpx_codec for sanity.

[llandwerlin-old, 2015-08-05 18:20:12]

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

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:43: // -8..8).
On 2015/08/05 17:42:57, bbudge wrote:
> If you can, explain why how these values were chosen for anyone trying to tune
> this in the future.

Essentially same a WebRTC.

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:47: // Default quantizer min/max
values.
On 2015/08/05 17:42:57, bbudge wrote:
> Ditto. The comment doesn't add anything.

Done.

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:53: // VP9 adaptive quantization
strategy (live video conferencing).
On 2015/08/05 17:42:57, bbudge wrote:
> This comment is good.

Done.

https://codereview.chromium.org/1132833002/diff/80001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:74: default:
On 2015/08/05 17:44:20, bbudge wrote:
> zero vpx_codec for sanity.

Done.

[llandwerlin-old, 2015-08-05 18:27:43]

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

[llandwerlin-old, 2015-08-05 18:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sbc@chromium.org,
bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1132833002/#ps100001
(title: "Additional comments")

[commit-bot: I haz the power, 2015-08-05 18:29:05]

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

[commit-bot: I haz the power, 2015-08-05 20:53:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-05 20:53:41]

Patchset 6 (id:??) landed as
https://crrev.com/dec70f66db4941ce6e8d37ff33a5750a8f55af2c
Cr-Commit-Position: refs/heads/master@{#341970}