Cast: Plumb raw event logs to cast extension, install EncodingEventSubscriber on CastSessionDelegat…

Cast: Plumb raw event logs up to CastSession, install EncodingEventSubscriber on CastSessionDelegate.

CastSessionDelegate will have one or two EncodingEventSubscribers listening to audio / video events respectively. These subscribers are created alongside the CastSender / CastTransportSender.


Next CL will be adding a JS API and a function in CastStreamingNativeHandler to get logs from CastSession.

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

[imcheng, 2014-02-20 23:04:27]

I was debating whether to put the subscribers in CastRtpStream,
CastSessionDelegate, or CastSenderImpl. It doesn't seem to belong in
CastRtpStream or CastSessionDelegate, because they are extension objects and
seem too high level for this. It makes the most sense to me to put it in
CastSenderImpl which is a cast library object. Let me know what you think,
thanks.

[Alpha Left Google, 2014-02-20 23:31:27]

On 2014/02/20 23:04:27, imcheng1 wrote:
> I was debating whether to put the subscribers in CastRtpStream,
> CastSessionDelegate, or CastSenderImpl. It doesn't seem to belong in
> CastRtpStream or CastSessionDelegate, because they are extension objects and
> seem too high level for this. It makes the most sense to me to put it in
> CastSenderImpl which is a cast library object. Let me know what you think,
> thanks.

CastRtpStream is a render thread object. And it doesn't know about CastSender.
So it shouldn't have the subscriber.

CastSessionDelegate is the user of CastSender. And it knows what it like to
export. Subscriber should be placed here.

CastSenderImpl doesn't know what the user wants about the events. So it
shouldn't own the encoding event subscribers.

[imcheng, 2014-02-21 00:44:32]

I think you're right. Logging should probably live alongside transport and the
overall sender. I have made the changes so the subscribers are now in
CastSessionDelegate. PTAL.

[Alpha Left Google, 2014-02-21 01:59:19]

LGTM with nits.

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:154: // Currently on main thread,
can call logging directly.
This comment is not necessary.

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:166: // Currently on main thread,
can call logging directly.
This comment is not necessary.

[imcheng, 2014-02-21 03:17:25]

+scherkus for review.

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:154: // Currently on main thread,
can call logging directly.
On 2014/02/21 01:59:19, Alpha wrote:
> This comment is not necessary.

I suppose... it helps to remind me this is making an assumption here since I am
still kind of new to this code.

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:166: // Currently on main thread,
can call logging directly.
On 2014/02/21 01:59:19, Alpha wrote:
> This comment is not necessary.

Done.

https://codereview.chromium.org/173713004/diff/60001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:166: // Currently on main thread,
can call logging directly.
On 2014/02/21 01:59:19, Alpha wrote:
> This comment is not necessary.

Done.

[scherkus (not reviewing), 2014-02-21 15:59:02]

lgtm

[imcheng, 2014-02-21 18:33:14]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:33:30]

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

[commit-bot: I haz the power, 2014-02-21 18:49:07]

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

[commit-bot: I haz the power, 2014-02-21 18:49:08]

Try jobs failed on following builders: mac_chromium_rel

[imcheng, 2014-02-21 19:02:07]

The CQ bit was checked by imcheng@chromium.org

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

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

[commit-bot: I haz the power, 2014-02-21 19:16:02]

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

[commit-bot: I haz the power, 2014-02-21 19:48:11]

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

[commit-bot: I haz the power, 2014-02-21 19:48:11]

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

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

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2014-02-21 19:52:02]

The CQ bit was unchecked by imcheng@chromium.org

[imcheng, 2014-02-21 19:59:51]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-21 20:00:57]

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

[commit-bot: I haz the power, 2014-02-21 21:54:41]

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

[commit-bot: I haz the power, 2014-02-21 21:54:41]

Try jobs failed on following builders: linux_chromium_rel

[imcheng, 2014-02-21 23:43:44]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2014-02-21 23:44:14]

The CQ bit was unchecked by imcheng@chromium.org

[imcheng, 2014-02-21 23:47:30]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-21 23:47:48]

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

[commit-bot: I haz the power, 2014-02-22 01:27:52]

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

[commit-bot: I haz the power, 2014-02-22 03:52:40]

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

[commit-bot: I haz the power, 2014-02-22 03:52:41]

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-22 04:57:25]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-22 04:57:41]

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

[commit-bot: I haz the power, 2014-02-22 08:30:19]

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

[commit-bot: I haz the power, 2014-02-22 08:30:20]

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-22 12:41:50]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-02-22 12:42:00]

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

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

Change committed as 252813