Add a ParseHeader method to RtcpPacket, for parsing common RTCP header.

Add a ParseHeader method to RtcpPacket, for parsing common RTCP header.

Also refactor TransportFeedback to use this.

BUG=

Committed: https://crrev.com/73a93e82579d6eeb3a1c4a63ef4b64c3c4d9bb18
Cr-Commit-Position: refs/heads/master@{#9935}

[sprang_webrtc, 2015-09-02 12:50:23]

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

[åsapersson, 2015-09-03 08:57:21]

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

https://codereview.webrtc.org/1307663004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet.cc:759: CommonHeader* header) {
Looks like RTCPUtility::RTCPParseCommonHeader can be used?

[sprang_webrtc, 2015-09-04 14:12:50]

I've merged the parsing from RtcpPacket into the old one in RTCPUtility. Now has
some additional checks and also unit tests. Did some light refactoring as well,
to be more in line with current code standards. 
What do you think?

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

https://codereview.webrtc.org/1307663004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet.cc:759: CommonHeader* header) {
On 2015/09/03 08:57:20, åsapersson wrote:
> Looks like RTCPUtility::RTCPParseCommonHeader can be used?

I've merged this into RTCPUtility.

[åsapersson, 2015-09-08 10:20:42]

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet.h (right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet.h:102: static void
CreateHeader(uint8_t count_or_format,
remove static?

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc
(right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc:17:
#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
should be first according to style guide?

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc (right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:72: static const size_t
kBufferCapacity = 40;
kHeaderSize -> kHeaderSizeInBytes
kBufferCapacity -> kBufferCapacityInBytes

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:76: 
I think these parsing tests should be where the parsing code is, for example in
rtcp_utility_unittest.cc (while RtcpPacket is for building packets).

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:77:
TEST_F(RtcpPacketHeaderTest, HeaderSize) {
Maybe rename to something like TooShortHeaderSize.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:110: const size_t
kPayloadSize = kSizeInBytes - kHeaderSize;
kPayloadSize -> kPayloadSizeInBytes

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:127:
TEST_F(RtcpPacketHeaderTest, FormatAndPayloadType) {}
remove

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:164: if (bytes_left == 0)
checked within RtcpParseCommonHeader?

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:420: const bool success =
RtcpParseCommonHeader(
could _ptrRTCPDataBegin be larger than _ptrRTCPDataEnd?

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:470: size_t max_length,
maybe remove index

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:473: size_t length = max_length -
index;
what if index > max_length

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:498: << " bytes) to fit a
FeedbackPacket of "
remove FeedbackPacket in comment

[sprang_webrtc, 2015-09-09 09:18:54]

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet.h (right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet.h:102: static void
CreateHeader(uint8_t count_or_format,
On 2015/09/08 10:20:41, åsapersson wrote:
> remove static?

Why keep it as a member? It never reads/writes any state...

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc
(right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc:17:
#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
On 2015/09/08 10:20:41, åsapersson wrote:
> should be first according to style guide?

Done. No idea why it was moved here.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc (right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:72: static const size_t
kBufferCapacity = 40;
On 2015/09/08 10:20:41, åsapersson wrote:
> kHeaderSize -> kHeaderSizeInBytes
> kBufferCapacity -> kBufferCapacityInBytes

Done.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:76: 
On 2015/09/08 10:20:41, åsapersson wrote:
> I think these parsing tests should be where the parsing code is, for example
in
> rtcp_utility_unittest.cc (while RtcpPacket is for building packets).

Fair enough. My long (long) term hope is to rewrite rtcp_receiver to use these
packets as well. But let's do that when/if it happens.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:77:
TEST_F(RtcpPacketHeaderTest, HeaderSize) {
On 2015/09/08 10:20:41, åsapersson wrote:
> Maybe rename to something like TooShortHeaderSize.

Renamed to TooSmallBuffer.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:110: const size_t
kPayloadSize = kSizeInBytes - kHeaderSize;
On 2015/09/08 10:20:41, åsapersson wrote:
> kPayloadSize -> kPayloadSizeInBytes

Done.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc:127:
TEST_F(RtcpPacketHeaderTest, FormatAndPayloadType) {}
On 2015/09/08 10:20:41, åsapersson wrote:
> remove

Or implement :)

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:164: if (bytes_left == 0)
On 2015/09/08 10:20:42, åsapersson wrote:
> checked within RtcpParseCommonHeader?

I think it would be better for RtcpParseCommonHeader to look for actual errors.
In the context here, having 0 bytes left in the buffer is OK. On the other hand,
having 3 bytes left is probably not OK but would yield the same return value.

Rather than adding a special case to RtcpParseCommonHeader for 0 bytes left (so
it doesn't spam the logs with warnings), I think it's better to handle that case
here.

Does that make sense?

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:420: const bool success =
RtcpParseCommonHeader(
On 2015/09/08 10:20:42, åsapersson wrote:
> could _ptrRTCPDataBegin be larger than _ptrRTCPDataEnd?

It shouldn't, but I can add a check anyway.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:470: size_t max_length,
On 2015/09/08 10:20:42, åsapersson wrote:
> maybe remove index

Ok. I use an offset in other upcoming CL's but I can change them to add offset
to the parameter instead.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:473: size_t length = max_length -
index;
On 2015/09/08 10:20:42, åsapersson wrote:
> what if index > max_length

Removed index.

https://codereview.webrtc.org/1307663004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:498: << " bytes) to fit a
FeedbackPacket of "
On 2015/09/08 10:20:42, åsapersson wrote:
> remove FeedbackPacket in comment

Done.

[åsapersson, 2015-09-10 13:27:34]

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:469: const size_t kHeaderLength =
4;
use define in RtcpCommonHeader?

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:502: padding_bytes =
packet[payload_size - 1];
if size_bytes is 4 and packet_size_words is 1 (length = 0), the padding_bytes
will be read from the length field (which is 0 so will work). perhaps change?

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc (right):

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:75: // Override
RtcpPacket so we can test protected methods.
override RtcpPacket?

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:76: class
RtcpPacketHeaderTest : public ::testing::Test, protected RtcpPacket {
maybe call the test RtcpParseCommonHeaderTest

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:93: static const size_t
kHeaderSizeBytes = 4;
use the define in RtcpCommonHeader

[sprang_webrtc, 2015-09-11 13:03:19]

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:469: const size_t kHeaderLength =
4;
On 2015/09/10 13:27:33, åsapersson wrote:
> use define in RtcpCommonHeader?

Done.

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:502: padding_bytes =
packet[payload_size - 1];
On 2015/09/10 13:27:33, åsapersson wrote:
> if size_bytes is 4 and packet_size_words is 1 (length = 0), the padding_bytes
> will be read from the length field (which is 0 so will work). perhaps change?

Done.
Good catch!

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc (right):

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:75: // Override
RtcpPacket so we can test protected methods.
On 2015/09/10 13:27:33, åsapersson wrote:
> override RtcpPacket?

Done.

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:76: class
RtcpPacketHeaderTest : public ::testing::Test, protected RtcpPacket {
On 2015/09/10 13:27:33, åsapersson wrote:
> maybe call the test RtcpParseCommonHeaderTest

Done.

https://codereview.webrtc.org/1307663004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:93: static const size_t
kHeaderSizeBytes = 4;
On 2015/09/10 13:27:33, åsapersson wrote:
> use the define in RtcpCommonHeader

Done.

[åsapersson, 2015-09-14 14:23:47]

lgtm (see comments)

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:501: if (payload_size -
RtcpCommonHeader::kHeaderSizeBytes < 1) {
or maybe if (payload_size <= RtcpCommonHeader::kHeaderSizeBytes)

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc (right):

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:15: #include
"webrtc/modules/rtp_rtcp/source/rtcp_packet.h"
remove rtcp_packet.h

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:83: size_t BlockLength()
const { return 0; }
remove BlockLength()

[sprang_webrtc, 2015-09-14 15:58:37]

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:501: if (payload_size -
RtcpCommonHeader::kHeaderSizeBytes < 1) {
On 2015/09/14 14:23:46, åsapersson wrote:
> or maybe if (payload_size <= RtcpCommonHeader::kHeaderSizeBytes)

Done.

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc (right):

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:15: #include
"webrtc/modules/rtp_rtcp/source/rtcp_packet.h"
On 2015/09/14 14:23:46, åsapersson wrote:
> remove rtcp_packet.h

Done.

https://codereview.webrtc.org/1307663004/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility_unittest.cc:83: size_t BlockLength()
const { return 0; }
On 2015/09/14 14:23:46, åsapersson wrote:
> remove BlockLength()

Done.

[sprang_webrtc, 2015-09-14 15:58:41]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2015-09-14 15:58:41]

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/1307663004/#ps100001 (title:
"Addressed comments, rebase")

[commit-bot: I haz the power, 2015-09-14 15:58:48]

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

[commit-bot: I haz the power, 2015-09-14 17:58:57]

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

[commit-bot: I haz the power, 2015-09-14 17:58:58]

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

[sprang_webrtc, 2015-09-14 19:49:30]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2015-09-14 19:49:43]

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

[commit-bot: I haz the power, 2015-09-14 19:50:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-14 19:50:56]

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