Add a webrtc{en,de}coderfactory implementation for VideoToolbox

Add a webrtc{en,de}coderfactory implementation for VideoToolbox

This CL removes the coupling of the VideoToolbox h264 implementation
to the generic h264 code. The files have been moved into sdb/obj/Framework
and all dependency on them has been removed from the rest of WebRTC.
We now add it as an external encoder via a factory supplied to the
CreatePeerConnectionFactory call. This also brings the iOS implementation
closer to what we do on Android for MediaCodec.

BUG=webrtc:6619
Committed: https://crrev.com/6a5047dad31f14f53dd9f8bc1ecde19e1dede2e4
Cr-Commit-Position: refs/heads/master@{#14953}

[kthelgason, 2016-11-01 08:57:24]

kthelgason@webrtc.org changed reviewers:
+ magjed@webrtc.org

[kthelgason, 2016-11-01 08:57:42]

The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 08:57:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kthelgason, 2016-11-01 09:08:49]

kthelgason@webrtc.org changed reviewers:
+ holmer@webrtc.org

[kthelgason, 2016-11-01 09:08:50]

added holmer@ as an owner

[commit-bot: I haz the power, 2016-11-01 09:36:04]

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

[commit-bot: I haz the power, 2016-11-01 09:36:06]

Dry run: Try jobs failed on following builders:
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2674)

[magjed_webrtc, 2016-11-01 14:00:21]

Thanks a lot, you fixed it fast and the CL is very clean!

https://codereview.webrtc.org/2463313002/diff/1/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/DEPS#newcode30
webrtc/DEPS:30: "+webrtc/common_video",
Should this be common_video/include instead? Also, can you remove the special
cases for common_video elsewhere in this file?

https://codereview.webrtc.org/2463313002/diff/1/webrtc/DEPS#newcode32
webrtc/DEPS:32: "+webrtc/modules/video_coding",
I'm uncertain about adding webrtc/modules/video_coding in the top DEPS file. Can
you try to add it to a DEPS filer closer to the usage, i.e. webrtc/sdk/DEPS?
Maybe we should even create a new webrtc/sdk/objc/Framework/Classes/DEPS file if
that makes sense.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/h264/h264.gypi:20: 'h264_objc.mm',
Is it possible to nuke h264_objc.mm and remove this whole '['OS=="ios"' clause?

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:49: const auto
encoder_factory = new webrtc::VideoToolboxVideoEncoderFactory();
nit: Maybe this would be slightly more clear:

std::unique_ptr<cricket::WebRtcVideoEncoderFactory> encoder_factory(
    new webrtc::VideoToolboxVideoEncoderFactory());
...
encoder_factory.release()

but I don't feel strongly about it.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:21: static
const char* NameFromCodecType(webrtc::VideoCodecType type) {
nit: We should probably move this function to webrtc/media/base/codec.h. I see
the same logic duplicated at least in:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
This is fine for now though, and we can fix it in a separate CL.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:41:
supported_codecs_.push_back(cricket::VideoCodec("H264"));
nit: Maybe we should use cricket::kH264CodecName here.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:49: #if
defined(WEBRTC_IOS)
Can we have just one ifdef in the ctor and leave the rest of the code generic,
i.e. not have ifdefs here and in the rest of the functions.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:86: if
(IsCodecSupported(supported_codecs_, codec)) {
Thanks for implementing it like this. I want to replace webrtc::VideoCodecType
with cricket::VideoCodec in the near future, and now we have already prepared
for that.

[kthelgason, 2016-11-01 15:14:31]

Thanks for the review magjed@. I think I addressed all of your comments. Care to
take another look?

https://codereview.webrtc.org/2463313002/diff/1/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/DEPS#newcode30
webrtc/DEPS:30: "+webrtc/common_video",
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> Should this be common_video/include instead? Also, can you remove the special
> cases for common_video elsewhere in this file?

Actually, it could be common_video/h264 but I think leaving it like this and
removing the other special cases is better.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/DEPS#newcode32
webrtc/DEPS:32: "+webrtc/modules/video_coding",
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> I'm uncertain about adding webrtc/modules/video_coding in the top DEPS file.
Can
> you try to add it to a DEPS filer closer to the usage, i.e. webrtc/sdk/DEPS?
> Maybe we should even create a new webrtc/sdk/objc/Framework/Classes/DEPS file
if
> that makes sense.

Acknowledged.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/h264/h264.gypi:20: 'h264_objc.mm',
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> Is it possible to nuke h264_objc.mm and remove this whole '['OS=="ios"'
clause?

Yup, great, thanks for pointing that out!

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:49: const auto
encoder_factory = new webrtc::VideoToolboxVideoEncoderFactory();
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> nit: Maybe this would be slightly more clear:
> 
> std::unique_ptr<cricket::WebRtcVideoEncoderFactory> encoder_factory(
>     new webrtc::VideoToolboxVideoEncoderFactory());
> ...
> encoder_factory.release()
> 
> but I don't feel strongly about it.

I think that's a lot of typing to no specific purpose, but I understand what you
mean. I added a comment clarifying the transfer of ownership, but I can do this
as well if you want.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc (right):

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:21: static
const char* NameFromCodecType(webrtc::VideoCodecType type) {
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> nit: We should probably move this function to webrtc/media/base/codec.h. I see
> the same logic duplicated at least in:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
> This is fine for now though, and we can fix it in a separate CL.

Yeah, I agree that it's dirty to be pasting this around, but I was hoping it
would go away quickly with the update of WebRtcVideoDecoderFactory.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:41:
supported_codecs_.push_back(cricket::VideoCodec("H264"));
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> nit: Maybe we should use cricket::kH264CodecName here.

Done.

https://codereview.webrtc.org/2463313002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:49: #if
defined(WEBRTC_IOS)
On 2016/11/01 14:00:20, magjed_webrtc wrote:
> Can we have just one ifdef in the ctor and leave the rest of the code generic,
> i.e. not have ifdefs here and in the rest of the functions.

We could do that, but that requires compiling the files on macOS as well.
There's nothing really wrong with that it just seemed like a bit of a waste.

[magjed_webrtc, 2016-11-01 15:28:21]

lgtm

[magjed_webrtc, 2016-11-03 13:32:04]

holmer - ping. I would like to get this in soon since it's blocking other CL:s.
Don't be intimated by the line count - this CL is actually small, just that
files have been moved around.

[stefan-webrtc, 2016-11-07 13:44:44]

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

[stefan-webrtc, 2016-11-07 13:44:45]

lgtm

[kthelgason, 2016-11-07 13:47:57]

The CQ bit was checked by kthelgason@webrtc.org

[commit-bot: I haz the power, 2016-11-07 13:48:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-07 15:26:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-07 15:26:09]

Description was changed from

==========
Add a webrtc{en,de}coderfactory implementation for VideoToolbox

This CL removes the coupling of the VideoToolbox h264 implementation
to the generic h264 code. The files have been moved into sdb/obj/Framework
and all dependency on them has been removed from the rest of WebRTC.
We now add it as an external encoder via a factory supplied to the
CreatePeerConnectionFactory call. This also brings the iOS implementation
closer to what we do on Android for MediaCodec.

BUG=webrtc:6619
==========

to

==========
Add a webrtc{en,de}coderfactory implementation for VideoToolbox

This CL removes the coupling of the VideoToolbox h264 implementation
to the generic h264 code. The files have been moved into sdb/obj/Framework
and all dependency on them has been removed from the rest of WebRTC.
We now add it as an external encoder via a factory supplied to the
CreatePeerConnectionFactory call. This also brings the iOS implementation
closer to what we do on Android for MediaCodec.

BUG=webrtc:6619

Committed: https://crrev.com/6a5047dad31f14f53dd9f8bc1ecde19e1dede2e4
Cr-Commit-Position: refs/heads/master@{#14953}
==========

[commit-bot: I haz the power, 2016-11-07 15:26:10]

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

[kwiberg-webrtc, 2016-11-08 16:49:34]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2484913003/ by kwiberg@webrtc.org.

The reason for reverting is: Broke dependent project because the .gn changes
weren't accompanied by corresponding .gyp changes..