rtcp::Bye packet moved to own file and got a Parse function

RTCP Bye packet moved to own file
 Bye class got support for Parsing
 Reason field implemented

Committed: https://crrev.com/50c5136cb2ad11eb9ba3df1a1d54d527c8a0dc77
Cr-Commit-Position: refs/heads/master@{#10741}

[danilchap, 2015-11-10 18:53:02]

Description was changed from

==========
RTCP Bye packet moved to own file
 Bye class got support for Parsing
 Reason field implemented
==========

to

==========
RTCP Bye packet moved to own file
 Bye class got support for Parsing
 Reason field implemented
==========

[danilchap, 2015-11-10 18:53:03]

danilchap@webrtc.org changed reviewers:
+ asapersson@webrtc.org, sprang@webrtc.org

[sprang_webrtc, 2015-11-12 16:28:31]

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc (right):

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:35: Bye::Bye() : RtcpPacket(),
sender_ssrc_(0) {}
Don't think you explicit RtcpPacket() call?

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:43: LOG(LS_WARNING) << "Bye
packet should have at least 1 src";
Bye packet must contain SSRC.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:56: LOG(LS_WARNING) << "Reason
length is unreasonable";
"Invalid reason length: " << reason_length;

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:72: reason_.clear();
Use brackets:

if () {
  ...
} else {
  ...
}

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h (right):

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h:42: uint32_t csrc(size_t index)
const { return csrcs_[index]; }
I'd rather get a const std::vector<uint32_t>&

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h:59: }
Would prefer not to inline

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc (right):

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:135: }
I'd also like a test case for parsing the doesn't work. Eg empty packet, invalid
(reason) length...

[danilchap, 2015-11-13 09:05:42]

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc (right):

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:35: Bye::Bye() : RtcpPacket(),
sender_ssrc_(0) {}
On 2015/11/12 16:28:31, språng wrote:
> Don't think you explicit RtcpPacket() call?

Wasn't sure what style is preferred, so left like it was before me.
Adjusted: explicit call to base constructor removed.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:43: LOG(LS_WARNING) << "Bye
packet should have at least 1 src";
On 2015/11/12 16:28:31, språng wrote:
> Bye packet must contain SSRC.

Done.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:56: LOG(LS_WARNING) << "Reason
length is unreasonable";
On 2015/11/12 16:28:31, språng wrote:
> "Invalid reason length: " << reason_length;

Done.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:72: reason_.clear();
On 2015/11/12 16:28:31, språng wrote:
> Use brackets:
> 
> if () {
>   ...
> } else {
>   ...
> }

Done.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h (right):

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h:42: uint32_t csrc(size_t index)
const { return csrcs_[index]; }
On 2015/11/12 16:28:31, språng wrote:
> I'd rather get a const std::vector<uint32_t>&

Do not want expose to user that vector is used for implementation.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h:59: }
On 2015/11/12 16:28:31, språng wrote:
> Would prefer not to inline

Done.

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc (right):

https://codereview.webrtc.org/1430013003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:135: }
On 2015/11/12 16:28:31, språng wrote:
> I'd also like a test case for parsing the doesn't work. Eg empty packet,
invalid
> (reason) length...

Done.

[sprang_webrtc, 2015-11-16 10:04:59]

lgtm, though I'm still not a fan of the "uint32_t csrc(size_t index) const"
method. Åsa, I'll let your vote decide there :)

[danilchap, 2015-11-17 10:29:38]

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

[commit-bot: I haz the power, 2015-11-17 10:29:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430013003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430013003/120001

[commit-bot: I haz the power, 2015-11-17 11:33:05]

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

[commit-bot: I haz the power, 2015-11-17 11:33:05]

Dry run: This issue passed the CQ dry run.

[åsapersson, 2015-11-20 09:44:59]

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc (right):

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:16: #include
"webrtc/modules/rtp_rtcp/source/byte_io.h"
alphabetic order

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:43: LOG(LS_WARNING) << "Bye
packet must contain SSRC.";
I think a SC value of zero is valid.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:52: uint8_t reason_length;
initialize to 0 and remove else?

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:88: size_t size_in_32bits =
(BlockLength() - kHeaderLength) / 4u;
Can use HeaderLength() instead?

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:126: if (reason.size() > 0xff)
{
should we use RTC_DCHECK

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:135: size_t source_count = (1
+ csrcs_.size());
maybe use same name for source count in Parse and BlockLength

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h (right):

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h:18: #include
"webrtc/base/checks.h"
needed?

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc (right):

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:22: using
webrtc::rtcp::Bye;
alphabetic order

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:40:
EXPECT_TRUE(header.packet_type == Bye::kPacketType);
Checked within Parse?

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:68: EXPECT_EQ(16u,
packet->Length());
Add comment for 16 as in test below.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:77:
TEST_F(RtcpPacketByeTest, WithCsecsAndReason) {
WithCsrcsAndReason

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:108:
EXPECT_TRUE(bye.WithReason(std::string(0xff, 'r')));
maybe use a constant kMaxReasonLength and kMaxReasonLength + 1

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:152:
TEST_F(RtcpPacketByeTest, ParseFailOnInvalidPacket) {
maybe ParseFailOnInvalidSourceCount

[danilchap, 2015-11-20 11:03:21]

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc (right):

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:16: #include
"webrtc/modules/rtp_rtcp/source/byte_io.h"
On 2015/11/20 09:44:58, åsapersson wrote:
> alphabetic order

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:43: LOG(LS_WARNING) << "Bye
packet must contain SSRC.";
On 2015/11/20 09:44:58, åsapersson wrote:
> I think a SC value of zero is valid.

Yes, "A count value of zero is valid, but useless."
Fixed

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:52: uint8_t reason_length;
On 2015/11/20 09:44:58, åsapersson wrote:
> initialize to 0 and remove else?

Wasn't sure if logic 'assign to correct value as soon as it is known what to
assign to' is better.
Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:88: size_t size_in_32bits =
(BlockLength() - kHeaderLength) / 4u;
On 2015/11/20 09:44:58, åsapersson wrote:
> Can use HeaderLength() instead?

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:126: if (reason.size() > 0xff)
{
On 2015/11/20 09:44:58, åsapersson wrote:
> should we use RTC_DCHECK

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.cc:135: size_t source_count = (1
+ csrcs_.size());
On 2015/11/20 09:44:58, åsapersson wrote:
> maybe use same name for source count in Parse and BlockLength

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h (right):

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h:18: #include
"webrtc/base/checks.h"
On 2015/11/20 09:44:58, åsapersson wrote:
> needed?

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc (right):

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:22: using
webrtc::rtcp::Bye;
On 2015/11/20 09:44:59, åsapersson wrote:
> alphabetic order

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:40:
EXPECT_TRUE(header.packet_type == Bye::kPacketType);
On 2015/11/20 09:44:58, åsapersson wrote:
> Checked within Parse?

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:68: EXPECT_EQ(16u,
packet->Length());
On 2015/11/20 09:44:59, åsapersson wrote:
> Add comment for 16 as in test below.

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:77:
TEST_F(RtcpPacketByeTest, WithCsecsAndReason) {
On 2015/11/20 09:44:58, åsapersson wrote:
> WithCsrcsAndReason

Done.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:108:
EXPECT_TRUE(bye.WithReason(std::string(0xff, 'r')));
On 2015/11/20 09:44:58, åsapersson wrote:
> maybe use a constant kMaxReasonLength and kMaxReasonLength + 1

Reason length would be DCHECKed, so this test become useless.

https://codereview.webrtc.org/1430013003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:152:
TEST_F(RtcpPacketByeTest, ParseFailOnInvalidPacket) {
On 2015/11/20 09:44:58, åsapersson wrote:
> maybe ParseFailOnInvalidSourceCount

Done. (ParseFailOnInalidSrcCount since those fields are usually called
ssrc/csrc)

[åsapersson, 2015-11-20 12:11:45]

lgtm

https://codereview.webrtc.org/1430013003/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc (right):

https://codereview.webrtc.org/1430013003/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:55:
EXPECT_EQ(kSenderSsrc, parsed_bye.sender_ssrc());
maybe check that csrcs and reason empty

https://codereview.webrtc.org/1430013003/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:110:
EXPECT_EQ(kSenderSsrc, parsed_bye.sender_ssrc());
maybe check that csrcs empty

[danilchap, 2015-11-20 12:44:03]

https://codereview.webrtc.org/1430013003/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc (right):

https://codereview.webrtc.org/1430013003/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:55:
EXPECT_EQ(kSenderSsrc, parsed_bye.sender_ssrc());
On 2015/11/20 12:11:44, åsapersson wrote:
> maybe check that csrcs and reason empty

Done.

https://codereview.webrtc.org/1430013003/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/bye_unittest.cc:110:
EXPECT_EQ(kSenderSsrc, parsed_bye.sender_ssrc());
On 2015/11/20 12:11:45, åsapersson wrote:
> maybe check that csrcs empty

Done.

[danilchap, 2015-11-20 12:44:18]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2015-11-20 12:44:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1430013003/#ps160001 (title:
"merged with master")

[commit-bot: I haz the power, 2015-11-20 12:44:22]

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

[commit-bot: I haz the power, 2015-11-20 13:00:34]

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

[commit-bot: I haz the power, 2015-11-20 13:00:35]

Try jobs failed on following builders:
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/10796)

[danilchap, 2015-11-20 15:34:37]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2015-11-20 15:34:38]

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

[commit-bot: I haz the power, 2015-11-20 15:34:45]

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

[commit-bot: I haz the power, 2015-11-20 16:27:54]

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

[commit-bot: I haz the power, 2015-11-20 16:27:55]

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

[danilchap, 2015-11-22 13:18:50]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2015-11-22 13:18:55]

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

[commit-bot: I haz the power, 2015-11-22 17:03:15]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-22 17:03:22]

Patchset 10 (id:??) landed as
https://crrev.com/50c5136cb2ad11eb9ba3df1a1d54d527c8a0dc77
Cr-Commit-Position: refs/heads/master@{#10741}