H264 codec: Check profile-level-id when matching

H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337, chromium:645599
Committed: https://crrev.com/68979ab7dd971ab6e983b23c83154ba05e183fb8
Cr-Commit-Position: refs/heads/master@{#14546}

[magjed_webrtc, 2016-09-17 11:50:30]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

BUG=chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

BUG=webrtc:6337,chromium:645599
==========

[magjed_webrtc, 2016-09-17 11:50:41]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-09-17 11:59:43]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough for determining
a match. The profile-level-id must also match.
This CL:
 - Adds a  convenience Codec::GetParam function with default value.
 - Specializes the VideoCodec::Matches function with extra logic for matching
H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

[magjed_webrtc, 2016-09-17 12:00:30]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough for determining
a match. The profile-level-id must also match.
This CL:
 - Adds a  convenience Codec::GetParam function with default value.
 - Specializes the VideoCodec::Matches function with extra logic for matching
H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Adds a  convenience Codec::GetParam function with default value.
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

[magjed_webrtc, 2016-09-17 12:00:50]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Adds a  convenience Codec::GetParam function with default value.
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Adds a convenience Codec::GetParam function with default value.
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

[magjed_webrtc, 2016-09-17 14:11:10]

Patchset #1 (id:20001) has been deleted

[magjed_webrtc, 2016-09-17 14:11:45]

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

[magjed_webrtc, 2016-09-17 14:12:45]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Adds a convenience Codec::GetParam function with default value.
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds a convenience Codec::GetParam function that takes a default
   value as argument.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

[magjed_webrtc, 2016-09-18 15:04:23]

Please take a look.

[hta-webrtc, 2016-09-19 06:38:36]

Some commentary. Profile-level-id is tricky.

Also some thoughts on structuring.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:257: if (name != kH264CodecName)
<standard rant>

This is another infection of the H.264 codec functionality into generic code.
Can you please consider if this can be made into "if
codec.matchesParameters(this.params) and subclassed for H.264?

That way, the negotiation can be put inside H.264-specific code.

Yes, I know cricket::VideoCodec is a struct, can't be reasonably subclassed, and
you want to get rid of it. At least add a TODO that says that this is what you
should be doing.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:275: }
Where's the test that we can successfully negotiate with the Firefox 42E0..
profile-level-ID?

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.h
File webrtc/media/base/codec.h (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.h...
webrtc/media/base/codec.h:87: const std::string& default_value) const;
Suggest not having this function. I've had this pair of functions for constraint
fetching, and decided it was a bad idea, since it required programmers to
remember two idioms instead of one.

Alternatively, name it GetParamOrDefault.

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

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec_u...
webrtc/media/base/codec_unittest.cc:226:
baseline_level2.SetParam(cricket::kH264FmtpProfileLevelId, "660014");
Are you sure about that 00 in the middle of this one?
It allows interlace, bi-prediction or something else obscure (see the message I
forwarded from Mo Zanaty).

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec_u...
webrtc/media/base/codec_unittest.cc:287: TEST(CodecTest, TestDefaultParam) {
See other comment on this function.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/mediaco...
File webrtc/media/base/mediaconstants.cc (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/mediaco...
webrtc/media/base/mediaconstants.cc:102: const char
kH264FmtpDefaultProfileLevelId[] = "66000a";
I don't think this is baseline.

Remember that the bits in the middle are *restrictive* bits - you turn them on
to restrict the profile to this functionality set only.

Also, our default profile is *constrained* baseline, not just baseline.

My general advice when trying to define profile-level-id values is to use only
values you have seen in the wild.

[magjed_webrtc, 2016-09-19 12:40:31]

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:257: if (name != kH264CodecName)
On 2016/09/19 06:38:36, hta-webrtc wrote:
> <standard rant>
> 
> This is another infection of the H.264 codec functionality into generic code.
> Can you please consider if this can be made into "if
> codec.matchesParameters(this.params) and subclassed for H.264?
> 
> That way, the negotiation can be put inside H.264-specific code.
> 
> Yes, I know cricket::VideoCodec is a struct, can't be reasonably subclassed,
and
> you want to get rid of it. At least add a TODO that says that this is what you
> should be doing.

Yes, I agree this is ugly and that it would be better with a H264 subclass of
VideoCodec where we can put the H264 specific logic. But like you say, this is a
struct, and the required refactoring work to make this an interface looks quite
daunting. I filed an issue for this refactoring and added a TODO with a link to
that issue.

I don't think we should block this fix on the refactoring work, because we have
already landed H264, and profile-level-id is not working properly.

At least this H264 specific code is inside the VideoCodec class.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:275: }
On 2016/09/19 06:38:36, hta-webrtc wrote:
> Where's the test that we can successfully negotiate with the Firefox 42E0..
> profile-level-ID?

The tests are in codec_unittest.cc and the specific test for level asymmetry is
TestVideoCodecMatchesH264LevelAsymmetry. I used 42000a instead of 42e00a, but I
have now changed it to 42e00a so that it's exactly what Firefox uses in the
wild.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.h
File webrtc/media/base/codec.h (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.h...
webrtc/media/base/codec.h:87: const std::string& default_value) const;
On 2016/09/19 06:38:36, hta-webrtc wrote:
> Suggest not having this function. I've had this pair of functions for
constraint
> fetching, and decided it was a bad idea, since it required programmers to
> remember two idioms instead of one.
> 
> Alternatively, name it GetParamOrDefault.

Ok, I removed it from the public interface and made it a private helper function
in codec.cc. I hope that's ok since it's exactly the functionality I want and I
don't want to inline it four times.

The modern way of doing this would be to return an rtc::Optional<std::String>
(or std::Optional when that becomes available). Then we can write code like
GetParam(...).value_or(default_value).

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

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec_u...
webrtc/media/base/codec_unittest.cc:226:
baseline_level2.SetParam(cricket::kH264FmtpProfileLevelId, "660014");
On 2016/09/19 06:38:36, hta-webrtc wrote:
> Are you sure about that 00 in the middle of this one?
> It allows interlace, bi-prediction or something else obscure (see the message
I
> forwarded from Mo Zanaty).

No, I'm not sure about the middle 00. It doesn't really matter what the
constraint set flags mean here in this test, as long as they are equal. I'm only
testing level difference. See the last comment regarding 00.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec_u...
webrtc/media/base/codec_unittest.cc:287: TEST(CodecTest, TestDefaultParam) {
On 2016/09/19 06:38:36, hta-webrtc wrote:
> See other comment on this function.

Done, I have removed it.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/mediaco...
File webrtc/media/base/mediaconstants.cc (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/mediaco...
webrtc/media/base/mediaconstants.cc:102: const char
kH264FmtpDefaultProfileLevelId[] = "66000a";
On 2016/09/19 06:38:36, hta-webrtc wrote:
> I don't think this is baseline.
> 
> Remember that the bits in the middle are *restrictive* bits - you turn them on
> to restrict the profile to this functionality set only.
> 
> Also, our default profile is *constrained* baseline, not just baseline.
> 
> My general advice when trying to define profile-level-id values is to use only
> values you have seen in the wild.

Sorry, it should be 66 in decimal, i.e. 0x42 in hex.

This is the default when no profile-level-id is specified. We still use
Constrained Baseline in cricket::DefaultVideoCodecList(), that logic has not
been touched. 

This default is not something I made up, I'm trying to base it on the spec. This
is from https://www.iana.org/assignments/media-types/video/H264-SVC: "If no
profile-level-id is present, the Baseline Profile without additional constraints
at Level 1 MUST be implied." I interpreted this as the constraints set flags
should all be zero, but please correct me if I'm wrong.

I guess we could still set constraint_set0_flag, even though it's redundant.
This is from your message with Mo Zanaty:
"constraint_set0_flag: baseline profile features only (which includes
ASO/FMO/RS)
...
It is a bit redundant to signal constraint set 0 (BP) when using profile=XX=42
(BP), which is still ok, but this signals full/unconstrained BP unless
constraint set 1 and/or 2 are also set to signal CBP."

[magjed_webrtc, 2016-09-19 12:41:55]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds a convenience Codec::GetParam function that takes a default
   value as argument.
 - Adds unittests for matching H264 video codecs.
 - Adds unittest for the new GetParam function.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

[kthelgason, 2016-09-20 13:34:13]

I'm not qualified to comment on whether all the logic regarding comparison of
codecs, but this seems like a very reasonable way to implement this, and the
tests seem quite thorough. I agree with your assessment that we should not block
this on the refactoring effort needed to turn cricket::VideoCodec into an
interface.
In conclusion, LGTM.

[magjed_webrtc, 2016-09-30 12:33:32]

Ping Harald, can you take another look? This CL is a first step towards proper
profile-level-id negotiation and it will fix the specific Firefox/Chromium H264
interop problem.

[hta-webrtc, 2016-10-05 11:56:29]

Lgtm

Apologies for the delay.

I still think it's worth it setting the bits in the second byte, but otherwise
the code looks good to me. Your choice.

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/mediaco...
File webrtc/media/base/mediaconstants.cc (right):

https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/mediaco...
webrtc/media/base/mediaconstants.cc:102: const char
kH264FmtpDefaultProfileLevelId[] = "66000a";
On 2016/09/19 12:40:30, magjed_webrtc wrote:
> On 2016/09/19 06:38:36, hta-webrtc wrote:
> > I don't think this is baseline.
> > 
> > Remember that the bits in the middle are *restrictive* bits - you turn them
on
> > to restrict the profile to this functionality set only.
> > 
> > Also, our default profile is *constrained* baseline, not just baseline.
> > 
> > My general advice when trying to define profile-level-id values is to use
only
> > values you have seen in the wild.
> 
> Sorry, it should be 66 in decimal, i.e. 0x42 in hex.
> 
> This is the default when no profile-level-id is specified. We still use
> Constrained Baseline in cricket::DefaultVideoCodecList(), that logic has not
> been touched. 
> 
> This default is not something I made up, I'm trying to base it on the spec.
This
> is from https://www.iana.org/assignments/media-types/video/H264-SVC: "If no
> profile-level-id is present, the Baseline Profile without additional
constraints
> at Level 1 MUST be implied." I interpreted this as the constraints set flags
> should all be zero, but please correct me if I'm wrong.
> 
> I guess we could still set constraint_set0_flag, even though it's redundant.
> This is from your message with Mo Zanaty:
> "constraint_set0_flag: baseline profile features only (which includes
> ASO/FMO/RS)
> ...
> It is a bit redundant to signal constraint set 0 (BP) when using profile=XX=42
> (BP), which is still ok, but this signals full/unconstrained BP unless
> constraint set 1 and/or 2 are also set to signal CBP."

Yes, I think we should set those 2 flags. They are a bit illogical to my mind,
because setting them to false means that the profile is not constrained, while
setting them to true means that the profile is constrained - I'd think that the
opposite was logical (true = more permitted), but that's not what the spec says.

[magjed_webrtc, 2016-10-05 14:33:15]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-10-05 14:33:20]

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-05 16:33:50]

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

[commit-bot: I haz the power, 2016-10-05 16:33:51]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[magjed_webrtc, 2016-10-06 09:07:48]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-10-06 09:07:56]

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-06 09:15:51]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

[commit-bot: I haz the power, 2016-10-06 09:15:52]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-10-06 09:15:59]

Description was changed from

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599
==========

to

==========
H264 codec: Check profile-level-id when matching

For the H264 video codec, comparing the codec name is not enough
for determining a match. The profile-level-id must also match.
This CL:
 - Specializes the VideoCodec::Matches function with extra logic for
   matching H264 codecs.
 - Adds unittests for matching H264 video codecs.
 - Removes the unnecessary CodecTest fixture class.

BUG=webrtc:6337,chromium:645599

Committed: https://crrev.com/68979ab7dd971ab6e983b23c83154ba05e183fb8
Cr-Commit-Position: refs/heads/master@{#14546}
==========

[commit-bot: I haz the power, 2016-10-06 09:16:00]

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

[magjed_webrtc, 2016-10-21 14:08:48]

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

The reason for reverting is: Breaks H264 for external encoders in WebRTC as well
as breaking H264 interop with e.g. Edge..

[tommi, 2016-10-25 09:19:57]

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

[tommi, 2016-10-25 09:19:58]

since this will be relanded, I figured I'd share some suggestions for smaller,
faster code ;)

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:29: return (iter == codec.params.end()) ?
default_value : iter->second;
nit:

this function should just return |const char*|.  The values for key and
default_value are always const char*, so there's no need for code that copies
them or to create a string copy for return. |codec| is also passed in, so
returning iter->second.c_str() should be fine.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:279: GetParamOrDefault(codec,
kH264FmtpLevelAsymmetryAllowed, "0") == "1";
nit: "0" -> "" (or nullptr?)

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:282: their_profile_level_id.substr(0, 4));
three nits:

* do you know if our_profile_level_id.length() >= 4? (same for
their_profile_level_id).
* it would be good to document why we use 4, or have a constant for it.
* There's no need to create two string copies just to compare sub strings. 
Instead you could use strncmp or string::compare().

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:283: return level_asymmetry_allowed &&
is_profile_match;
nit: if level_asymmetry_allowed is required, you might as well check that and
return before checking for a profile match.

[hta-webrtc, 2016-10-25 09:36:26]

A few words of explanation to Tommi's comment.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:282: their_profile_level_id.substr(0, 4));
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> three nits:
> 
> * do you know if our_profile_level_id.length() >= 4? (same for
> their_profile_level_id).
> * it would be good to document why we use 4, or have a constant for it.
> * There's no need to create two string copies just to compare sub strings. 
> Instead you could use strncmp or string::compare().
> 

1) profile-level-id is always six characters; anything other than six characters
is a protocol error.
2) there are 3 separate fields in profile-level-id, each using two characters of
the string. This function compares the first 2.

[magjed_webrtc, 2016-10-25 10:58:01]

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:29: return (iter == codec.params.end()) ?
default_value : iter->second;
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> nit:
> 
> this function should just return |const char*|.  The values for key and
> default_value are always const char*, so there's no need for code that copies
> them or to create a string copy for return. |codec| is also passed in, so
> returning iter->second.c_str() should be fine.

The reason for not returning char* was that CodecParameterMap is holding
strings. I can do iter->second.c_str() like you say, but that's a raw pointer
and we need to be careful to sync the lifetime with |codec|, but yes it's
possible.

I can make the key arg char* as well. I don't think it will make a difference
performance-wise since std::map<std::string, std::string>::find will convert it
to a std::string.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:279: GetParamOrDefault(codec,
kH264FmtpLevelAsymmetryAllowed, "0") == "1";
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> nit: "0" -> "" (or nullptr?)

Yes, should be possible. I wanted to be clear here. "0" is the default value for
level asymmetry if it's not set explicitly.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:282: their_profile_level_id.substr(0, 4));
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> three nits:
> 
> * do you know if our_profile_level_id.length() >= 4? (same for
> their_profile_level_id).
> * it would be good to document why we use 4, or have a constant for it.
> * There's no need to create two string copies just to compare sub strings. 
> Instead you could use strncmp or string::compare().
> 

I will add explicit functionality for parsing the profile-level-id string into
two enums (profile (BP, CBP, MP, HP) + level) in a separate CL, so these
comments will be automatically addressed there.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:283: return level_asymmetry_allowed &&
is_profile_match;
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> nit: if level_asymmetry_allowed is required, you might as well check that and
> return before checking for a profile match.

Sure. As long as the code is as clear as possible. This is not a tight function.