Cast: Refactor RTCP handling

Cast: Refactor RTCP handling

OBJECTIVES

This change is to refactor RTCP handling in media/cast such that RTCP
message parsing is done in CastTransportSender.

This side effect of this refactoring is much cleaner code: RTCP handling
code are grouped under one module. VideoSender and AudioSender do not
handle packets directly.

This also affect the architectutre in Chrome that RTCP message parsing
is now done in the browser process. A parsed RTCP message is passed to
the renderer instead of raw packets.

REATIONALE

RTCP is used as a feedback mechanism in Cast Streaming. It used to be
parsed and handled by VideoSender and AudioSender in the render process.
This was very ineffective because packetization and sending is done in
the browser process. This created inefficiencies in packet
retransmission. It also made improvement to the transport protocol /
algorithm much more difficult. A side effect is very messy code.

DETAILS

Rtcp - This class is responsibile for maintaining a bi-directional RTCP
session. It is now owned by CastTransportSender.

CastTransportSender - It now performs packetization and also parsing of
incoming RTCP packets. RTCP packets are small UDP packets for signaling
only. Eventually retransmission can also be moved to this class. It will
handle all packet level send transport.

AudioSender / VideoSender - They don't handle packet directly any more.
Which means render process doesn't see raw packets.

FrameSender - Base class for AudioSender and VideoSender to share code
for RTCP message handling. Eventually AudioSender and VideoSender will
merge here but it is not the point of this change.

RtcpBuilder - Removed and merged with RtcpSender.

Net deleted 400 lines of code and simulation shows the same results.

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

[Alpha Left Google, 2014-07-13 02:29:48]

Please review before I start to change the IPC layer.

[miu, 2014-07-16 00:09:32]

Still working on the review.  First round comments on the first half or so:

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:82: virtual void SendRtcpFromRtpSender(
Can we name this SendSenderReport instead?  Or, even more specific,
SendLipSyncReport?

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:47: logging_(),
nit: You can just remove this.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:95:
status_callback_.Run(TRANSPORT_AUDIO_INITIALIZED);
Let's move this line to the bottom of the method, so that the callback is
invoked once all the initialization is complete.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:98:
status_callback_.Run(TRANSPORT_AUDIO_UNINITIALIZED);
Need a return statement after this statement.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:128:
status_callback_.Run(TRANSPORT_VIDEO_INITIALIZED);
ditto: Move this statement to the bottom of this method.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:131:
status_callback_.Run(TRANSPORT_VIDEO_UNINITIALIZED);
Need a return statement after this statement.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:192: LOG(ERROR) << "Invalid request
for sending RTCP packet.";
IMHO, this should should be a NOTREACHED().

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:193: return;
You can remove this return statement now.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:225:
event_subscriber_->GetPacketEventsAndReset(&packet_events);
Hopefully, event_subscriber_ only resets packet event state, and not also the
frame event state.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:25: RtcpMessageHandlerImpl(Rtcp* rtcp)
explicit

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:68: base::TickClock* clock,
style/consistency nit: Can you make |clock| be the first arg of this ctor, so
that the ordering is the same as initialization?

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:87: rtcp_receiver_.reset(new
RtcpReceiver(handler_.get(), local_ssrc));
Can you move this into the initializer list?  ...and then make the scoped_ptr<>
const in the header file?

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:99: if (!RtcpReceiver::IsRtcpPacket(data, length)) {
These 5 LOC seem to be redundant, since rtcp_parser.IsValid() is checked below
now.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:50: class RtcpMessageHandlerImpl;
Any reason why this isn't just an private internal class of Rtcp?  Seems this
would be simpler than forward declaring it here and also "friending" it later.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:71: void SendRtcpFromRtpSender(
ditto re: method rename: SendSenderReport

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:89: // and uesd to maintain a RTCP session.
s/uesd/used/

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:120: friend class RtcpMessageHandlerImpl;
s/friend //  (see comment above)

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:134: base::TickClock* clock_;  // Not owned by this
class.
nit: base::TickClock* const clock_;

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:142: scoped_ptr<RtcpMessageHandlerImpl> handler_;
nit: const scoped_ptr<RtcpMessageHandlerImpl> handler_;

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:143: scoped_ptr<RtcpSender> rtcp_sender_;
nit: const

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_receiver.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:47: receiver_event_history_size_(0) {}
To simplify, consider adding DCHECK(handler_) here, and then remove all the NULL
checks in the rest of this module.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:52: bool RtcpReceiver::IsRtcpPacket(const
uint8* packet, size_t length) {
You could get rid of this method, and roll it into GetSsrcOfSender.  See
comments below.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:66: DCHECK_GE(length, kMinLengthOfRtcp) <<
"Invalid RTCP packet";
Don't need this DCHECK if checking big_endian_reader.XXX() return values (see
below).

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:70: big_endian_reader.Skip(4);  // Skip
header
Note: These two method calls on big_endian_reader return false on failure (i.e.,
they check bounds).  Suggest replacing with this code:

  uint8 packet_type;
  if (big_endian_reader.Skip(2) &&
      big_endian_reader.ReadU8(&packet_type) &&
      packet_type >= kPacketTypeLow &&
      packet_type <= kPacketTypeHigh &&
      big_endian_reader.Skip(1) &&
      big_endian_reader.ReadU32(&ssrc_of_sender)) {
    return ssrc_of_sender;
  } else {
    return 0;
  }

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_receiver.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.h:43: explicit
RtcpReceiver(RtcpMessageHandler* handler,
Don't need explicit keyword here.

[miu, 2014-07-16 19:58:03]

...and I'm now finished reviewing PS2:

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_sender.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_sender.cc:235: return;  // Sanity - don't send empty
packets.
I wonder if there should be a NOTREACHED() here?  It seems bad that the calling
code would ask to send a report without any information in it.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_unittest.cc:71:
LocalRtcpTransport(base::SimpleTestTickClock* testing_clock)
explicit

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender.cc:12: #include
"media/cast/net/rtcp/rtcp_defines.h"
Remove this include.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
File media/cast/sender/audio_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender.h:17: #include "media/cast/cast_environment.h"
You can remove the following includes:

cast_environment.h
logging_defines.h
rtcp.h
rtp_timestamp_helper.h

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender.h:44: CastInitializationStatus
InitializationResult() const {
This is common to both AudioSender and VideoSender, and should be moved into the
new FrameSender base class.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
File media/cast/sender/audio_sender_unittest.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender_unittest.cc:16: #include
"media/cast/net/rtcp/rtcp.h"
This, and possibly other #includes can now be removed.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:6: // TODO(hclam): Move AudioSender and
VideoSender into this class.
nit: For clarity, could this TODO read as: Refactor such that there is no
separate AudioSender vs. VideoSender, and the functionality of both is rolled
into this class.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:65: const bool is_audio_;
Could you add a TODO to remove this is_audio bool throughout the class?  In a
later change, you plan to have a RegisterSsrcPriority() option in the
CastTransportSender, so you will no longer need to differentiate here.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.cc:16: #include
"media/cast/net/rtcp/rtcp_defines.h"
Remove this include.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.h:16: #include "media/cast/cast_environment.h"
You can remove the following includes:

cast_environment.h
logging_defines.h
rtcp.h
rtp_timestamp_helper.h

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.h:29: class CastTransportSender;
Remove this forward decl.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.h:51: CastInitializationStatus
InitializationResult() const {
ditto: Can be moved to FrameSender.

[miu, 2014-07-16 19:59:25]

BTW--I really like this change, on the whole.  It's moving the
structure/factoring of a lot of code in the right direction, and it's great to
see all the clean-ups that are happening along the way.

[Alpha Left Google, 2014-07-17 01:01:47]

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:82: virtual void SendRtcpFromRtpSender(
On 2014/07/16 00:09:30, miu wrote:
> Can we name this SendSenderReport instead?  Or, even more specific,
> SendLipSyncReport?

Let's use the RTCP terminology of "SenderReport".

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:47: logging_(),
On 2014/07/16 00:09:30, miu wrote:
> nit: You can just remove this.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:95:
status_callback_.Run(TRANSPORT_AUDIO_INITIALIZED);
On 2014/07/16 00:09:30, miu wrote:
> Let's move this line to the bottom of the method, so that the callback is
> invoked once all the initialization is complete.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:98:
status_callback_.Run(TRANSPORT_AUDIO_UNINITIALIZED);
On 2014/07/16 00:09:31, miu wrote:
> Need a return statement after this statement.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:128:
status_callback_.Run(TRANSPORT_VIDEO_INITIALIZED);
On 2014/07/16 00:09:30, miu wrote:
> ditto: Move this statement to the bottom of this method.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:131:
status_callback_.Run(TRANSPORT_VIDEO_UNINITIALIZED);
On 2014/07/16 00:09:31, miu wrote:
> Need a return statement after this statement.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:192: LOG(ERROR) << "Invalid request
for sending RTCP packet.";
On 2014/07/16 00:09:30, miu wrote:
> IMHO, this should should be a NOTREACHED().

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:193: return;
On 2014/07/16 00:09:31, miu wrote:
> You can remove this return statement now.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender_impl.cc:225:
event_subscriber_->GetPacketEventsAndReset(&packet_events);
On 2014/07/16 00:09:31, miu wrote:
> Hopefully, event_subscriber_ only resets packet event state, and not also the
> frame event state.

Yes, that's what the contract says.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:25: RtcpMessageHandlerImpl(Rtcp* rtcp)
On 2014/07/16 00:09:31, miu wrote:
> explicit

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:68: base::TickClock* clock,
On 2014/07/16 00:09:31, miu wrote:
> style/consistency nit: Can you make |clock| be the first arg of this ctor, so
> that the ordering is the same as initialization?

Swapped the members instead. It's much bigger to swap Rtcp() parameters.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:87: rtcp_receiver_.reset(new
RtcpReceiver(handler_.get(), local_ssrc));
On 2014/07/16 00:09:31, miu wrote:
> Can you move this into the initializer list?  ...and then make the
scoped_ptr<>
> const in the header file?

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.cc:99: if (!RtcpReceiver::IsRtcpPacket(data, length)) {
On 2014/07/16 00:09:31, miu wrote:
> These 5 LOC seem to be redundant, since rtcp_parser.IsValid() is checked below
> now.

This checks for Cast specific RTCP payload. While RtcpParser just validates
generally well formed RTCP packets.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:50: class RtcpMessageHandlerImpl;
On 2014/07/16 00:09:31, miu wrote:
> Any reason why this isn't just an private internal class of Rtcp?  Seems this
> would be simpler than forward declaring it here and also "friending" it later.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:71: void SendRtcpFromRtpSender(
On 2014/07/16 00:09:31, miu wrote:
> ditto re: method rename: SendSenderReport

I'd like to do this in a later change. This is so that SenderRtcpFromRtp* looks
consistent in this class.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:89: // and uesd to maintain a RTCP session.
On 2014/07/16 00:09:31, miu wrote:
> s/uesd/used/

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:120: friend class RtcpMessageHandlerImpl;
On 2014/07/16 00:09:31, miu wrote:
> s/friend //  (see comment above)

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:134: base::TickClock* clock_;  // Not owned by this
class.
On 2014/07/16 00:09:31, miu wrote:
> nit: base::TickClock* const clock_;

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:143: scoped_ptr<RtcpSender> rtcp_sender_;
On 2014/07/16 00:09:31, miu wrote:
> nit: const

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_receiver.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:47: receiver_event_history_size_(0) {}
On 2014/07/16 00:09:32, miu wrote:
> To simplify, consider adding DCHECK(handler_) here, and then remove all the
NULL
> checks in the rest of this module.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:52: bool RtcpReceiver::IsRtcpPacket(const
uint8* packet, size_t length) {
On 2014/07/16 00:09:32, miu wrote:
> You could get rid of this method, and roll it into GetSsrcOfSender.  See
> comments below.

0 is a valid SSRC so I would like to keep this method.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.cc:70: big_endian_reader.Skip(4);  // Skip
header
On 2014/07/16 00:09:32, miu wrote:
> Note: These two method calls on big_endian_reader return false on failure
(i.e.,
> they check bounds).  Suggest replacing with this code:
> 
>   uint8 packet_type;
>   if (big_endian_reader.Skip(2) &&
>       big_endian_reader.ReadU8(&packet_type) &&
>       packet_type >= kPacketTypeLow &&
>       packet_type <= kPacketTypeHigh &&
>       big_endian_reader.Skip(1) &&
>       big_endian_reader.ReadU32(&ssrc_of_sender)) {
>     return ssrc_of_sender;
>   } else {
>     return 0;
>   }

IsRtcpPacket is actually useful because 0 is also a valid SSRC. The bounds check
is guaranteed by kMinLengthOfRtcp already, I've made it an if statement.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_receiver.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_receiver.h:43: explicit
RtcpReceiver(RtcpMessageHandler* handler,
On 2014/07/16 00:09:32, miu wrote:
> Don't need explicit keyword here.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_sender.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_sender.cc:235: return;  // Sanity - don't send empty
packets.
On 2014/07/16 19:58:02, miu wrote:
> I wonder if there should be a NOTREACHED() here?  It seems bad that the
calling
> code would ask to send a report without any information in it.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_unittest.cc:71:
LocalRtcpTransport(base::SimpleTestTickClock* testing_clock)
On 2014/07/16 19:58:03, miu wrote:
> explicit

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender.cc:12: #include
"media/cast/net/rtcp/rtcp_defines.h"
On 2014/07/16 19:58:03, miu wrote:
> Remove this include.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
File media/cast/sender/audio_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender.h:17: #include "media/cast/cast_environment.h"
On 2014/07/16 19:58:03, miu wrote:
> You can remove the following includes:
> 
> cast_environment.h
> logging_defines.h
> rtcp.h
> rtp_timestamp_helper.h

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender.h:44: CastInitializationStatus
InitializationResult() const {
On 2014/07/16 19:58:03, miu wrote:
> This is common to both AudioSender and VideoSender, and should be moved into
the
> new FrameSender base class.

Let's do the merge separately.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
File media/cast/sender/audio_sender_unittest.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/audio...
media/cast/sender/audio_sender_unittest.cc:16: #include
"media/cast/net/rtcp/rtcp.h"
On 2014/07/16 19:58:03, miu wrote:
> This, and possibly other #includes can now be removed.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:6: // TODO(hclam): Move AudioSender and
VideoSender into this class.
On 2014/07/16 19:58:03, miu wrote:
> nit: For clarity, could this TODO read as: Refactor such that there is no
> separate AudioSender vs. VideoSender, and the functionality of both is rolled
> into this class.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:65: const bool is_audio_;
On 2014/07/16 19:58:03, miu wrote:
> Could you add a TODO to remove this is_audio bool throughout the class?  In a
> later change, you plan to have a RegisterSsrcPriority() option in the
> CastTransportSender, so you will no longer need to differentiate here.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.cc:16: #include
"media/cast/net/rtcp/rtcp_defines.h"
On 2014/07/16 19:58:03, miu wrote:
> Remove this include.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.h:16: #include "media/cast/cast_environment.h"
On 2014/07/16 19:58:03, miu wrote:
> You can remove the following includes:
> 
> cast_environment.h
> logging_defines.h
> rtcp.h
> rtp_timestamp_helper.h

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.h:29: class CastTransportSender;
On 2014/07/16 19:58:03, miu wrote:
> Remove this forward decl.

Done.

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/video...
media/cast/sender/video_sender.h:51: CastInitializationStatus
InitializationResult() const {
On 2014/07/16 19:58:03, miu wrote:
> ditto: Can be moved to FrameSender.

Let's do the merge for RTCP-unrelated items later.

[Alpha Left Google, 2014-07-17 04:41:16]

+palmer +tsepez for reviewing IPC changes and overall security review.

Patch Set 4 contains the IPC changes to move the RTCP parsing to the browser
process. Please diff it from patch set 3.

Incoming RTCP packets are parsed, two new signals are added:
* CastMsg_Rtt
  For RTT report.
* CastMsg_RtcpCastMessage
  For sending RtcpCastMessage to the render process.

RTCP packet parsing happens in these files:
* media/cast/net/rtcp/rtcp_parser.cc
* media/cast/net/rtcp/rtcp_receiver.cc

[Alpha Left Google, 2014-07-17 04:43:24]

> RTCP packet parsing happens in these files:
> * media/cast/net/rtcp/rtcp_parser.cc
> * media/cast/net/rtcp/rtcp_receiver.cc

Correction. It should be:
* media/cast/net/rtcp/rtcp_parser.cc
* media/cast/net/rtcp/rtcp_utillity.cc

The entry point for parsing RTCP packets is Rtcp::IncomingRtcpPacket().

[Tom Sepez, 2014-07-17 16:35:20]

Forgive my ignorance, but parsing anything in the browser seems like a bad idea
from a security perspective compared to parsing it in the renderer.  The input
to the parser is untrusted and comes off the network, right?

[Alpha Left Google, 2014-07-17 17:48:26]

On 2014/07/17 16:35:20, Tom Sepez wrote:
> Forgive my ignorance, but parsing anything in the browser seems like a bad
idea
> from a security perspective compared to parsing it in the renderer.  The input
> to the parser is untrusted and comes off the network, right?

Yes it is untrusted and comes off the UDP socket.

We do have some measures to check the message is well formed. Each message is no
more than a UDP sized packet, i.e. max 1500 bytes. The messages are mostly for
flow control and don't form any payload.

the major reason we're doing this is to improvement the performance of the send
transport. Frames are already encrypted, packetized and sent in the browser
process. The feedback for flow control however has to go through a lot of hops:
browser -> renderer (parser) -> browser (transport). This change is such that
these feedback messages directly goes back to the transport, for retransmission,
etc. This also reduces a lot of complexity.

I would like to know if we can do anything to alleviate the risk of doing this
parsing operation in the browser process.

[Alpha Left Google, 2014-07-17 17:50:38]

> I would like to know if we can do anything to alleviate the risk of doing this
> parsing operation in the browser process.

Suggestion: what if I move RTCP parsing logic to net/ to give it more scrutiny?

[Tom Sepez, 2014-07-17 18:46:37]

On 2014/07/17 17:50:38, Alpha wrote:
> > I would like to know if we can do anything to alleviate the risk of doing
this
> > parsing operation in the browser process.
> 
> Suggestion: what if I move RTCP parsing logic to net/ to give it more
scrutiny?
I'm trying to balance the sheer terror of being only one bug removed from a full
sandbox bypass against the need for simpler code. We are not happy that we
currently parse http in the browser prices, and we would like to isolate it into
its own sandbox process someday. This feels like a step in the wrong direction.
As to /net, I'm not convinced the location of the coffee has bearing on the
scrutiny it receives, scrutiny being an insufficient replacement for sandboxing
in the first place.

[Tom Sepez, 2014-07-17 18:46:40]

On 2014/07/17 17:50:38, Alpha wrote:
> > I would like to know if we can do anything to alleviate the risk of doing
this
> > parsing operation in the browser process.
> 
> Suggestion: what if I move RTCP parsing logic to net/ to give it more
scrutiny?
I'm trying to balance the sheer terror of being only one bug removed from a full
sandbox bypass against the need for simpler code. We are not happy that we
currently parse http in the browser prices, and we would like to isolate it into
its own sandbox process someday. This feels like a step in the wrong direction.
As to /net, I'm not convinced the location of the coffee has bearing on the
scrutiny it receives, scrutiny being an insufficient replacement for sandboxing
in the first place.

[Tom Sepez, 2014-07-17 18:57:21]

So much for tablet Thursday.
S/prices/process/
S/coffee/code.
Sorry :(

[Tom Sepez, 2014-07-17 18:57:24]

So much for tablet Thursday.
S/prices/process/
S/coffee/code.
Sorry :(

[Alpha Left Google, 2014-07-17 21:53:17]

> I'm trying to balance the sheer terror of being only one bug removed from a
full
> sandbox bypass against the need for simpler code. We are not happy that we
> currently parse http in the browser prices, and we would like to isolate it
into
> its own sandbox process someday. This feels like a step in the wrong
direction.
> As to /net, I'm not convinced the location of the coffee has bearing on the
> scrutiny it receives, scrutiny being an insufficient replacement for
sandboxing
> in the first place.

I hope I can help to alleviate the concerns. :)

Just to give you some context. We went through a security review by palmer@
before
for the part that encrypts, packetize and sends to UDP in the browser process.
This code is a uni-directional network transport. I'm proposing to move the part
that
does feedback for retransmission, bandwidth estimation and flow control to the
browser process in this change.

The packets that are read off the UDP socket have rigid wire format and it is
much
less complex than HTTP. In fact there are roughly ~4 types of messages that we
need. This is part of the RTP/RTCP protocol for flow control.

The effort is more than just code cleanup (although it's very nice to have). It
helps
to improve the performance of retransmission and better flow control.

[Alpha Left Google, 2014-07-17 22:34:28]

> The packets that are read off the UDP socket have rigid wire format and it is
> much
> less complex than HTTP. In fact there are roughly ~4 types of messages that we
> need. This is part of the RTP/RTCP protocol for flow control.

By the way the parser now parses many more types of messages but many can be
removed.
We only care about 9 today:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/net/cas...
And this can be further reduced.

[Tom Sepez, 2014-07-17 23:07:54]

Ok.  One more thing that will have to move to a utility process when the rest of
the network stack does as well. I'll take a look at the details ow.

[Alpha Left Google, 2014-07-17 23:11:07]

On 2014/07/17 23:07:54, Tom Sepez wrote:
> Ok.  One more thing that will have to move to a utility process when the rest
of
> the network stack does as well. I'll take a look at the details ow.

Heh about this networking utility process. This was the idea I proposed (but
rejected because of sandboxing sockets on windows) early on about having a
separate rtc process which does mostly network transport for media. So if we
decide to move all networking code to a utility process in the future I'll be
very happy to do that.

[Alpha Left Google, 2014-07-17 23:20:01]

> Heh about this networking utility process. This was the idea I proposed (but
> rejected because of sandboxing sockets on windows) early on about having a
> separate rtc process which does mostly network transport for media. So if we
> decide to move all networking code to a utility process in the future I'll be
> very happy to do that.

Whoops spoke too fast. Maybe I misunderstood. I'll wait until I see the details.
:)

[Tom Sepez, 2014-07-17 23:21:46]

Messages LGTM.

[miu, 2014-07-18 00:06:23]

lgtm % a few things to address before commit:

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/387933005/diff/100001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:65: const bool is_audio_;
On 2014/07/17 01:01:46, Alpha wrote:
> On 2014/07/16 19:58:03, miu wrote:
> > Could you add a TODO to remove this is_audio bool throughout the class?  In
a
> > later change, you plan to have a RegisterSsrcPriority() option in the
> > CastTransportSender, so you will no longer need to differentiate here.
> 
> Done.

Much better!  :)

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:124:
base::Bind(&CastTransportHostFilter::CastMessage,
The base::Unretained() makes me nervous.  Can you write a quick comment about
why this is safe?

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:27: void RawEvents(int32
channel_id,
naming: Let's prepend a verb at the beginning of these three methods. 
Suggestions:

  SendRawEvents
  SendRtt
  SendCastMessage

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_ipc_dispatcher_unittest.cc (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_ipc_dispatcher_unittest.cc:43:
media::cast::FrameEvent frame_event;
I think you meant this LOC to be these two:

  frame_events.push_back();
  media::cast::FrameEvent& frame_event = frame_events.back();

Otherwise, you're always sending an empty frame_events vector.  ;-)

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:99: if (clients_.find(ssrc)
== clients_.end()) {
This code is doing 3 map look-ups.  Consider just using an iterator for one
look-up:

  const std::map<uint32, ClientCallbacks> it = clients_.find(ssrc);
  if (it == clients_.end()) {
    ...
  }
  if (it->rtt_cb.is_null())
    return;
  it->rtt_cb.Run(
    ...);

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:115: if (clients_.find(ssrc)
== clients_.end()) {
ditto: Use an iterator.

https://codereview.chromium.org/387933005/diff/140001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/387933005/diff/140001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: I could be wrong, but I don't think you're supposed to change the copyright
year.  (and in the other files)

[Alpha Left Google, 2014-07-18 01:14:31]

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:124:
base::Bind(&CastTransportHostFilter::CastMessage,
On 2014/07/18 00:06:22, miu wrote:
> The base::Unretained() makes me nervous.  Can you write a quick comment about
> why this is safe?

Changed it to WeakPtr.

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:27: void RawEvents(int32
channel_id,
On 2014/07/18 00:06:23, miu wrote:
> naming: Let's prepend a verb at the beginning of these three methods. 
> Suggestions:
> 
>   SendRawEvents
>   SendRtt
>   SendCastMessage

Done.

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_ipc_dispatcher_unittest.cc (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_ipc_dispatcher_unittest.cc:43:
media::cast::FrameEvent frame_event;
On 2014/07/18 00:06:23, miu wrote:
> I think you meant this LOC to be these two:
> 
>   frame_events.push_back();
>   media::cast::FrameEvent& frame_event = frame_events.back();
> 
> Otherwise, you're always sending an empty frame_events vector.  ;-)

Done.

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:99: if (clients_.find(ssrc)
== clients_.end()) {
On 2014/07/18 00:06:23, miu wrote:
> This code is doing 3 map look-ups.  Consider just using an iterator for one
> look-up:
> 
>   const std::map<uint32, ClientCallbacks> it = clients_.find(ssrc);
>   if (it == clients_.end()) {
>     ...
>   }
>   if (it->rtt_cb.is_null())
>     return;
>   it->rtt_cb.Run(
>     ...);

Done.

https://codereview.chromium.org/387933005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:115: if (clients_.find(ssrc)
== clients_.end()) {
On 2014/07/18 00:06:23, miu wrote:
> ditto: Use an iterator.

Done.

https://codereview.chromium.org/387933005/diff/140001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/387933005/diff/140001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/07/18 00:06:23, miu wrote:
> nit: I could be wrong, but I don't think you're supposed to change the
copyright
> year.  (and in the other files)

git cl upload doesn't complain though.

[Alpha Left Google, 2014-07-18 01:14:45]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-18 01:17:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/387933005/160001

[commit-bot: I haz the power, 2014-07-18 03:05:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30129)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/34543)

[commit-bot: I haz the power, 2014-07-18 03:07:41]

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

[commit-bot: I haz the power, 2014-07-18 03:07:42]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30158)

[Alpha Left Google, 2014-07-18 18:39:06]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-18 18:40:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/387933005/180001

[Alpha Left Google, 2014-07-18 20:05:40]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-18 20:07:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/387933005/200001

[Alpha Left Google, 2014-07-18 23:14:23]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:15:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/387933005/220001

[commit-bot: I haz the power, 2014-07-19 02:48:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-19 05:06:49]

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

[commit-bot: I haz the power, 2014-07-19 05:06:50]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Alpha Left Google, 2014-07-19 23:34:05]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-19 23:34:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/387933005/240001

[commit-bot: I haz the power, 2014-07-20 04:30:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-20 04:42:52]

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

[commit-bot: I haz the power, 2014-07-20 04:42:54]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Alpha Left Google, 2014-07-20 05:05:14]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-07-20 05:06:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/387933005/240001

[commit-bot: I haz the power, 2014-07-20 07:13:32]

Change committed as 284363