Cast: Limit number of events/packets in EncodingEventSubscriber protos.

Cast: Limit number of events/packets in EncodingEventSubscriber protos.

There is now a limit on number of events per proto (AggregatedFrameEvent
and BasePacketEvent). There is also a limit on number of packets per
frame (i.e. number of BasePacketEvent in AggregatedPacketEvent). If
either limit is exceeded, a new proto will be created for that frame.

The underlying implementation of EncodingEventSubscriber is also
changed. When the internal map reaches a certain size, its old entries
will be transferred to a vector for storage. This is done to keep the
map small and lookup fast.

A proto will also be
transferred to storage if adding an event to it would result in
exceeding the limit stated above. The scheme is not perfect, but such
situation rarely happens in practice.

Also changed deserialization to be able to merge protos of the same
frame.

This is done to keep protos size in check. Also, LogSerializer will now
fail DCHECK if the protos exceed the maximum size 2^16 - 1. (Because
we encode size as 16 bits).


BUG=

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

[Alpha Left Google, 2014-04-17 22:24:48]

Do we actually see this often?

[imcheng, 2014-04-17 22:25:27]

Herve showed me a couple of logs yesterday that had this problem, which messed
up the deserialization logic. I had a workaround for deserialization for now.

[Alpha Left Google, 2014-04-17 22:28:30]

On 2014/04/17 22:25:27, imcheng1 wrote:
> Herve showed me a couple of logs yesterday that had this problem, which messed
> up the deserialization logic. I had a workaround for deserialization for now.

Okay. Do we know what caused this? It seems to be a severe problem that this
happens. Or we just retransmit too much?

[imcheng, 2014-04-17 22:42:00]

I was able to reproduce this using the cast_sender_app / cast_receiver_app. It
seems the sender keeps trying to retransmit video packets after it lost
connection with the receiver, which causes VIDEO_PACKET_RETRANSMITTED events to
build up. I don't see that behavior with audio. I will file a bug for this.

Adding hguihot@ who showed me a couple of logs where deserialization failed, to
see if he has any insights.

Regardless, this cl is still needed because we need to make sure the
serialization is properly formatted.

[Alpha Left Google, 2014-04-17 22:58:59]

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

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:29: const int kMaxPacketsPerFrame
= 64;
This number seems small. Why is this 64? Instead of dropping events can we log
with an extra protobuf?

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:33: const int kMaxEventsPerProto
= 16;
Why is this 16? It seems small..

[imcheng, 2014-04-18 00:05:30]

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

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:29: const int kMaxPacketsPerFrame
= 64;
On 2014/04/17 22:59:00, Alpha wrote:
> This number seems small. Why is this 64? Instead of dropping events can we log
> with an extra protobuf?

How often will we have more than 64 packets per frame? What number would you
suggest?

I would rather keep it simple and just pick a max number here.

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:33: const int kMaxEventsPerProto
= 16;
On 2014/04/17 22:59:00, Alpha wrote:
> Why is this 16? It seems small..

How about 32? Normally a proto will rarely have more than 10 events. The only
events that would repeat are retransmission and rtcp events (to a limited
extent).

[Alpha Left Google, 2014-04-21 18:14:04]

If the limit for a proto is 64kbytes the limit seems very small compared to
that. I'd prefer not to drop events. What about you set the limit but create a
new proto if there's more packet events?

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

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.cc:54: if
(event_proto->event_type_size() >= kMaxEventsPerProto) {
I think we should keep as many as types as possible.

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

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:29: const int kMaxPacketsPerFrame
= 64;
On 2014/04/18 00:05:31, imcheng1 wrote:
> On 2014/04/17 22:59:00, Alpha wrote:
> > This number seems small. Why is this 64? Instead of dropping events can we
log
> > with an extra protobuf?
> 
> How often will we have more than 64 packets per frame? What number would you
> suggest?
> 
> I would rather keep it simple and just pick a max number here.

Sometimes this can happen. A scene change frame can be large (~100kb). Plus
retransmission there can be many more.

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:33: const int kMaxEventsPerProto
= 16;
On 2014/04/18 00:05:31, imcheng1 wrote:
> On 2014/04/17 22:59:00, Alpha wrote:
> > Why is this 16? It seems small..
> 
> How about 32? Normally a proto will rarely have more than 10 events. The only
> events that would repeat are retransmission and rtcp events (to a limited
> extent).

I don't see much value in this limit. This limits the types of events in a proto
right? If the most common event type is packet event then having this value
isn't very useful I think.

[imcheng, 2014-04-22 01:17:38]

Per offline discussion, an extra proto will be created if the proto exceeds
either of the stated limit.

I also added a new field to be used as storage for old entries in the map.

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

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:29: const int kMaxPacketsPerFrame
= 64;
ACK.

On 2014/04/21 18:14:04, Alpha wrote:
> On 2014/04/18 00:05:31, imcheng1 wrote:
> > On 2014/04/17 22:59:00, Alpha wrote:
> > > This number seems small. Why is this 64? Instead of dropping events can we
> log
> > > with an extra protobuf?
> > 
> > How often will we have more than 64 packets per frame? What number would you
> > suggest?
> > 
> > I would rather keep it simple and just pick a max number here.
> 
> Sometimes this can happen. A scene change frame can be large (~100kb). Plus
> retransmission there can be many more.

https://codereview.chromium.org/241833002/diff/20001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.h:33: const int kMaxEventsPerProto
= 16;
This limits the number of events, this helps keep individual protos down in
size, when there are a lot of retransmit events for the same packet.

On 2014/04/21 18:14:04, Alpha wrote:
> On 2014/04/18 00:05:31, imcheng1 wrote:
> > On 2014/04/17 22:59:00, Alpha wrote:
> > > Why is this 16? It seems small..
> > 
> > How about 32? Normally a proto will rarely have more than 10 events. The
only
> > events that would repeat are retransmission and rtcp events (to a limited
> > extent).
> 
> I don't see much value in this limit. This limits the types of events in a
proto
> right? If the most common event type is packet event then having this value
> isn't very useful I think.

[Alpha Left Google, 2014-04-22 18:20:41]

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

https://codereview.chromium.org/241833002/diff/70001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.cc:227: for (size_t i = 0; i <
num_entries; i++, ++it)
Try to erase the entry while iterating. That way you don't have to look up
twice.

http://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-ite...

And this is less error-prone because you don't have to worry about the map
having no less entries than num_entries.

[imcheng, 2014-04-22 19:29:33]

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

https://codereview.chromium.org/241833002/diff/70001/media/cast/logging/encod...
media/cast/logging/encoding_event_subscriber.cc:227: for (size_t i = 0; i <
num_entries; i++, ++it)
On 2014/04/22 18:20:42, Alpha wrote:
> Try to erase the entry while iterating. That way you don't have to look up
> twice.
> 
>
http://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-ite...
> 
> And this is less error-prone because you don't have to worry about the map
> having no less entries than num_entries.

Per offline discussion, changed the semantics of function to transfer
max_num_entries and add a check to break loop when it has reached the end of
map.

[Alpha Left Google, 2014-04-22 19:55:13]

lgtm

[imcheng, 2014-04-25 08:11:27]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-04-25 08:32:05]

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

[commit-bot: I haz the power, 2014-04-25 23:39:51]

Change committed as 266289