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 iPhone 6 + iPad Mini + OS/X. Future
work to get it working on simulator (renders black screen currently) and with
the Android AppRTCDemo. Currently protected with a compile time guard.


BUG=4081

[tkchin, 2015-05-28 21:34:07]

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

[tkchin, 2015-05-28 21:35:42]

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

[tkchin, 2015-05-28 22:37:22]

Patchset #1 (id:1) has been deleted

[tkchin, 2015-05-28 22:37:28]

Patchset #1 (id:20001) has been deleted

[tkchin, 2015-05-28 22:37:33]

Patchset #1 (id:40001) has been deleted

[tkchin, 2015-05-28 22:37:39]

Patchset #1 (id:60001) has been deleted

[tkchin, 2015-05-28 22:56:21]

Patchset #1 (id:80001) has been deleted

[tkchin, 2015-05-28 23:00:11]

Patchset #1 (id:100001) has been deleted

[tkchin, 2015-05-28 23:38:50]

Patchset #1 (id:120001) has been deleted

[tkchin, 2015-05-28 23:42:38]

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

[tkchin, 2015-05-28 23:43:38]

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

[tkchin, 2015-05-28 23:44:14]

+kjellander for the build file owners.

[noahric (chromium), 2015-05-29 00:25:52]

Cool beans :) I'm heading out in a sec, but I'll take a longer look at the
implementation tomorrow. These are mostly style/naming things.

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
File talk/app/webrtc/objc/h264decoderfactory.cc (right):

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
talk/app/webrtc/objc/h264decoderfactory.cc:35: // TODO(tkchin): check that
decoder can be created.
I'd clarify this TODO a bit; do you expect there to be cases where it can't be
created?

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
File talk/app/webrtc/objc/h264encoderfactory.h (right):

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
talk/app/webrtc/objc/h264encoderfactory.h:50: std::vector<VideoCodec>
supported_codecs_;
Add a quick comment and consider renaming this; it'll always be "h264" or
nothing (as implemented", but supported_codecs_ implies that there may be some
other stuff hiding there :)

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/build/common....
File talk/build/common.gypi (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/build/common....
talk/build/common.gypi:56: # This will break interop with Android clients with
H264 support.
Enabling it will break interop?

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/examples/objc...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.h (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/examples/objc...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.h:39: withVideoCodec:(NSString
*)codec;
withPreferredVideoCodec?

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264.cc:15: #include <Availability.h>
It's a C header, but it's unclear to me that it's *guaranteed* to be a C header,
so this may not actually be safe in a .cc file. Either way, consider renaming to
.mm.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264.cc:24: #if (defined(__IPHONE_8_0)
&&                            \
I'd do this once in h264.h and #define the result so it can be used in other
files. Generally not great to export defines like that, but probably better than
repeating it.

(If you didn't want to export it publicly, you could make an internal header,
though exporting it publicly may actually be useful, if something externally
wants to know that the compilation target supports h264 at all).

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:49: //
copy.
TODO for texture frames :-p

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:57: //
TODO(tkchin): use a pool.
Can you bold that text? :)

Though does that get more complicated since I420VideoFrame takes a ref? E.g. do
you end up having to do some "is only reference && do these other states hold"?

(no current action required, just that this is worth getting right; doing a full
buffer allocation on every frame is sadtimes)

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:131: //
The buffer we receive via RTP has 0x0001 start code artifically embedded
Prefer to write this out as 00 00 00 01 or 0x00000001. 0x0001 looks like two
bytes instead of 4.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:134: bool
is_sps = nalu_type == 0x7;
You don't need to do this anymore; I updated the h264 header to include the nalu
type (of the first nalu in the buffer).

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:136: if
(!H264RTPBufferToCMSampleBuffer(input_image._buffer, input_image._length,
Consider naming this H264AnnexBBuffer. RTP doesn't have any real meaning at this
point.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:137:
is_sps, video_format_, &sample_buffer)) {
Also the naming of "is_sps" disagrees in a subtle way with is_keyframe. It'd
probably be safer to either:
1) Not check at all here, and just let H264RTPBufferToCMSampleBuffer do the
right thing.
2) Pass in the keyframe flag on the header.
3) Pass in header nalu_type == 0x7, but change the name of the arg to
first_nalu_is_sps or something like that.

Of course, since H264...Buffer can do the same check, #3 really should be just
#1.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:175: // Parse
the SPS and PPS into a CMVideoFormatDescription.
Be careful about assuming they are in the same buffer. In practice, they are,
since many encoders produce them in a single buffer, they are small, and they
tend to get aggregated in packetization, but nothing is forcing them to be in
the same buffer. It'd be safer to have [something] ensure they are delivered all
at once; that could be the jitter_buffer, I guess, or it could be in your
decoder.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:309: // NALUs
are separated by an 0x0001 header. Scan the byte stream starting from
Ditto an earlier comment about writing this out in actual hex (00 00 00 01 or 0
0 0 1 etc.).

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:337: uint32_t
big_endian_length = CFSwapInt32HostToBig(data_size);
Consider using ByteBuffer for these operations; you can set the byte order,
WriteUint16, and do WriteBytes for the general memcpy case.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:32: bool
H264CMSampleBufferToRTPBuffer(CMSampleBufferRef avcc_sample_buffer,
Ditto on AnnexBBuffer instead of RTP buffer. Though this does produce a
fragmentation header, so either is probably fine.

[Stefan, 2015-05-30 11:32:03]

I have a few early comments. In general it would be nice if the codec wrapper
could be unittested somehow, any ideas?

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:190:
CMSampleBufferGetPresentationTimeStamp(sample_buffer);
Is this the presentation timestamp that we passed in when encoding this frame at
line 294?

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:197:
static_cast<int64_t>(CMTimeGetSeconds(presentation_timestamp) * 1000.0f);
Alternatively we could pass the timestamps from the input_image through the
FrameEncodeParams.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:231:
bitrate_ = codec_settings->startBitrate * 1024;
* 1000

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:279: }
Should only be one element in this array since this isn't a simulcast enabled
encoder.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:283: //
input_image.
There should be a timestamp, see
input_image.timestamp() for a 90 khz timestamp and
input_image.render_time_ms() for the same timestamp in milliseconds.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:320:
bitrate_ = new_bitrate_kbit * 1024;
I think you should be multiplying with 1000 here.

[kjellander, 2015-06-01 08:53:15]

Please introduce a rtc_use_objc_h264 variable in
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/webrt...
and perform all .gypi changes to the BUILD.gn files as well.

[tkchin, 2015-06-11 21:24:42]

Patchset #3 (id:180001) has been deleted

[tkchin, 2015-06-11 21:26:56]

@kjellander - afaik there isn't GN support for iOS frameworks, which means I
can't put this in GN files.

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
File talk/app/webrtc/objc/h264decoderfactory.cc (right):

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
talk/app/webrtc/objc/h264decoderfactory.cc:35: // TODO(tkchin): check that
decoder can be created.
On 2015/05/29 00:25:51, noahric (chromium) wrote:
> I'd clarify this TODO a bit; do you expect there to be cases where it can't be
> created?

Removed. It should succeed as long as we're running on iOS8. Added runtime
checks.

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
File talk/app/webrtc/objc/h264encoderfactory.h (right):

https://webrtc-codereview.appspot.com/57499004/diff/140001/talk/app/webrtc/ob...
talk/app/webrtc/objc/h264encoderfactory.h:50: std::vector<VideoCodec>
supported_codecs_;
On 2015/05/29 00:25:51, noahric (chromium) wrote:
> Add a quick comment and consider renaming this; it'll always be "h264" or
> nothing (as implemented", but supported_codecs_ implies that there may be some
> other stuff hiding there :)

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/build/common....
File talk/build/common.gypi (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/build/common....
talk/build/common.gypi:56: # This will break interop with Android clients with
H264 support.
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Enabling it will break interop?

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/talk/build/common....
talk/build/common.gypi:56: # This will break interop with Android clients with
H264 support.
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Enabling it will break interop?

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264.cc:15: #include <Availability.h>
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> It's a C header, but it's unclear to me that it's *guaranteed* to be a C
header,
> so this may not actually be safe in a .cc file. Either way, consider renaming
to
> .mm.

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264.cc:24: #if (defined(__IPHONE_8_0)
&&                            \
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> I'd do this once in h264.h and #define the result so it can be used in other
> files. Generally not great to export defines like that, but probably better
than
> repeating it.
> 
> (If you didn't want to export it publicly, you could make an internal header,
> though exporting it publicly may actually be useful, if something externally
> wants to know that the compilation target supports h264 at all).

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:49: //
copy.
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> TODO for texture frames :-p

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:57: //
TODO(tkchin): use a pool.
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Can you bold that text? :)
> 
> Though does that get more complicated since I420VideoFrame takes a ref? E.g.
do
> you end up having to do some "is only reference && do these other states
hold"?
> 
> (no current action required, just that this is worth getting right; doing a
full
> buffer allocation on every frame is sadtimes)

There is an existing buffer pool implementation already, just need to find a
good place to insert it.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
Taking a ref is fine.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:131: //
The buffer we receive via RTP has 0x0001 start code artifically embedded
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Prefer to write this out as 00 00 00 01 or 0x00000001. 0x0001 looks like two
> bytes instead of 4.

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:134: bool
is_sps = nalu_type == 0x7;
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> You don't need to do this anymore; I updated the h264 header to include the
nalu
> type (of the first nalu in the buffer).

Think I'm missing something - how can I access that header/nalu_type from here?

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:136: if
(!H264RTPBufferToCMSampleBuffer(input_image._buffer, input_image._length,
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Consider naming this H264AnnexBBuffer. RTP doesn't have any real meaning at
this
> point.

I wanted to make a distinction that this wasn't a general purpose AnnexB parser,
because it isn't. It just handles AnnexB as our RTP packetizer/depacketizer
defines it. But I can put that in the comments I suppose.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:137:
is_sps, video_format_, &sample_buffer)) {
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Also the naming of "is_sps" disagrees in a subtle way with is_keyframe. It'd
> probably be safer to either:
> 1) Not check at all here, and just let H264RTPBufferToCMSampleBuffer do the
> right thing.
> 2) Pass in the keyframe flag on the header.
> 3) Pass in header nalu_type == 0x7, but change the name of the arg to
> first_nalu_is_sps or something like that.
> 
> Of course, since H264...Buffer can do the same check, #3 really should be just
> #1.

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc
(right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:190:
CMSampleBufferGetPresentationTimeStamp(sample_buffer);
On 2015/05/30 11:32:02, Stefan wrote:
> Is this the presentation timestamp that we passed in when encoding this frame
at
> line 294?

Yes, or at least, it should be.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:197:
static_cast<int64_t>(CMTimeGetSeconds(presentation_timestamp) * 1000.0f);
On 2015/05/30 11:32:02, Stefan wrote:
> Alternatively we could pass the timestamps from the input_image through the
> FrameEncodeParams.

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:231:
bitrate_ = codec_settings->startBitrate * 1024;
On 2015/05/30 11:32:03, Stefan wrote:
> * 1000

! thank you.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:279: }
On 2015/05/30 11:32:02, Stefan wrote:
> Should only be one element in this array since this isn't a simulcast enabled
> encoder.

Ok. Are you suggesting that we just check the first element? Looping seems safer
than making assumption.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:283: //
input_image.
On 2015/05/30 11:32:02, Stefan wrote:
> There should be a timestamp, see
> input_image.timestamp() for a 90 khz timestamp and
> input_image.render_time_ms() for the same timestamp in milliseconds.

Odd, could've sworn I saw zeros there before. Thanks!

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc:320:
bitrate_ = new_bitrate_kbit * 1024;
On 2015/05/30 11:32:02, Stefan wrote:
> I think you should be multiplying with 1000 here.

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:175: // Parse
the SPS and PPS into a CMVideoFormatDescription.
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Be careful about assuming they are in the same buffer. In practice, they are,
> since many encoders produce them in a single buffer, they are small, and they
> tend to get aggregated in packetization, but nothing is forcing them to be in
> the same buffer. It'd be safer to have [something] ensure they are delivered
all
> at once; that could be the jitter_buffer, I guess, or it could be in your
> decoder.

That's fair. Added a TODO just so we can get something working checked in first
/ avoid CL bloat. Those CLs should be smaller.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:309: // NALUs
are separated by an 0x0001 header. Scan the byte stream starting from
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Ditto an earlier comment about writing this out in actual hex (00 00 00 01 or
0
> 0 0 1 etc.).

Done.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:337: uint32_t
big_endian_length = CFSwapInt32HostToBig(data_size);
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Consider using ByteBuffer for these operations; you can set the byte order,
> WriteUint16, and do WriteBytes for the general memcpy case.

Wanted to use the OS's functions (since CFSwapInt32HostToBig calls corresponding
OSSwap... method), looks like ByteBuffer does manual bit manipulation.
But I somehow doubt that there is significant perf gains.

More relevantly though, when using ByteBuffer we will write into its own
allocated memory and will have to copy it out to block buffer memory. Would be
good to avoid that.

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h (right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.h:32: bool
H264CMSampleBufferToRTPBuffer(CMSampleBufferRef avcc_sample_buffer,
On 2015/05/29 00:25:52, noahric (chromium) wrote:
> Ditto on AnnexBBuffer instead of RTP buffer. Though this does produce a
> fragmentation header, so either is probably fine.

Done.

[tkchin, 2015-06-16 20:04:57]

ping. Would be great to get this in the next day or so to avoid having to
migrate it to the new CR tool.

[kjellander, 2015-06-16 20:53:48]

On 2015/06/11 21:26:56, tkchin wrote:
> @kjellander - afaik there isn't GN support for iOS frameworks, which means I
> can't put this in GN files.

Sigh, I already replied to this a few days ago but that message seem to be lost
in space due to a bug in the new Rietveld UI not handling the warning for >1000
characters in a message. 

So, you're right about the lack of full iOS support in GN:
https://crbug.com/297668. 
May I ask that you add as much as possible for the GN build in this CL,
referencing https://crbug.com/297668? That way, it will be a lot easier to get a
fully working build when we transition in the future. With nothing in this CL,
one will have to back-track and manually compare every GN vs GYP file, which is
time consuming.

[noahric (chromium), 2015-06-17 22:54:21]

LGTM.

All nits, I don't see anything that looks wrong. My LGTM shouldn't count for
much, though, so I'd wait for someone else to take a look :) Consider poking
jiawei to take a look too.

(Assuming it'll take a few days for others to LGTM, especially with the GN
stuff, I may take another look at the annex b/avcc magic)

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://webrtc-codereview.appspot.com/57499004/diff/160001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:134: bool
is_sps = nalu_type == 0x7;
On 2015/06/11 21:26:55, tkchin wrote:
> On 2015/05/29 00:25:52, noahric (chromium) wrote:
> > You don't need to do this anymore; I updated the h264 header to include the
> nalu
> > type (of the first nalu in the buffer).
> 
> Think I'm missing something - how can I access that header/nalu_type from
here? 

Ah, you're right. I thought it was going to be in codec_specific_info, but I
guess it isn't carried through at this point. Parsing the buffer is fine.

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
File talk/examples/objc/AppRTCDemo/ARDAppClient.m (right):

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
talk/examples/objc/AppRTCDemo/ARDAppClient.m:349: RTCSessionDescription
*mangledSDP =
Consider a name other than "mangled" :) Maybe modifiedSDP? Or sdpPreferringH264?

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.h (right):

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.h:34: // Mangles the original SDP
description to instead prefer the specified video
"Updates" :-p

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.h:39: withVideoCodec:(NSString
*)codec;
Consider withPreferredVideoCodec: or just preferredVideoCodec: (depending on
which style this codebase ascribes to). Something to make it more obvious from
the call site.

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.m (right):

https://webrtc-codereview.appspot.com/57499004/diff/200001/talk/examples/objc...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.m:40: // Copied from
PeerConnectionClient.java.
I'd TODO or open a bug for this, then. It's pretty hairy code to require every
application to duplicate in their app's language of choice, especially when the
outer API is relatively simple.

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm
(right):

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:73:
LOG(LS_ERROR) << "Error converting NV12 to I420 :" << ret;
extra space before : (but should have a space after)

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:91:
LOG(LS_ERROR) << "Failed to decode frame. Status:" << status;
space after :

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:97:
webrtc::kVideoRotation_0);
Will this break with CVO? Either way, can you enable CVO by passing rotation
information in the last param of VTDecompressionSessionDecodeFrame?

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(right):

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:226:
width_ = codec_settings->width;
Something to TODO to check for: assuming you're testing with a camera, you're
probably getting nicely aligned dimensions. Depending on the usage/context, you
may have to enforce some kind of alignment requirement here. E.g. on Android,
not respecting implicit alignment values will, depending on the device, (a)
work, (b) work, but crop things in sometimes humorous ways, (c) fail to create
the coder, or (d) crash. Hopefully not as bad here, but something to watch out
for :)

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:239: const
CodecSpecificInfo* codec_specific_info,
You should plumb this through the VTCompressionSessionEncodeFrame.
CodecSpecificInfoH264 is blank today, but it probably won't be forever, so it's
not sufficient to just create an empty one on the other side.

(At least TODO this)

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:241: if
(input_image.IsZeroSize()) {
That's not actually an error, so consider just OKing it optionally with an
assert to say that it's a valid but wrong code path.

(Encoders registered with internal sources will receive zero size input image
Encode requests as keyframe requests, because it fakes up frames to use since
the frames aren't flowing through the normal camera pipeline. internal source
stuff probably won't make it to webrtcvideoengine2 or at least not live long if
it does).

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:258: //
TODO(tkchin): make sure that this drops frames but future encodes
Can probably remove this TODO. Quick inspection should tell you one way or the
other (I'm pretty sure I looked at this last week and the value is basically
ignored, so only this frame is dropped).

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm (right):

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:313: const
uint8_t* current = start + offset;
We should probably find a place to share this now, at least between android and
ios. Not necessary for this CL.

https://webrtc-codereview.appspot.com/57499004/diff/200001/webrtc/modules/vid...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:314: const
uint8_t* const end = start + length;
Consider subtracting off sizeof(kAnnexBHeaderBytes) here. Hopefully it'll be
optimized to that anyways, but there's no great reason to calculate end halfway.
You can also add a comment if you want it clearer:
// Stop sizeof(kAnnexBHeaderBytes) early because the loop reads that many bytes
at a time.

[Stefan, 2015-06-18 07:47:33]

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

[Stefan, 2015-06-18 07:47:34]

I'll leave the main review of this CL to pbos, who's focusing a bit more on HW
codecs.

[pbos, 2015-06-18 12:14:23]

Some initial things in webrtc/, I really hope Chuck can help out with
iOS/Mac/ObjC specifics.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264.gypi:25: 'h264.mm',
Shouldn't these sources be under the ios/mac conditional?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264.gypi:34: 'target_name':
'webrtc_h264_video_toolbox',
What's a toolbox here? I don't understand this target name (or why there's two
targets, isn't webrtc_h264 enough?).

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264.mm (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264.mm:28: // Bail if we aren't
actually running on an iOS8 device.
Is there any other way you can check for API availability that doesn't scream
"version number"?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.h
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.h:24: //
experimentation. Use at your own risk ¯\_(ツ)_/¯
Remove emojis, especially non-ascii characters.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:14: #if
defined(WEBRTC_VIDEO_TOOLBOX_SUPPORTED)
Can this be defined/known from the gyp file so that we can remove the defines
from the source files?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:55: //
TODO(tkchin): <b>Use a pool!</b>
Remove html tags, Use a frame-buffer pool.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:185: // We
want decoded frames to exist on GPU so that we can avoid a texture
Can you explain what you're doing rather than what you want?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:187: //
i420 frame after decode, but eventually we will be able to plumb
I420

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:188: //
CVPixelBuffers directly through the engine.
directly to the renderer

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:189: //
TODO(tkchin): maybe only do this if we know that we will be getting native
native handles from where? decoder? what is the "this" that we're doing?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:25: //
experimentation. Use at your own risk ¯\_(ツ)_/¯
Remove emoji, also Use at your own risk, it's implied.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:36: //
Copies characters from a CFStringRef into a std::string.
Do you benefit a lot from not copying this string? It's only used for errors, so
return std::string and use it?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:38: if
(!cf_string || !std_string) {
DCHECK these?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:48: if
(!buffer) {
That doesn't happen, new doesn't return null for non-zero sizes, it throws
exceptions (bad_alloc) if it doesn't work.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:115: // We
receive I420Frames as input, but we need to feed CVPixelBuffers into the
Should this be where we have also accept a native handle, or is that a future
change?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:164:
LOG(LS_ERROR) << "H264 encoding failed.";
Should this already have logged an error (do we double-log any errors)?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:182: //
TODO(tkchin): allocate buffers through a pool.
Allocate

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:289:
encode_params.reset(new internal::FrameEncodeParams(callback_, width_,
Isn't all this heap allocation in an active path expensive?

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:403: //
TODO(tkchin): look at entropy mode and colorspace matrices.
Look

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:405: //
TODO(tkchin): investigate to see if there's any way to make this work.
Investigate, also remove /**/ comments (since // are a lot more common, just use
those, it's 3 lines and you can remove your added 2 lines.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:411: //
Require a keyframe every 240 frames or every 240 seconds, whichever comes
Every 240 frames sounds like a lot of keyframes (for VP8 we have 3k). There's no
point in emitting them in itself.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:54:
DCHECK_EQ(nalu_header_size, (int)4);
you shouldn't need a cast here

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:55:
DCHECK_EQ(param_set_count, (size_t)2);
2u

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:106:
CFRetain(contiguous_buffer);
Can you make a scoped_ptr equivalent for this CF thing? I really don't like all
the manual CFReleases etc. Especially since I don't know the semantics.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/include/h264.h (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/include/h264.h:19:
__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_8_0) || \
Can this be determined from the gyp file instead?

[tkchin, 2015-06-19 00:10:10]

Migrating to https://codereview.webrtc.org/1187573004 since this CR tool is
becoming read-only.

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
File talk/examples/objc/AppRTCDemo/ARDAppClient.m (right):

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
talk/examples/objc/AppRTCDemo/ARDAppClient.m:349: RTCSessionDescription
*mangledSDP =
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> Consider a name other than "mangled" :) Maybe modifiedSDP? Or
sdpPreferringH264?

Done.

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.h (right):

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.h:34: // Mangles the original SDP
description to instead prefer the specified video
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> "Updates" :-p

Done.

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.h:39: withVideoCodec:(NSString
*)codec;
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> Consider withPreferredVideoCodec: or just preferredVideoCodec: (depending on
> which style this codebase ascribes to). Something to make it more obvious from
> the call site.

Done.

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
File talk/examples/objc/AppRTCDemo/ARDSDPUtils.m (right):

https://review.webrtc.org/57499004/diff/200001/talk/examples/objc/AppRTCDemo/...
talk/examples/objc/AppRTCDemo/ARDSDPUtils.m:40: // Copied from
PeerConnectionClient.java.
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> I'd TODO or open a bug for this, then. It's pretty hairy code to require every
> application to duplicate in their app's language of choice, especially when
the
> outer API is relatively simple.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264.gypi:25: 'h264.mm',
On 2015/06/18 12:14:21, pbos wrote:
> Shouldn't these sources be under the ios/mac conditional?

No. See other comment.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264.gypi:34: 'target_name':
'webrtc_h264_video_toolbox',
On 2015/06/18 12:14:22, pbos wrote:
> What's a toolbox here? I don't understand this target name (or why there's two
> targets, isn't webrtc_h264 enough?).

VIdeoToolbox is the name of the iOS/OSX framework (library) that handles h264
encoding/decoding.
There are two targets because I anticipate moving the Android equivalent here as
well, as opposed to having it bundled into AppRTCDemo. That will make it easier
for third party consumers to use.
People who care about h264 should add webrtc_h264 as a dependency, and based on
config we will add the correct implementation for the platform.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264.mm (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264.mm:28: // Bail if we aren't
actually running on an iOS8 device.
On 2015/06/18 12:14:22, pbos wrote:
> Is there any other way you can check for API availability that doesn't scream
> "version number"?

No. You can compile using an iOS8 SDK and deploy on an older OS, eg iOS7. iOS8
specific method calls will then fail.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.h
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.h:24: //
experimentation. Use at your own risk ¯\_(ツ)_/¯
On 2015/06/18 12:14:22, pbos wrote:
> Remove emojis, especially non-ascii characters.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:14: #if
defined(WEBRTC_VIDEO_TOOLBOX_SUPPORTED)
On 2015/06/18 12:14:22, pbos wrote:
> Can this be defined/known from the gyp file so that we can remove the defines
> from the source files?

No. The value of this define is derived from an SDK include. See h264.h.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:55: //
TODO(tkchin): <b>Use a pool!</b>
On 2015/06/18 12:14:22, pbos wrote:
> Remove html tags, Use a frame-buffer pool.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:73:
LOG(LS_ERROR) << "Error converting NV12 to I420 :" << ret;
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> extra space before : (but should have a space after)

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:91:
LOG(LS_ERROR) << "Failed to decode frame. Status:" << status;
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> space after :

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:97:
webrtc::kVideoRotation_0);
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> Will this break with CVO? Either way, can you enable CVO by passing rotation
> information in the last param of VTDecompressionSessionDecodeFrame?

Added a TODO to figure out how CVO works on iOS. Afaik the capturer right now
already adjusts it so nothing needs to be rotated.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:185: // We
want decoded frames to exist on GPU so that we can avoid a texture
On 2015/06/18 12:14:22, pbos wrote:
> Can you explain what you're doing rather than what you want?

This comment is actually explaining what we are doing. But you're right that you
won't understand this comment if you haven't interacted with iOS texture things
before.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:187: //
i420 frame after decode, but eventually we will be able to plumb
On 2015/06/18 12:14:22, pbos wrote:
> I420

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:188: //
CVPixelBuffers directly through the engine.
On 2015/06/18 12:14:22, pbos wrote:
> directly to the renderer

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.mm:189: //
TODO(tkchin): maybe only do this if we know that we will be getting native
On 2015/06/18 12:14:22, pbos wrote:
> native handles from where? decoder? what is the "this" that we're doing?

let me know if this is still too cryptic for you.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:25: //
experimentation. Use at your own risk ¯\_(ツ)_/¯
On 2015/06/18 12:14:22, pbos wrote:
> Remove emoji, also Use at your own risk, it's implied.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:36: //
Copies characters from a CFStringRef into a std::string.
On 2015/06/18 12:14:22, pbos wrote:
> Do you benefit a lot from not copying this string? It's only used for errors,
so
> return std::string and use it?

Returning std::string is fine, I'm down with saving lines.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:38: if
(!cf_string || !std_string) {
On 2015/06/18 12:14:22, pbos wrote:
> DCHECK these?

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:48: if
(!buffer) {
On 2015/06/18 12:14:22, pbos wrote:
> That doesn't happen, new doesn't return null for non-zero sizes, it throws
> exceptions (bad_alloc) if it doesn't work.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:115: // We
receive I420Frames as input, but we need to feed CVPixelBuffers into the
On 2015/06/18 12:14:22, pbos wrote:
> Should this be where we have also accept a native handle, or is that a future
> change?

Future change (the native handle == CVPixelBuffer). So need to check for native
handle and pass it along instead of copy.
Native handle bits weren't around when the CL started so I'd rather do it later.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:164:
LOG(LS_ERROR) << "H264 encoding failed.";
On 2015/06/18 12:14:22, pbos wrote:
> Should this already have logged an error (do we double-log any errors)?

No. This is the result of the encoding.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:182: //
TODO(tkchin): allocate buffers through a pool.
On 2015/06/18 12:14:22, pbos wrote:
> Allocate

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:226:
width_ = codec_settings->width;
On 2015/06/17 22:54:21, noahric (chromium) wrote:
> Something to TODO to check for: assuming you're testing with a camera, you're
> probably getting nicely aligned dimensions. Depending on the usage/context,
you
> may have to enforce some kind of alignment requirement here. E.g. on Android,
> not respecting implicit alignment values will, depending on the device, (a)
> work, (b) work, but crop things in sometimes humorous ways, (c) fail to create
> the coder, or (d) crash. Hopefully not as bad here, but something to watch out
> for :)

That's good to know, thanks.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:239: const
CodecSpecificInfo* codec_specific_info,
On 2015/06/17 22:54:20, noahric (chromium) wrote:
> You should plumb this through the VTCompressionSessionEncodeFrame.
> CodecSpecificInfoH264 is blank today, but it probably won't be forever, so
it's
> not sufficient to just create an empty one on the other side.
> 
> (At least TODO this)

Makes sense, thanks.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:241: if
(input_image.IsZeroSize()) {
On 2015/06/17 22:54:21, noahric (chromium) wrote:
> That's not actually an error, so consider just OKing it optionally with an
> assert to say that it's a valid but wrong code path.
> 
> (Encoders registered with internal sources will receive zero size input image
> Encode requests as keyframe requests, because it fakes up frames to use since
> the frames aren't flowing through the normal camera pipeline. internal source
> stuff probably won't make it to webrtcvideoengine2 or at least not live long
if
> it does).

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:258: //
TODO(tkchin): make sure that this drops frames but future encodes
On 2015/06/17 22:54:21, noahric (chromium) wrote:
> Can probably remove this TODO. Quick inspection should tell you one way or the
> other (I'm pretty sure I looked at this last week and the value is basically
> ignored, so only this frame is dropped).

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:289:
encode_params.reset(new internal::FrameEncodeParams(callback_, width_,
On 2015/06/18 12:14:22, pbos wrote:
> Isn't all this heap allocation in an active path expensive?

No perf data as of yet. Tuning CL comes after initial impl.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:403: //
TODO(tkchin): look at entropy mode and colorspace matrices.
On 2015/06/18 12:14:22, pbos wrote:
> Look

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:405: //
TODO(tkchin): investigate to see if there's any way to make this work.
On 2015/06/18 12:14:22, pbos wrote:
> Investigate, also remove /**/ comments (since // are a lot more common, just
use
> those, it's 3 lines and you can remove your added 2 lines.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:411: //
Require a keyframe every 240 frames or every 240 seconds, whichever comes
On 2015/06/18 12:14:22, pbos wrote:
> Every 240 frames sounds like a lot of keyframes (for VP8 we have 3k). There's
no
> point in emitting them in itself.

I'm fine killing this for now and tuning later.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:54:
DCHECK_EQ(nalu_header_size, (int)4);
On 2015/06/18 12:14:23, pbos wrote:
> you shouldn't need a cast here

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:55:
DCHECK_EQ(param_set_count, (size_t)2);
On 2015/06/18 12:14:23, pbos wrote:
> 2u

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:106:
CFRetain(contiguous_buffer);
On 2015/06/18 12:14:22, pbos wrote:
> Can you make a scoped_ptr equivalent for this CF thing? I really don't like
all
> the manual CFReleases etc. Especially since I don't know the semantics.

There's one in chromium that we can potentially rip, but I don't want to do that
in this CL.
Perhaps you should read up on the semantics then :-) It's really not so
different from regular ref counting interfaces, and it's a requisite bit of
knowledge when interacting with Core Foundation.
https://developer.apple.com/library/ios/documentation/CoreFoundation/Referenc...

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:313: const
uint8_t* current = start + offset;
On 2015/06/17 22:54:21, noahric (chromium) wrote:
> We should probably find a place to share this now, at least between android
and
> ios. Not necessary for this CL.

Yeah, hopefully one day we can put everything in codecs/h264.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.mm:314: const
uint8_t* const end = start + length;
On 2015/06/17 22:54:21, noahric (chromium) wrote:
> Consider subtracting off sizeof(kAnnexBHeaderBytes) here. Hopefully it'll be
> optimized to that anyways, but there's no great reason to calculate end
halfway.
> You can also add a comment if you want it clearer:
> // Stop sizeof(kAnnexBHeaderBytes) early because the loop reads that many
bytes
> at a time.

Done.

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
File webrtc/modules/video_coding/codecs/h264/include/h264.h (right):

https://review.webrtc.org/57499004/diff/200001/webrtc/modules/video_coding/co...
webrtc/modules/video_coding/codecs/h264/include/h264.h:19:
__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_8_0) || \
On 2015/06/18 12:14:23, pbos wrote:
> Can this be determined from the gyp file instead?

No. Requires include of Availability.h.