Add functionality for parsing H264 profile-level-id

Add functionality for parsing H264 profile-level-id

The new code is only exercised in tests so far. The H264 profile-level-id
parsing is not complete, but it should be enough for our purposes for
now.

BUG=webrtc:6400, webrtc:6337
Committed: https://crrev.com/fffc1e5578b051ca8d4e210fc59435d2b4b2b42a
Cr-Commit-Position: refs/heads/master@{#14850}

[magjed_webrtc, 2016-10-28 09:01:09]

Patchset #4 (id:60001) has been deleted

[magjed_webrtc, 2016-10-28 09:01:20]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2016-10-28 09:01:26]

Patchset #2 (id:40001) has been deleted

[magjed_webrtc, 2016-10-28 09:01:35]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-10-28 09:02:40]

Description was changed from

==========
Add functionality for parsing H264 profile-level-id

BUG=webrtc:6400,webrtc:6337
==========

to

==========
Add functionality for parsing H264 profile-level-id

The new code is only exercised in tests so far. The H264 profile-level-id
parsing is not complete, but it should be enough for our purposes for
now.

BUG=webrtc:6400,webrtc:6337
==========

[magjed_webrtc, 2016-10-28 09:06:27]

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

[magjed_webrtc, 2016-10-28 09:07:00]

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

[magjed_webrtc, 2016-10-28 09:07:13]

Please take a look.

[hta-webrtc, 2016-10-28 09:55:23]

My only strong objection at the moment is where the code sits. H.264 stuff
shouldn't be in common_types.h/.cc.

The code looks fine. Good to see a complete parsing approach!

I assume that matching two profiles (with checking that profile A <= profile B)
is going to be done in a later CL.

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:28: return (str[0] == c) << 7
Can you insert a DCHECK that strlen(str) == 8?
This is not intended for runtime data, so no need for CHECK, but DCHECK will
catch typos in updates.

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:151: rtc::Optional<H264ProfileLevelId>
ParseH264ProfileLevelId(const char* str) {
Same comment goes for this function.

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:502: enum H264Profile {
Query: Is it really necessary to pollute webrtc/common_types.h (probably the
most included file in the whole webrtc stack) with this degree of H.264
specifics?

In the interest of preparing for runtime-injectable codecs, I'd like to put
codec-specific definitions as close to the codec-specific includes as possible -
the fact that we're sinning dramatically against this in the present files is no
excuse for continuing the practice.

[tommi, 2016-10-28 10:09:48]

drive by high five. I'm digging the parsing code!

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:46: bool isMatch(uint8_t value) const { return
masked_value_ == (value & mask_); }
nit: IsMatch

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/media/base/codec_u...
File webrtc/media/base/codec_unittest.cc (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/media/base/codec_u...
webrtc/media/base/codec_unittest.cc:323: // Malformed strings.
test too long strings too?

[magjed_webrtc, 2016-10-28 14:53:44]

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

[magjed_webrtc, 2016-10-28 14:59:28]

> I assume that matching two profiles (with checking that profile A <= profile
B)
> is going to be done in a later CL.
Yeah, I plan to add the actual negotiation code in a later CL. However, I think
the only profile A <= profile B logic will be to compare the level. Each H264
profile will have to be added as a separate codec and payload type so I think
the actual profiles will only be compared for equality.

> drive by high five. I'm digging the parsing code!
Thanks :)

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:28: return (str[0] == c) << 7
On 2016/10/28 09:55:22, hta-webrtc wrote:
> Can you insert a DCHECK that strlen(str) == 8?
> This is not intended for runtime data, so no need for CHECK, but DCHECK will
> catch typos in updates.

It's not possible to call non-constexpr functions like strlen or DCHECK from
constexpr functions. I tried a simpler static_assert(str[8] == '\0') check, but
that didn't work either because str is not constexpr. According to
stackoverflow, the way to do CHECKS in constexpr functions is to throw an
exception, and that will be caught at compile time, but exceptions are disabled
in WebRTC so it doesn't compile. I also tried changing the str argument type
from 'const char str[]' to 'const char str[9]' and that compiles, but still
doesn't catch when it's called with strings of incorrect lengths. It would be
possible to check without exceptions using template magic, but I don't think
it's worth it.

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:46: bool isMatch(uint8_t value) const { return
masked_value_ == (value & mask_); }
On 2016/10/28 10:09:47, tommi (webrtc) wrote:
> nit: IsMatch

Done.

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:151: rtc::Optional<H264ProfileLevelId>
ParseH264ProfileLevelId(const char* str) {
On 2016/10/28 09:55:23, hta-webrtc wrote:
> Same comment goes for this function.

I check the string length in this function and return nothing if it's not 6. The
input string will come from SDP over the network so we should simply reject
malformed codecs rather than crash with CHECKs in this function.

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.h#new...
webrtc/common_types.h:502: enum H264Profile {
On 2016/10/28 09:55:23, hta-webrtc wrote:
> Query: Is it really necessary to pollute webrtc/common_types.h (probably the
> most included file in the whole webrtc stack) with this degree of H.264
> specifics?
> 
> In the interest of preparing for runtime-injectable codecs, I'd like to put
> codec-specific definitions as close to the codec-specific includes as possible
-
> the fact that we're sinning dramatically against this in the present files is
no
> excuse for continuing the practice.

Good point. I moved everything to webrtc/common_video/h264 instead. We don't
need to store the H264 profile as a setting in
VideoCodecH264/VideoCodecUnion/webrtc::VideoCodec at all. The plan is to
negotiate the profile in cricket::VideoCodec that is now passed as argument to
WebRtcVideoEncoderFactory::CreateVideoEncoder(), so it shouldn't be necessary to
also send the profile as argument in webrtc::VideoCodec in
VideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings).

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/media/base/codec_u...
File webrtc/media/base/codec_unittest.cc (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/media/base/codec_u...
webrtc/media/base/codec_unittest.cc:323: // Malformed strings.
On 2016/10/28 10:09:47, tommi (webrtc) wrote:
> test too long strings too?

Done, I added a test with 8 hex characters. I also already have one test with a
leading space that is 7 characters.

[tommi, 2016-10-28 15:02:21]

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
webrtc/common_types.cc:28: return (str[0] == c) << 7
On 2016/10/28 14:59:27, magjed_webrtc wrote:
> On 2016/10/28 09:55:22, hta-webrtc wrote:
> > Can you insert a DCHECK that strlen(str) == 8?
> > This is not intended for runtime data, so no need for CHECK, but DCHECK will
> > catch typos in updates.
> 
> It's not possible to call non-constexpr functions like strlen or DCHECK from
> constexpr functions. I tried a simpler static_assert(str[8] == '\0') check,
but
> that didn't work either because str is not constexpr. According to
> stackoverflow, the way to do CHECKS in constexpr functions is to throw an
> exception, and that will be caught at compile time, but exceptions are
disabled
> in WebRTC so it doesn't compile. I also tried changing the str argument type
> from 'const char str[]' to 'const char str[9]' and that compiles, but still
> doesn't catch when it's called with strings of incorrect lengths. It would be
> possible to check without exceptions using template magic, but I don't think
> it's worth it.
> 

as one more try, sizeof() == 9?  or even specify the parameter as |const char
str[9]|?

[magjed_webrtc, 2016-10-28 15:19:21]

On 2016/10/28 15:02:21, tommi (webrtc) wrote:
> https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc
> File webrtc/common_types.cc (right):
> 
>
https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
> webrtc/common_types.cc:28: return (str[0] == c) << 7
> On 2016/10/28 14:59:27, magjed_webrtc wrote:
> > On 2016/10/28 09:55:22, hta-webrtc wrote:
> > > Can you insert a DCHECK that strlen(str) == 8?
> > > This is not intended for runtime data, so no need for CHECK, but DCHECK
will
> > > catch typos in updates.
> > 
> > It's not possible to call non-constexpr functions like strlen or DCHECK from
> > constexpr functions. I tried a simpler static_assert(str[8] == '\0') check,
> but
> > that didn't work either because str is not constexpr. According to
> > stackoverflow, the way to do CHECKS in constexpr functions is to throw an
> > exception, and that will be caught at compile time, but exceptions are
> disabled
> > in WebRTC so it doesn't compile. I also tried changing the str argument type
> > from 'const char str[]' to 'const char str[9]' and that compiles, but still
> > doesn't catch when it's called with strings of incorrect lengths. It would
be
> > possible to check without exceptions using template magic, but I don't think
> > it's worth it.
> > 
> 
> as one more try, sizeof() == 9?  or even specify the parameter as |const char
> str[9]|?

As I said, I tried 'const char str[9]|' but it doesn't help. I investigated a
bit further and C/C++ adjust array function parameters to pointers so that's why
it doesn't help. Apparently, a workaround is to pass in a reference to the
array, i.e. 'const char (&str)[9]'. Now it works and the length requirement is
enforced by the type system :P

[tommi, 2016-10-28 15:29:20]

On 2016/10/28 15:19:21, magjed_webrtc wrote:
> On 2016/10/28 15:02:21, tommi (webrtc) wrote:
> > https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc
> > File webrtc/common_types.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2459633002/diff/80001/webrtc/common_types.cc#ne...
> > webrtc/common_types.cc:28: return (str[0] == c) << 7
> > On 2016/10/28 14:59:27, magjed_webrtc wrote:
> > > On 2016/10/28 09:55:22, hta-webrtc wrote:
> > > > Can you insert a DCHECK that strlen(str) == 8?
> > > > This is not intended for runtime data, so no need for CHECK, but DCHECK
> will
> > > > catch typos in updates.
> > > 
> > > It's not possible to call non-constexpr functions like strlen or DCHECK
from
> > > constexpr functions. I tried a simpler static_assert(str[8] == '\0')
check,
> > but
> > > that didn't work either because str is not constexpr. According to
> > > stackoverflow, the way to do CHECKS in constexpr functions is to throw an
> > > exception, and that will be caught at compile time, but exceptions are
> > disabled
> > > in WebRTC so it doesn't compile. I also tried changing the str argument
type
> > > from 'const char str[]' to 'const char str[9]' and that compiles, but
still
> > > doesn't catch when it's called with strings of incorrect lengths. It would
> be
> > > possible to check without exceptions using template magic, but I don't
think
> > > it's worth it.
> > > 
> > 
> > as one more try, sizeof() == 9?  or even specify the parameter as |const
char
> > str[9]|?
> 
> As I said, I tried 'const char str[9]|' but it doesn't help. I investigated a
> bit further and C/C++ adjust array function parameters to pointers so that's
why
> it doesn't help. Apparently, a workaround is to pass in a reference to the
> array, i.e. 'const char (&str)[9]'. Now it works and the length requirement is
> enforced by the type system :P

But did you try using const char str[9]?

ok, just kidding :D sorry I missed that. You fixed it though and we all learned
something (I wonder if I threw out something else that might have been more
worthwhile to remember...).  In any case, lgtm.

[hta-webrtc, 2016-10-30 23:57:26]

lgtm

[stefan-webrtc, 2016-10-31 09:34:53]

lgtm

https://codereview.webrtc.org/2459633002/diff/120001/webrtc/common_video/comm...
File webrtc/common_video/common_video.gyp (right):

https://codereview.webrtc.org/2459633002/diff/120001/webrtc/common_video/comm...
webrtc/common_video/common_video.gyp:62: 'h264/profile_level_id.h',
These should go after pps_* below

[magjed_webrtc, 2016-10-31 12:07:47]

Patchset #4 (id:140001) has been deleted

[magjed_webrtc, 2016-10-31 12:08:03]

https://codereview.webrtc.org/2459633002/diff/120001/webrtc/common_video/comm...
File webrtc/common_video/common_video.gyp (right):

https://codereview.webrtc.org/2459633002/diff/120001/webrtc/common_video/comm...
webrtc/common_video/common_video.gyp:62: 'h264/profile_level_id.h',
On 2016/10/31 09:34:53, stefan-webrtc (holmer) wrote:
> These should go after pps_* below

Good catch, thanks.

[magjed_webrtc, 2016-10-31 12:08:07]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-10-31 12:08:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
hta@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2459633002/#ps160001 (title:
"Fix include order in common_video.gyp")

[commit-bot: I haz the power, 2016-10-31 12:08:12]

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-10-31 12:55:59]

Description was changed from

==========
Add functionality for parsing H264 profile-level-id

The new code is only exercised in tests so far. The H264 profile-level-id
parsing is not complete, but it should be enough for our purposes for
now.

BUG=webrtc:6400,webrtc:6337
==========

to

==========
Add functionality for parsing H264 profile-level-id

The new code is only exercised in tests so far. The H264 profile-level-id
parsing is not complete, but it should be enough for our purposes for
now.

BUG=webrtc:6400,webrtc:6337
==========

[commit-bot: I haz the power, 2016-10-31 12:56:00]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-10-31 12:56:09]

Description was changed from

==========
Add functionality for parsing H264 profile-level-id

The new code is only exercised in tests so far. The H264 profile-level-id
parsing is not complete, but it should be enough for our purposes for
now.

BUG=webrtc:6400,webrtc:6337
==========

to

==========
Add functionality for parsing H264 profile-level-id

The new code is only exercised in tests so far. The H264 profile-level-id
parsing is not complete, but it should be enough for our purposes for
now.

BUG=webrtc:6400,webrtc:6337

Committed: https://crrev.com/fffc1e5578b051ca8d4e210fc59435d2b4b2b42a
Cr-Commit-Position: refs/heads/master@{#14850}
==========

[commit-bot: I haz the power, 2016-10-31 12:56:10]

Patchset 4 (id:??) landed as
https://crrev.com/fffc1e5578b051ca8d4e210fc59435d2b4b2b42a
Cr-Commit-Position: refs/heads/master@{#14850}