Add write support for the RtpStreamId and RepairedRtpStreamId header extensions.

Add write support for the RtpStreamId and RepairedRtpStreamId header extensions.

BUG=webrtc:7433
Review-Url: https://codereview.webrtc.org/2871813003
Cr-Commit-Position: refs/heads/master@{#18093}
Committed: https://chromium.googlesource.com/external/webrtc/+/e6b16194c7a28c46a5309a4ab17803a104b7c88d

[erikvarga1, 2017-05-09 15:23:30]

Description was changed from

==========
Add write support for the RtpStreamId and RepairedRtpStreamId header extensions.

BUG=webrtc:7433
==========

to

==========
Add write support for the RtpStreamId and RepairedRtpStreamId header extensions.

BUG=webrtc:7433
==========

[erikvarga1, 2017-05-09 15:23:30]

erikvarga@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[danilchap, 2017-05-09 15:40:52]

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:30: static size_t
ValueSize(int64_t time_ms) { return kValueSizeBytes; }
may be put this function just before Write function to show it's parameter
closely match it.

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:84: static size_t
ValueSize(VideoRotation* value) { return kValueSizeBytes; }
remove *

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:106: static size_t
ValueSize(const PlayoutDelay& playout_delay) {
feel free to omit parameter name when it doesn't add any description to the type
name.

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:157: return
RtpStreamId::ValueSize(rsid);
may be move implementation to .cc for consistency with Parse/Write functions

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc (right):

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:143: 
add tests where you try to set empty rsid and too long rsid.

[erikvarga1, 2017-05-10 10:52:27]

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:30: static size_t
ValueSize(int64_t time_ms) { return kValueSizeBytes; }
On 2017/05/09 15:40:52, danilchap wrote:
> may be put this function just before Write function to show it's parameter
> closely match it.

Done.

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:84: static size_t
ValueSize(VideoRotation* value) { return kValueSizeBytes; }
On 2017/05/09 15:40:51, danilchap wrote:
> remove *

Done.

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:106: static size_t
ValueSize(const PlayoutDelay& playout_delay) {
On 2017/05/09 15:40:52, danilchap wrote:
> feel free to omit parameter name when it doesn't add any description to the
type
> name.

Done.

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:157: return
RtpStreamId::ValueSize(rsid);
On 2017/05/09 15:40:51, danilchap wrote:
> may be move implementation to .cc for consistency with Parse/Write functions

Done. Should this also be done for the ValueSize functions of the other
extensions? Those also have the Parse/Write functions in the .cc file, but
they're just one-liners, so I'm not sure.

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc (right):

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:143: 
On 2017/05/09 15:40:52, danilchap wrote:
> add tests where you try to set empty rsid and too long rsid.

Done. I also added an extra check to SetExtension to avoid failing the
RTC_DCHECKs in AllocateExtension.

[danilchap, 2017-05-10 11:25:18]

lgtm % nits

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:157: return
RtpStreamId::ValueSize(rsid);
On 2017/05/10 10:52:27, erikvarga1 wrote:
> On 2017/05/09 15:40:51, danilchap wrote:
> > may be move implementation to .cc for consistency with Parse/Write functions
> 
> Done. Should this also be done for the ValueSize functions of the other
> extensions? Those also have the Parse/Write functions in the .cc file, but
> they're just one-liners, so I'm not sure.

it is not required - functions that short are ok to inline.
(personally prefer ValueSize function to be inlined, like you did.)
For RepairedRtpStreamId I asked only to be consistent with other
RepairedRtpStreamId functions.
alternative is to inline Parse/Write function of the RepairedRtpStreamId
instead.

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:279: if (rsid.empty() ||
rsid.size() > StreamId::kMaxSize)
may be DCHECK here:
caller must ensure |data| has exact allocated size for the |rsid|

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc (right):

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:134:
extensions.Register(kRtpExtensionRtpStreamId, kRtpStreamIdExtensionId);
May be use template syntax for registering.
extensions.Register<RtpStreamId>(kRtpStreamIdExtensionId);
Since constant for extension type looks too similar to constant for the
extensions id.

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:153: constexpr char
kLongStreamId[] = "looooooooong_rsid";  // length=17
Avoid '_' as it is an invalid character for rsid and may be checked in future:
"Lo...ongRsid"
may be instead of comment add 
ASSERT_EQ(strlen(kLongStreamId), 17);

[erikvarga1, 2017-05-10 14:48:19]

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2871813003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:157: return
RtpStreamId::ValueSize(rsid);
On 2017/05/10 11:25:17, danilchap wrote:
> On 2017/05/10 10:52:27, erikvarga1 wrote:
> > On 2017/05/09 15:40:51, danilchap wrote:
> > > may be move implementation to .cc for consistency with Parse/Write
functions
> > 
> > Done. Should this also be done for the ValueSize functions of the other
> > extensions? Those also have the Parse/Write functions in the .cc file, but
> > they're just one-liners, so I'm not sure.
> 
> it is not required - functions that short are ok to inline.
> (personally prefer ValueSize function to be inlined, like you did.)
> For RepairedRtpStreamId I asked only to be consistent with other
> RepairedRtpStreamId functions.
> alternative is to inline Parse/Write function of the RepairedRtpStreamId
> instead.

Alright, in that case I think it should be OK like it is now.

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:279: if (rsid.empty() ||
rsid.size() > StreamId::kMaxSize)
On 2017/05/10 11:25:18, danilchap wrote:
> may be DCHECK here:
> caller must ensure |data| has exact allocated size for the |rsid|

Done.

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc (right):

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:134:
extensions.Register(kRtpExtensionRtpStreamId, kRtpStreamIdExtensionId);
On 2017/05/10 11:25:18, danilchap wrote:
> May be use template syntax for registering.
> extensions.Register<RtpStreamId>(kRtpStreamIdExtensionId);
> Since constant for extension type looks too similar to constant for the
> extensions id.

Done.

https://codereview.webrtc.org/2871813003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:153: constexpr char
kLongStreamId[] = "looooooooong_rsid";  // length=17
On 2017/05/10 11:25:18, danilchap wrote:
> Avoid '_' as it is an invalid character for rsid and may be checked in future:
> "Lo...ongRsid"
> may be instead of comment add 
> ASSERT_EQ(strlen(kLongStreamId), 17);

Done.

[erikvarga1, 2017-05-11 08:48:41]

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

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

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-05-11 09:31:16]

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

[commit-bot: I haz the power, 2017-05-11 09:31:17]

Dry run: This issue passed the CQ dry run.

[erikvarga1, 2017-05-11 09:33:52]

The CQ bit was checked by erikvarga@webrtc.org

[erikvarga1, 2017-05-11 09:33:52]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2871813003/#ps40001 (title:
"Patch 3")

[commit-bot: I haz the power, 2017-05-11 09:34:01]

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-05-11 09:36:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494495232111540,
"parent_rev": "198a9300cd26b437800eb708940064f5d1786d9c", "commit_rev":
"e6b16194c7a28c46a5309a4ab17803a104b7c88d"}

[commit-bot: I haz the power, 2017-05-11 09:36:38]

Description was changed from

==========
Add write support for the RtpStreamId and RepairedRtpStreamId header extensions.

BUG=webrtc:7433
==========

to

==========
Add write support for the RtpStreamId and RepairedRtpStreamId header extensions.

BUG=webrtc:7433

Review-Url: https://codereview.webrtc.org/2871813003
Cr-Commit-Position: refs/heads/master@{#18093}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e6b16194c7a28c46a5309a4ab...
==========

[commit-bot: I haz the power, 2017-05-11 09:36:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/e6b16194c7a28c46a5309a4ab...