Add power monitoring to the Cast VideoToolbox encoder.

Add power monitoring to the Cast VideoToolbox encoder.

Chromium provides PowerObserver and PowerMonitor objects to allow
subsystems to react to power suspend and resume events. On iOS this
actually tracks app lifecycle events for backgrounding.

The patch adds a power observer that destroys the compression session
on power suspend events and re-initializes the compression session on
power resume events. This allows the encoder to properly transition to
the background on iOS and resume when the app comes back to the
foreground.

Note that for this to work overall, the sender client must create a
background task to allow networking to continue. Otherwise the receiver
will timeout per spec.

R=miu@chromium.org
BUG=477895
Committed: https://crrev.com/5f83e5beada49c0c18e2162478992098f4f3323d
Cr-Commit-Position: refs/heads/master@{#326938}

[jfroy, 2015-04-21 23:22:37]

This is stacked on https://codereview.chromium.org/1100643002. I'm not sure if
there is a formal way to represent stacked CLs.

[miu, 2015-04-22 04:02:58]

All of the implementation looks good.  Just one comment about class structure:

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

https://codereview.chromium.org/1094403002/diff/1/media/cast/sender/h264_vt_e...
media/cast/sender/h264_vt_encoder.h:85: class PowerObserver : public
base::PowerObserver {
Instead of a nested class, how about making H264VideoToolboxEncoder extend
base::PowerObserver itself and implement the OnSuspend() and OnResume() methods?

[jfroy, 2015-04-22 04:18:46]

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

https://codereview.chromium.org/1094403002/diff/1/media/cast/sender/h264_vt_e...
media/cast/sender/h264_vt_encoder.h:85: class PowerObserver : public
base::PowerObserver {
On 2015/04/22 04:02:58, miu wrote:
> Instead of a nested class, how about making H264VideoToolboxEncoder extend
> base::PowerObserver itself and implement the OnSuspend() and OnResume()
methods?

I considered that but decided to use an auxiliary class because of the rather
generic name of the interface methods.

I'll move the implementation directly in the encoder though since you seem to
prefer that.

[jfroy, 2015-04-22 05:53:41]

PTAL

The diff is a clean rebase on top of the latest version of
https://codereview.chromium.org/1100643002/.

[miu, 2015-04-22 17:38:54]

lgtm, but a few comments to consider:

1. Should the pool be set to null in OnSuspend() (or in
DestroyCompressionSession())?  Is it possible for there to be any racy behavior
where a thread will request a frame from VideoFrameFactoryImpl after we have
suspended, but before power suspension freezes the thread?

2. Should OnResume() do nothing and just let a frame request from the
VideoFrameFactoruImpl cause everything to be reinitialized (like a frame size
change)?

3. Unit tests for basic functionality added by this change and to test
scenarios/assumptions for #1 and #2 above?

[jfroy, 2015-04-22 17:51:50]

On 2015/04/22 17:38:54, miu wrote:
> lgtm, but a few comments to consider:
> 
> 1. Should the pool be set to null in OnSuspend() (or in
> DestroyCompressionSession())?  Is it possible for there to be any racy
behavior
> where a thread will request a frame from VideoFrameFactoryImpl after we have
> suspended, but before power suspension freezes the thread?

I avoided resetting the video frame factory's pool from
DestroyCompressionSession() because in the normal frame resize flow it would
cause the update lock to be acquired twice (once to null the pool and again to
set the new pool). Arguably, if resetting the compression session fails, the
video frame factory's pool should be set to null. And for the case you mention,
the pool will remain valid as an object (it is ref-counted), but it will return
null buffers.

I think this is worth cleaning up. I can fix the issues unrelated to power
observation in the other CL and update this CL afterwards.

> 2. Should OnResume() do nothing and just let a frame request from the
> VideoFrameFactoruImpl cause everything to be reinitialized (like a frame size
> change)?

The problem with doing nothing in OnResume() is that the frame change will not
have changed, so EncodeVideoFrame() will just fail with "no compression
session". Do you think there is a cleaner way of handling this?

> 3. Unit tests for basic functionality added by this change and to test
> scenarios/assumptions for #1 and #2 above?

Working on them.

Thanks for the comments.

[miu, 2015-04-22 18:15:28]

On 2015/04/22 17:51:50, jfroy wrote:
> On 2015/04/22 17:38:54, miu wrote:
> > 2. Should OnResume() do nothing and just let a frame request from the
> > VideoFrameFactoruImpl cause everything to be reinitialized (like a frame
size
> > change)?
> 
> The problem with doing nothing in OnResume() is that the frame change will not
> have changed, so EncodeVideoFrame() will just fail with "no compression
> session". Do you think there is a cleaner way of handling this?

Maybe this would this work (use empty frame size to trigger automatic re-init
after resume):

1. OnSuspend() calls DestroyCompressionSession(), which calls
VideoFrameFactoryImpl::Update(null, gfx::Size()).  This releases the old pool
and sets the pool_frame_size_ to empty.  H264VideoToolboxEncoder::frame_size_
should be set to empty as well.

2. OnResume() does nothing.

3. After the system resumes, the client can call either
VideoFrameFactoryImpl::MaybeCreateFrame() to trigger encoder re-init for the
correct frame size, or H264VideoToolboxEncoder::EncodeVideoFrame().  In the
latter case, the video frame's size is not equal to the empty frame_size_ and
this will result in UpdateFrameSize() being called (on line 513).

This idea doesn't work if there's a racy condition where another thread can call
MaybeCreateFrame() or EncodeVideoFrame() after OnSuspend() is called, but before
iOS actually freezes all threads for power-down.  So, maybe what you have is
better.

[miu, 2015-04-22 18:18:47]

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

https://codereview.chromium.org/1094403002/diff/20001/media/cast/sender/h264_...
media/cast/sender/h264_vt_encoder.cc:635: ResetCompressionSession();
I just realized: You shouldn't call ResetCompressionSession() if either:

1. video_frame_factory_ is null (i.e., was never set in the ctor).
2. frame_size_ is empty.

Since these would indicate there wasn't a compression session when OnSuspend()
was called.

[jfroy, 2015-04-22 18:22:45]

On 2015/04/22 18:15:28, miu wrote:
> On 2015/04/22 17:51:50, jfroy wrote:
> > On 2015/04/22 17:38:54, miu wrote:
> > > 2. Should OnResume() do nothing and just let a frame request from the
> > > VideoFrameFactoruImpl cause everything to be reinitialized (like a frame
> size
> > > change)?
> > 
> > The problem with doing nothing in OnResume() is that the frame change will
not
> > have changed, so EncodeVideoFrame() will just fail with "no compression
> > session". Do you think there is a cleaner way of handling this?
> 
> Maybe this would this work (use empty frame size to trigger automatic re-init
> after resume):
> 
> 1. OnSuspend() calls DestroyCompressionSession(), which calls
> VideoFrameFactoryImpl::Update(null, gfx::Size()).  This releases the old pool
> and sets the pool_frame_size_ to empty.  H264VideoToolboxEncoder::frame_size_
> should be set to empty as well.
> 
> 2. OnResume() does nothing.
> 
> 3. After the system resumes, the client can call either
> VideoFrameFactoryImpl::MaybeCreateFrame() to trigger encoder re-init for the
> correct frame size, or H264VideoToolboxEncoder::EncodeVideoFrame().  In the
> latter case, the video frame's size is not equal to the empty frame_size_ and
> this will result in UpdateFrameSize() being called (on line 513).
> 
> This idea doesn't work if there's a racy condition where another thread can
call
> MaybeCreateFrame() or EncodeVideoFrame() after OnSuspend() is called, but
before
> iOS actually freezes all threads for power-down.  So, maybe what you have is
> better.

The race you describe is likely to happen. So something similar to what the
current code does is probably the more reliable way to go. For example, I
leverage the state where the frame size is the same but there is no pool in the
video frame factory to avoid repeatedly posting a task to reset the compression
session. Changing the code to setting the frame size to zero to trigger a
compression session reset breaks that and would need to add more complexity.

[jfroy, 2015-04-22 18:24:36]

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

https://codereview.chromium.org/1094403002/diff/20001/media/cast/sender/h264_...
media/cast/sender/h264_vt_encoder.cc:635: ResetCompressionSession();
On 2015/04/22 18:18:47, miu wrote:
> I just realized: You shouldn't call ResetCompressionSession() if either:
> 
> 1. video_frame_factory_ is null (i.e., was never set in the ctor).
> 2. frame_size_ is empty.
> 
> Since these would indicate there wasn't a compression session when OnSuspend()
> was called.

Thanks for pointing this out. I'll update the code and make sure to test that
case.

[jfroy, 2015-04-23 06:00:28]

PTAL at PS 3. It adds power suspension state tracking to prevent compression
session resets while suspended (this is mostly for tests, but is an extra
conservative guard) and unit tests.

Rebased on the latest version of the
https://codereview.chromium.org/1100643002/, namely PS 5 to reject empty frames.

[miu, 2015-04-24 20:38:41]

lgtm

https://codereview.chromium.org/1094403002/diff/40001/media/cast/sender/h264_...
File media/cast/sender/h264_vt_encoder_unittest.cc (right):

https://codereview.chromium.org/1094403002/diff/40001/media/cast/sender/h264_...
media/cast/sender/h264_vt_encoder_unittest.cc:195: void
NoopFrameEncodedCallback(
FYI--No change needed here, but just so you know: hubbe@'s fixing the need for
"no-op" deleter functions like this
(https://codereview.chromium.org/1074963003/).  Just letting you know here for
your future code changes.

[jfroy, 2015-04-24 20:42:26]

https://codereview.chromium.org/1094403002/diff/40001/media/cast/sender/h264_...
File media/cast/sender/h264_vt_encoder_unittest.cc (right):

https://codereview.chromium.org/1094403002/diff/40001/media/cast/sender/h264_...
media/cast/sender/h264_vt_encoder_unittest.cc:195: void
NoopFrameEncodedCallback(
On 2015/04/24 20:38:40, miu wrote:
> FYI--No change needed here, but just so you know: hubbe@'s fixing the need for
> "no-op" deleter functions like this
> (https://codereview.chromium.org/1074963003/).  Just letting you know here for
> your future code changes.

Acknowledged.

[jfroy, 2015-04-24 23:11:18]

The CQ bit was checked by jfroy@chromium.org

[jfroy, 2015-04-24 23:11:18]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1094403002/#ps60001
(title: "Rebase on latest version of 1094403002.")

[commit-bot: I haz the power, 2015-04-24 23:12:18]

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

[commit-bot: I haz the power, 2015-04-24 23:56:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-24 23:57:13]

Patchset 4 (id:??) landed as
https://crrev.com/5f83e5beada49c0c18e2162478992098f4f3323d
Cr-Commit-Position: refs/heads/master@{#326938}