[Cast] Size-Adaptable platform video encoders.

[Cast] Size-Adaptable platform video encoders.

Provides a proxy VideoEncoder implementation that wraps
ExternalVideoEncoder or H264VTEncoder to handle frame size changes.
When the frame size changes, the proxy 1) waits for current encoder
instance to finish encoding any frames in-progress, then 2) destroys the
encoder, and 3) finally re-creates/initializes a new encoder instance.
During this process, frames provided via InsertRawVideoFrame() are
simply dropped.

Merged external_encoder_unittest.cc and video_encoder_impl_unittest.cc
into a single video_encoder_unittest.cc that tests *all* cast
VideoEncoder implementations.  Made a number of fixes to allow testing
of the H264VTEncoder on desktop Macs and the trybots.

BUG=451277
Committed: https://crrev.com/9005303cba2d9ab58dd4439295695a93c8801e96
Cr-Commit-Position: refs/heads/master@{#315723}

[miu, 2015-02-10 05:00:27]

Patchset #1 (id:1) has been deleted

[miu, 2015-02-10 21:49:22]

Patchset #1 (id:20001) has been deleted

[miu, 2015-02-10 21:56:17]

miu@chromium.org changed reviewers:
+ hubbe@chromium.org, jfroy@chromium.org

[miu, 2015-02-10 21:56:18]

jfroy: PTAL at media/cast/cast_sender* and media/cast/sender/h264* and
media/cast/sender/video_frame_factory*.  There is a behavior change in the
VideoFrameFactory interface needed to support variable frame sizes.  I've
changed the names of the methods to reflect the new behavior, and to purposely
break compilation outside the Chromium project so client code has to be updated.
 Also, note that now the H264VTEncoder will be tested on the bots (via
video_encoder_unittest.cc).

hubbe: PTAL at the rest.

[jfroy, 2015-02-10 22:11:37]

On 2015/02/10 21:56:18, miu wrote:
> jfroy: PTAL at media/cast/cast_sender* and media/cast/sender/h264* and
> media/cast/sender/video_frame_factory*.  There is a behavior change in the
> VideoFrameFactory interface needed to support variable frame sizes.  I've
> changed the names of the methods to reflect the new behavior, and to purposely
> break compilation outside the Chromium project so client code has to be
updated.
>  Also, note that now the H264VTEncoder will be tested on the bots (via
> video_encoder_unittest.cc).
> 
> hubbe: PTAL at the rest.

Looking.

[jfroy, 2015-02-10 22:51:37]

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:216: return nullptr;  // Buffer pool is not
a match for requested frame size.
Not sure about chromium style, but maybe wrap in {}?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:222: !buffer)
There should never be a case where buffer == nullptr and the return value ==
kCVReturnSuccess. If you have seen such a case, then that's bananas. Otherwise,
I'd just check the return value and maybe DCHECK the buffer after.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:223: return nullptr;  // Buffer pool has
run out of pixel buffers.
Not sure about chromium style, but maybe wrap in {}?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:228: CVBufferSetAttachment(buffer,
kCVImageBufferColorPrimariesKey,
As discussed, instead of setting these attributes here on each buffer, add a
'kCVBufferPropagatedAttachmentsKey' kv pair to the 'buffer_attributes' argument
passed to VTCompressionSessionCreate() in Initialize(). The value should be a
dictionary containing each of the colorimetric kv pairs set here.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:365: //
VideoFrameFactoryCVPixelBufferPoolImpl::MaybeCreateFrame().
Update this comment if the colorimetric keys are moved to Initialize(). And yes,
it's kind of a shitty that these keys don't influence the buffers that are
vended by the pool! They are instead used to encode the SPS and PPS NAL units :/

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:407: return false;
Not sure about chromium style, but maybe wrap in {}?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:471: return nullptr;
Not sure about chromium style, but maybe wrap in {}?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:575: CopySampleBufferToAnnexBBuffer(sbuf,
&encoded_frame->data, keyframe);
Not sure about chromium style, but maybe wrap in {}?

[miu, 2015-02-11 00:31:25]

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:216: return nullptr;  // Buffer pool is not
a match for requested frame size.
On 2015/02/10 22:51:37, jfroy wrote:
> Not sure about chromium style, but maybe wrap in {}?

Unfortunately, in Chromium the style is to NOT wrap one-liner then clauses in
braces.  I wish it weren't so...

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:222: !buffer)
On 2015/02/10 22:51:37, jfroy wrote:
> There should never be a case where buffer == nullptr and the return value ==
> kCVReturnSuccess. If you have seen such a case, then that's bananas.
Otherwise,
> I'd just check the return value and maybe DCHECK the buffer after.

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:223: return nullptr;  // Buffer pool has
run out of pixel buffers.
On 2015/02/10 22:51:37, jfroy wrote:
> Not sure about chromium style, but maybe wrap in {}?

Acknowledged.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:228: CVBufferSetAttachment(buffer,
kCVImageBufferColorPrimariesKey,
On 2015/02/10 22:51:37, jfroy wrote:
> As discussed, instead of setting these attributes here on each buffer, add a
> 'kCVBufferPropagatedAttachmentsKey' kv pair to the 'buffer_attributes'
argument
> passed to VTCompressionSessionCreate() in Initialize(). The value should be a
> dictionary containing each of the colorimetric kv pairs set here.

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:365: //
VideoFrameFactoryCVPixelBufferPoolImpl::MaybeCreateFrame().
On 2015/02/10 22:51:37, jfroy wrote:
> Update this comment if the colorimetric keys are moved to Initialize(). And
yes,
> it's kind of a shitty that these keys don't influence the buffers that are
> vended by the pool! They are instead used to encode the SPS and PPS NAL units
:/

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:407: return false;
On 2015/02/10 22:51:37, jfroy wrote:
> Not sure about chromium style, but maybe wrap in {}?

Acknowledged.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:471: return nullptr;
On 2015/02/10 22:51:37, jfroy wrote:
> Not sure about chromium style, but maybe wrap in {}?

Acknowledged.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:575: CopySampleBufferToAnnexBBuffer(sbuf,
&encoded_frame->data, keyframe);
On 2015/02/10 22:51:37, jfroy wrote:
> Not sure about chromium style, but maybe wrap in {}?

Acknowledged.

[hubbe, 2015-02-11 00:47:54]

lgtm

Just nits...

https://codereview.chromium.org/906403006/diff/40001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/cast_sender.h...
media/cast/cast_sender.h:52: // this case, the caller must be able to handle a
nullptr return value.
And do what?
Should the caller create an un-optimized frame, or should they wait?

https://codereview.chromium.org/906403006/diff/40001/media/cast/cast_sender.h...
media/cast/cast_sender.h:53: virtual scoped_refptr<VideoFrame>
MaybeCreateOptimizedFrame(
I think the "Maybe" is redundant.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:616: class VideoFrameFactoryProxy : public
VideoFrameFactory {
Personally I think local classes are harder to read.
Can we just put this in an anonymous namespace before the function instead?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:644: next_encoder_frame_size(),
It feels a little awkward to use "next_encoder_frame_size()" here.
Maybe size should be an argument?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:654: holder_->factory =
current_factory.Pass();
Call baseclass OnEncoderReplaced()

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
File media/cast/sender/size_adaptable_video_encoder_base.cc (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.cc:31:
weak_factory_.InvalidateWeakPtrs();
This is kind of weird.
Why invalidate weak pointers before destroying the encoder?
(Add a comment?)

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.cc:112: void
SizeAdaptableVideoEncoderBase::MaybeSpawnReplacementEncoder(
I don't like "Maybe"
I think "Try" or "" would be better.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
File media/cast/sender/size_adaptable_video_encoder_base.h (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:59: // Returns a callback
that calls OnEncoderStatusChange().
Why not just make OnEncoderStatusChange() protected?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:63: virtual
scoped_ptr<VideoEncoder> CreateReplacementEncoder() = 0;
Wouldn't this also create the initial encoder?
(Shouldn't it just be called CreateEncoder() ?)

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:96:
scoped_ptr<VideoEncoder> current_encoder_;
Maybe make a struct which contains an encoder and it's size?

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:101:
scoped_ptr<VideoEncoder> next_encoder_;
Do we really need this?
As far as i can tell, we never actually have two encoders at the same time.

[jfroy, 2015-02-11 01:00:34]

lgtm, with very minor nits.

https://codereview.chromium.org/906403006/diff/60001/media/cast/sender/h264_v...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/906403006/diff/60001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:41: CFTypeRef* keys,
nit: const CFTypeRef

https://codereview.chromium.org/906403006/diff/60001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:42: CFTypeRef* values,
nit: const CFTypeRef

[miu, 2015-02-11 02:07:29]

New patchsets have been uploaded after l-g-t-m from
hubbe@chromium.org,jfroy@chromium.org

[miu, 2015-02-11 02:13:45]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-02-11 02:14:17]

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

[miu, 2015-02-11 02:14:24]

Thanks guys for the quick turnaround on this code review.  It was a biggie.  All
comments addressed:

https://codereview.chromium.org/906403006/diff/40001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/cast_sender.h...
media/cast/cast_sender.h:52: // this case, the caller must be able to handle a
nullptr return value.
On 2015/02/11 00:47:53, hubbe wrote:
> And do what?
> Should the caller create an un-optimized frame, or should they wait?

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/cast_sender.h...
media/cast/cast_sender.h:53: virtual scoped_refptr<VideoFrame>
MaybeCreateOptimizedFrame(
On 2015/02/11 00:47:54, hubbe wrote:
> I think the "Maybe" is redundant.

I put it in there for two reasons: 1) The naming strongly implies a nullptr can
be returned.  2) I want to break compilation on code outside of the Chromium
project that calls this method since its behavior has changed (as discussed w/
jfroy@).

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:616: class VideoFrameFactoryProxy : public
VideoFrameFactory {
On 2015/02/11 00:47:54, hubbe wrote:
> Personally I think local classes are harder to read.
> Can we just put this in an anonymous namespace before the function instead?

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:644: next_encoder_frame_size(),
On 2015/02/11 00:47:54, hubbe wrote:
> It feels a little awkward to use "next_encoder_frame_size()" here.
> Maybe size should be an argument?

Eliminated "next_encoder" part of things, as discussed.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:654: holder_->factory =
current_factory.Pass();
On 2015/02/11 00:47:54, hubbe wrote:
> Call baseclass OnEncoderReplaced()

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
File media/cast/sender/size_adaptable_video_encoder_base.cc (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.cc:31:
weak_factory_.InvalidateWeakPtrs();
On 2015/02/11 00:47:54, hubbe wrote:
> This is kind of weird.
> Why invalidate weak pointers before destroying the encoder?
> (Add a comment?)

Done.  Consolidated into DestroyCurrentEncoder(), and there's a comment there to
explain.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.cc:112: void
SizeAdaptableVideoEncoderBase::MaybeSpawnReplacementEncoder(
On 2015/02/11 00:47:54, hubbe wrote:
> I don't like "Maybe"
> I think "Try" or "" would be better.

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
File media/cast/sender/size_adaptable_video_encoder_base.h (right):

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:59: // Returns a callback
that calls OnEncoderStatusChange().
On 2015/02/11 00:47:54, hubbe wrote:
> Why not just make OnEncoderStatusChange() protected?

I didn't want to expose the weak pointer invalidation to subclasses.  I added a
comment about when the callback would be canceled and why.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:63: virtual
scoped_ptr<VideoEncoder> CreateReplacementEncoder() = 0;
On 2015/02/11 00:47:54, hubbe wrote:
> Wouldn't this also create the initial encoder?
> (Shouldn't it just be called CreateEncoder() ?)

Done.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:96:
scoped_ptr<VideoEncoder> current_encoder_;
On 2015/02/11 00:47:54, hubbe wrote:
> Maybe make a struct which contains an encoder and it's size?

Eliminated the "next_encoder" stuff, as discussed.

https://codereview.chromium.org/906403006/diff/40001/media/cast/sender/size_a...
media/cast/sender/size_adaptable_video_encoder_base.h:101:
scoped_ptr<VideoEncoder> next_encoder_;
On 2015/02/11 00:47:54, hubbe wrote:
> Do we really need this?
> As far as i can tell, we never actually have two encoders at the same time.

Done.

https://codereview.chromium.org/906403006/diff/60001/media/cast/sender/h264_v...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/906403006/diff/60001/media/cast/sender/h264_v...
media/cast/sender/h264_vt_encoder.cc:41: CFTypeRef* keys,
On 2015/02/11 01:00:34, jfroy wrote:
> nit: const CFTypeRef

I tried, and the compiler didn't like it.  Either it's an API issue or the
CFTypeRef is modified when the reference count is increased (when the
keys/values are added to the dictionary).

[commit-bot: I haz the power, 2015-02-11 03:28:54]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-02-11 03:29:34]

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