Cast: Added a new RawEventSubscriber implementation for use in cast extension.

Cast: Added a new RawEventSubscriber implementation for use in cast extension.

Added corresponding protocol buffer definitions for raw events, but in aggregated format.
Added EncodingEventSubscriber:
- encodes events in protocol buffer format
- (we need to test how much aggregation helps with reducing the size, if at all)
Added unit tests.

TODO in the future CLs: add event filtering, size-based windowing

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

[imcheng, 2014-02-11 03:58:51]

Please review, thanks!

Also after this CL, I might run a test app to do fake logging to see how
aggregation performs against flattened structure. There's also different
compression algorithms and encoding tricks that can be used so I will take those
into account before making a decision.

Thanks,
Derek

[Alpha Left Google, 2014-02-11 22:14:03]

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.h (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Isn't this subscriber specific to Cast Streaming API? This feels like it should
belong to chrome/media/renderer.

[Alpha Left Google, 2014-02-11 22:49:12]

https://codereview.chromium.org/138913014/diff/190001/media/cast/cast.gyp
File media/cast/cast.gyp (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/cast.gyp#new...
media/cast/cast.gyp:37: 'logging/proto/proto_utils.cc',
This should be moved to cast_logging_proto_lib.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.cc (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:30:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:35:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:65:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:107:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:126:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:133:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:140:
thread_checker_.CalledOnValidThread();
DCHECK(thread_checker_.CalledOnValidThread()).

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.h (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/02/11 22:14:03, Alpha wrote:
> Isn't this subscriber specific to Cast Streaming API? This feels like it
should
> belong to chrome/media/renderer.

Nevermind my previous comment. Putting it in this dir is fine.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:35: // |main_thread_proxy|: Only
used for ensuring the subscriber is only called
This is not very useful. I suggest you remove this. We can already ensure the
caller of the construct that it runs on the main thread.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:42: virtual void
OnReceiveFrameEvent(const FrameEvent& frame_event) OVERRIDE;
nit:

// RawReventSubscriber implementations.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/prot...
File media/cast/logging/proto/raw_events.proto (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/prot...
media/cast/logging/proto/raw_events.proto:13: // Keep in sync with
media/cast/logging/logging_defines.h
nit: period at the end.

Mention that for compatibility reason you can only append to the list.

[imcheng, 2014-02-12 08:44:13]

https://codereview.chromium.org/138913014/diff/190001/media/cast/cast.gyp
File media/cast/cast.gyp (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/cast.gyp#new...
media/cast/cast.gyp:37: 'logging/proto/proto_utils.cc',
On 2014/02/11 22:49:13, Alpha wrote:
> This should be moved to cast_logging_proto_lib.

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.cc (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:30:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Oops, I just realized the DCHECK are missing from my earlier refactoring CL too.
I will create a separate CL to fix them.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:35:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:65:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:107:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:126:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:133:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:140:
thread_checker_.CalledOnValidThread();
On 2014/02/11 22:49:13, Alpha wrote:
> DCHECK(thread_checker_.CalledOnValidThread()).

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.h (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/02/11 22:49:13, Alpha wrote:
> On 2014/02/11 22:14:03, Alpha wrote:
> > Isn't this subscriber specific to Cast Streaming API? This feels like it
> should
> > belong to chrome/media/renderer.
> 
> Nevermind my previous comment. Putting it in this dir is fine.

Ack.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:35: // |main_thread_proxy|: Only
used for ensuring the subscriber is only called
On 2014/02/11 22:49:13, Alpha wrote:
> This is not very useful. I suggest you remove this. We can already ensure the
> caller of the construct that it runs on the main thread.

I suppose that's true. The only difference it will blow up later rather than
sooner if it were somehow constructed on a thread other than main.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.h:42: virtual void
OnReceiveFrameEvent(const FrameEvent& frame_event) OVERRIDE;
On 2014/02/11 22:49:13, Alpha wrote:
> nit:
> 
> // RawReventSubscriber implementations.
>  

Done.

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/prot...
File media/cast/logging/proto/raw_events.proto (right):

https://codereview.chromium.org/138913014/diff/190001/media/cast/logging/prot...
media/cast/logging/proto/raw_events.proto:13: // Keep in sync with
media/cast/logging/logging_defines.h
On 2014/02/11 22:49:13, Alpha wrote:
> nit: period at the end.
> 
> Mention that for compatibility reason you can only append to the list.

Done.

[miu, 2014-02-13 02:02:11]

lgtm

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.cc (right):

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:7: #include <functional>
Is <functional> needed?

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:8: #include <map>
You can remove <map>, and "linked_ptr.h" since these were included in the header
file.

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/prot...
File media/cast/logging/proto/raw_events.proto (right):

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/prot...
media/cast/logging/proto/raw_events.proto:13: // Keep in sync with
media/cast/logging/logging_defines.h.
Rather than keep the two in-sync, can you just let the proto definition become
the "official" enum?  Meaning, in logging_defines.h:

  #include "media/cast/logging/proto/raw_events.pb.h"
  ...
  // instead of the CastLoggingEvent enum that is there:
  typedef media::cast::proto::EventType CastLoggingEvent;

This way, you don't need proto_utils.h/.cc at all.

Also worth noting: I see the enum names in CastLoggingEvent use the "kSomething"
style, but Chromium style requires us to use the all-caps "SOMETHING" style, as
you've used in the proto file.  See the "Naming" section here:
http://www.chromium.org/developers/coding-style

Finally, I also see that all this could be a huge change in itself, so feel free
to do all this is a follow-up CL.  ;-)

[imcheng, 2014-02-13 02:38:07]

Thanks for the review!

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber.cc (right):

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:7: #include <functional>
On 2014/02/13 02:02:12, miu wrote:
> Is <functional> needed?

It is not needed. What I do need here is <utility> for std::make_pair.

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber.cc:8: #include <map>
On 2014/02/13 02:02:12, miu wrote:
> You can remove <map>, and "linked_ptr.h" since these were included in the
header
> file.

Done.

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/prot...
File media/cast/logging/proto/raw_events.proto (right):

https://codereview.chromium.org/138913014/diff/440001/media/cast/logging/prot...
media/cast/logging/proto/raw_events.proto:13: // Keep in sync with
media/cast/logging/logging_defines.h.
I think that's a good idea if we can get it to work. I am a bit worried about
things like how it will affect the receiver side since, IIUC, they would start
depending on protobufs if they weren't already. Speaking of that, I am going to
split the EncodingEventSubscriber into a separate target out of cast_config
since it's not really part of the cast library. I will make futher refactoring
in later CLs.

On 2014/02/13 02:02:12, miu wrote:
> Rather than keep the two in-sync, can you just let the proto definition become
> the "official" enum?  Meaning, in logging_defines.h:
> 
>   #include "media/cast/logging/proto/raw_events.pb.h"
>   ...
>   // instead of the CastLoggingEvent enum that is there:
>   typedef media::cast::proto::EventType CastLoggingEvent;
> 
> This way, you don't need proto_utils.h/.cc at all.
> 
> Also worth noting: I see the enum names in CastLoggingEvent use the
"kSomething"
> style, but Chromium style requires us to use the all-caps "SOMETHING" style,
as
> you've used in the proto file.  See the "Naming" section here:
> http://www.chromium.org/developers/coding-style
> 
> Finally, I also see that all this could be a huge change in itself, so feel
free
> to do all this is a follow-up CL.  ;-)

[imcheng, 2014-02-13 02:38:56]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:40:28]

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

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

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

[commit-bot: I haz the power, 2014-02-13 02:57:28]

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

[Alpha Left Google, 2014-02-13 07:12:26]

On 2014/02/13 02:57:28, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

LGTM.

[Alpha Left Google, 2014-02-13 07:12:31]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-02-13 07:12:41]

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

[commit-bot: I haz the power, 2014-02-13 15:27:20]

Change committed as 251021