[cast] Handle frame size changes directly in the VideoToolbox encoder (v2).

[cast] Handle frame size changes directly in the VideoToolbox encoder.

To implement various kinds of re-initialization conditions more easily,
this CL subsumes responsibility for handling frame size changes directly
in the VideoToolbox encoder.

The design is very similar to the previous one with a proxy encoder.
Instead of proxying the entire encoder, only the video frame factory is
proxied. Both the encoder and the proxy own a ref-counted reference to
the factory, which in turn owns a weak back-reference to the encoder and
a ref-counted reference to the current pixel buffer pool.

When a frame size change is detected either by the encoder or by the
video frame factory, the internal compression session is reset. This is
done synchronously when executing on the Cast main thread, and by
posting a task to the cast main thread when not.

The code to re-initialize the compression session will be re-used in
upcoming work where additional conditions, such as backgrounding, need
to be monitored for session reinitialization.

This new CL fixes the cast unit tests by modifying the tests to handle
encoders that do not have a frame delay when changing the video size.

R=dcaiafa@chromium.org, miu@chromium.org
BUG=477895
Committed: https://crrev.com/89a288bd741c318d11085c4b6fdbbfa32f746335
Cr-Commit-Position: refs/heads/master@{#326907}

[jfroy, 2015-04-20 23:32:31]

This version of the patch passes all unit tests. I'm really sorry for the churn.

[miu, 2015-04-21 07:20:01]

In the future, for re-land attempts, please upload the old change as Patch Set
1, and then the new change as Patch Set 2.  That way, the code reviewer can just
review the few LOC that were changed to get things working.

https://codereview.chromium.org/1100643002/diff/1/media/cast/sender/h264_vt_e...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/1100643002/diff/1/media/cast/sender/h264_vt_e...
media/cast/sender/h264_vt_encoder.cc:502: // Tests and clients expect
|EncodeVideoFrame| to fail on frame size changes
Wait.  We shouldn't sacrifice production performance just to make the tests
happy.  Can't we modify the unit tests to allow EncodeVideoFrame to succeed on
frame size changes when the toolbox encoder is being used?  Out of curiosity,
which tests failed on the last attempt?

[jfroy, 2015-04-21 16:07:45]

On 2015/04/21 07:20:01, miu wrote:
> In the future, for re-land attempts, please upload the old change as Patch Set
> 1, and then the new change as Patch Set 2.  That way, the code reviewer can
just
> review the few LOC that were changed to get things working.

Sorry, first time doing thing and I didn't find a useful guide on the chromium
web site. Thanks for the tip.

[jfroy, 2015-04-21 16:07:54]

https://codereview.chromium.org/1100643002/diff/1/media/cast/sender/h264_vt_e...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/1100643002/diff/1/media/cast/sender/h264_vt_e...
media/cast/sender/h264_vt_encoder.cc:502: // Tests and clients expect
|EncodeVideoFrame| to fail on frame size changes
On 2015/04/21 07:20:00, miu wrote:
> Wait.  We shouldn't sacrifice production performance just to make the tests
> happy.  Can't we modify the unit tests to allow EncodeVideoFrame to succeed on
> frame size changes when the toolbox encoder is being used?  Out of curiosity,
> which tests failed on the last attempt?

video_encoder_unittest EncodesVariedFrameSizes, this code:

  // Encode one frame at each size.  For the platform encoders, expect no frames
  // to be delivered since each frame size change will sprun re-initialization
  // of the underlying encoder.  Otherwise, expect all key frames to come out.
  for (const auto& frame_size : frame_sizes) {
    EXPECT_EQ(!is_testing_platform_encoder(),
              EncodeAndCheckDelivery(CreateTestVideoFrame(frame_size),
                                     frame_id,
                                     frame_id));
    RunTasksAndAdvanceClock();
    if (!is_testing_platform_encoder())
      ++frame_id;
  }

I'm not sure it's worth complicating the implementation and the tests for a one
frame delay.

[miu, 2015-04-22 03:57:24]

On 2015/04/21 16:07:54, jfroy wrote:
> On 2015/04/21 07:20:00, miu wrote:
> > Wait.  We shouldn't sacrifice production performance just to make the tests
> 
> video_encoder_unittest EncodesVariedFrameSizes, this code:
> 
>   // Encode one frame at each size.  For the platform encoders, expect no
frames
>   // to be delivered since each frame size change will sprun re-initialization
>   // of the underlying encoder.  Otherwise, expect all key frames to come out.
>   for (const auto& frame_size : frame_sizes) {
>     EXPECT_EQ(!is_testing_platform_encoder(),

Simply change the above line to:

  EXPECT_EQ(!is_testing_platform_encoder() ||
is_testing_video_toolbox_encoder(),

...and the test should pass.  BTW--You'll also want to adjust the comment above
this for-loop.

[jfroy, 2015-04-22 04:19:51]

On 2015/04/22 03:57:24, miu wrote:
> On 2015/04/21 16:07:54, jfroy wrote:
> > On 2015/04/21 07:20:00, miu wrote:
> > > Wait.  We shouldn't sacrifice production performance just to make the
tests
> > 
> > video_encoder_unittest EncodesVariedFrameSizes, this code:
> > 
> >   // Encode one frame at each size.  For the platform encoders, expect no
> frames
> >   // to be delivered since each frame size change will sprun
re-initialization
> >   // of the underlying encoder.  Otherwise, expect all key frames to come
out.
> >   for (const auto& frame_size : frame_sizes) {
> >     EXPECT_EQ(!is_testing_platform_encoder(),
> 
> Simply change the above line to:
> 
>   EXPECT_EQ(!is_testing_platform_encoder() ||
> is_testing_video_toolbox_encoder(),
> 
> ...and the test should pass.  BTW--You'll also want to adjust the comment
above
> this for-loop.


I wanted to avoid more special-casing in the tests and stay closer to other
encoder implementations, but OK I'll revert the encoder change and instead alter
the test.

[jfroy, 2015-04-22 04:19:51]

On 2015/04/22 03:57:24, miu wrote:
> On 2015/04/21 16:07:54, jfroy wrote:
> > On 2015/04/21 07:20:00, miu wrote:
> > > Wait.  We shouldn't sacrifice production performance just to make the
tests
> > 
> > video_encoder_unittest EncodesVariedFrameSizes, this code:
> > 
> >   // Encode one frame at each size.  For the platform encoders, expect no
> frames
> >   // to be delivered since each frame size change will sprun
re-initialization
> >   // of the underlying encoder.  Otherwise, expect all key frames to come
out.
> >   for (const auto& frame_size : frame_sizes) {
> >     EXPECT_EQ(!is_testing_platform_encoder(),
> 
> Simply change the above line to:
> 
>   EXPECT_EQ(!is_testing_platform_encoder() ||
> is_testing_video_toolbox_encoder(),
> 
> ...and the test should pass.  BTW--You'll also want to adjust the comment
above
> this for-loop.


I wanted to avoid more special-casing in the tests and stay closer to other
encoder implementations, but OK I'll revert the encoder change and instead alter
the test.

[jfroy, 2015-04-22 04:21:27]

So apparently the Publish button doesn't get disabled after you click on it.
Sending review comments while riding the shuttle back to SF is more hazardous
than I thought... Sorry about that.

[jfroy, 2015-04-22 05:45:55]

PTAL

For the encoder tests, I added a utility method to return if the encoder being
tested has a resize delay or not. Reworded the various comments around that.

I also did a final round of formatting and cleanup.

[miu, 2015-04-22 17:19:11]

lgtm

[jfroy, 2015-04-22 17:20:03]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2015-04-22 17:22:58]

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

[jfroy, 2015-04-22 17:52:03]

The CQ bit was unchecked by jfroy@chromium.org

[jfroy, 2015-04-23 02:26:03]

miu, PTAL at the latest patch set. This should address the issue you pointed out
in https://codereview.chromium.org/1094403002.

[jfroy, 2015-04-23 04:09:45]

Patch set 5 adds a check for empty frame sizes to the video frame factory and
returns null right away.

[miu, 2015-04-24 20:28:54]

lgtm % one thing you should change before committing:

https://codereview.chromium.org/1100643002/diff/80001/media/cast/sender/h264_...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/1100643002/diff/80001/media/cast/sender/h264_...
media/cast/sender/h264_vt_encoder.cc:491: video_frame_factory_->Update(
This should be moved to the top of this code block, since we should ensure the
pool is killed, from the perspective of other threads, before
VTCompressionSessionInvalidate() and compression_session_.reset().

[jfroy, 2015-04-24 20:52:23]

https://codereview.chromium.org/1100643002/diff/80001/media/cast/sender/h264_...
File media/cast/sender/h264_vt_encoder.cc (right):

https://codereview.chromium.org/1100643002/diff/80001/media/cast/sender/h264_...
media/cast/sender/h264_vt_encoder.cc:491: video_frame_factory_->Update(
On 2015/04/24 20:28:54, miu wrote:
> This should be moved to the top of this code block, since we should ensure the
> pool is killed, from the perspective of other threads, before
> VTCompressionSessionInvalidate() and compression_session_.reset().

Thanks, that's a good suggestion. Done.

[miu, 2015-04-24 21:23:27]

Still lgtm.

[jfroy, 2015-04-24 21:28:28]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2015-04-24 21:29:01]

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

[commit-bot: I haz the power, 2015-04-24 22:30:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-24 22:31:03]

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