Cast: Adding support for GPU accelerated encode

Cast: Adding support for GPU accelerated encode

Removed the old VideoEncoderController interface and
made video_encoder a pure virtual class that
ExternalVideoEncoder and VideoEncoderImpl implements. 
This CL also remove the possibility to send in
pre-encoded video frames.

BUG=325998
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243589

[pwestin, 2013-12-16 19:49:12]

Please review

[Ami GONE FROM CHROMIUM, 2013-12-16 21:33:55]

drive-by

https://codereview.chromium.org/116623002/diff/20001/media/cast/DEPS
File media/cast/DEPS (right):

https://codereview.chromium.org/116623002/diff/20001/media/cast/DEPS#newcode2
media/cast/DEPS:2: "+content/common/gpu/client",
This is not ok; c/c/g/c depends on media and allowing this DEPS would mean a
circular dependency.

Fortunately it seems you don't actually use this, so you can simply revert this
file?

[pwestin, 2013-12-17 22:12:26]

On 2013/12/16 21:33:55, Ami Fischman wrote:
> drive-by
> 
> https://codereview.chromium.org/116623002/diff/20001/media/cast/DEPS
> File media/cast/DEPS (right):
> 
> https://codereview.chromium.org/116623002/diff/20001/media/cast/DEPS#newcode2
> media/cast/DEPS:2: "+content/common/gpu/client",
> This is not ok; c/c/g/c depends on media and allowing this DEPS would mean a
> circular dependency.
> 
> Fortunately it seems you don't actually use this, so you can simply revert
this
> file?

Correct; removed that dependency

[pwestin, 2013-12-17 22:13:31]

https://codereview.chromium.org/116623002/diff/20001/media/cast/DEPS
File media/cast/DEPS (right):

https://codereview.chromium.org/116623002/diff/20001/media/cast/DEPS#newcode2
media/cast/DEPS:2: "+content/common/gpu/client",
On 2013/12/16 21:33:55, Ami Fischman wrote:
> This is not ok; c/c/g/c depends on media and allowing this DEPS would mean a
> circular dependency.
> 
> Fortunately it seems you don't actually use this, so you can simply revert
this
> file?

Done.

[mikhal1, 2013-12-17 22:39:46]

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
File media/cast/test/fake_gpu_video_accelerator_factories.h (right):

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
media/cast/test/fake_gpu_video_accelerator_factories.h:67: #endif  //
MEDIA_CAST_TEST_FAKE_GPU_VIDEO_ACCELERATOR_FACTORIES_H_
missing new line

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
File media/cast/test/fake_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
media/cast/test/fake_video_encode_accelerator.cc:16: : client_(client ),
remove extra space

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
media/cast/test/fake_video_encode_accelerator.cc:17: first_(true) {
DCHECK on client?

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
media/cast/test/fake_video_encode_accelerator.cc:52: if (first_) {
If it's the first frame, won't the encoder create a key frame by default?

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/test/f...
media/cast/test/fake_video_encode_accelerator.cc:66: // No-op.
What does this mean?

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
File media/cast/video_sender/external_video_encoder.cc (right):

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:19: //
RequireBitstreamBuffers().
The comment is unclear to me.

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:75: if
(!video_encode_accelerator_) return;
How would the caller know that this wasn't a valid config?

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:146: this, index));
ident

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:197: // Called to allocate the
in and output buffers.
input for consistency

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:298: void
FinishedWithInBuffer(int index) {
input

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:383: if (skip_next_frame_) {
set this to false after skipping.

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder.cc:423: // Do nothing not
supported.
Add NotSuPPORTED

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
File media/cast/video_sender/external_video_encoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/external_video_encoder_unittest.cc:78:
video_config_.min_qp = 0;
general q: what config params do we have for h264?

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
File media/cast/video_sender/video_encoder.h (right):

https://chromiumcodereview.appspot.com/116623002/diff/40001/media/cast/video_...
media/cast/video_sender/video_encoder.h:52: virtual ~VideoEncoder() {}
Doesn't this belong on the top? Just as a convention.

[pwestin, 2013-12-19 16:03:46]

PTAL

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_gpu...
File media/cast/test/fake_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_gpu...
media/cast/test/fake_gpu_video_accelerator_factories.h:67: #endif  //
MEDIA_CAST_TEST_FAKE_GPU_VIDEO_ACCELERATOR_FACTORIES_H_
On 2013/12/17 22:39:47, mikhal1 wrote:
> missing new line

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
File media/cast/test/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.cc:16: : client_(client ),
On 2013/12/17 22:39:47, mikhal1 wrote:
> remove extra space

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.cc:17: first_(true) {
On 2013/12/17 22:39:47, mikhal1 wrote:
> DCHECK on client?

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.cc:52: if (first_) {
On 2013/12/17 22:39:47, mikhal1 wrote:
> If it's the first frame, won't the encoder create a key frame by default?

in normal case yes but this is a fake encoder, no where else to put the code

https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.cc:66: // No-op.
On 2013/12/17 22:39:47, mikhal1 wrote:
> What does this mean?

No operation; frequently used in chrome ~940 times

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/external_video_encoder.cc (right):

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:19: //
RequireBitstreamBuffers().
On 2013/12/17 22:39:47, mikhal1 wrote:
> The comment is unclear to me. 

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:75: if
(!video_encode_accelerator_) return;
On 2013/12/17 22:39:47, mikhal1 wrote:
> How would the caller know that this wasn't a valid config?

Will add that in a future CL

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:146: this, index));
On 2013/12/17 22:39:47, mikhal1 wrote:
> ident

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:197: // Called to allocate the
in and output buffers.
On 2013/12/17 22:39:47, mikhal1 wrote:
> input for consistency

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:298: void
FinishedWithInBuffer(int index) {
On 2013/12/17 22:39:47, mikhal1 wrote:
> input

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:383: if (skip_next_frame_) {
On 2013/12/17 22:39:47, mikhal1 wrote:
> set this to false after skipping.

Done.

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:423: // Do nothing not
supported.
On 2013/12/17 22:39:47, mikhal1 wrote:
> Add NotSuPPORTED

It will still be called

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/external_video_encoder_unittest.cc (right):

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder_unittest.cc:78:
video_config_.min_qp = 0;
On 2013/12/17 22:39:47, mikhal1 wrote:
> general q: what config params do we have for h264?

we dont have any right now; the HW api does not expose any of this

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/video_encoder.h (right):

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/video_encoder.h:52: virtual ~VideoEncoder() {}
On 2013/12/17 22:39:47, mikhal1 wrote:
> Doesn't this belong on the top? Just as a convention. 

Done.

[mikhal1, 2013-12-19 16:46:49]

On 2013/12/19 16:03:46, pwestin wrote:
> PTAL
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_gpu...
> File media/cast/test/fake_gpu_video_accelerator_factories.h (right):
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_gpu...
> media/cast/test/fake_gpu_video_accelerator_factories.h:67: #endif  //
> MEDIA_CAST_TEST_FAKE_GPU_VIDEO_ACCELERATOR_FACTORIES_H_
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > missing new line
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
> File media/cast/test/fake_video_encode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
> media/cast/test/fake_video_encode_accelerator.cc:16: : client_(client ),
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > remove extra space
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
> media/cast/test/fake_video_encode_accelerator.cc:17: first_(true) {
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > DCHECK on client?
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
> media/cast/test/fake_video_encode_accelerator.cc:52: if (first_) {
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > If it's the first frame, won't the encoder create a key frame by default?
> 
> in normal case yes but this is a fake encoder, no where else to put the code
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/test/fake_vid...
> media/cast/test/fake_video_encode_accelerator.cc:66: // No-op.
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > What does this mean?
> 
> No operation; frequently used in chrome ~940 times
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> File media/cast/video_sender/external_video_encoder.cc (right):
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:19: //
> RequireBitstreamBuffers().
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > The comment is unclear to me. 
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:75: if
> (!video_encode_accelerator_) return;
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > How would the caller know that this wasn't a valid config?
> 
> Will add that in a future CL
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:146: this, index));
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > ident
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:197: // Called to allocate
the
> in and output buffers.
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > input for consistency
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:298: void
> FinishedWithInBuffer(int index) {
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > input
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:383: if (skip_next_frame_) {
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > set this to false after skipping.
> 
> Done.
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder.cc:423: // Do nothing not
> supported.
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > Add NotSuPPORTED
> 
> It will still be called
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> File media/cast/video_sender/external_video_encoder_unittest.cc (right):
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/external_video_encoder_unittest.cc:78:
> video_config_.min_qp = 0;
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > general q: what config params do we have for h264?
> 
> we dont have any right now; the HW api does not expose any of this
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> File media/cast/video_sender/video_encoder.h (right):
> 
>
https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
> media/cast/video_sender/video_encoder.h:52: virtual ~VideoEncoder() {}
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > Doesn't this belong on the top? Just as a convention. 
> 
> Done.

lgtm + nits

[mikhal1, 2013-12-19 17:23:09]

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/external_video_encoder.cc (right):

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:75: if
(!video_encode_accelerator_) return;
Add a todo
On 2013/12/19 16:03:47, pwestin wrote:
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > How would the caller know that this wasn't a valid config?
> 
> Will add that in a future CL

https://codereview.chromium.org/116623002/diff/40001/media/cast/video_sender/...
media/cast/video_sender/external_video_encoder.cc:146: this, index));
don't think this is right either.
On 2013/12/19 16:03:47, pwestin wrote:
> On 2013/12/17 22:39:47, mikhal1 wrote:
> > ident
> 
> Done.

[commit-bot: I haz the power, 2013-12-19 19:01:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/116623002/120001

[commit-bot: I haz the power, 2013-12-19 19:37:23]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-19 19:46:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/116623002/140001

[commit-bot: I haz the power, 2013-12-19 21:42:45]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2014-01-06 21:27:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/116623002/590001

[commit-bot: I haz the power, 2014-01-07 10:08:57]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-07 15:32:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/116623002/590001

[commit-bot: I haz the power, 2014-01-07 21:15:23]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-07 21:19:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/116623002/590001

[commit-bot: I haz the power, 2014-01-08 02:04:39]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-08 16:13:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/116623002/590001

[commit-bot: I haz the power, 2014-01-08 17:26:41]

Change committed as 243589