Cast:Updating logging stats

Cast:Updating Logging stats 

Modify stats to hold more "native values" such as min, max, 
count and sum. 
This will allow the user to get more basic 
information, while still be able to compute higher level 
stats, such as frame rate and bit rate. 

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

[mikhal1, 2014-01-22 19:17:31]

Hello All,
Here's the update to the logging stats that we've been discussing.
A few points:
 - The new version keeps track of "native" values and does not compute any
values. The user should be able to compute the values of interest.
 - Avg/Var - I can add moving avg/var calculations but then comes the question
of how those should be used/computed, and if this is really useful for values
that will be read frequently (~1S). 
 - UMA - since these are very native values, I've removed many of the UMA stats.
I don't mind keeping them, but we should decide what's worth keeping in UMA, and
perhaps this should be done in a higher level based on needs.

Derek/Herve - I would appreciate your opinion. As discussed, this implementation
does have the inherent assumption that you have another layer above this and
using it to your needs, hence the last two bullets. 

Please feel free to ping me if you have any questions.
Thanks,
-Mikhal

[Alpha Left Google, 2014-01-22 19:44:22]

lgtm

Derek should review this. Removing myself from the reviewer list.

[Alpha Left Google, 2014-01-22 19:45:26]

Shooot.. I clicked some random area in the page and accidentally hit the quick
lgtm button.. :(

Not lgtm. Derek should review. Removing myself from reviewer list.

[imcheng, 2014-01-22 21:03:36]

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:114: // Some of the metrics will only be
set when applicable, e.g. delay and size.
For metrics that aren't always set, could you be more specific about when they
are set? Maybe a comment just above the fields saying "Applicable only for
kVideoRenderDelay" or something like that. That way it's easier to find out when
they're set without digging through too much code. Same with FrameEvent above.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:121: int size_sum;
size_t?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:165: typedef std::map<CastLoggingEvent,
linked_ptr<FrameLogStats > > FrameStatsMap;
Question: Why do stats maps use linked_ptrs but the ones above don't? Would
prefer to keep them consistent if possible.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_impl.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.h:68: AudioRtcpRawMap GetAudioRtcpRawData();
GetAudioRtcpRawData() and GetVideoRtcpRawData() behave differently from the
other three above in that they reset the underlying map. Could this be
documented?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.h:76: // Reset raw logging data.
Document that this does not reset AudioRtcpRawMap and VideoRtcpRawMap.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_stats.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:56: // Using the average delay as a counter,
will divide by the counter when
Remove comment

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:65: void
LoggingStats::InsertBaseFrameEvent(const base::TimeTicks& time_of_event,
Maybe have this function return a pointer to the FrameLogStats, so you don't
have to do another lookup in InsertFrameEventWithSize /
InsertFrameEventWithDelay?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:80: ++it->second->event_counter;
nit: Does this work? I am paranoid about operator precedence here.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:104: ++it->second->event_counter;
Same as above regarding operator precedence.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:125: ++it->second->event_counter;
Same as above regarding operator precedence.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_stats.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.h:50: const FrameStatsMap* GetFrameStatsData()
const;
Can you provide some documentation? (e.g. ownership of returned object, etc.)

[hguihot1, 2014-01-22 21:13:04]

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:133: size_t size_sum;
|size_sum| here, but FrameLogStats uses |sum_delay|. Naming should be
consistent.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:142: int sum;
Potential overflow? Should we use 64-bit for all sums?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:144: int max;
Having sum of squares as well would allow the caller to compute variance /
standard variation.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_impl.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.h:68: AudioRtcpRawMap GetAudioRtcpRawData();
Or could it be consistent and not Reset()?

On 2014/01/22 21:03:37, imcheng1 wrote:
> GetAudioRtcpRawData() and GetVideoRtcpRawData() behave differently from the
> other three above in that they reset the underlying map. Could this be
> documented?

[hguihot1, 2014-01-22 21:41:13]

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_impl.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.cc:57:
UMA_HISTOGRAM_COUNTS("Cast.AudioFrameEncoded", frame_size);
AudioFrameEncodedSize?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.cc:59:
UMA_HISTOGRAM_COUNTS("Cast.VideoFrameEncoded", frame_size);
VideoFrameEncodedSize?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.cc:171: UMA_HISTOGRAM_COUNTS("Cast.PacketLoss",
value);
Maybe a better name than PacketLoss? It does not say what this is (percentage,
absolute value, etc)

[mikhal1, 2014-01-23 19:53:11]

PTAL

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:114: // Some of the metrics will only be
set when applicable, e.g. delay and size.
On 2014/01/22 21:03:37, imcheng1 wrote:
> For metrics that aren't always set, could you be more specific about when they
> are set? Maybe a comment just above the fields saying "Applicable only for
> kVideoRenderDelay" or something like that. That way it's easier to find out
when
> they're set without digging through too much code. Same with FrameEvent above.

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:121: int size_sum;
On 2014/01/22 21:03:37, imcheng1 wrote:
> size_t?

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:133: size_t size_sum;
On 2014/01/22 21:13:05, hguihot1 wrote:
> |size_sum| here, but FrameLogStats uses |sum_delay|. Naming should be
> consistent.

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:142: int sum;
Depends on how often you'll reset it. Let's stick with this, and increment to 64
if we see there's a problem.
On 2014/01/22 21:13:05, hguihot1 wrote:
> Potential overflow? Should we use 64-bit for all sums?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:144: int max;
On the granularity of every time you poll it. If you're fine with that. Then I'm
adding it.
On 2014/01/22 21:13:05, hguihot1 wrote:
> Having sum of squares as well would allow the caller to compute variance /
> standard variation.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:165: typedef std::map<CastLoggingEvent,
linked_ptr<FrameLogStats > > FrameStatsMap;
On 2014/01/22 21:03:37, imcheng1 wrote:
> Question: Why do stats maps use linked_ptrs but the ones above don't? Would
> prefer to keep them consistent if possible.

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_impl.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.cc:57:
UMA_HISTOGRAM_COUNTS("Cast.AudioFrameEncoded", frame_size);
On 2014/01/22 21:41:13, hguihot1 wrote:
> AudioFrameEncodedSize?

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.cc:59:
UMA_HISTOGRAM_COUNTS("Cast.VideoFrameEncoded", frame_size);
On 2014/01/22 21:41:13, hguihot1 wrote:
> VideoFrameEncodedSize?

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.cc:171: UMA_HISTOGRAM_COUNTS("Cast.PacketLoss",
value);
On 2014/01/22 21:41:13, hguihot1 wrote:
> Maybe a better name than PacketLoss? It does not say what this is (percentage,
> absolute value, etc)

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_impl.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.h:68: AudioRtcpRawMap GetAudioRtcpRawData();
Get and reset makes more sense for this use case, and keeping it.
On 2014/01/22 21:13:05, hguihot1 wrote:
> Or could it be consistent and not Reset()?
> 
> On 2014/01/22 21:03:37, imcheng1 wrote:
> > GetAudioRtcpRawData() and GetVideoRtcpRawData() behave differently from the
> > other three above in that they reset the underlying map. Could this be
> > documented?
>

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.h:68: AudioRtcpRawMap GetAudioRtcpRawData();
On 2014/01/22 21:03:37, imcheng1 wrote:
> GetAudioRtcpRawData() and GetVideoRtcpRawData() behave differently from the
> other three above in that they reset the underlying map. Could this be
> documented?

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_impl.h:76: // Reset raw logging data.
On 2014/01/22 21:03:37, imcheng1 wrote:
> Document that this does not reset AudioRtcpRawMap and VideoRtcpRawMap.

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_stats.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:56: // Using the average delay as a counter,
will divide by the counter when
On 2014/01/22 21:03:37, imcheng1 wrote:
> Remove comment

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:65: void
LoggingStats::InsertBaseFrameEvent(const base::TimeTicks& time_of_event,
Was deliberating about that one too. I will still need to do the look up in case
of the first element, but can keep it in this function (still will be slightly
ugly).
What do you say? 
On 2014/01/22 21:03:37, imcheng1 wrote:
> Maybe have this function return a pointer to the FrameLogStats, so you don't
> have to do another lookup in InsertFrameEventWithSize /
> InsertFrameEventWithDelay?

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:80: ++it->second->event_counter;
my tests implies this works, but I'm adding a parenthesis to ease your paranoia.
On 2014/01/22 21:03:37, imcheng1 wrote:
> nit: Does this work? I am paranoid about operator precedence here.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:104: ++it->second->event_counter;
On 2014/01/22 21:03:37, imcheng1 wrote:
> Same as above regarding operator precedence.

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:125: ++it->second->event_counter;
On 2014/01/22 21:03:37, imcheng1 wrote:
> Same as above regarding operator precedence.

Done.

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_stats.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.h:50: const FrameStatsMap* GetFrameStatsData()
const;
On 2014/01/22 21:03:37, imcheng1 wrote:
> Can you provide some documentation? (e.g. ownership of returned object, etc.)

Done.

[imcheng, 2014-01-23 21:17:17]

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_stats.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:65: void
LoggingStats::InsertBaseFrameEvent(const base::TimeTicks& time_of_event,
In the case of inserting, map::insert returns a pair<iterator,bool> where the
iterator is pointing to the inserted element.

On 2014/01/23 19:53:11, mikhal1 wrote:
> Was deliberating about that one too. I will still need to do the look up in
case
> of the first element, but can keep it in this function (still will be slightly
> ugly).
> What do you say? 
> On 2014/01/22 21:03:37, imcheng1 wrote:
> > Maybe have this function return a pointer to the FrameLogStats, so you don't
> > have to do another lookup in InsertFrameEventWithSize /
> > InsertFrameEventWithDelay?
>

https://chromiumcodereview.appspot.com/130423007/diff/1/media/cast/logging/lo...
media/cast/logging/logging_stats.cc:80: ++it->second->event_counter;
Thanks!

On 2014/01/23 19:53:11, mikhal1 wrote:
> my tests implies this works, but I'm adding a parenthesis to ease your
paranoia.
> On 2014/01/22 21:03:37, imcheng1 wrote:
> > nit: Does this work? I am paranoid about operator precedence here.
>

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
File media/cast/logging/logging_defines.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_defines.cc:91: FrameEvent::FrameEvent() {}
Seems like we need to initialize the POD member varibles here and below as well?

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_defines.h:144: int64 sum_squared;
nit: uint64?

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_defines.h:167: typedef std::map<CastLoggingEvent,
FrameLogStats > FrameStatsMap;
nit: extra space before closing angle bracket here and below

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
File media/cast/logging/logging_impl.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_impl.h:65: // Pointers are owned by the logging
class, and are assumed valid on the main
You are returning a copy of the data though, so this comment doesn't apply.

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_impl.h:76: // Pointers are owned by the logging
class, and are assumed valid on the main
Same here, the code was changed from returning a pointer to a copy of the map so
this comment no longer applies. Just say it returns a copy of the data.

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
File media/cast/logging/logging_stats.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_stats.cc:119: stats.sum_squared = pow(value, 2);
nit: Why not just value * value?

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
File media/cast/logging/logging_unittest.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/260001/media/cast/loggi...
media/cast/logging/logging_unittest.cc:235: int value = 1;//kBaseValue +
base::RandInt(-5, 5);
nit: rm comment

[mikhal1, 2014-01-23 23:02:16]

PTAL

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
File media/cast/logging/logging_defines.cc (right):

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_defines.cc:91: FrameEvent::FrameEvent() {}
On 2014/01/23 21:17:18, imcheng1 wrote:
> Seems like we need to initialize the POD member varibles here and below as
well?

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
File media/cast/logging/logging_defines.h (right):

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_defines.h:144: int64 sum_squared;
On 2014/01/23 21:17:18, imcheng1 wrote:
> nit: uint64?

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_defines.h:167: typedef std::map<CastLoggingEvent,
FrameLogStats > FrameStatsMap;
On 2014/01/23 21:17:18, imcheng1 wrote:
> nit: extra space before closing angle bracket here and below

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
File media/cast/logging/logging_impl.h (right):

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_impl.h:65: // Pointers are owned by the logging
class, and are assumed valid on the main
On 2014/01/23 21:17:18, imcheng1 wrote:
> You are returning a copy of the data though, so this comment doesn't apply.

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_impl.h:76: // Pointers are owned by the logging
class, and are assumed valid on the main
On 2014/01/23 21:17:18, imcheng1 wrote:
> Same here, the code was changed from returning a pointer to a copy of the map
so
> this comment no longer applies. Just say it returns a copy of the data.

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
File media/cast/logging/logging_stats.cc (right):

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_stats.cc:119: stats.sum_squared = pow(value, 2);
On 2014/01/23 21:17:18, imcheng1 wrote:
> nit: Why not just value * value?

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_stats.cc:119: stats.sum_squared = pow(value, 2);
On 2014/01/23 21:17:18, imcheng1 wrote:
> nit: Why not just value * value?

Done.

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
File media/cast/logging/logging_unittest.cc (right):

https://codereview.chromium.org/130423007/diff/260001/media/cast/logging/logg...
media/cast/logging/logging_unittest.cc:235: int value = 1;//kBaseValue +
base::RandInt(-5, 5);
On 2014/01/23 21:17:18, imcheng1 wrote:
> nit: rm comment

Done.

[hguihot1, 2014-01-23 23:42:49]

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_defines.h:17: static const uint32 kFrameIdUnknown =
0xFFFF;
By the way, shouldn't it be 0xFFFFFFFF?

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
File media/cast/logging/logging_impl.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_impl.h:71: VideoRtcpRawMap
GetAndResetVideoRtcpRawData();
I don't quite get why these are GetAndReset() (when other ones have 2 API, Get()
and Reset()). Any specific reason?

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
File media/cast/logging/logging_stats.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_stats.cc:42: FrameStatsMap::iterator it =
frame_stats_.find(event);
For each event, it will call find() twice: once in InsertBaseFrameEvent, once in
here. Can we avoid that?

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_stats.cc:55: FrameStatsMap::iterator it =
frame_stats_.find(event);
Same comment here.

[imcheng, 2014-01-24 18:54:21]

lgtm after addressing Herve's comments.

[mikhal1, 2014-01-29 19:25:48]

Responding to hguihot's comments and adding pwestin as a reviewer.

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_defines.h:17: static const uint32 kFrameIdUnknown =
0xFFFF;
On 2014/01/23 23:42:50, hguihot1 wrote:
> By the way, shouldn't it be 0xFFFFFFFF?

Done.

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
File media/cast/logging/logging_impl.h (right):

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_impl.h:71: VideoRtcpRawMap
GetAndResetVideoRtcpRawData();
I would rather them both have a GetAndReset, but you (as the customer) preferred
not to have it that way, and the RTCP implementation preferred it this way.
Regardless, it will become obsolete with latest refactoring efforts. 
On 2014/01/23 23:42:50, hguihot1 wrote:
> I don't quite get why these are GetAndReset() (when other ones have 2 API,
Get()
> and Reset()). Any specific reason?

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
File media/cast/logging/logging_stats.cc (right):

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_stats.cc:42: FrameStatsMap::iterator it =
frame_stats_.find(event);
Please see my comment to imcheng about this. 
Again, would rather not bother, as this code is going through a serious
refactoring.
On 2014/01/23 23:42:50, hguihot1 wrote:
> For each event, it will call find() twice: once in InsertBaseFrameEvent, once
in
> here. Can we avoid that?

https://chromiumcodereview.appspot.com/130423007/diff/370001/media/cast/loggi...
media/cast/logging/logging_stats.cc:55: FrameStatsMap::iterator it =
frame_stats_.find(event);
same reply.
On 2014/01/23 23:42:50, hguihot1 wrote:
> Same comment here.

[pwestin, 2014-02-04 00:10:25]

lgtm

[mikhal1, 2014-02-04 21:24:43]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-04 22:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/130423007/590001

[commit-bot: I haz the power, 2014-02-05 01:13:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/130423007/590001

[mikhal1, 2014-02-05 18:04:18]

The CQ bit was unchecked by mikhal@chromium.org

[mikhal1, 2014-02-05 18:04:30]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-06 09:21:00]

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

[commit-bot: I haz the power, 2014-02-06 09:21:00]

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

[mikhal1, 2014-02-06 15:43:13]

The CQ bit was checked by mikhal@chromium.org

[commit-bot: I haz the power, 2014-02-06 15:43:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/130423007/590001

[commit-bot: I haz the power, 2014-02-06 17:36:44]

Change committed as 249423