Fix VideoToolbox backgrounding issues

Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.

Also fixes a leak in the decoder.

BUG=webrtc:4081
Committed: https://crrev.com/5ed5ed953d2d0d667892a0bf528f65eb430cab18
Cr-Commit-Position: refs/heads/master@{#11916}

[tkchin_webrtc, 2016-03-02 23:27:52]

Patchset #1 (id:1) has been deleted

[tkchin_webrtc, 2016-03-02 23:28:00]

Patchset #1 (id:20001) has been deleted

[tkchin_webrtc, 2016-03-02 23:30:45]

Description was changed from

==========
Fix VideoToolbox backgrounding issues

BUG=webrtc:4081
==========

to

==========
Fix VideoToolbox backgrounding issues

BUG=webrtc:4081
==========

[tkchin_webrtc, 2016-03-02 23:30:45]

tkchin@webrtc.org changed reviewers:
+ haysc@webrtc.org, noahric@chromium.org, pbos@webrtc.org

[pbos-webrtc, 2016-03-03 10:08:14]

Hoping the others can review the code better, can you put a more descriptive
description in the description? :)

[tkchin_webrtc, 2016-03-03 21:41:22]

Description was changed from

==========
Fix VideoToolbox backgrounding issues

BUG=webrtc:4081
==========

to

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.


BUG=webrtc:4081
==========

[tkchin_webrtc, 2016-03-03 21:41:55]

Description was changed from

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.


BUG=webrtc:4081
==========

to

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.


BUG=webrtc:4081
==========

[tkchin_webrtc, 2016-03-03 21:47:02]

On 2016/03/03 10:08:14, pbos-webrtc wrote:
> Hoping the others can review the code better, can you put a more descriptive
> description in the description? :)

Done. Mostly I wanted to check with you about the status codes returned by the
encoder/decoder to make sure I won't screw something up that cares about it:
For encoder:- Encoder will return OK when app is backgrounded.
For decoder:- Decoder will return NO_OUTPUT when app is backgrounded, ERROR
after foregrounded again until keyframe is received.

[Chuck, 2016-03-03 21:59:11]

lgtm

[noahric, 2016-03-03 22:10:55]

lgtm

https://codereview.webrtc.org/1732953003/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h (right):

https://codereview.webrtc.org/1732953003/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:47: bool
H264AnnexBBufferHasVideoFormatDescription(const uint8_t* annexb_buffer,
Comments, since the rest of the file is nicely commented :) Same on
CreateVideoFormatDescription.

[pbos-webrtc, 2016-03-04 10:38:33]

pbos@webrtc.org changed reviewers:
+ stefan@webrtc.org

[pbos-webrtc, 2016-03-04 10:38:34]

+R stefan@, do you think an encoder that just stops producing bits has any
adverse effects on bandwidth estimation?

[pbos-webrtc, 2016-03-04 10:44:16]

lgtm

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:135: bool
is_app_active = RTCIsUIApplicationActive();
Do you need to store this? When it's false we abort.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:142:
return WEBRTC_VIDEO_CODEC_NO_OUTPUT;
Looks fine.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:166:
LOG(LS_WARNING) << "Key frame required.";
Log something about the actual error here so the logs are better (put more of
the //comment into the log message).

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:188:
is_app_active &&
Isn't is_app_active implied here?

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:245: bool
is_app_active = RTCIsUIApplicationActive();
Just if(RTC..) ?

[tkchin_webrtc, 2016-03-04 19:03:06]

tkchin@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[tkchin_webrtc, 2016-03-04 19:03:07]

Thanks folks!

+kjellander@ for build file OWNERS.

https://codereview.webrtc.org/1732953003/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h (right):

https://codereview.webrtc.org/1732953003/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:47: bool
H264AnnexBBufferHasVideoFormatDescription(const uint8_t* annexb_buffer,
On 2016/03/03 22:10:55, noahric wrote:
> Comments, since the rest of the file is nicely commented :) Same on
> CreateVideoFormatDescription.

Done.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:135: bool
is_app_active = RTCIsUIApplicationActive();
On 2016/03/04 10:44:15, pbos-webrtc wrote:
> Do you need to store this? When it's false we abort.

Done.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:142:
return WEBRTC_VIDEO_CODEC_NO_OUTPUT;
On 2016/03/04 10:44:15, pbos-webrtc wrote:
> Looks fine.

Acknowledged.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:166:
LOG(LS_WARNING) << "Key frame required.";
On 2016/03/04 10:44:15, pbos-webrtc wrote:
> Log something about the actual error here so the logs are better (put more of
> the //comment into the log message).

Done.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:188:
is_app_active &&
On 2016/03/04 10:44:15, pbos-webrtc wrote:
> Isn't is_app_active implied here?

Done.

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:245: bool
is_app_active = RTCIsUIApplicationActive();
On 2016/03/04 10:44:15, pbos-webrtc wrote:
> Just if(RTC..) ?

Done.

[stefan-webrtc, 2016-03-07 15:00:02]

https://codereview.webrtc.org/1732953003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h (right):

https://codereview.webrtc.org/1732953003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:52: size_t
annexb_buffer_size);
Add unittests for these methods.

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:289:
CFRelease(decompression_session_);
This isn't mentioned in the CL description, AFAICT. Does it fix a memory leak?

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:258: //
Resetting the session when this happens fixes the issue.
Based on this comment it sounds like this should be within #ifdef WEBRTC_IOS,
but it's not. Can we update the comment to explain why?

[tkchin_webrtc, 2016-03-07 20:39:05]

Description was changed from

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.


BUG=webrtc:4081
==========

to

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.

Also fixes a leak in the decoder.

BUG=webrtc:4081
==========

[kjellander_webrtc, 2016-03-07 21:52:08]

*.gyp*,*.gn: lgtm

[tkchin_webrtc, 2016-03-08 04:51:41]

PTAL

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:289:
CFRelease(decompression_session_);
On 2016/03/07 15:00:02, stefan-webrtc (holmer) wrote:
> This isn't mentioned in the CL description, AFAICT. Does it fix a memory leak?

Yes, fixes a leak.

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://codereview.webrtc.org/1732953003/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:258: //
Resetting the session when this happens fixes the issue.
On 2016/03/07 15:00:02, stefan-webrtc (holmer) wrote:
> Based on this comment it sounds like this should be within #ifdef WEBRTC_IOS,
> but it's not. Can we update the comment to explain why?

Done.

[tkchin_webrtc, 2016-03-08 04:52:09]

https://codereview.webrtc.org/1732953003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h (right):

https://codereview.webrtc.org/1732953003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:52: size_t
annexb_buffer_size);
On 2016/03/07 15:00:02, stefan-webrtc (holmer) wrote:
> Add unittests for these methods.

Done.

[stefan-webrtc, 2016-03-08 10:06:56]

lgtm

[tkchin_webrtc, 2016-03-08 18:50:15]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2016-03-08 18:50:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
noahric@chromium.org, haysc@webrtc.org, pbos@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1732953003/#ps160001 (title:
"CR comments")

[commit-bot: I haz the power, 2016-03-08 18:50:18]

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

[commit-bot: I haz the power, 2016-03-08 18:51:57]

Description was changed from

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.

Also fixes a leak in the decoder.

BUG=webrtc:4081
==========

to

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.

Also fixes a leak in the decoder.

BUG=webrtc:4081
==========

[commit-bot: I haz the power, 2016-03-08 18:51:59]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-03-08 18:52:06]

Description was changed from

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.

Also fixes a leak in the decoder.

BUG=webrtc:4081
==========

to

==========
Fix VideoToolbox backgrounding issues.

When the iOS application is not in the foreground, the hardware encoder and
decoder become invalidated. There doesn't seem to be a way to query their state
so we don't know they're invalid until we get an error code after an
encode/decode request. To solve the issue, we just don't encode/decode when the
app is not active, and reinitialize the encoder/decoder when the app is active
again.

Also fixes a leak in the decoder.

BUG=webrtc:4081

Committed: https://crrev.com/5ed5ed953d2d0d667892a0bf528f65eb430cab18
Cr-Commit-Position: refs/heads/master@{#11916}
==========

[commit-bot: I haz the power, 2016-03-08 18:52:07]

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