Cast: Implemented size limiting and event filtering in EncodingEventSubscriber.

Cast: Implemented size limiting and event filtering in EncodingEventSubscriber.

All raw events are categorized into one of three types: video, audio, or other (i.e. not associated with a particular
stream). EncodingEventSubscriber can be configured to only process either audio or video events.

The subscriber can also be configured on having a max limit on number of frames to store in the frame and packet maps.
This helps bound the memory usage of logging and the size of the blob we are uploading at the end.

How they are going to be used: for each CastRtpStream (one audio, one video) we will install a EncodingEventSubscriber,
listening to AUDIO / VIDEO events respectively. The number of events to store is yet to be determined and is
dependent on how well the data can be compressed, or what's the size limit for uploading.

Lastly, EncodingEventSubscriber has been changed to only process frame and packet related events and not generic events. We are not interested in generic events right now, and they can be computed from frame / packet events anyways.

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

[Alpha Left Google, 2014-02-14 18:32:12]

On 2014/02/14 01:31:45, imcheng1 wrote:

I don't think we'll listen to RttMs, JitterMs. These are computed values and are
not really actual events.

[Alpha Left Google, 2014-02-14 18:42:39]

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
File media/cast/logging/encoding_event_subscriber.cc (right):

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.cc:120: void
EncodingEventSubscriber::OnReceiveGenericEvent(
What are the generic events that we capture? I'm not sure we care about these
events.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
File media/cast/logging/encoding_event_subscriber.h (right):

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:30: struct
EncodingEventSubscriberConfig {
Why not give the arguments directly to the constructor of
EncodingEventSubscriber?

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:31: // The subscriber will only
process events that belong in |event_media_type|.
nit: that corresponds to |event_media_type|.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:40: // Note that there is no
active process to remove old events periodically.
What does this mean?

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:43: // How many time-value pairs
to keep per generic event.
I'm not sure we care about the generic events.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/loggi...
File media/cast/logging/logging_defines.cc (right):

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/loggi...
media/cast/logging/logging_defines.cc:77: case kUnknown:
From kUnoknown to kRembBitrate they are not something we care to keep for
debugging. RttMs kPacketLoss, kJitterMs can all be computed from existing audio
and video events. kRembBitrate has little use.

I don't think we need to care about OTHER_EVENT.

[imcheng, 2014-02-14 20:24:16]

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
File media/cast/logging/encoding_event_subscriber.cc (right):

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.cc:120: void
EncodingEventSubscriber::OnReceiveGenericEvent(
On 2014/02/14 18:42:39, Alpha wrote:
> What are the generic events that we capture? I'm not sure we care about these
> events.

As mentioned in the other file, there are 7 generic events. There are 4 that it
sounds like we are not interested in, and the other 3 shouldn't be generic
events in the first place.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
File media/cast/logging/encoding_event_subscriber.h (right):

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:30: struct
EncodingEventSubscriberConfig {
On 2014/02/14 18:42:39, Alpha wrote:
> Why not give the arguments directly to the constructor of
> EncodingEventSubscriber?

I was thinking that with 3 parameters it's better to put them in a struct to
avoid cluttering the constructor. Since we might be getting rid of 1 of the
parameters and I'm not sure if we will be adding extra args in the future, I
will change it to pass the args directly to ctor.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:31: // The subscriber will only
process events that belong in |event_media_type|.
On 2014/02/14 18:42:39, Alpha wrote:
> nit: that corresponds to |event_media_type|.

Done.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:40: // Note that there is no
active process to remove old events periodically.
On 2014/02/14 18:42:39, Alpha wrote:
> What does this mean?

Removing this. I was implementing windowing based on time earlier, so this
comment no longer applies. It referred to that there is no periodic timer to
remove events older than a certain time.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:43: // How many time-value pairs
to keep per generic event.
On 2014/02/14 18:42:39, Alpha wrote:
> I'm not sure we care about the generic events.

These are the generic events currently:

kRttMs, kPacketLoss, kJitterMs, kRembBitrate, kVideoAckReceived, kAudioAckSent,
kVideoAckSent.

You mentioned that we are not interested in the first 4. The last 3 seems like a
mistake to me - they should probably be frame events. I filed bug 339590 for
kAudioAckSent, kVideoAckSent. I am going to file a bug for kVideoAckReceived.

In the mean time, I will remove this param and stop listening to generic events.
Can add back later if needed.

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/loggi...
File media/cast/logging/logging_defines.cc (right):

https://codereview.chromium.org/165723002/diff/50001/media/cast/logging/loggi...
media/cast/logging/logging_defines.cc:77: case kUnknown:
On 2014/02/14 18:42:39, Alpha wrote:
> From kUnoknown to kRembBitrate they are not something we care to keep for
> debugging. RttMs kPacketLoss, kJitterMs can all be computed from existing
audio
> and video events. kRembBitrate has little use.
> 
> I don't think we need to care about OTHER_EVENT.

Ack. The other 4 here (kPacketSentToPacer, kPacketSentToNetwork,
kPacketRetransmitted, kDuplicatePacketReceived) need to be split so that they
are video/audio events. The first 3 are not even being logged yet - filing a bug
for that. The last one I have already taken care of and the patch landed this
morning, so I will rebase and make the change here.

[Alpha Left Google, 2014-02-14 21:10:31]

Please update CL description regarding the other events and generic events.

[Alpha Left Google, 2014-02-14 21:13:21]

lgtm

[imcheng, 2014-02-14 21:19:47]

Updated CL description.

[imcheng, 2014-02-14 21:20:01]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:26:46]

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

[commit-bot: I haz the power, 2014-02-14 22:43:42]

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

[commit-bot: I haz the power, 2014-02-14 22:43:43]

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

[imcheng, 2014-02-14 22:44:39]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:53:28]

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

[commit-bot: I haz the power, 2014-02-15 04:57:26]

Change committed as 251521