Cast: IPC from browser to renderer to send packet events from transport to cast library.

Cast: IPC from browser to renderer to send packet events from transport to cast library.

- Added a new IPC message: CastMsg_RawEvents
- Transmit cast logging settings to transport on browser side by adding a new field in CastTransportConfig.
- Install a LoggingImpl on transport side
-- If raw event logging is enabled, install a SimpleEventSubscriber to capture packet events.
-- If raw event logging is enabled, a RepeatingTimer will   be started to call the subscriber to collect packet events and send back to cast library via the IPC.

NOTE: Currently, no actual packet events are logged on the transport. That will be in the next CL. There are two ways to do this:
- Pass LoggingImpl to sub components on the transport (paced_sender, rtp_packetizer, etc.)
- Pass a callback that will be propagated down and invoked at the right place.

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

[imcheng, 2014-02-25 02:42:39]

PTAL. I am going to write tests for this but I wanted to gather some initial
feedback on this first. Thanks.

[Alpha Left Google, 2014-02-25 03:35:04]

On 2014/02/25 03:34:44, Alpha wrote:

Removing myself from reviewer list. Yuri is going help reviewing this one.

[hubbe, 2014-02-25 22:22:16]

Did you try building and running cast_unittests?
It seems like there should be some changes to the tests too?

https://codereview.chromium.org/178073004/diff/20001/chrome/browser/media/cas...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/178073004/diff/20001/chrome/browser/media/cas...
chrome/browser/media/cast_transport_host_filter.cc:77:
logging_config.enable_raw_data_collection = config.enable_raw_event_logging;
This seems really weird to me.
What is the point of CreateCastTransportSender taking a "logging_config" and a
"config", if the "config" already contains the   only value that we set in the
logging_config??

https://codereview.chromium.org/178073004/diff/20001/chrome/common/cast_messa...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/178073004/diff/20001/chrome/common/cast_messa...
chrome/common/cast_messages.h:93: IPC_STRUCT_TRAITS_MEMBER(rtp_timestamp)
Indentation...

[imcheng, 2014-02-25 23:14:55]

Yes, I had to make some changes to cast_unittests. Functionally the tests
remained the same for the most parts. Once I start logging packet events in a
follow-up CL, I will be able to write some more tests. PTAL.

https://codereview.chromium.org/178073004/diff/20001/chrome/browser/media/cas...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/178073004/diff/20001/chrome/browser/media/cas...
chrome/browser/media/cast_transport_host_filter.cc:77:
logging_config.enable_raw_data_collection = config.enable_raw_event_logging;
On 2014/02/25 22:22:16, hubbe wrote:
> This seems really weird to me.
> What is the point of CreateCastTransportSender taking a "logging_config" and a
> "config", if the "config" already contains the   only value that we set in the
> logging_config??

I was hoping to have a LoggingImpl in CastTransportSenderImpl without using a
scoped_ptr. I made some changes to CastLoggingConfig so that I don't have to
pass it in.

https://codereview.chromium.org/178073004/diff/20001/chrome/common/cast_messa...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/178073004/diff/20001/chrome/common/cast_messa...
chrome/common/cast_messages.h:93: IPC_STRUCT_TRAITS_MEMBER(rtp_timestamp)
On 2014/02/25 22:22:16, hubbe wrote:
> Indentation...

Done.

[hubbe, 2014-02-26 01:01:06]

https://codereview.chromium.org/178073004/diff/40001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/40001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender_unittest.cc:102: // Do nothing, we don't
care about logging in this test.
Is there *anything* we can add here to make it actually care a little bit?

https://codereview.chromium.org/178073004/diff/40001/media/cast/rtcp/rtcp_uni...
File media/cast/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/40001/media/cast/rtcp/rtcp_uni...
media/cast/rtcp/rtcp_unittest.cc:179: // Do nothing, we don't care about logging
in this test.
Can we care?

https://codereview.chromium.org/178073004/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/video_sender_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/40001/media/cast/video_sender/...
media/cast/video_sender/video_sender_unittest.cc:116: // Do nothing, we don't
care about logging in this test.
Maybe we should care?
Or perhaps write a separate test for the logging events?

[imcheng, 2014-02-26 10:08:25]

https://codereview.chromium.org/178073004/diff/40001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/40001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender_unittest.cc:102: // Do nothing, we don't
care about logging in this test.
On 2014/02/26 01:01:07, hubbe wrote:
> Is there *anything* we can add here to make it actually care a little bit?

Not really. Logging is disabled in this test, so this callback is never invoked.
We could FAIL() here, but that's not very meaningful. This makes me wonder if
this could be designed a little bit better - from the caller's perspective, it
will be good to not have to specify callback if logging is disabled. Since only
some test code doesn't care about logging, I modified the constructor to check
for null callbacks, which it will interpret as "don't care about logging".

https://codereview.chromium.org/178073004/diff/40001/media/cast/rtcp/rtcp_uni...
File media/cast/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/40001/media/cast/rtcp/rtcp_uni...
media/cast/rtcp/rtcp_unittest.cc:179: // Do nothing, we don't care about logging
in this test.
On 2014/02/26 01:01:07, hubbe wrote:
> Can we care?

See other reply.

https://codereview.chromium.org/178073004/diff/40001/media/cast/video_sender/...
File media/cast/video_sender/video_sender_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/40001/media/cast/video_sender/...
media/cast/video_sender/video_sender_unittest.cc:116: // Do nothing, we don't
care about logging in this test.
On 2014/02/26 01:01:07, hubbe wrote:
> Maybe we should care?
> Or perhaps write a separate test for the logging events?

See other reply.

I wrote a test for CastTransportSenderImpl that tests the repeating timer
invoking the callback, but it will only get more interesting once it actually
starts logging events.

[imcheng, 2014-02-26 18:33:32]

Fix compile

[hubbe, 2014-02-26 22:35:37]

LGTM (when it compiles)

[miu, 2014-02-26 22:43:51]

lgtm, but please address the comments below:

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

https://codereview.chromium.org/178073004/diff/200001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:14: static const int
kSendRawEventsIntervalSecs = 1;
nit: No need for 'static' if this is in the anonymous namespace.

https://codereview.chromium.org/178073004/diff/200001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:74: if
(!packet_events.empty())
Out of curiosity: Are we concerned about how much data is going to be sent in
one IPC?  If more than, say, 100 KB, we might need to break this up into
multiple IPCs.  The reason is that the IPC channel needs to remain low-latency
and responsive for timing-sensitive uses.

https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
chrome/renderer/media/cast_ipc_dispatcher.cc:124: LOG(ERROR) <<
"CastIPCDispatcher::OnRawEvents on non-existing channel.";
This, and the other LOG(ERROR) statements in this file, should either be removed
or changed to NOT_REACHED() instead.  If, we expect the |sender| look-up from
|id_map_| to fail under normal circumstances, we shouldn't be spamming the log
at all, since this is not an error.

https://codereview.chromium.org/178073004/diff/200001/media/cast/transport/ca...
File media/cast/transport/cast_transport_sender_impl_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/200001/media/cast/transport/ca...
media/cast/transport/cast_transport_sender_impl_unittest.cc:85:
scoped_ptr<base::SimpleTestTickClock> testing_clock_;
nit: This can be part of the class, rather than a pointer to another heap
object.

[imcheng, 2014-02-26 23:35:54]

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

https://codereview.chromium.org/178073004/diff/200001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:14: static const int
kSendRawEventsIntervalSecs = 1;
On 2014/02/26 22:43:51, miu wrote:
> nit: No need for 'static' if this is in the anonymous namespace.

Done.

https://codereview.chromium.org/178073004/diff/200001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:74: if
(!packet_events.empty())
On 2014/02/26 22:43:51, miu wrote:
> Out of curiosity: Are we concerned about how much data is going to be sent in
> one IPC?  If more than, say, 100 KB, we might need to break this up into
> multiple IPCs.  The reason is that the IPC channel needs to remain low-latency
> and responsive for timing-sensitive uses.

Here's my rough calculation of how much data is being sent per call (which
happens every 1 second):

30 (fps) * 2 (audio/video) * 10 (packets per frame) * 3 (events per packet) * 40
(bytes per packet event) = 72KB per second.

10 packets per frame on average might be a stretch but it is under 100KB. (is
it?)

I didn't think about this before, but it sounds like it isn't going to cause any
issues?

In the next CL I can try to measure how much data we are sending when are
actually logging events.

https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
chrome/renderer/media/cast_ipc_dispatcher.cc:124: LOG(ERROR) <<
"CastIPCDispatcher::OnRawEvents on non-existing channel.";
On 2014/02/26 22:43:51, miu wrote:
> This, and the other LOG(ERROR) statements in this file, should either be
removed
> or changed to NOT_REACHED() instead.  If, we expect the |sender| look-up from
> |id_map_| to fail under normal circumstances, we shouldn't be spamming the log
> at all, since this is not an error.

Is it an error? It looks like the CastTransportSenderIPC can be destroyed before
while browser side is sending a message for it. I think it should be expected
that lookups will sometimes fail. The log is still somewhat useful though for
debugging, so how about making these DVLOG(1), and consistent with
cast_transport_host_filter.cc?

https://codereview.chromium.org/178073004/diff/200001/media/cast/transport/ca...
File media/cast/transport/cast_transport_sender_impl_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/200001/media/cast/transport/ca...
media/cast/transport/cast_transport_sender_impl_unittest.cc:85:
scoped_ptr<base::SimpleTestTickClock> testing_clock_;
On 2014/02/26 22:43:51, miu wrote:
> nit: This can be part of the class, rather than a pointer to another heap
> object.

Done.

[imcheng, 2014-02-26 23:43:24]

Adding +dcheng for chrome/common/cast_messages.h

Hey Daniel, in this CL I am making some incremental changes to cast's use of IPC
to send some log data from browser back to renderer. I believe the original plan
has already been reviewed before: https://codereview.chromium.org/138753004. If
you could take a look that would be great.

[miu, 2014-02-26 23:57:52]

https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
chrome/renderer/media/cast_ipc_dispatcher.cc:124: LOG(ERROR) <<
"CastIPCDispatcher::OnRawEvents on non-existing channel.";
On 2014/02/26 23:35:55, imcheng1 wrote:
> debugging, so how about making these DVLOG(1), and consistent with
> cast_transport_host_filter.cc?

SGTM.  Can you fix the others too, please?

[imcheng, 2014-02-27 00:26:19]

On 2014/02/26 23:57:52, miu wrote:
>
https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
> File chrome/renderer/media/cast_ipc_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/178073004/diff/200001/chrome/renderer/media/c...
> chrome/renderer/media/cast_ipc_dispatcher.cc:124: LOG(ERROR) <<
> "CastIPCDispatcher::OnRawEvents on non-existing channel.";
> On 2014/02/26 23:35:55, imcheng1 wrote:
> > debugging, so how about making these DVLOG(1), and consistent with
> > cast_transport_host_filter.cc?
> 
> SGTM.  Can you fix the others too, please?

Done. Just one more place in this CL I believe.

[imcheng, 2014-02-27 21:49:32]

Ping for Daniel.

[dcheng, 2014-02-27 22:27:19]

Completely unrelated, but it looks like the implementation of
LoggingStats::InsertPacketEvent forgets to update last_event_time when adding
the data to an existing event.

I'd also like to understand how the logged PacketEvents are used. It looks like
the data saved when config_.enable_stats_data_collection is true is currently
only read by a unit test. Can you confirm this?

I'm also still looking at RawEventSubscriber; this is the only way to access the
data if config_.enable_raw_data_collection is true, right?

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

https://codereview.chromium.org/178073004/diff/250001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:95: base::Unretained(this),
Why is it OK to use base::Unretained(this)? Would be nice to have a comment
above this block. It's not clear to me at all that this is safe.

Similar comments to other uses of base::Unretained(this) in
cast_session_delegate.cc

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
File media/cast/logging/logging_defines.h (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
media/cast/logging/logging_defines.h:74: kNumOfLoggingEvents
I think it would be better if this were kCastLoggingEventLast =
kDuplicateVideoPacketReceived

For an example, see
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...

This is also consistent with the naming of the other cast enums.

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
media/cast/test/end2end_unittest.cc:600: base::MessageLoopForIO message_loop_;
Where is this used?

[imcheng, 2014-02-28 00:14:35]

On 2014/02/27 22:27:19, dcheng wrote:
> Completely unrelated, but it looks like the implementation of
> LoggingStats::InsertPacketEvent forgets to update last_event_time when adding
> the data to an existing event.
> 

Thanks for pointing that out! I filed bug 347750 to fix this.


> I'd also like to understand how the logged PacketEvents are used. 
Recently we did a refactoring to move cast mirroring's packet-sending logic to
the browser process. As a result we need a way to send packet event logs back to
the renderer process where the overall logging data is kept. We export the data
from the cast library to the extension and to the cast streaming API, see here -
https://codereview.chromium.org/170063006/. We plan to upload these logs for a
subset of users to the cloud to do session related analysis and for debugging
purposes.

> It looks like
> the data saved when config_.enable_stats_data_collection is true is currently
> only read by a unit test. Can you confirm this?

That is correct. I have submitted a CL to enable stats collection in the
extension in the past day, but more plumbing will be required before it is
accessible via the cast streaming API.
https://codereview.chromium.org/179743004/ 

> 
> I'm also still looking at RawEventSubscriber; this is the only way to access
the
> data if config_.enable_raw_data_collection is true, right?
> 

RawEventSubscriber are the only way to access raw event data. If
config_.enable_raw_data_collection is false, then raw events won't be logged at
all.

LoggingImpl is the only way to access aggregated stats, and
config_.enable_stats_data_collection has to be true.



>
https://codereview.chromium.org/178073004/diff/250001/chrome/browser/media/ca...
> File chrome/browser/media/cast_transport_host_filter.cc (right):
> 
>
https://codereview.chromium.org/178073004/diff/250001/chrome/browser/media/ca...
> chrome/browser/media/cast_transport_host_filter.cc:95: base::Unretained(this),
> Why is it OK to use base::Unretained(this)? Would be nice to have a comment
> above this block. It's not clear to me at all that this is safe.
> 
> Similar comments to other uses of base::Unretained(this) in
> cast_session_delegate.cc
> 
>
https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
> File media/cast/logging/logging_defines.h (right):
> 
>
https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
> media/cast/logging/logging_defines.h:74: kNumOfLoggingEvents
> I think it would be better if this were kCastLoggingEventLast =
> kDuplicateVideoPacketReceived
> 
> For an example, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...
> 
> This is also consistent with the naming of the other cast enums.
> 
>
https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
> File media/cast/test/end2end_unittest.cc (right):
> 
>
https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
> media/cast/test/end2end_unittest.cc:600: base::MessageLoopForIO message_loop_;
> Where is this used?

[imcheng, 2014-02-28 00:15:06]

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

https://codereview.chromium.org/178073004/diff/250001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:95: base::Unretained(this),
On 2014/02/27 22:27:20, dcheng wrote:
> Why is it OK to use base::Unretained(this)? Would be nice to have a comment
> above this block. It's not clear to me at all that this is safe.
> 
> Similar comments to other uses of base::Unretained(this) in
> cast_session_delegate.cc

I did not write this code originally but from looking at the code, the
CastTransportHostFilter owns the sender and doesn't give away ownership. This
callback and the ones below are guaranteed to be only called when the
CastTransportHostFilter still exists. I will add some comment here. I believe it
can be reasoned similarly for CastSessionDelegate.

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
File media/cast/logging/logging_defines.h (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
media/cast/logging/logging_defines.h:74: kNumOfLoggingEvents
On 2014/02/27 22:27:20, dcheng wrote:
> I think it would be better if this were kCastLoggingEventLast =
> kDuplicateVideoPacketReceived
> 
> For an example, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...
> 
> This is also consistent with the naming of the other cast enums.

I tried it but it doesn't work as well as I would have wanted. In particular
this messes up the places where we switch on the value, e.g.
CastLoggingToString() below. So I am not sure if setting kNumOfLoggingEvents =
kDuplicateVideoPacketReceived is worth it.

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
media/cast/test/end2end_unittest.cc:600: base::MessageLoopForIO message_loop_;
On 2014/02/27 22:27:20, dcheng wrote:
> Where is this used?

CastTransportSenderImpl creates a RepeatingTimer which relies on having a
SingleThreadTaskRunner in the current thread. This is kind of ugly but I am not
sure what options we have if we are keeping that RepeatingTimer. Or we can
choose not to use RepeatingTimer and just pass in a FakeSingleThreadTaskRunner
and implement the repeating logic there, but then we would have to make it
SupportsWeakPtr<>, which doesn't sound appealing either.

[dcheng, 2014-02-28 19:46:10]

Sorry. Tracing execution through RawEventSubscriber to verify that the code is
safe was a non-trivial task, especially the call to BuildReceiverLog and
CastSessionDelegate::GetEventLogsAndReset.

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
File media/cast/logging/logging_defines.h (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
media/cast/logging/logging_defines.h:74: kNumOfLoggingEvents
On 2014/02/28 00:15:06, imcheng1 wrote:
> On 2014/02/27 22:27:20, dcheng wrote:
> > I think it would be better if this were kCastLoggingEventLast =
> > kDuplicateVideoPacketReceived
> > 
> > For an example, see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...
> > 
> > This is also consistent with the naming of the other cast enums.
> 
> I tried it but it doesn't work as well as I would have wanted. In particular
> this messes up the places where we switch on the value, e.g.
> CastLoggingToString() below. So I am not sure if setting kNumOfLoggingEvents =
> kDuplicateVideoPacketReceived is worth it.

Why would this complicate those functions?
case kNumOfLoggingEvents:
      NOTREACHED();
      return OTHER_EVENT;
should simply be removed. The compiler will warn if you don't handle all
possible num values in the switch anyway.

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
media/cast/test/end2end_unittest.cc:600: base::MessageLoopForIO message_loop_;
On 2014/02/28 00:15:06, imcheng1 wrote:
> On 2014/02/27 22:27:20, dcheng wrote:
> > Where is this used?
> 
> CastTransportSenderImpl creates a RepeatingTimer which relies on having a
> SingleThreadTaskRunner in the current thread. This is kind of ugly but I am
not
> sure what options we have if we are keeping that RepeatingTimer. Or we can
> choose not to use RepeatingTimer and just pass in a FakeSingleThreadTaskRunner
> and implement the repeating logic there, but then we would have to make it
> SupportsWeakPtr<>, which doesn't sound appealing either.

OK, perhaps add a reference to the fact that CastTransportSenderImpl uses a
RepeatingTimer to the comment:
// transport_sender_ has a RepeatingTimer which needs a message loop.
Otherwise, a clueless reader like me doesn't see the connection =)

Also, why can't it be just MessageLoop?

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

https://codereview.chromium.org/178073004/diff/270001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:89: // that they are only
called when |this| is still alive.
OK, having read over this code more, I understand why this is safe now. So I
think this comment is actually unnecessary (the object is deleted in the
destructor).

I /do/ think it would be a bit cleaner to have some sort of observer/delegate
interface rather than binding five callbacks here, but that can be saved for a
separate patch. That will require some refactoring I think, but it will avoid
repetition of a lot of arguments (channel_id and this), save some refcounting,
and possibly instantiate fewer Callback<> templates. A similar comment applies
to CastTransportSenderIPC.

[imcheng, 2014-03-03 19:00:08]

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
File media/cast/logging/logging_defines.h (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/logging/logg...
media/cast/logging/logging_defines.h:74: kNumOfLoggingEvents
On 2014/02/28 19:46:11, dcheng wrote:
> On 2014/02/28 00:15:06, imcheng1 wrote:
> > On 2014/02/27 22:27:20, dcheng wrote:
> > > I think it would be better if this were kCastLoggingEventLast =
> > > kDuplicateVideoPacketReceived
> > > 
> > > For an example, see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...
> > > 
> > > This is also consistent with the naming of the other cast enums.
> > 
> > I tried it but it doesn't work as well as I would have wanted. In particular
> > this messes up the places where we switch on the value, e.g.
> > CastLoggingToString() below. So I am not sure if setting kNumOfLoggingEvents
=
> > kDuplicateVideoPacketReceived is worth it.
> 
> Why would this complicate those functions?
> case kNumOfLoggingEvents:
>       NOTREACHED();
>       return OTHER_EVENT;
> should simply be removed. The compiler will warn if you don't handle all
> possible num values in the switch anyway.

Yes, that will work. But that will also make kNumOfLoggingEvents valid in code
paths which is something I didn't like. Anyway, I made the change to remove it
from switches so that it can be assigned to the same value as
kDuplicateVideoPacketReceived.

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/178073004/diff/250001/media/cast/test/end2end...
media/cast/test/end2end_unittest.cc:600: base::MessageLoopForIO message_loop_;
On 2014/02/28 19:46:11, dcheng wrote:
> On 2014/02/28 00:15:06, imcheng1 wrote:
> > On 2014/02/27 22:27:20, dcheng wrote:
> > > Where is this used?
> > 
> > CastTransportSenderImpl creates a RepeatingTimer which relies on having a
> > SingleThreadTaskRunner in the current thread. This is kind of ugly but I am
> not
> > sure what options we have if we are keeping that RepeatingTimer. Or we can
> > choose not to use RepeatingTimer and just pass in a
FakeSingleThreadTaskRunner
> > and implement the repeating logic there, but then we would have to make it
> > SupportsWeakPtr<>, which doesn't sound appealing either.
> 
> OK, perhaps add a reference to the fact that CastTransportSenderImpl uses a
> RepeatingTimer to the comment:
> // transport_sender_ has a RepeatingTimer which needs a message loop.
> Otherwise, a clueless reader like me doesn't see the connection =)
> 
> Also, why can't it be just MessageLoop?

Changed comments. I also changed it to a MessageLoop.

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

https://codereview.chromium.org/178073004/diff/270001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:89: // that they are only
called when |this| is still alive.
On 2014/02/28 19:46:11, dcheng wrote:
> OK, having read over this code more, I understand why this is safe now. So I
> think this comment is actually unnecessary (the object is deleted in the
> destructor).
> 
> I /do/ think it would be a bit cleaner to have some sort of observer/delegate
> interface rather than binding five callbacks here, but that can be saved for a
> separate patch. That will require some refactoring I think, but it will avoid
> repetition of a lot of arguments (channel_id and this), save some refcounting,
> and possibly instantiate fewer Callback<> templates. A similar comment applies
> to CastTransportSenderIPC.

ACK. I think in many cases a line can be drawn where the use of callbacks is
better than observer interface and vice versa. Since we have a lot of callbacks,
we can reassess whether it's better to use an observer interface, and make
changes in a separate patch.

[dcheng, 2014-03-03 23:25:23]

chrome/common/cast_messages.h LGTM.

My main comment from this review is there's a lot of scary serialization going
on in CastSessionDelegate::GetEventLogsAndReset() and
RtcpSender::SendRtcpFromRtpReceiver() and it's very difficult to verify the
correctness (for example, I have to wonder if a cast to uint8 is safe because
the check that it never overflows 255 is contained in some other function).
Since these are both in the renderer, it's less bad if there's a bug in the
routines. But in the long term, it would be nice to clean up this code so a
reviewer can easily see that the serialization/deserialization is safe.

Also, please add an IPC reviewer for your followup changes that use the logged
PacketEvents. Thank you for your patience!

[imcheng, 2014-03-04 02:15:00]

On 2014/03/03 23:25:23, dcheng wrote:
> chrome/common/cast_messages.h LGTM.
> 
> My main comment from this review is there's a lot of scary serialization going
> on in CastSessionDelegate::GetEventLogsAndReset() and
> RtcpSender::SendRtcpFromRtpReceiver() and it's very difficult to verify the
> correctness (for example, I have to wonder if a cast to uint8 is safe because
> the check that it never overflows 255 is contained in some other function).
> Since these are both in the renderer, it's less bad if there's a bug in the
> routines. But in the long term, it would be nice to clean up this code so a
> reviewer can easily see that the serialization/deserialization is safe.
> 

LogSerializer is still work in progress. I agree it's difficult to verify
correctness by looking at the code. This is something I can address in later
CLs. I am also adding a test to make sure serialize-deserialize works as
expected. I would like to see RtcpSender cleaned up as well and I already
made some changes in another CL for that.

> Also, please add an IPC reviewer for your followup changes that use the logged
> PacketEvents. Thank you for your patience!

Thanks for reviewing!

[imcheng, 2014-03-04 02:15:53]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-04 02:19:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/320001

[commit-bot: I haz the power, 2014-03-04 02:19:08]

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

[commit-bot: I haz the power, 2014-03-04 02:19:09]

Failed to apply patch for chrome/browser/media/cast_transport_host_filter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/media/cast_transport_host_filter.cc
  Hunk #2 FAILED at 68.
  Hunk #3 FAILED at 82.
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/media/cast_transport_host_filter.cc.rej

Patch:       chrome/browser/media/cast_transport_host_filter.cc
Index: chrome/browser/media/cast_transport_host_filter.cc
diff --git a/chrome/browser/media/cast_transport_host_filter.cc
b/chrome/browser/media/cast_transport_host_filter.cc
index
393090f80a2ab8747900aa7f4bc4d19edccd723c..59e113fd3ec578bc1d71fd1693e08249576e2a71
100644
--- a/chrome/browser/media/cast_transport_host_filter.cc
+++ b/chrome/browser/media/cast_transport_host_filter.cc
@@ -8,6 +8,13 @@
 #include "chrome/browser/net/chrome_net_log.h"
 #include "media/cast/transport/cast_transport_sender.h"
 
+namespace {
+
+// How often to send raw events.
+const int kSendRawEventsIntervalSecs = 1;
+
+}
+
 namespace cast {
 
 CastTransportHostFilter::CastTransportHostFilter()
@@ -61,9 +68,17 @@ void CastTransportHostFilter::ReceivedRtpStatistics(
                                  rtp_timestamp));
 }
 
+void CastTransportHostFilter::RawEvents(
+    int32 channel_id,
+    const std::vector<media::cast::PacketEvent>& packet_events) {
+  if (!packet_events.empty())
+    Send(new CastMsg_RawEvents(channel_id, packet_events));
+}
+
 void CastTransportHostFilter::OnNew(
     int32 channel_id,
-    const media::cast::transport::CastTransportConfig& config) {
+    const media::cast::transport::CastTransportConfig& config,
+    const media::cast::CastLoggingConfig& logging_config) {
   media::cast::transport::CastTransportSender* sender =
       id_map_.Lookup(channel_id);
   if (sender) {
@@ -75,9 +90,14 @@ void CastTransportHostFilter::OnNew(
           g_browser_process->net_log(),
           &clock_,
           config,
+          logging_config,
           base::Bind(&CastTransportHostFilter::NotifyStatusChange,
                      base::Unretained(this),
                      channel_id),
+          base::Bind(&CastTransportHostFilter::RawEvents,
+                     base::Unretained(this),
+                     channel_id),
+          base::TimeDelta::FromSeconds(kSendRawEventsIntervalSecs),
           base::MessageLoopProxy::current());
   sender->SetPacketReceiver(
           base::Bind(&CastTransportHostFilter::ReceivedPacket,

[imcheng, 2014-03-04 09:20:53]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-04 09:20:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/360001

[commit-bot: I haz the power, 2014-03-04 10:46:10]

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

[commit-bot: I haz the power, 2014-03-04 10:46:10]

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

[imcheng, 2014-03-04 18:38:52]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-04 18:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/400001

[commit-bot: I haz the power, 2014-03-04 19:32:08]

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

[commit-bot: I haz the power, 2014-03-04 19:32:09]

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

[imcheng, 2014-03-04 21:24:17]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/420001

[imcheng, 2014-03-04 23:53:10]

The CQ bit was unchecked by imcheng@chromium.org

[imcheng, 2014-03-05 05:42:50]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-05 05:43:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/440001

[commit-bot: I haz the power, 2014-03-05 08:55:35]

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

[commit-bot: I haz the power, 2014-03-05 08:55:36]

Try jobs failed on following builders: mac_chromium_compile_dbg

[imcheng, 2014-03-05 20:18:02]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-05 20:22:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/480001

[commit-bot: I haz the power, 2014-03-05 20:36:30]

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

[commit-bot: I haz the power, 2014-03-05 20:36:32]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[imcheng, 2014-03-05 20:42:48]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-05 20:44:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/480001

[commit-bot: I haz the power, 2014-03-05 21:18:59]

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

[commit-bot: I haz the power, 2014-03-05 21:19:04]

Try jobs failed on following builders: mac_chromium_rel

[imcheng, 2014-03-07 00:20:29]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-07 00:24:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/178073004/500001

[commit-bot: I haz the power, 2014-03-07 11:26:30]

Change committed as 255579