[rtp_rtcp] rtcp::Sli packet moved into own file and got Parse function

[rtp_rtcp] rtcp::Sli packet moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson

Committed: https://crrev.com/92e677a1f8d24dfa0031d307c4a7d8e530cd4eb4
Cr-Commit-Position: refs/heads/master@{#11228}

[åsapersson, 2016-01-11 12:13:55]

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:48: uint16_t count) {
call count -> number?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:51: RTC_DCHECK_LE(picture_id,
0x3d);
3d -> 3f?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:71: size_t macroblocks_number
=
maybe items?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:91:
RTC_DCHECK(!macroblocks_.empty());
use RTC_CHECK here?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:28: class Macroblocks {
maybe Item?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:53: void
WithMacroblocks(Macroblocks macroblocks) {
not used, remove?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:56: void
WithMacroblocks(uint8_t picture_id, uint16_t first_mb, uint16_t count) {
maybe WithItem?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:74: std::vector<Macroblocks>
macroblocks_;
maybe call items_?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:45: using
RTCPUtility::RtcpParseCommonHeader;
move line 44, 45 to other using above?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:48:
EXPECT_THAT(header.BlockSize(), Eq(length));
EXPECT_EQ(length, header.BlockSize());

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:69:
EXPECT_THAT(parsed.sender_ssrc(), Eq(kSenderSsrc));
use EXPECT_EQ?
EXPECT_EQ(kSenderSsrc, parsed.sender_ssrc());

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:76:
TEST(RtcpPacketSliTest, ParseFailesOnSmallPacket) {
s/Failes/Fails

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:76:
TEST(RtcpPacketSliTest, ParseFailesOnSmallPacket) {
maybe ParseFailsOnTooSmallPacket

[danilchap, 2016-01-11 15:36:32]

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:48: uint16_t count) {
On 2016/01/11 12:13:54, åsapersson wrote:
> call count -> number?

Done.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:51: RTC_DCHECK_LE(picture_id,
0x3d);
On 2016/01/11 12:13:54, åsapersson wrote:
> 3d -> 3f?

Done.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:71: size_t macroblocks_number
=
On 2016/01/11 12:13:54, åsapersson wrote:
> maybe items?

Done.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:91:
RTC_DCHECK(!macroblocks_.empty());
On 2016/01/11 12:13:54, åsapersson wrote:
> use RTC_CHECK here?

Done, but isn't it better to send invalid sli packet than to crash?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:28: class Macroblocks {
On 2016/01/11 12:13:54, åsapersson wrote:
> maybe Item?

Item is too generic name.
In case of SLI packet this item contains Macroblocks (as described in the RFC
4585, section 6.3.2.1)
Item itself is called 'SLI' (e.g. in phrase "the number of SLIs contained in the
FCI field" in section 6.3.2.2), but it would be confusing to call this structure
same as parent class.
Other name alternatives: 
 rename Macroblocks to MacroblockRange.
 rename Macroblocks to Sli while Sli to SliPacket.
 rename Macroblocks to SliceLossIndication.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:53: void
WithMacroblocks(Macroblocks macroblocks) {
On 2016/01/11 12:13:54, åsapersson wrote:
> not used, remove?

Done, likely will not be used.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:56: void
WithMacroblocks(uint8_t picture_id, uint16_t first_mb, uint16_t count) {
On 2016/01/11 12:13:54, åsapersson wrote:
> maybe WithItem?

WithItem sounds too generic, maybe restore original name:
WithPictureId(uint8_t picture_id, uint16_t first_macroblock = 0, uint16_t
number_macroblocks = 0x1fff);

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h:74: std::vector<Macroblocks>
macroblocks_;
On 2016/01/11 12:13:54, åsapersson wrote:
> maybe call items_?

Done. Internally they are treated as items in the FCI field, not as Macroblock
Range, so agree that name fits better.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:45: using
RTCPUtility::RtcpParseCommonHeader;
On 2016/01/11 12:13:54, åsapersson wrote:
> move line 44, 45 to other using above?

Because of plans to drop dependency on RTCPUtility namespace, prefer to keep
this particular using as local as possible.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:48:
EXPECT_THAT(header.BlockSize(), Eq(length));
On 2016/01/11 12:13:54, åsapersson wrote:
> EXPECT_EQ(length, header.BlockSize());
EXPECT_THAT syntax makes it more clear which value is tested, what is expected.
Should that advantage be dropped for consistency?

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:76:
TEST(RtcpPacketSliTest, ParseFailesOnSmallPacket) {
On 2016/01/11 12:13:54, åsapersson wrote:
> s/Failes/Fails

Done.

[åsapersson, 2016-01-12 12:35:26]

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:71: size_t macroblocks_number
=
On 2016/01/11 15:36:32, danilchap wrote:
> On 2016/01/11 12:13:54, åsapersson wrote:
> > maybe items?
> 
> Done.

maybe just items or num_items, number_of_items

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:91:
RTC_DCHECK(!macroblocks_.empty());
On 2016/01/11 15:36:32, danilchap wrote:
> On 2016/01/11 12:13:54, åsapersson wrote:
> > use RTC_CHECK here?
> 
> Done, but isn't it better to send invalid sli packet than to crash?

Ok, RTC_DCHECK might be better.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:45: using
RTCPUtility::RtcpParseCommonHeader;
On 2016/01/11 15:36:32, danilchap wrote:
> On 2016/01/11 12:13:54, åsapersson wrote:
> > move line 44, 45 to other using above?
> 
> Because of plans to drop dependency on RTCPUtility namespace, prefer to keep
> this particular using as local as possible.

Acknowledged.

[danilchap, 2016-01-12 12:56:35]

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:71: size_t macroblocks_number
=
On 2016/01/12 12:35:26, åsapersson wrote:
> On 2016/01/11 15:36:32, danilchap wrote:
> > On 2016/01/11 12:13:54, åsapersson wrote:
> > > maybe items?
> > 
> > Done.
> 
> maybe just items or num_items, number_of_items
Done. number_of_items is obviously more clear name.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.cc:91:
RTC_DCHECK(!macroblocks_.empty());
On 2016/01/12 12:35:26, åsapersson wrote:
> On 2016/01/11 15:36:32, danilchap wrote:
> > On 2016/01/11 12:13:54, åsapersson wrote:
> > > use RTC_CHECK here?
> > 
> > Done, but isn't it better to send invalid sli packet than to crash?
> 
> Ok, RTC_DCHECK might be better.

Reverted.

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc (right):

https://codereview.webrtc.org/1551893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sli_unittest.cc:45: using
RTCPUtility::RtcpParseCommonHeader;
On 2016/01/12 12:35:26, åsapersson wrote:
> On 2016/01/11 15:36:32, danilchap wrote:
> > On 2016/01/11 12:13:54, åsapersson wrote:
> > > move line 44, 45 to other using above?
> > 
> > Because of plans to drop dependency on RTCPUtility namespace, prefer to keep
> > this particular using as local as possible.
> 
> Acknowledged.

Reconsidered: dropping will be equally easy from both places, so no need to
ignore consistency now.

[åsapersson, 2016-01-12 13:11:40]

lgtm

[danilchap, 2016-01-12 17:00:21]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-01-12 17:00:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1551893002/#ps90001 (title:
" ")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1551893002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1551893002/90001

[commit-bot: I haz the power, 2016-01-12 18:04:55]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-01-12 18:05:05]

Description was changed from

==========
[rtp_rtcp] rtcp::Sli packet moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson
==========

to

==========
[rtp_rtcp] rtcp::Sli packet moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson

Committed: https://crrev.com/92e677a1f8d24dfa0031d307c4a7d8e530cd4eb4
Cr-Commit-Position: refs/heads/master@{#11228}
==========

[commit-bot: I haz the power, 2016-01-12 18:05:06]

Patchset 6 (id:??) landed as
https://crrev.com/92e677a1f8d24dfa0031d307c4a7d8e530cd4eb4
Cr-Commit-Position: refs/heads/master@{#11228}