ObjC: Implement HW codecs in ObjC instead of C++

ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

Based on andersc@ CL: https://codereview.webrtc.org/2978623002/.

BUG=webrtc:7924
Review-Url: https://codereview.webrtc.org/2987413002
Cr-Commit-Position: refs/heads/master@{#19255}
Committed: https://chromium.googlesource.com/external/webrtc/+/73c0eb50143a04eaf835aba0cc8ddc654661aedf

[magjed_webrtc, 2017-08-03 12:23:37]

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

[commit-bot: I haz the power, 2017-08-03 12:23:45]

Dry run: CQ is trying da patch.

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

[magjed_webrtc, 2017-08-03 13:42:22]

Description was changed from

==========
ObjC codecs: Remove C++ -> ObjC wrapping

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
==========

to

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
==========

[magjed_webrtc, 2017-08-03 14:54:24]

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

[commit-bot: I haz the power, 2017-08-03 14:54:26]

Dry run: CQ is trying da patch.

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

[magjed_webrtc, 2017-08-03 14:54:45]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2017-08-03 14:54:53]

Patchset #1 (id:20001) has been deleted

[magjed_webrtc, 2017-08-03 14:55:01]

Patchset #1 (id:40001) has been deleted

[magjed_webrtc, 2017-08-03 14:55:42]

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

[commit-bot: I haz the power, 2017-08-03 14:55:45]

Dry run: CQ is trying da patch.

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

[magjed_webrtc, 2017-08-03 14:55:54]

Patchset #1 (id:60001) has been deleted

[magjed_webrtc, 2017-08-03 14:56:13]

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

[magjed_webrtc, 2017-08-03 14:58:28]

Anders - take a look if you have time, otherwise I'll Daniela review.

[magjed_webrtc, 2017-08-03 18:51:59]

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

[commit-bot: I haz the power, 2017-08-03 18:52:05]

Dry run: CQ is trying da patch.

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

[magjed_webrtc, 2017-08-03 18:56:28]

Patchset #2 (id:100001) has been deleted

[magjed_webrtc, 2017-08-03 18:56:39]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-08-03 19:24:17]

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

[commit-bot: I haz the power, 2017-08-03 19:24:18]

Dry run: This issue passed the CQ dry run.

[andersc, 2017-08-03 20:29:22]

On 2017/08/03 14:58:28, magjed_webrtc wrote:
> Anders - take a look if you have time, otherwise I'll Daniela review.

Has anything important changed between https://codereview.webrtc.org/2978623002/
and this code, or is it mostly a rebase? Otherwise I guess I'd mostly be
reviewing my own code.

[magjed_webrtc, 2017-08-04 12:38:03]

On 2017/08/03 20:29:22, andersc wrote:
> On 2017/08/03 14:58:28, magjed_webrtc wrote:
> > Anders - take a look if you have time, otherwise I'll Daniela review.
> 
> Has anything important changed between
https://codereview.webrtc.org/2978623002/
> and this code, or is it mostly a rebase? Otherwise I guess I'd mostly be
> reviewing my own code.

It's mostly a rebase. I did some fixes that I found. One import fix is that I
re-added the parsing of the H264 profile-level-id. I saw that you tried to send
it as a string in SDP using label "videotoolbox-profile", but this is not
following the standard and would break interop with all other clients except our
own iOS and Mac implementation. I did some simplifications of the
objc_codec_h264_test and webrtc/modules/video_coding/BUILD.gn. Also, I don't
move any files in this CL, and I re-arranged the code in order to minimize the
diff against master as much as I could.

[magjed_webrtc, 2017-08-04 15:11:31]

Description was changed from

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
==========

to

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
TBR=andersc@webrtc.org
==========

[magjed_webrtc, 2017-08-04 15:12:27]

Description was changed from

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
TBR=andersc@webrtc.org
==========

to

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
==========

[magjed_webrtc, 2017-08-04 15:12:27]

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

[magjed_webrtc, 2017-08-04 15:17:19]

brandtr - please review the changes to webrtc/modules/video_coding.

[andersc, 2017-08-04 18:29:27]

On 2017/08/04 12:38:03, magjed_webrtc wrote:
> On 2017/08/03 20:29:22, andersc wrote:
> > On 2017/08/03 14:58:28, magjed_webrtc wrote:
> > > Anders - take a look if you have time, otherwise I'll Daniela review.
> > 
> > Has anything important changed between
> https://codereview.webrtc.org/2978623002/
> > and this code, or is it mostly a rebase? Otherwise I guess I'd mostly be
> > reviewing my own code.
> 
> It's mostly a rebase. I did some fixes that I found. One import fix is that I
> re-added the parsing of the H264 profile-level-id. I saw that you tried to
send
> it as a string in SDP using label "videotoolbox-profile", but this is not
> following the standard and would break interop with all other clients except
our
> own iOS and Mac implementation. I did some simplifications of the
> objc_codec_h264_test and webrtc/modules/video_coding/BUILD.gn. Also, I don't
> move any files in this CL, and I re-arranged the code in order to minimize the
> diff against master as much as I could.

I see, that all sounds good.

lgtm

[magjed_webrtc, 2017-08-05 14:36:50]

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

[commit-bot: I haz the power, 2017-08-05 14:36:56]

Dry run: 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, 2017-08-05 15:05:47]

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

[commit-bot: I haz the power, 2017-08-05 15:05:48]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-08-07 08:01:57]

video_coding/ lgtm

https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:184:
cricket::VideoCodec codecInfo = encoder_factory->supported_codecs().at(0);
Is there a benefit from using .at here, vs. the regular []?

[magjed_webrtc, 2017-08-07 12:57:50]

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

[commit-bot: I haz the power, 2017-08-07 12:58:01]

Dry run: CQ is trying da patch.

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

[magjed_webrtc, 2017-08-07 12:59:34]

https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:184:
cricket::VideoCodec codecInfo = encoder_factory->supported_codecs().at(0);
On 2017/08/07 08:01:57, brandtr wrote:
> Is there a benefit from using .at here, vs. the regular []?

Not much, at() will check if the index is within bound and throw an exception,
compared to [] that will not check the bounds and segfault instead.

[magjed_webrtc, 2017-08-07 13:00:28]

Description was changed from

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

BUG=webrtc:7924
==========

to

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

Based on andersc@ CL: https://codereview.webrtc.org/2978623002/.

BUG=webrtc:7924
==========

[commit-bot: I haz the power, 2017-08-07 13:38:14]

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

[commit-bot: I haz the power, 2017-08-07 13:38:15]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-08-07 13:51:17]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2017-08-07 13:51:18]

The patchset sent to the CQ was uploaded after l-g-t-m from andersc@webrtc.org,
brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2987413002/#ps160001 (title:
"Move RTCH264PacketizationMode to RTCVideoCodec.h")

[commit-bot: I haz the power, 2017-08-07 13:51:21]

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, 2017-08-07 13:55:29]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1502113877066550,
"parent_rev": "bea36fdee86f7c5eb80dc9d1032b61af01e92ccb", "commit_rev":
"73c0eb50143a04eaf835aba0cc8ddc654661aedf"}

[commit-bot: I haz the power, 2017-08-07 13:55:36]

Description was changed from

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

Based on andersc@ CL: https://codereview.webrtc.org/2978623002/.

BUG=webrtc:7924
==========

to

==========
ObjC: Implement HW codecs in ObjC instead of C++

The current ObjC HW encoder is implemented as a C++
webrtc::VideoEncoder. We then wrap it two times in the following way:
webrtc::VideoEncoder -> RTCVideoEncoder -> webrtc::VideoEncoder.
This was originally done to minimize the code diff when landing the
injectable encoder.

This CL removes the first wrapping and implements the ObjC HW encoder
as a RTCVideoEncoder directly. Similarly, the decoder is implemented
as a RTCVideoDecoder directly.

Based on andersc@ CL: https://codereview.webrtc.org/2978623002/.

BUG=webrtc:7924

Review-Url: https://codereview.webrtc.org/2987413002
Cr-Commit-Position: refs/heads/master@{#19255}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c0eb50143a04eaf835aba0c...
==========

[commit-bot: I haz the power, 2017-08-07 13:55:37]

Committed patchset #3 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/73c0eb50143a04eaf835aba0c...

[brandtr, 2017-08-07 14:45:55]

On 2017/08/07 12:59:34, magjed_webrtc wrote:
>
https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
> File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
> (right):
> 
>
https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:184:
> cricket::VideoCodec codecInfo = encoder_factory->supported_codecs().at(0);
> On 2017/08/07 08:01:57, brandtr wrote:
> > Is there a benefit from using .at here, vs. the regular []?
> 
> Not much, at() will check if the index is within bound and throw an exception,
> compared to [] that will not check the bounds and segfault instead.

Right, but since we are not using exceptions, will the crash from the exception
be nicer than the segfault?

[magjed_webrtc, 2017-08-08 14:10:08]

On 2017/08/07 14:45:55, brandtr wrote:
> On 2017/08/07 12:59:34, magjed_webrtc wrote:
> >
>
https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
> > File
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
> > (right):
> > 
> >
>
https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
> >
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:184:
> > cricket::VideoCodec codecInfo = encoder_factory->supported_codecs().at(0);
> > On 2017/08/07 08:01:57, brandtr wrote:
> > > Is there a benefit from using .at here, vs. the regular []?
> > 
> > Not much, at() will check if the index is within bound and throw an
exception,
> > compared to [] that will not check the bounds and segfault instead.
> 
> Right, but since we are not using exceptions, will the crash from the
exception
> be nicer than the segfault?

The exception failure will look like this on iOS:
libc++abi.dylib: terminating with uncaught exception of type std::out_of_range:
vector
I'm not sure it will always segfault with [], I think it depends on if the
vector has allocated memory or not.
It's not a big issue regardless, we can change to [] for consistency.

[brandtr, 2017-08-08 15:10:35]

On 2017/08/08 14:10:08, magjed_webrtc wrote:
> On 2017/08/07 14:45:55, brandtr wrote:
> > On 2017/08/07 12:59:34, magjed_webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
> > > File
> webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2987413002/diff/140001/webrtc/modules/video_cod...
> > >
> webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:184:
> > > cricket::VideoCodec codecInfo = encoder_factory->supported_codecs().at(0);
> > > On 2017/08/07 08:01:57, brandtr wrote:
> > > > Is there a benefit from using .at here, vs. the regular []?
> > > 
> > > Not much, at() will check if the index is within bound and throw an
> exception,
> > > compared to [] that will not check the bounds and segfault instead.
> > 
> > Right, but since we are not using exceptions, will the crash from the
> exception
> > be nicer than the segfault?
> 
> The exception failure will look like this on iOS:
> libc++abi.dylib: terminating with uncaught exception of type
std::out_of_range:
> vector
> I'm not sure it will always segfault with [], I think it depends on if the
> vector has allocated memory or not.
> It's not a big issue regardless, we can change to [] for consistency.

That does seem like an improvement from the segfault/weird behaviour of indexing
out-of-bounds using []. No need to change anything here.