Replace rtcp packet parsing in the RtcpReceiver.

Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260
Committed: https://crrev.com/1b1863a11aa795ceb5286a73b0d391b1be86f556
Cr-Commit-Position: refs/heads/master@{#14301}

[danilchap, 2016-09-16 10:12:38]

Description was changed from

==========
[Draft] Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260
==========

to

==========
Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260
==========

[danilchap, 2016-09-16 11:06:46]

Patchset #6 (id:100001) has been deleted

[danilchap, 2016-09-16 11:11:10]

Patchset #6 (id:120001) has been deleted

[danilchap, 2016-09-16 11:19:58]

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

[philipel, 2016-09-19 11:01:38]

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:46: using
::webrtc::rtcp::CommonHeader;
remove ::webrtc::

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:308: ptrdiff_t tail_size =
packet_end - next_block;
block_size

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:412: ++num_skipped_packets_;
Maybe change all RTCPReceiver::Handle* functions to return a bool and increment
|num_skipped_packets_| in ParseCompoundPackets?

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:452: for (const
rtcp::ReportBlock report_block : sender_report.report_blocks()) {
remove {}

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:482: for (const ReportBlock&
report_block : receiver_report.report_blocks()) {
remove {}

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:491:
EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPReceiver) {
remove thread annotation from .cc file

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:791: if (receiver_only_ ||
main_ssrc_ != nack.media_ssrc()) {
remove {}

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:796: for (uint16_t packet_id :
nack.packet_ids()) {
remove {}

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:878: if
(registered_ssrcs_.find(rti.ssrc) == registered_ssrcs_.end()) {
remove {}

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:967: for (const auto& item :
tmmbn.items()) {
remove {}

[danilchap, 2016-09-19 13:35:26]

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:46: using
::webrtc::rtcp::CommonHeader;
On 2016/09/19 11:01:38, philipel wrote:
> remove ::webrtc::

Done.
Planned to ack, reread guide more carefully, changed, thank you.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:308: ptrdiff_t tail_size =
packet_end - next_block;
On 2016/09/19 11:01:38, philipel wrote:
> block_size

s/tail/remaining_blocks/
It is more than one block, it is size of all remaining blocks combined.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:412: ++num_skipped_packets_;
On 2016/09/19 11:01:38, philipel wrote:
> Maybe change all RTCPReceiver::Handle* functions to return a bool and
increment
> |num_skipped_packets_| in ParseCompoundPackets?

Interesting idea, but it seems wrong for Handle<> function to return true when
it skips packet because of mismatched remote ssrc.
Mismatch remote ssrc is common and should be ignored without increase
num_skipped_packets.

While I agree the block "if (!typed_packet.Parse()) Skip();" repeated too many
times and better to unify, prefer to tackle that problem in a different,
dedicated CL.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:452: for (const
rtcp::ReportBlock report_block : sender_report.report_blocks()) {
On 2016/09/19 11:01:38, philipel wrote:
> remove {}

Done.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:482: for (const ReportBlock&
report_block : receiver_report.report_blocks()) {
On 2016/09/19 11:01:38, philipel wrote:
> remove {}

Done.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:491:
EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPReceiver) {
On 2016/09/19 11:01:38, philipel wrote:
> remove thread annotation from .cc file

oops, Removed.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:791: if (receiver_only_ ||
main_ssrc_ != nack.media_ssrc()) {
On 2016/09/19 11:01:38, philipel wrote:
> remove {}

Done.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:796: for (uint16_t packet_id :
nack.packet_ids()) {
On 2016/09/19 11:01:38, philipel wrote:
> remove {}

Done.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:878: if
(registered_ssrcs_.find(rti.ssrc) == registered_ssrcs_.end()) {
On 2016/09/19 11:01:38, philipel wrote:
> remove {}

Done.

https://codereview.webrtc.org/2316093002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:967: for (const auto& item :
tmmbn.items()) {
On 2016/09/19 11:01:38, philipel wrote:
> remove {}

Done.

[philipel, 2016-09-19 13:48:34]

lgtm

[sprang_webrtc, 2016-09-19 14:10:53]

lgtm with nit

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:312: if (first_block) {
Don't need this temp bool?
Also, would be nice with a comment explaining why we only treat the first block
failure as failure.

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:760: for (const auto& chunk :
sdes.chunks()) {
nit: Personally I prefer the actual type here, rather than auto, since it makes
it a bit easier to see what we are dealing with. Unless it's a long ugly stl
iterator or other verbose thing.
Up to you though.

[sprang_webrtc, 2016-09-19 14:10:53]

lgtm with nit

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:312: if (first_block) {
Don't need this temp bool?
Also, would be nice with a comment explaining why we only treat the first block
failure as failure.

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:760: for (const auto& chunk :
sdes.chunks()) {
nit: Personally I prefer the actual type here, rather than auto, since it makes
it a bit easier to see what we are dealing with. Unless it's a long ugly stl
iterator or other verbose thing.
Up to you though.

[danilchap, 2016-09-19 18:06:23]

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:312: if (first_block) {
On 2016/09/19 14:10:53, språng wrote:
> Don't need this temp bool?

It served as a comment what is really checked there,
split meaning (first_block) from the way it is calculated (next_block ==
packet_begin).
Removed since not helpful, comment can explain more.

> Also, would be nice with a comment explaining why we only treat the first
block
> failure as failure.

Simple reason: to keep old behavior.
Better reason: (wrote as a comment): no point to trigger any callbacks in this
case since nothing had changed.

https://codereview.webrtc.org/2316093002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:760: for (const auto& chunk :
sdes.chunks()) {
On 2016/09/19 14:10:53, språng wrote:
> nit: Personally I prefer the actual type here, rather than auto, since it
makes
> it a bit easier to see what we are dealing with. Unless it's a long ugly stl
> iterator or other verbose thing.
> Up to you though.

Done.
type not very descriptive.... but probably better to add it to show it is local,
not global type.

[danilchap, 2016-09-20 08:31:38]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-09-20 08:31:39]

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

[commit-bot: I haz the power, 2016-09-20 08:31:50]

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-09-20 08:39:56]

Description was changed from

==========
Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260
==========

to

==========
Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260
==========

[commit-bot: I haz the power, 2016-09-20 08:39:57]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-09-20 08:40:05]

Description was changed from

==========
Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260
==========

to

==========
Replace rtcp packet parsing in the RtcpReceiver.

BUG=webrtc:5260

Committed: https://crrev.com/1b1863a11aa795ceb5286a73b0d391b1be86f556
Cr-Commit-Position: refs/heads/master@{#14301}
==========

[commit-bot: I haz the power, 2016-09-20 08:40:06]

Patchset 8 (id:??) landed as
https://crrev.com/1b1863a11aa795ceb5286a73b0d391b1be86f556
Cr-Commit-Position: refs/heads/master@{#14301}