Decode h264 fmtp sprop-parameter-sets to binary.

Decode h264 fmtp sprop-parameter-sets to binary.

BUG=webrtc:5948
Committed: https://crrev.com/b0a111108b4f7a7def3558aed9d138ca7f90fd06
Cr-Commit-Position: refs/heads/master@{#15474}

[johan, 2016-12-02 13:48:17]

johan@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2016-12-06 18:23:19]

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.cc (right):

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:21:
std::vector<uint8_t>* binary) {
Why static?

We usually just put these kind of helper methods and constants inside an
anonymous namespace.

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:27: if (!success) {
Don't need this temporary, just if (!DecodeFromArray(...)) return false;

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:44: bool success;
dito

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:47: LOG(LS_WARNING) <<
"Failed to decode sps *" << sprop << "*";
Should that be sps_str?

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
File webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc (right):

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc:14: #include
"webrtc/base/bitbuffer.h"
Are these used?

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc:34:
TEST_F(H264SpropParameterSetsTest, InvalidData) {
maybe add a non-empty string without comma and one with two commas - just for
completeness?

[johan, 2016-12-07 17:00:56]

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

[commit-bot: I haz the power, 2016-12-07 17:01:00]

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

[johan, 2016-12-07 17:10:22]

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

[commit-bot: I haz the power, 2016-12-07 17:10:30]

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

[johan, 2016-12-07 17:10:41]

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.cc (right):

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:21:
std::vector<uint8_t>* binary) {
On 2016/12/06 18:23:18, språng wrote:
> Why static?
> 
> We usually just put these kind of helper methods and constants inside an
> anonymous namespace.

Well, old habit from plain c days. *In this place* the "static" keyword
accomplishes nearly the same thing as an anonymous namespace. Changing this to
anonymous namespace.

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:27: if (!success) {
On 2016/12/06 18:23:18, språng wrote:
> Don't need this temporary, just if (!DecodeFromArray(...)) return false;

Done.

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:44: bool success;
On 2016/12/06 18:23:18, språng wrote:
> dito

Done.

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:47: LOG(LS_WARNING) <<
"Failed to decode sps *" << sprop << "*";
On 2016/12/06 18:23:18, språng wrote:
> Should that be sps_str?

Actually I want to output the entire string in case of an parsing error.
Clarified the log message.

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
File webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc (right):

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc:14: #include
"webrtc/base/bitbuffer.h"
On 2016/12/06 18:23:18, språng wrote:
> Are these used?
Removing them and adding actual used headers.

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc:34:
TEST_F(H264SpropParameterSetsTest, InvalidData) {
On 2016/12/06 18:23:18, språng wrote:
> maybe add a non-empty string without comma and one with two commas - just for
> completeness?

Good one.

[commit-bot: I haz the power, 2016-12-07 17:35:48]

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

[commit-bot: I haz the power, 2016-12-07 17:35:49]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2016-12-08 10:08:38]

just a few minor style comments

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.cc (right):

https://codereview.webrtc.org/2544493005/diff/1/webrtc/modules/video_coding/h...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:27: if (!success) {
On 2016/12/07 17:10:40, johan wrote:
> On 2016/12/06 18:23:18, språng wrote:
> > Don't need this temporary, just if (!DecodeFromArray(...)) return false;
> 
> Done.

Seems to still be there?

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.cc (right):

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:21: static constexpr
rtc::Base64::DecodeFlags flags = rtc::Base64::DO_STRICT;
nit: kDecodeFlags or DECODE_FLAGS

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.h (right):

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets.h:17: #include
"webrtc/base/base64.h"
Include this only in the implementation file instead.

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets.h:30: std::vector<uint8_t>
pps_;
Might want to add RTC_DISALLOW_COPY_AND_ASSIGN(H264SpropParameterSets); as well

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc (right):

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc:13: #include
<inttypes.h>
I think "webrtc/base/basictypes.h" is the preferred one, even though it looks
like that in turn includes inttypes.h

[johan, 2016-12-08 11:14:38]

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.cc (right):

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets.cc:21: static constexpr
rtc::Base64::DecodeFlags flags = rtc::Base64::DO_STRICT;
On 2016/12/08 10:08:37, språng wrote:
> nit: kDecodeFlags or DECODE_FLAGS

This is used only once anyway. Going to inline this.

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sprop_parameter_sets.h (right):

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets.h:17: #include
"webrtc/base/base64.h"
On 2016/12/08 10:08:38, språng wrote:
> Include this only in the implementation file instead.

Acknowledged.

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets.h:30: std::vector<uint8_t>
pps_;
On 2016/12/08 10:08:38, språng wrote:
> Might want to add RTC_DISALLOW_COPY_AND_ASSIGN(H264SpropParameterSets); as
well

Acknowledged.

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc (right):

https://codereview.webrtc.org/2544493005/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sprop_parameter_sets_unittest.cc:13: #include
<inttypes.h>
On 2016/12/08 10:08:38, språng wrote:
> I think "webrtc/base/basictypes.h" is the preferred one, even though it looks
> like that in turn includes inttypes.h

Ok.

[johan, 2016-12-08 11:14:56]

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

[commit-bot: I haz the power, 2016-12-08 11:15:01]

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, 2016-12-08 11:17:11]

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

[commit-bot: I haz the power, 2016-12-08 11:17:12]

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

[sprang_webrtc, 2016-12-08 11:19:22]

lgtm with the compilation error fixed :)

[johan, 2016-12-08 11:23:10]

The CQ bit was checked by johan@webrtc.org

[johan, 2016-12-08 11:23:10]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2544493005/#ps80001 (title:
"Fix compilation error.")

[commit-bot: I haz the power, 2016-12-08 11:23: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-12-08 11:57:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481196190022230,
"parent_rev": "1624f479b574e02192c2b5485180e5d5e6157fc5", "commit_rev":
"7a7102650f5ce94c2bb14b0b42fa35e33aea2149"}

[commit-bot: I haz the power, 2016-12-08 11:57:22]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Decode h264 fmtp sprop-parameter-sets to binary.

BUG=webrtc:5948
==========

to

==========
Decode h264 fmtp sprop-parameter-sets to binary.

BUG=webrtc:5948

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

[commit-bot: I haz the power, 2016-12-08 11:57:32]

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