iOS HW H264 support.

iOS HW H264 support.

First step towards supporting H264 on iOS. More tuning/experimentation
required in future CLs. Tested using AppRTCDemo on iPhone6 + iPad Mini.
Future work to get it working on OS/X, simulator (renders black screen
currently) and with the Android AppRTCDemo. Currently protected with a
compile time guard.

BUG=webrtc:4081
R=andrew@webrtc.org, haysc@webrtc.org, holmer@google.com, jiayl@webrtc.org, kjellander@webrtc.org, pbos@webrtc.org, phoglund@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/71f6f4405c1c5f60097f8d10841378088e78e8b9

[tkchin_webrtc, 2015-06-19 00:10:26]

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

[tkchin_webrtc, 2015-06-19 00:10:26]

Migrating https://review.webrtc.org/57499004/ since old CR tool is about to
become read-only.

[phoglund, 2015-06-22 08:17:39]

I can't remember receiving the original version of this review? Anyway, what I
have to say is: please add unit tests :)

https://codereview.webrtc.org/1187573004/diff/20001/talk/examples/objc/AppRTC...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.m (right):

https://codereview.webrtc.org/1187573004/diff/20001/talk/examples/objc/AppRTC...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.m:33: 
This looks like something that would greatly benefit from a unit test. You
should be able to straightforwardly feed in session descriptions and test that
the SDP that comes out at the other end is correct?

[tkchin_webrtc, 2015-06-22 22:38:37]

On 2015/06/22 at 08:17:39, phoglund wrote:
> I can't remember receiving the original version of this review? Anyway, what I
have to say is: please add unit tests :)
> 
>
https://codereview.webrtc.org/1187573004/diff/20001/talk/examples/objc/AppRTC...
> File talk/examples/objc/AppRTCDemo/ARDSDPUtils.m (right):
> 
>
https://codereview.webrtc.org/1187573004/diff/20001/talk/examples/objc/AppRTC...
> talk/examples/objc/AppRTCDemo/ARDSDPUtils.m:33: 
> This looks like something that would greatly benefit from a unit test. You
should be able to straightforwardly feed in session descriptions and test that
the SDP that comes out at the other end is correct?

PTAL

@phoglund
I had initially put kjellander@ as a reviewer for the build files, but since
he's on paternity now I included you instead.
I added some basic UTs. fyi the SDP thing is pretty much a hack until we get to
implement RTPSender.

@pbos
any further comments?

[Chuck, 2015-06-22 22:42:59]

My only comments were agreeing about a unit test, but looks like you did that
already.

I can't claim expertise in this area, but LGTM.

[phoglund, 2015-06-23 07:23:56]

On 2015/06/22 22:38:37, tkchin_webrtc wrote:
> On 2015/06/22 at 08:17:39, phoglund wrote:
> > I can't remember receiving the original version of this review? Anyway, what
I
> have to say is: please add unit tests :)
> > 
> >
>
https://codereview.webrtc.org/1187573004/diff/20001/talk/examples/objc/AppRTC...
> > File talk/examples/objc/AppRTCDemo/ARDSDPUtils.m (right):
> > 
> >
>
https://codereview.webrtc.org/1187573004/diff/20001/talk/examples/objc/AppRTC...
> > talk/examples/objc/AppRTCDemo/ARDSDPUtils.m:33: 
> > This looks like something that would greatly benefit from a unit test. You
> should be able to straightforwardly feed in session descriptions and test that
> the SDP that comes out at the other end is correct?
> 
> PTAL
> 
> @phoglund
> I had initially put kjellander@ as a reviewer for the build files, but since
> he's on paternity now I included you instead.

gyp/gn files lgtm

> I added some basic UTs. fyi the SDP thing is pretty much a hack until we get
to
> implement RTPSender.

Ok. In that case I advise you name it hackhackSdpThing to signify it's a hack or
add some big TODOs, so this thing doesn't accidentally end up being used :)

> 
> @pbos
> any further comments?

[pbos-webrtc, 2015-06-23 19:13:56]

Seems like you can get rid of the encoder/decoder factories. I'd like you to do
that. If it's a software encoder we support I prefer we add it properly.

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
File talk/app/webrtc/objc/h264decoderfactory.h (right):

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
talk/app/webrtc/objc/h264decoderfactory.h:37: // eventually go away once we
build H264 support into the video engine.
"into the video engine"? Can you not do this now? If you add
H264Encoder::IsSupported() then WebRtcVideoEngine2 can use a check inside
CodecIsInternallySupported() and you can add support for it through there
instead.

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
File talk/app/webrtc/objc/h264encoderfactory.cc (right):

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
talk/app/webrtc/objc/h264encoderfactory.cc:39: // TODO(tkchin): Find better way
to check that encoder is supported. e.g.
Yes, please do that now. bool H264Encoder::IsSupported() const; and never have
::Create return a nullptr.

https://codereview.webrtc.org/1187573004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264.mm (right):

https://codereview.webrtc.org/1187573004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264.mm:28: // Bail if we aren't
actually running on an iOS8 device.
I do not believe we handle "return nullptr;" well at all outside this function.
If we claim to support H264 through cricket::VideoCodec (SDP) we must also
support it. You need to turn this into a query so that we can actually check
whether the encoder is supported or not before trying to create them.

[pbos-webrtc, 2015-06-23 19:15:17]

(I've also not checked the ObjC code very well.)

https://codereview.webrtc.org/1187573004/diff/40001/talk/build/common.gypi
File talk/build/common.gypi (right):

https://codereview.webrtc.org/1187573004/diff/40001/talk/build/common.gypi#ne...
talk/build/common.gypi:55: # Enable this to use HW H.264 encoder/decoder on
iOS/Mac peerconnections.
PeerConnections (or remove peerconnections).

[tkchin_webrtc, 2015-06-23 23:53:35]

Patchset #4 (id:60001) has been deleted

[tkchin_webrtc, 2015-06-24 21:13:42]

Patchset #5 (id:100001) has been deleted

[tkchin_webrtc, 2015-06-24 21:20:07]

Patchset #5 (id:120001) has been deleted

[tkchin_webrtc, 2015-06-24 21:21:48]

Patchset #5 (id:140001) has been deleted

[tkchin_webrtc, 2015-06-24 23:21:14]

PTAL

I removed OSX support because of problems mixing libstdc++ and libc++ bits on
10.9. This now happens because we're calling the h264 code from codec_database.
I originally had it there to make testing easier, but it's not super important
atm imo. Also I renamed the files to .cc because of some build error using .mm.
We'd changed it to .mm before because although <Availability.h> is a C-header,
nothing explicitly says it will stay a C header. If anything breaks it will be
between iOS versions and we will know really quickly anyway, so I'd rather fix
it if and when it happens rather than construct some weird build rules right
now.

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
File talk/app/webrtc/objc/h264decoderfactory.h (right):

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
talk/app/webrtc/objc/h264decoderfactory.h:37: // eventually go away once we
build H264 support into the video engine.
On 2015/06/23 19:13:56, pbos-webrtc wrote:
> "into the video engine"? Can you not do this now? If you add
> H264Encoder::IsSupported() then WebRtcVideoEngine2 can use a check inside
> CodecIsInternallySupported() and you can add support for it through there
> instead.

Done.

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
File talk/app/webrtc/objc/h264encoderfactory.cc (right):

https://codereview.webrtc.org/1187573004/diff/40001/talk/app/webrtc/objc/h264...
talk/app/webrtc/objc/h264encoderfactory.cc:39: // TODO(tkchin): Find better way
to check that encoder is supported. e.g.
On 2015/06/23 at 19:13:56, pbos-webrtc wrote:
> Yes, please do that now. bool H264Encoder::IsSupported() const; and never have
::Create return a nullptr.

Done.

https://codereview.webrtc.org/1187573004/diff/40001/talk/build/common.gypi
File talk/build/common.gypi (right):

https://codereview.webrtc.org/1187573004/diff/40001/talk/build/common.gypi#ne...
talk/build/common.gypi:55: # Enable this to use HW H.264 encoder/decoder on
iOS/Mac peerconnections.
On 2015/06/23 19:15:17, pbos-webrtc wrote:
> PeerConnections (or remove peerconnections).

Done.

https://codereview.webrtc.org/1187573004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264.mm (right):

https://codereview.webrtc.org/1187573004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264.mm:28: // Bail if we aren't
actually running on an iOS8 device.
On 2015/06/23 at 19:13:56, pbos-webrtc wrote:
> I do not believe we handle "return nullptr;" well at all outside this
function. If we claim to support H264 through cricket::VideoCodec (SDP) we must
also support it. You need to turn this into a query so that we can actually
check whether the encoder is supported or not before trying to create them.

Done.

[kjellander_webrtc, 2015-06-25 07:16:03]

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

[kjellander_webrtc, 2015-06-25 07:16:04]

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/build/common.gypi...
webrtc/build/common.gypi:126: 'rtc_use_objc_h264%': 0,
Traditionally we've only used the 'rtc_' prefix for the GN variables (they're
enforced by the GN style guide) but not in GYP, so I'd prefer to drop it here
for consistency (or change all in a separate CL).

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/BUILD.gn (right):

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/BUILD.gn:135: "../../system_wrappers",
nit: I prefer single-item lists on one line (recommended but not enforced in the
styleguide).

[tkchin_webrtc, 2015-06-25 17:36:11]

Patchset #6 (id:180001) has been deleted

[tkchin_webrtc, 2015-06-25 18:03:45]

Patchset #6 (id:200001) has been deleted

[tkchin_webrtc, 2015-06-25 18:13:54]

PTAL

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/build/common.gypi...
webrtc/build/common.gypi:126: 'rtc_use_objc_h264%': 0,
On 2015/06/25 07:16:03, kjellander (OOO til Aug 31) wrote:
> Traditionally we've only used the 'rtc_' prefix for the GN variables (they're
> enforced by the GN style guide) but not in GYP, so I'd prefer to drop it here
> for consistency (or change all in a separate CL).

Done.

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/BUILD.gn (right):

https://codereview.webrtc.org/1187573004/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/BUILD.gn:135: "../../system_wrappers",
On 2015/06/25 07:16:03, kjellander (OOO til Aug 31) wrote:
> nit: I prefer single-item lists on one line (recommended but not enforced in
the
> styleguide).

presubmit complained and made me run:
gn format --in-place webrtc/modules/video_coding/BUILD.gn
which reformats it to be multi-line.

[pbos-webrtc, 2015-06-25 20:20:13]

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

[pbos-webrtc, 2015-06-25 20:20:15]

Not much left for mah rubber, I'd like to have another eye (+R stefan@) on the
H264 parts since he has done H264 packetization he knows about it more than I
generally.

I have no idea about the codec whatsoever, but can better comment on wireup etc.

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/base/constant...
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/base/constant...
talk/media/base/constants.cc:135: // Matches VCM_H264_PAYLOAD_TYPE in
video_coding_defines.h.
Does this line matter? Also have you made sure that this doesn't overlap with
any other payload types?

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:162: return
webrtc::IsH264CodecSupported();
H264Encoder::IsSupported() && H264Decoder::IsSupported(), I like to keep these
decoupled in lower-level code even though SDP negotiation requires them to be
supported on both ends.

This causes a 2x function call, but you can have H264Decoder::IsSupported return
H264Encoder::IsSupported or smth to avoid a double implementation

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_objc.mm (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_objc.mm:27: // Not supported on
OS/X until we stop mixing libstdc++ and libc++ on
Add this as a TODO(tkchin)

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/include/h264.h (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/include/h264.h:30: extern bool
IsH264CodecSupported();
Remove from public interface, put inside static methods inside
H264Encoder/H264Decoder

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/codec_database.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/codec_database.cc:669: if
(IsH264CodecSupported()) {
H264Encoder::IsSupported()

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/codec_database.cc:709: if
(IsH264CodecSupported()) {
H264Decoder::IsSupported()

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
File webrtc/video/video_decoder.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
webrtc/video/video_decoder.cc:23: DCHECK(IsH264CodecSupported());
H264Encoder::IsSupported()

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
webrtc/video/video_decoder.cc:39: case kVideoCodecH264:
No DCHECK here?

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_encod...
File webrtc/video/video_encoder.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_encod...
webrtc/video/video_encoder.cc:23: DCHECK(IsH264CodecSupported());
Ditto

[tkchin_webrtc, 2015-06-25 21:37:50]

Stefan was the one who deferred this CL and added you instead pbos. 
Therefore, I will take your stamp as Stefan's stamp :P

If it makes you feel better, Noah lgtm-ed the version of this CL from the
previous CR tool if you look at the link, and he knows about the h264
packet/depacket stuff.

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/base/constant...
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/base/constant...
talk/media/base/constants.cc:135: // Matches VCM_H264_PAYLOAD_TYPE in
video_coding_defines.h.
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> Does this line matter? Also have you made sure that this doesn't overlap with
> any other payload types?

Apparently not - why is pltype defined in two places then?
No, I'd assumed that they should match. You should tell me what I should be
using here - I don't know the ranges for payload types  - a comment in
videoengine2.cc says 120-127 is reserved for external factories, so this should
probably be something else. 104 seems to work fine.

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:162: return
webrtc::IsH264CodecSupported();
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> H264Encoder::IsSupported() && H264Decoder::IsSupported(), I like to keep these
> decoupled in lower-level code even though SDP negotiation requires them to be
> supported on both ends.
> 
> This causes a 2x function call, but you can have H264Decoder::IsSupported
return
> H264Encoder::IsSupported or smth to avoid a double implementation

Done.

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_objc.mm (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_objc.mm:27: // Not supported on
OS/X until we stop mixing libstdc++ and libc++ on
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> Add this as a TODO(tkchin)

Done.

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/include/h264.h (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/include/h264.h:30: extern bool
IsH264CodecSupported();
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> Remove from public interface, put inside static methods inside
> H264Encoder/H264Decoder

Done.

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/codec_database.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/codec_database.cc:669: if
(IsH264CodecSupported()) {
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> H264Encoder::IsSupported()

Done.

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/codec_database.cc:709: if
(IsH264CodecSupported()) {
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> H264Decoder::IsSupported()

Done.

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
File webrtc/video/video_decoder.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
webrtc/video/video_decoder.cc:39: case kVideoCodecH264:
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> No DCHECK here?

Why? It doesn't make sense to me that enum conversion is dependent on whether
codec is supported. Makes sense when actually trying to create the decoder.

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_encod...
File webrtc/video/video_encoder.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_encod...
webrtc/video/video_encoder.cc:23: DCHECK(IsH264CodecSupported());
On 2015/06/25 20:20:14, pbos-webrtc wrote:
> Ditto

Done.

[pbos-webrtc, 2015-06-26 05:28:31]

lgtm with payload type changed, I've no idea how they interact if they overlap

holmer@: I guess you have until tkchin@ wakes up if you wanna check this CL out.
:)

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/base/constant...
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/talk/media/base/constant...
talk/media/base/constants.cc:135: // Matches VCM_H264_PAYLOAD_TYPE in
video_coding_defines.h.
On 2015/06/25 21:37:49, tkchin_webrtc wrote:
> On 2015/06/25 20:20:14, pbos-webrtc wrote:
> > Does this line matter? Also have you made sure that this doesn't overlap
with
> > any other payload types?
> 
> Apparently not - why is pltype defined in two places then?

Legacy reasons I think, not sure.

> No, I'd assumed that they should match. You should tell me what I should be
> using here - I don't know the ranges for payload types  - a comment in
> videoengine2.cc says 120-127 is reserved for external factories, so this
should
> probably be something else. 104 seems to work fine.

I believe you should pick something not used here nor for
talk/media/webrtc/webrtcvoiceengine.cc ( yes, more than one place defines these
:( ). 104 is default for 32k ISAC, 107 looks free. I hope there's no third
place.

I don't like it. https://www.youtube.com/watch?v=OP7H2fMdfqk

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
File webrtc/video/video_decoder.cc (right):

https://codereview.webrtc.org/1187573004/diff/220001/webrtc/video/video_decod...
webrtc/video/video_decoder.cc:39: case kVideoCodecH264:
On 2015/06/25 21:37:49, tkchin_webrtc wrote:
> On 2015/06/25 20:20:14, pbos-webrtc wrote:
> > No DCHECK here?
> 
> Why? It doesn't make sense to me that enum conversion is dependent on whether
> codec is supported. Makes sense when actually trying to create the decoder.

My bad, didn't pay attention here. :)

[stefan-webrtc, 2015-06-26 07:19:41]

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

https://codereview.webrtc.org/1187573004/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:425: //   
 kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration, 240);
Do you have any idea what the default is? Wondering if we should set it to 3000
as we do for VP8 and VP9.

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

https://codereview.webrtc.org/1187573004/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:49: class
AnnexBBufferReader final {
This class and the writer below deserve to have unittests I think.

[stefan-webrtc, 2015-06-26 07:21:58]

The description says that it's future work to get this working with the Android
AppRTCDemo. Is that referring to interop with Android?

[kjellander_webrtc, 2015-06-26 16:16:06]

*.gn* and .gyp*: lgtm

[tkchin_webrtc, 2015-06-26 20:28:09]

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

https://codereview.webrtc.org/1187573004/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:425: //   
 kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration, 240);
On 2015/06/26 07:19:41, stefan-webrtc (holmer) wrote:
> Do you have any idea what the default is? Wondering if we should set it to
3000
> as we do for VP8 and VP9.

No, I wanted to do all that work as part of a separate CL that tunes the
performance per the TODO.

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

https://codereview.webrtc.org/1187573004/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:49: class
AnnexBBufferReader final {
On 2015/06/26 07:19:41, stefan-webrtc (holmer) wrote:
> This class and the writer below deserve to have unittests I think.

These have already been included. Please see h264_video_toolbox_nalu_test.cc

[holmer, 2015-06-26 22:36:03]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2015-06-26 22:36:04]

lgtm

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

https://codereview.webrtc.org/1187573004/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:49: class
AnnexBBufferReader final {
On 2015/06/26 20:28:09, tkchin_webrtc wrote:
> On 2015/06/26 07:19:41, stefan-webrtc (holmer) wrote:
> > This class and the writer below deserve to have unittests I think.
> 
> These have already been included. Please see h264_video_toolbox_nalu_test.cc

Ah thanks. I would prefer if you named it ..._unittest.cc and put it in the same
directory as the .cc.

[tkchin_webrtc, 2015-06-29 18:43:55]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2015-06-29 18:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org,
haysc@webrtc.org, pbos@webrtc.org, kjellander@webrtc.org, holmer@google.com
Link to the patchset: https://codereview.webrtc.org/1187573004/#ps300001 (title:
"Move unittest file.")

[commit-bot: I haz the power, 2015-06-29 18:44:13]

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

[commit-bot: I haz the power, 2015-06-29 18:45:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-29 18:45:51]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/49)

[stefan-webrtc, 2015-06-29 18:50:22]

My mistake. LGTM.

[tkchin_webrtc, 2015-06-29 20:17:28]

tkchin@webrtc.org changed reviewers:
+ andrew@webrtc.org, jiayl@webrtc.org

[tkchin_webrtc, 2015-06-29 20:18:36]

+andrew for OWNERS of:
    webrtc/video_decoder.h
    webrtc/video_encoder.h

+jiayl for OWNERS of:
    talk/media/base/constants.cc
    talk/media/base/constants.h

[jiayl2, 2015-06-29 20:31:26]

lgtm for talk/media/base/constants.*

[Andrew MacDonald, 2015-06-29 21:20:34]

lgtm for webrtc/video_{en,de}coder.h

[tkchin_webrtc, 2015-06-29 21:21:56]

The CQ bit was checked by tkchin@webrtc.org

[commit-bot: I haz the power, 2015-06-29 21:22:12]

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

[commit-bot: I haz the power, 2015-06-29 21:23:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-29 21:23:55]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/51)

[tkchin_webrtc, 2015-06-29 21:35:14]

Committed patchset #10 (id:300001) manually as
71f6f4405c1c5f60097f8d10841378088e78e8b9 (presubmit successful).

[tkchin_webrtc, 2015-06-29 21:36:16]

Committed manually because cpplint is failing on ObjC files.

Thanks everyone for reviewing! Much appreciated.

[mpotapov, 2015-06-30 08:06:34]

vopatop.skam@gmail.com changed reviewers:
+ vopatop.skam@gmail.com

[mpotapov, 2015-06-30 08:06:35]

lgtm

https://codereview.webrtc.org/1187573004/diff/300001/talk/examples/objc/AppRT...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.m (right):

https://codereview.webrtc.org/1187573004/diff/300001/talk/examples/objc/AppRT...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.m:45: int mLineIndex = -1;
better to NSInteger here to avoid compiler warnings/errors