Add to-string function for H264 profile level id

Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta@webrtc.org

Committed: https://crrev.com/b92184ba0da8af0059ec08fafacccb67ad2475a8
Cr-Commit-Position: refs/heads/master@{#14912}

[magjed_webrtc, 2016-11-02 14:09:09]

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

[commit-bot: I haz the power, 2016-11-02 14:09:18]

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

[magjed_webrtc, 2016-11-02 14:09:30]

Description was changed from

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
==========

to

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
==========

[magjed_webrtc, 2016-11-02 14:09:30]

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

[magjed_webrtc, 2016-11-02 14:09:41]

Please take a look.

[commit-bot: I haz the power, 2016-11-02 14:33:25]

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

[commit-bot: I haz the power, 2016-11-02 14:33:25]

Dry run: This issue passed the CQ dry run.

[kthelgason, 2016-11-03 09:39:05]

A few suggestions

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
File webrtc/common_video/h264/profile_level_id.cc (right):

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
webrtc/common_video/h264/profile_level_id.cc:144: return std::string();
Maybe we should return an rtc::Optional<std::string> here, so the callee doesn't
have to remember to check for an empty string.

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
webrtc/common_video/h264/profile_level_id.cc:167: return std::string();
perhaps replace this with a RTC_NOTREACHED() if we expect this to never happen.

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
File webrtc/common_video/h264/profile_level_id.h (right):

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
webrtc/common_video/h264/profile_level_id.h:52: ProfileLevelId() :
profile(kProfileBaseline), level(kLevel1) {}
I'm not convinced that this default constructor is useful.

[magjed_webrtc, 2016-11-03 12:31:16]

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
File webrtc/common_video/h264/profile_level_id.cc (right):

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
webrtc/common_video/h264/profile_level_id.cc:144: return std::string();
On 2016/11/03 09:39:05, kthelgason wrote:
> Maybe we should return an rtc::Optional<std::string> here, so the callee
doesn't
> have to remember to check for an empty string.

You are probably right. It's clearer than returning an empty string and the type
system will enforce checking against it.

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
webrtc/common_video/h264/profile_level_id.cc:167: return std::string();
On 2016/11/03 09:39:05, kthelgason wrote:
> perhaps replace this with a RTC_NOTREACHED() if we expect this to never
happen.

If we handle some case, then we should not CHECK/NOT_REACHED also. This is from
the style guide:
"... A consequence of this is that you should not handle DCHECK() failures, even
if failure would result in a crash. Attempting to handle a DCHECK() failure is a
statement that the DCHECK() can fail, which contradicts the point of writing the
DCHECK()."

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
File webrtc/common_video/h264/profile_level_id.h (right):

https://codereview.webrtc.org/2472693002/diff/1/webrtc/common_video/h264/prof...
webrtc/common_video/h264/profile_level_id.h:52: ProfileLevelId() :
profile(kProfileBaseline), level(kLevel1) {}
On 2016/11/03 09:39:05, kthelgason wrote:
> I'm not convinced that this default constructor is useful.

Alright, let's remove the default constructor and force explicit initialization.

[kthelgason, 2016-11-03 12:38:27]

lgtm

[magjed_webrtc, 2016-11-03 13:10:42]

Description was changed from

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
==========

to

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta
==========

[magjed_webrtc, 2016-11-03 13:10:53]

Description was changed from

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta
==========

to

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta@webrtc.org
==========

[magjed_webrtc, 2016-11-03 13:12:16]

Harald - I'm TBR:ing you since I'm blocked on this CL. I need to have it in
Chromium in order to set correct profile-level-id for their external encoder,
and then I can reland the profile-level-id negotiation that was reverted.

[magjed_webrtc, 2016-11-03 13:12:21]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-11-03 13:12:24]

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

[hta-webrtc, 2016-11-03 13:23:06]

lgtm

Apologies for being late.... only add is suggestion for more
tests-as-documentation.

https://codereview.webrtc.org/2472693002/diff/20001/webrtc/common_video/h264/...
File webrtc/common_video/h264/profile_level_id_unittest.cc (right):

https://codereview.webrtc.org/2472693002/diff/20001/webrtc/common_video/h264/...
webrtc/common_video/h264/profile_level_id_unittest.cc:100:
ProfileLevelId(static_cast<Profile>(255), kLevel3_1)));
Can you also add some round-trip tests?

ie

EXPECT_EQ("4d100b", *ProfileLevelIdToString(ParseProfileLevelId("4d100b")))
EXPECT_EQ("4d100b",
*ProfileLevelIdToString(ParseProfileLevelId("4D100B")))

It's a very readable way to see that parsing of certain strings is
self-consistent.

[commit-bot: I haz the power, 2016-11-03 13:50:31]

Description was changed from

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta@webrtc.org
==========

to

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta@webrtc.org
==========

[commit-bot: I haz the power, 2016-11-03 13:50:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-03 13:50:38]

Description was changed from

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta@webrtc.org
==========

to

==========
Add to-string function for H264 profile level id

BUG=webrtc:6337
TBR=hta@webrtc.org

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

[commit-bot: I haz the power, 2016-11-03 13:50:39]

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