Cast: Restructure RTCP interface to better fit our usage.

Cast: Restructure RTCP interface to better fit our usage.

New structure allows a better interface to add sending and receiving
log messages from the remote peer over RTCP. The acctual RTCP messaging
and plugging this into the log class will follw in 2 seperate cls.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235807

[pwestin, 2013-11-12 20:04:29]

Please review.

[Alpha Left Google, 2013-11-13 23:58:39]

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp.h
File media/cast/rtcp/rtcp.h (right):

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp.h#newcode86
media/cast/rtcp/rtcp.h:86: void SendRtcpFromRtpSender(const
RtcpSenderLogMessage* sender_log_message);
What does it mean if the parameter is NULL? Looks like this sends a SR by
default and sender log is optional. A comment is needed.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp.h#newcode88
media/cast/rtcp/rtcp.h:88: void SendRtcpFromRtpReceiver(const RtcpCastMessage*
cast_message,
This method looks funny that it takes two const pointer of logs. What happen if
one of the input is NULL? What happen if both input are NULL? Please comment.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines.h
File media/cast/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:39: RtcpSenderFrameStatus frame_status_;
If this is a struct then just |frame_status| and |rtp_timestamp|.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:43: class RtcpSenderLogMessage {
This class is not really necessary. Just use a typedef:

typedef std::list<RtcpSenderFrameLogMessage> RtpSenderFrameLogList.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:50: CastLoggingEvent type_;
This should be |type| |event_timestamp| |delay_delta| |packet_id|.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:58: uint32 rtp_timestamp_;
|rtp_timestamp| and |event_log_messages|

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:62: class RtcpReceiverLogMessage {
This is not necessary. Just do a typedef.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.cc
File media/cast/rtcp/rtcp_sender.cc (right):

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.c...
media/cast/rtcp/rtcp_sender.cc:37: if (packet_type_flags & kRtcpRr ||
This condition in incorrect. It should be the opposite.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.c...
media/cast/rtcp/rtcp_sender.cc:66: if (packet.empty()) return;  // Sanity don't
send empty packets.
nit: drop return to next line.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.c...
media/cast/rtcp/rtcp_sender.cc:77: if (packet_type_flags & kRtcpSr ||
This condition is not correct. It should be the opposite.

[pwestin, 2013-11-15 00:20:31]

PTAL

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp.h
File media/cast/rtcp/rtcp.h (right):

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp.h#newcode86
media/cast/rtcp/rtcp.h:86: void SendRtcpFromRtpSender(const
RtcpSenderLogMessage* sender_log_message);
On 2013/11/13 23:58:40, Alpha wrote:
> What does it mean if the parameter is NULL? Looks like this sends a SR by
> default and sender log is optional. A comment is needed.
> 

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp.h#newcode88
media/cast/rtcp/rtcp.h:88: void SendRtcpFromRtpReceiver(const RtcpCastMessage*
cast_message,
On 2013/11/13 23:58:40, Alpha wrote:
> This method looks funny that it takes two const pointer of logs. What happen
if
> one of the input is NULL? What happen if both input are NULL? Please comment.

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines.h
File media/cast/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:39: RtcpSenderFrameStatus frame_status_;
On 2013/11/13 23:58:40, Alpha wrote:
> If this is a struct then just |frame_status| and |rtp_timestamp|.

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:43: class RtcpSenderLogMessage {
On 2013/11/13 23:58:40, Alpha wrote:
> This class is not really necessary. Just use a typedef:
> 
> typedef std::list<RtcpSenderFrameLogMessage> RtpSenderFrameLogList.

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:50: CastLoggingEvent type_;
On 2013/11/13 23:58:40, Alpha wrote:
> This should be |type| |event_timestamp| |delay_delta| |packet_id|.

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:58: uint32 rtp_timestamp_;
On 2013/11/13 23:58:40, Alpha wrote:
> |rtp_timestamp| and |event_log_messages|

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_defines....
media/cast/rtcp/rtcp_defines.h:62: class RtcpReceiverLogMessage {
On 2013/11/13 23:58:40, Alpha wrote:
> This is not necessary. Just do a typedef.

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.cc
File media/cast/rtcp/rtcp_sender.cc (right):

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.c...
media/cast/rtcp/rtcp_sender.cc:37: if (packet_type_flags & kRtcpRr ||
On 2013/11/13 23:58:40, Alpha wrote:
> This condition in incorrect. It should be the opposite.

Please clarify; this does what I want it to do. If any of the bits representing
kRtcpRr, kRtcpPli .. etc is set some one miss use the function.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.c...
media/cast/rtcp/rtcp_sender.cc:66: if (packet.empty()) return;  // Sanity don't
send empty packets.
On 2013/11/13 23:58:40, Alpha wrote:
> nit: drop return to next line.

Done.

https://codereview.chromium.org/70713002/diff/1/media/cast/rtcp/rtcp_sender.c...
media/cast/rtcp/rtcp_sender.cc:77: if (packet_type_flags & kRtcpSr ||
On 2013/11/13 23:58:40, Alpha wrote:
> This condition is not correct. It should be the opposite.

Please clarify; this does what I want it to do. If any of the bits representing
kRtcpSr, kRtcpDlrr .. etc is set some one miss use the function.

[Alpha Left Google, 2013-11-15 19:42:21]

LGTM.

[commit-bot: I haz the power, 2013-11-15 20:10:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/70713002/80001

[commit-bot: I haz the power, 2013-11-15 22:57:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/70713002/390001

[commit-bot: I haz the power, 2013-11-16 01:00:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/70713002/390001

[commit-bot: I haz the power, 2013-11-16 03:35:32]

Retried try job too often on win_rel for step(s) browser_tests,
chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests,
content_browsertests, mini_installer_test, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-18 16:31:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/70713002/810001

[commit-bot: I haz the power, 2013-11-18 18:05:01]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-11-18 18:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/70713002/1190001

[commit-bot: I haz the power, 2013-11-18 20:33:29]

Change committed as 235807