Cast: Add GetStats() extensions API.

Cast: Add getStats() extensions API.

It works similarly as getRawEvents() with the following differences:
- it returns a dictionary that can be read more easily by the
app.
- it reads from LoggingStats. We should probably refactor it into a
subscriber implementation in a later CL.
- currently it doesn't reset stats.

Also fixes bug 347750 on the side.
Also fixes bug where we are doing std::copy to an
uninitialized range. Fix is to use a back_inserter.

Making minimal changes to the cast library to get this working. More
changes to cast library to come in future CLs.

BUG=347750, 301920, 338574, 348491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255689

[imcheng, 2014-02-28 20:07:19]

I will clean up the cl description a bit soon.

[Alpha Left Google, 2014-03-03 07:11:39]

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:487: if (it !=
get_raw_events_callbacks_.end()) {
nit: Early return if it is not found. This the following code doesn't need to be
indented.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:506: if (it !=
get_stats_callbacks_.end()) {
nit: Early return if it is not found. This the following code doesn't need to be
indented.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:508:
callback_args[0] = v8::String::NewFromUtf8(
I think it's odd that we pass the javascript API with a JSON string. Can we give
it a javascript object? 

The stats we got from CastRtpStream can be a base::DictionaryValue. And then we
use a V8ValueConverter to convert it.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:73: void GetStatsAndReset(bool is_audio,
const StatsCallback& callback);
Statistics is continuous unlike events. So "AndReset" is not accurate. I think
we should just drop it from this method name.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.h:66: void GetStatsAndReset(bool
is_audio, const StatsCallback& callback);
Should be just GetStats.

https://codereview.chromium.org/184853003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/cast_streaming/basics.js (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_streaming/basics.js:48:
stateMachine.onGotLogs.bind(stateMachine, audioId, false));
This test is already complicated. I'd refer getStats() to go into a separate
test.

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/loggi...
File media/cast/logging/logging_impl.h (right):

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/loggi...
media/cast/logging/logging_impl.h:61: FrameStatsMap
GetFrameStatsData(EventMediaType media_type) const;
Have you considered having seprate stats object for audio and video?

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/loggi...
media/cast/logging/logging_impl.h:72: LoggingStats stats_;
Maybe a separate LoggingStats object for audio and video?

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/stats...
File media/cast/logging/stats_serializer.h (right):

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/stats...
media/cast/logging/stats_serializer.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
I don't think we need to serialize the stats to JSON. media/cast API can return
a base::DictionaryValue.

https://codereview.chromium.org/184853003/diff/40001/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/184853003/diff/40001/media/cast/test/sender.c...
media/cast/test/sender.cc:387: scoped_ptr<std::string> audio_stats_json(new
std::string);
Stats updates every second so getting the stat once at the end is not very
useful. If we want to make it useful I'd prefer to print the information on the
receiver window.

[imcheng, 2014-03-04 02:06:24]

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:487: if (it !=
get_raw_events_callbacks_.end()) {
On 2014/03/03 07:11:39, Alpha wrote:
> nit: Early return if it is not found. This the following code doesn't need to
be
> indented.

Done.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:506: if (it !=
get_stats_callbacks_.end()) {
On 2014/03/03 07:11:39, Alpha wrote:
> nit: Early return if it is not found. This the following code doesn't need to
be
> indented.

Done.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:508:
callback_args[0] = v8::String::NewFromUtf8(
On 2014/03/03 07:11:39, Alpha wrote:
> I think it's odd that we pass the javascript API with a JSON string. Can we
give
> it a javascript object? 
> 
> The stats we got from CastRtpStream can be a base::DictionaryValue. And then
we
> use a V8ValueConverter to convert it.


Per offline chat discussion, this api will return an object converted from
DictionaryValue.

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:73: void GetStatsAndReset(bool is_audio,
const StatsCallback& callback);
On 2014/03/03 07:11:39, Alpha wrote:
> Statistics is continuous unlike events. So "AndReset" is not accurate. I think
> we should just drop it from this method name.

In the implementation we will probably do a reset though, if we are interested
in real time stats (rather than stats that span the entire session) and we want
to implement this without using a sliding window implementation or similar
(because it will probably cost more memory than we would have liked, especially
with the amount of memory raw events is already using).

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.h:66: void GetStatsAndReset(bool
is_audio, const StatsCallback& callback);
On 2014/03/03 07:11:39, Alpha wrote:
> Should be just GetStats.

See other comment.

https://codereview.chromium.org/184853003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/cast_streaming/basics.js (right):

https://codereview.chromium.org/184853003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_streaming/basics.js:48:
stateMachine.onGotLogs.bind(stateMachine, audioId, false));
On 2014/03/03 07:11:39, Alpha wrote:
> This test is already complicated. I'd refer getStats() to go into a separate
> test.

Done.

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/loggi...
File media/cast/logging/logging_impl.h (right):

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/loggi...
media/cast/logging/logging_impl.h:61: FrameStatsMap
GetFrameStatsData(EventMediaType media_type) const;
On 2014/03/03 07:11:39, Alpha wrote:
> Have you considered having seprate stats object for audio and video?

Yes, the plan is have stats collection be a RawEventSubscriber implementation.
In LoggingImpl, if stats collection is enabled, then install separate
StatsSubscriber objects for video and audio events. But that's for a later CL.
In this CL I just want to get the JS API working without making too much
changes.

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/loggi...
media/cast/logging/logging_impl.h:72: LoggingStats stats_;
On 2014/03/03 07:11:39, Alpha wrote:
> Maybe a separate LoggingStats object for audio and video?

See above.

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/stats...
File media/cast/logging/stats_serializer.h (right):

https://codereview.chromium.org/184853003/diff/40001/media/cast/logging/stats...
media/cast/logging/stats_serializer.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2014/03/03 07:11:39, Alpha wrote:
> I don't think we need to serialize the stats to JSON. media/cast API can
return
> a base::DictionaryValue.

Done. I will rename this class to StatsConverter?

https://codereview.chromium.org/184853003/diff/40001/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/184853003/diff/40001/media/cast/test/sender.c...
media/cast/test/sender.cc:387: scoped_ptr<std::string> audio_stats_json(new
std::string);
On 2014/03/03 07:11:39, Alpha wrote:
> Stats updates every second so getting the stat once at the end is not very
> useful. If we want to make it useful I'd prefer to print the information on
the
> receiver window.

That sounds quite ambitious and that it'll take a while to implement. How about
write out the stats as a JSON string to a file every 5 seconds or so?

[Alpha Left Google, 2014-03-04 21:13:35]

LGTM after stats_converter is folded into logging_stats.

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
File media/cast/logging/stats_converter.cc (right):

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
media/cast/logging/stats_converter.cc:13: scoped_ptr<base::DictionaryValue>
ConvertStats(
Move this to logging_stats.cc?

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
File media/cast/logging/stats_converter.h (right):

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
media/cast/logging/stats_converter.h:20: scoped_ptr<base::DictionaryValue>
ConvertStats(
Why not move this method to logging_stats.h? It's just one function.

[imcheng, 2014-03-05 00:20:26]

Adding kalman@ for extensions code. PTAL, thanks.

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
File media/cast/logging/stats_converter.cc (right):

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
media/cast/logging/stats_converter.cc:13: scoped_ptr<base::DictionaryValue>
ConvertStats(
On 2014/03/04 21:13:35, Alpha wrote:
> Move this to logging_stats.cc?

Done.

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
File media/cast/logging/stats_converter.h (right):

https://codereview.chromium.org/184853003/diff/70001/media/cast/logging/stats...
media/cast/logging/stats_converter.h:20: scoped_ptr<base::DictionaryValue>
ConvertStats(
On 2014/03/04 21:13:35, Alpha wrote:
> Why not move this method to logging_stats.h? It's just one function.

Done.

[not at google - send to devlin, 2014-03-05 20:27:09]

IDL lgtm

https://codereview.chromium.org/184853003/diff/130001/chrome/common/extension...
File chrome/common/extensions/api/cast_streaming_rtp_stream.idl (right):

https://codereview.chromium.org/184853003/diff/130001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_rtp_stream.idl:65: //              a
stream.
this comment needs updating

[imcheng, 2014-03-05 20:35:59]

https://codereview.chromium.org/184853003/diff/130001/chrome/common/extension...
File chrome/common/extensions/api/cast_streaming_rtp_stream.idl (right):

https://codereview.chromium.org/184853003/diff/130001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_rtp_stream.idl:65: //              a
stream.
On 2014/03/05 20:27:10, kalman wrote:
> this comment needs updating

I changed the return type back to DOMString. But I also updated comment to say
the returned data is in compressed format.

[imcheng, 2014-03-05 20:36:09]

The CQ bit was checked by imcheng@chromium.org

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

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

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

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

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

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-03-06 23:48:02]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-06 23:55:17]

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

[commit-bot: I haz the power, 2014-03-07 02:12:54]

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

[commit-bot: I haz the power, 2014-03-07 02:12:54]

Try jobs failed on following builders: mac_chromium_rel

[imcheng, 2014-03-07 03:04:19]

The CQ bit was checked by imcheng@chromium.org

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

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

[commit-bot: I haz the power, 2014-03-07 09:45:33]

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

[commit-bot: I haz the power, 2014-03-07 09:45:33]

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-03-07 09:46:50]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-03-07 09:48:48]

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

[commit-bot: I haz the power, 2014-03-07 19:47:57]

Change committed as 255689