Clean up histogram'd media enum max values.

Clean up histogram'd media enum max values.

This adds a PRESUBMIT test to src/media/ which enforces the following when using UMA_HISTOGRAM_ENUMERATION:
- The max enum value should be suffixed with 'MAX' or 'Max' (and it should be equal to the largest valid entry ever logged).
- One should be added to that max value when used in the UMA_HISTOGRAM_ENUMERATION macro.

To handle past misuses of UMA_HISTOGRAM_ENUMERATION for non-enums a comment of '// IGNORE_PRESUBMIT_UMA_MAX' was added to silence the presubmit check.

BUG=165553
TBR=danakj

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

[rileya (GONE FROM CHROMIUM), 2014-01-27 20:59:38]

An initial stab at some cleanup of the media/ enums. This focuses mainly on
several enums reported from/related to the DecoderConfigs.

The consensus on the mailing list seemed to be that HISTOGRAM_MAX was the best
name for the enum maxes, and that they should be the final value (aka the last
valid value + 1). Most of the enums changed here live in media:: so calling the
max simply HISTOGRAM_MAX would cause collisions, so I used FOO_HISTOGRAM_MAX (or
kFooHistogramMax, depending on the enum's naming style).

https://codereview.chromium.org/148553003/diff/1/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_encoder.cc (left):

https://codereview.chromium.org/148553003/diff/1/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_encoder.cc:663:
media::VIDEO_CODEC_PROFILE_MAX);
This was actually a bug, as the enum max was set previously this should have had
a + 1.

https://codereview.chromium.org/148553003/diff/1/media/base/channel_layout.cc
File media/base/channel_layout.cc (right):

https://codereview.chromium.org/148553003/diff/1/media/base/channel_layout.cc...
media/base/channel_layout.cc:51: static const int
This possibly unnecessary indenting brought to you by clang-format!

[scherkus (not reviewing), 2014-01-27 22:37:30]

+fischman for his $0.02 since he was kind enough to provide his $0.02 on the
email thread

I'm not overly keen on seeing XXX_HISTOGRAM_MAX values being used in
non-histogramming applications (e.g., AudioParameters::IsValid()) ... which
would mean we'd go down the path of defining pairs of maximum values: one that
is the last legal value vs. the +1 equivalent for histogramming purposes

I need to think about it.

https://codereview.chromium.org/148553003/diff/20001/media/audio/pulse/pulse_...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/148553003/diff/20001/media/audio/pulse/pulse_...
media/audio/pulse/pulse_util.cc:46: default:
can you remove the default here?

https://codereview.chromium.org/148553003/diff/20001/media/base/channel_layou...
File media/base/channel_layout.cc (right):

https://codereview.chromium.org/148553003/diff/20001/media/base/channel_layou...
media/base/channel_layout.cc:56: {-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1},
FYI if dale was here he might be able to comment how much he cares about the
horizontal whitespace formatting here

https://codereview.chromium.org/148553003/diff/20001/media/base/channel_layout.h
File media/base/channel_layout.h (right):

https://codereview.chromium.org/148553003/diff/20001/media/base/channel_layou...
media/base/channel_layout.h:102: // Total number of layouts.
I'd update this to reflect its use w/ histrogramming

[Ami GONE FROM CHROMIUM, 2014-01-27 22:45:04]

Throwing my two pennies out the window while driving by at a high rate of speed.


In C++ "max" means "equal to the largest semantically-meaningful value" and
"end" means "one past the largest semantically-meaningful value". 
FOO_HISTOGRAM_MAX mixes these up and I don't like it.
I'd be fine with FOO_END being that value (ditto FOO_SENTINEL or FOO_<ANYTHING>
that made it clear it was not a legal value).

Although now, as then, my preference is to NOT use a sentinel and instead use a
_MAX value that is _equal_ to the largest semantically-meaningful value (when
needed), because  that avoids unpleasantness:
1) default: clauses can be removed from switches because the compiler has a
well-defined list of things to check
2) the enum is only as wide as it needs to be

The down-side is the peril of using the UMA API incorrectly.  I don't think that
justifies adding labels to all enums, esp. when they are used for not just UMA
purposes.

[scherkus (not reviewing), 2014-01-28 19:29:22]

On 2014/01/27 22:45:04, Ami Fischman wrote:
> Throwing my two pennies out the window while driving by at a high rate of
speed.
> 
> 
> In C++ "max" means "equal to the largest semantically-meaningful value" and
> "end" means "one past the largest semantically-meaningful value". 
> FOO_HISTOGRAM_MAX mixes these up and I don't like it.
> I'd be fine with FOO_END being that value (ditto FOO_SENTINEL or
FOO_<ANYTHING>
> that made it clear it was not a legal value).
> 
> Although now, as then, my preference is to NOT use a sentinel and instead use
a
> _MAX value that is _equal_ to the largest semantically-meaningful value (when
> needed), because  that avoids unpleasantness:
> 1) default: clauses can be removed from switches because the compiler has a
> well-defined list of things to check
> 2) the enum is only as wide as it needs to be
> 
> The down-side is the peril of using the UMA API incorrectly.  I don't think
that
> justifies adding labels to all enums, esp. when they are used for not just UMA
> purposes.

I wonder if PRESUBMIT.py could help if we were more consistent with our naming.

e.g., if we standardize on FOO_MAX then we could check that it's been +1'd when
used with HISTOGRAM_ENUMERATION

[rileya (GONE FROM CHROMIUM), 2014-01-28 20:39:44]

Framed with the C++ notion of max/end, 'FOO_MAX = last valid foo' sounds good.

> I wonder if PRESUBMIT.py could help if we were more consistent with our
naming.
>
> e.g., if we standardize on FOO_MAX then we could check that it's been +1'd
when
> used with HISTOGRAM_ENUMERATION

Yeah, I was thinking the same thing, I don't know how the presubmit scripts work
though, so I'll have a look into how that would work...

[scherkus (not reviewing), 2014-01-28 20:57:51]

On 2014/01/28 20:39:44, rileya wrote:
> Framed with the C++ notion of max/end, 'FOO_MAX = last valid foo' sounds good.
> 
> > I wonder if PRESUBMIT.py could help if we were more consistent with our
> naming.
> >
> > e.g., if we standardize on FOO_MAX then we could check that it's been +1'd
> when
> > used with HISTOGRAM_ENUMERATION
> 
> Yeah, I was thinking the same thing, I don't know how the presubmit scripts
work
> though, so I'll have a look into how that would work...

I found it to be pretty self-explanatory assuming you know a bit of python.
Basically you can iterate over all the lines of changed code and sprinkle in
some regular expressions.

Take a look at my patch to media/PRESUBMIT.py for some boilerplate code:
http://src.chromium.org/viewvc/chrome/trunk/src/media/PRESUBMIT.py?r1=243408&...

[rileya (GONE FROM CHROMIUM), 2014-01-29 19:24:10]

Okay, take 2.

I changed every UMA'd enum in src/media/ that codesearch turned up (I imagine
there's more enums or usages of media/ enums in content/... and elsewhere, but
I'd like some feedback before I hunt all of those down...).

Now kFooMax/FOO_MAX are set equal to the final valid element. This eliminates a
lot of silly NOTREACHED cases in switch statements. It also has some other side
effects which I've noted in comments.

https://codereview.chromium.org/148553003/diff/30001/media/audio/sample_rates.h
File media/audio/sample_rates.h (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/sample_rates...
media/audio/sample_rates.h:27: kAudioSampleRateMax =
This is a little iffy -- kUnexpectedAudioSampleRate sounds like a meaningful
value, but it's also used as the histogram cap. I added a max value to be
consistent with everything else, but adding a '+1' in the uma call seems
unnecessary if kUnexpectedAudioSampleRate was already used there...

https://codereview.chromium.org/148553003/diff/30001/media/base/channel_layou...
File media/base/channel_layout.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/base/channel_layou...
media/base/channel_layout.cc:51: static const int
kChannelOrderings[CHANNEL_LAYOUT_MAX + 1][CHANNELS_MAX + 1] = {
An interesting side effect of this change, places where we size arrays/etc by
the enum maxes require a +1 now.

https://codereview.chromium.org/148553003/diff/30001/media/filters/stream_par...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/filters/stream_par...
media/filters/stream_parser_factory.cc:52: HISTOGRAM_MAX = HISTOGRAM_OPUS  //
Must be equal to last entry!
HISTOGRAM_MAX now seems like kind of a misnomer, as it isn't the max we're using
for the UMA call, but rather the max of the enum itself. Given that everything
else is prefixed with HISTOGRAM_ I suppose this is alright though...

[rileya (GONE FROM CHROMIUM), 2014-01-29 21:12:19]

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:75: UMA_HISTOGRAM_ENUMERATION(
This isn't actually an enumeration, is there a better way of UMA-ing this?

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:81: UMA_HISTOGRAM_ENUMERATION(
Same here. More in the Fallback function below.

[Ami GONE FROM CHROMIUM, 2014-01-29 21:29:02]

I like this a lot!

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:75: UMA_HISTOGRAM_ENUMERATION(
On 2014/01/29 21:12:20, rileya wrote:
> This isn't actually an enumeration, is there a better way of UMA-ing this?

Sure, you can basically expand the macro.

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:113: asr, kAudioSampleRateMax);
If you had a PRESUBMIT it would bitch about this.  
Per elsewhere hopefully this can become Max+1.

https://codereview.chromium.org/148553003/diff/30001/media/audio/sample_rates.h
File media/audio/sample_rates.h (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/sample_rates...
media/audio/sample_rates.h:27: kAudioSampleRateMax =
On 2014/01/29 19:24:11, rileya wrote:
> This is a little iffy -- kUnexpectedAudioSampleRate sounds like a meaningful
> value, but it's also used as the histogram cap. I added a max value to be
> consistent with everything else, but adding a '+1' in the uma call seems
> unnecessary if kUnexpectedAudioSampleRate was already used there...

kUnexpected is a terrible fit since it doesn't get reported to UMA (hence the
weirdness w/ no explicit value and expected to change value if new rates are
added above it, even though this is a UMA-reported stat so should "never
change").
IMO set kAudioSampleRateMax = k192000Hz, drop kUnexpected, and take the hit on
changing
AudioSampleRate AsAudioSampleRate(int sample_rate);
to
bool ToAudioSampleRate(int sample_rate, AudioSampleRate* asr);
and return false if unexpected.

https://codereview.chromium.org/148553003/diff/30001/media/audio/sample_rates...
media/audio/sample_rates.h:28: kUnexpectedAudioSampleRate,  // Must always equal
last entry!
FWIW, here and elsewhere, the real requirement (for UMA) is that this equal the
largest value ever used.

https://codereview.chromium.org/148553003/diff/30001/media/filters/stream_par...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/filters/stream_par...
media/filters/stream_parser_factory.cc:52: HISTOGRAM_MAX = HISTOGRAM_OPUS  //
Must be equal to last entry!
On 2014/01/29 19:24:11, rileya wrote:
> HISTOGRAM_MAX now seems like kind of a misnomer, as it isn't the max we're
using
> for the UMA call, but rather the max of the enum itself. Given that everything
> else is prefixed with HISTOGRAM_ I suppose this is alright though...

Can this enum type be replaced with a union of audio & video codec enum?

[rileya (GONE FROM CHROMIUM), 2014-01-29 23:52:58]

I'll respond to the comments in a bit, but I want a quick opinion or two first.

So making a robust presubmit check for this is actually not as simple as it
seems. We want to look for UMA_HISTOGRAM_ENUMERATION and make sure that the
third argument contains the word 'Max'/'MAX' and has a '+ 1'. We can't just scan
line by line (like most presubmit test scenarios), because it can (and often
does) span multiple lines. And writing a regex for this is not trivial; I
started to write it out and was reminded a lot more than I'd like of the
compilers class I took...

I ended up with something like this, before realizing this was a little
questionable:
\bUMA_HISTOGRAM_ENUMERATION([\n\t ]*\"[^"]*",[\n\t ]*[^;],[\n\t
]*[A-Za-z0-9_](Max|MAX)[\n\t ]*+[\n\t ]1[\n\t ]*);

So is an imperfect check acceptable (as long as it doesn't result in false
positives)? Because I don't think a completely bulletproof check is possible
without writing a lexer/parser...

One alternative I see is making a media-specific uma macro
(MEDIA_UMA_HISTOGRAM_ENUMERATION?) that magically adds the 1 and then check that
UMA_HISTOGRAM_ENUMERATION isn't used.

[scherkus (not reviewing), 2014-01-30 01:05:20]

On 2014/01/29 23:52:58, rileya wrote:
> I'll respond to the comments in a bit, but I want a quick opinion or two
first.
> 
> So making a robust presubmit check for this is actually not as simple as it
> seems. We want to look for UMA_HISTOGRAM_ENUMERATION and make sure that the
> third argument contains the word 'Max'/'MAX' and has a '+ 1'. We can't just
scan
> line by line (like most presubmit test scenarios), because it can (and often
> does) span multiple lines. And writing a regex for this is not trivial; I
> started to write it out and was reminded a lot more than I'd like of the
> compilers class I took...
> 
> I ended up with something like this, before realizing this was a little
> questionable:
> \bUMA_HISTOGRAM_ENUMERATION([\n\t ]*\"[^"]*",[\n\t ]*[^;],[\n\t
> ]*[A-Za-z0-9_](Max|MAX)[\n\t ]*+[\n\t ]1[\n\t ]*);
> 
> So is an imperfect check acceptable (as long as it doesn't result in false
> positives)? Because I don't think a completely bulletproof check is possible
> without writing a lexer/parser...

Imperfect is OK.

You can also include a message to email you if it is a false positive, such as
other presubmits do:
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=59

We can test it out in src/media/ for starters.

> One alternative I see is making a media-specific uma macro
> (MEDIA_UMA_HISTOGRAM_ENUMERATION?) that magically adds the 1 and then check
that
> UMA_HISTOGRAM_ENUMERATION isn't used.

My $0.02 -- I'd like to avoid adding more media-isms like this.

[Ami GONE FROM CHROMIUM, 2014-01-30 03:51:58]

On Wed, Jan 29, 2014 at 5:05 PM, <scherkus@chromium.org> wrote:

> My $0.02 -- I'd like to avoid adding more media-isms like this


+1

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rileya (GONE FROM CHROMIUM), 2014-02-04 02:47:10]

I added a check to the src/media/PRESUBMIT.py, it ended up a bit long/convoluted
since the histogram macros can span multiple lines and such. It isn't perfect,
but assuming the style guide is followed, it should work reasonably. My Python
knowledge is rather limited, so my style may not be perfect...

This CL still needs some more cleanup elsewhere, so just take a look at the
PRESUBMIT for the moment.

[Ami GONE FROM CHROMIUM, 2014-02-04 02:56:46]

https://codereview.chromium.org/148553003/diff/120002/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/148553003/diff/120002/media/PRESUBMIT.py#newc...
media/PRESUBMIT.py:109: r'\bUMA_HISTOGRAM_ENUMERATION\(' + whitespace +
histogram_name + r',' +
Sadly this looks right ;)

(please make sure that every file under media/ obeys this expectation)

[scherkus (not reviewing), 2014-02-05 01:02:28]

nifty!

might I suggest some python unit tests? we don't have one for media yet, but I'm
sure we can add one:
https://code.google.com/p/chromium/codesearch#search/&q=file:PRESUBMIT.*test&...

[rileya (GONE FROM CHROMIUM), 2014-02-05 22:30:04]

Okay, I did more thorough cleanup, and found and adjusted all the instances of
affected media/ enum maxes (that codesearch turned up at least) outside of
media/.

I'll look into unit tests for the PRESUBMIT next.

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:75: UMA_HISTOGRAM_ENUMERATION(
On 2014/01/29 21:29:02, Ami Fischman wrote:
> On 2014/01/29 21:12:20, rileya wrote:
> > This isn't actually an enumeration, is there a better way of UMA-ing this?
> 
> Sure, you can basically expand the macro.

I did this, but it seems rather ugly. I think UMA_HISTOGRAM_CUSTOM_COUNTS would
be more appropriate, but I'm unsure if this would still match up with existing
data.

https://codereview.chromium.org/148553003/diff/190001/media/audio/sample_rate...
File media/audio/sample_rates.cc (right):

https://codereview.chromium.org/148553003/diff/190001/media/audio/sample_rate...
media/audio/sample_rates.cc:15: return true;
A more concise way of flowing this didn't seem immediately obvious to me, any
ideas?

[Ami GONE FROM CHROMIUM, 2014-02-05 22:42:54]

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:75: UMA_HISTOGRAM_ENUMERATION(
On 2014/02/05 22:30:05, rileya wrote:
> On 2014/01/29 21:29:02, Ami Fischman wrote:
> > On 2014/01/29 21:12:20, rileya wrote:
> > > This isn't actually an enumeration, is there a better way of UMA-ing this?
> > 
> > Sure, you can basically expand the macro.
> 
> I did this, but it seems rather ugly. I think UMA_HISTOGRAM_CUSTOM_COUNTS
would
> be more appropriate, but I'm unsure if this would still match up with existing
> data.

I would not trust myself to verify that by inspection and would have to test it
on all inputs to convince myself it was correct.
Why bother, though?  You can't reduce the # of buckets without messing up
historical data, so what would the gain be?  Just not using "ENUMERATION" for a
non-enum?  Doesn't seem worth it to me.

https://codereview.chromium.org/148553003/diff/190001/media/audio/sample_rate...
File media/audio/sample_rates.cc (right):

https://codereview.chromium.org/148553003/diff/190001/media/audio/sample_rate...
media/audio/sample_rates.cc:15: return true;
On 2014/02/05 22:30:05, rileya wrote:
> A more concise way of flowing this didn't seem immediately obvious to me, any
> ideas?

lol you could always
case 8000: return *asr = k8000Hz, true;
...

but what you have is fine.

[rileya (GONE FROM CHROMIUM), 2014-02-06 21:12:39]

Alright, I added a simple set of unit tests for the presubmit check. I don't see
any automated place to hook them into (and non of the existing PRESUBMIT_tests
do anything like this), so they can only be run manually.

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/30001/media/audio/audio_output...
media/audio/audio_output_resampler.cc:75: UMA_HISTOGRAM_ENUMERATION(
On 2014/02/05 22:42:54, Ami Fischman wrote:
> On 2014/02/05 22:30:05, rileya wrote:
> > On 2014/01/29 21:29:02, Ami Fischman wrote:
> > > On 2014/01/29 21:12:20, rileya wrote:
> > > > This isn't actually an enumeration, is there a better way of UMA-ing
this?
> > > 
> > > Sure, you can basically expand the macro.
> > 
> > I did this, but it seems rather ugly. I think UMA_HISTOGRAM_CUSTOM_COUNTS
> would
> > be more appropriate, but I'm unsure if this would still match up with
existing
> > data.
> 
> I would not trust myself to verify that by inspection and would have to test
it
> on all inputs to convince myself it was correct.
> Why bother, though?  You can't reduce the # of buckets without messing up
> historical data, so what would the gain be?  Just not using "ENUMERATION" for
a
> non-enum?  Doesn't seem worth it to me.

Fair enough, so does the current slightly verbose/ugly expanded macro seem
acceptable?

[Ami GONE FROM CHROMIUM, 2014-02-06 21:20:08]

On Thu, Feb 6, 2014 at 1:12 PM, <rileya@chromium.org> wrote:

> Fair enough, so does the current slightly verbose/ugly expanded macro
> seem acceptable?
>

No, I meant I'd just leave it using _ENUMERATION

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rileya (GONE FROM CHROMIUM), 2014-02-06 21:21:46]

On 2014/02/06 21:20:08, Ami Fischman wrote:
> On Thu, Feb 6, 2014 at 1:12 PM, <mailto:rileya@chromium.org> wrote:
> 
> > Fair enough, so does the current slightly verbose/ugly expanded macro
> > seem acceptable?
> >
> 
> No, I meant I'd just leave it using _ENUMERATION
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Oh, I misunderstood. The issue with that is that the presubmit check would
complain if anyone ever modifies those lines.

[rileya (GONE FROM CHROMIUM), 2014-02-12 02:46:35]

Giving this a bump. Would leaving those instances in
media/audio/audio_output_resampler.cc alone and letting the PRESUBMIT throw
false positives if they get modified be better than the ugly macro expansion?

Also: I ran the script on all source files in media/ which turned up only one
other false positive, where we report an FFmpeg enum via UMA:
https://code.google.com/p/chromium/codesearch#chromium/src/media/ffmpeg/ffmpe...

[Ami GONE FROM CHROMIUM, 2014-02-12 06:21:37]

You could add a // IGNORE_MAX_VIOLATION trailing comment to the instances that
can't/won't be changed and make the PRESUBMIT understand that.
(and name it something better)

Pro-tip: if you want to "ping" or "bump" a thread mention who you're waiting on
specifically, by name :)

[rileya (GONE FROM CHROMIUM), 2014-02-12 19:26:27]

On 2014/02/12 06:21:37, Ami Fischman wrote:
> You could add a // IGNORE_MAX_VIOLATION trailing comment to the instances that
> can't/won't be changed and make the PRESUBMIT understand that.
> (and name it something better)

Alright, sounds good, done (I used "// PRESUBMIT_IGNORE_UMA_MAX").

[Ami GONE FROM CHROMIUM, 2014-02-12 20:26:47]

LGTM % nits and scherkus' actual review (I only skimmed the PRESUBMIT test, for
example)

https://codereview.chromium.org/148553003/diff/650001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/148553003/diff/650001/media/PRESUBMIT.py#newc...
media/PRESUBMIT.py:157: 'Contact rileya@chromium.org if you have questions.' ,
problems)]
I don't want to cut down on your social life, but you might get fewer
questions^Wpitchforks if you doco in the CL description this regime.

https://codereview.chromium.org/148553003/diff/650001/media/PRESUBMIT_test.py
File media/PRESUBMIT_test.py (right):

https://codereview.chromium.org/148553003/diff/650001/media/PRESUBMIT_test.py...
media/PRESUBMIT_test.py:83: #Take an input and make sure the problems found
equals the expectation.
indent

https://codereview.chromium.org/148553003/diff/650001/media/audio/audio_outpu...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/650001/media/audio/audio_outpu...
media/audio/audio_output_resampler.cc:75: UMA_HISTOGRAM_ENUMERATION(
Maybe precede with a
// NOTE: PRESUBMIT_IGNORE_UMA_MAX below because we're abusing the ENUMERATION
API to report discrete values.

https://codereview.chromium.org/148553003/diff/650001/media/audio/sample_rate...
File media/audio/sample_rates.cc (right):

https://codereview.chromium.org/148553003/diff/650001/media/audio/sample_rate...
media/audio/sample_rates.cc:15: return true;
Is clang-format pleased with this?  
(having a statement follow the label on the same line, and then another on the
next line)

https://codereview.chromium.org/148553003/diff/650001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/148553003/diff/650001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:394: AVCOL_RANGE_NB);  // PRESUBMIT_IGNORE_UMA_MAX
NOTE why IGNORE (like the other comment)

https://codereview.chromium.org/148553003/diff/650001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/148553003/diff/650001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:30: MAX_EVENTS = RENDER_ERROR,
EVENT_MAX
?

[rileya (GONE FROM CHROMIUM), 2014-02-13 02:40:35]

Alrighty, addressed the nits.

[rileya (GONE FROM CHROMIUM), 2014-02-24 21:12:54]

ping: scherkus@ mind having a look?

[scherkus (not reviewing), 2014-02-25 03:44:48]

https://codereview.chromium.org/148553003/diff/730001/media/audio/audio_outpu...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/148553003/diff/730001/media/audio/audio_outpu...
media/audio/audio_output_resampler.cc:90: if
(media::ToAudioSampleRate(output_params.sample_rate(), &asr)) {
remove media::

https://codereview.chromium.org/148553003/diff/730001/media/audio/audio_outpu...
media/audio/audio_output_resampler.cc:118: if
(media::ToAudioSampleRate(output_params.sample_rate(), &asr)) {
remove media::

https://codereview.chromium.org/148553003/diff/730001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/148553003/diff/730001/media/base/audio_decode...
media/base/audio_decoder_config.cc:57: if
(media::ToAudioSampleRate(samples_per_second, &asr)) {
remove media::

https://codereview.chromium.org/148553003/diff/730001/media/base/channel_layo...
File media/base/channel_layout.h (right):

https://codereview.chromium.org/148553003/diff/730001/media/base/channel_layo...
media/base/channel_layout.h:102: // Max value
combine subsequent comment w/ this one? (or replace this one?)

https://codereview.chromium.org/148553003/diff/730001/media/base/pipeline_sta...
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/148553003/diff/730001/media/base/pipeline_sta...
media/base/pipeline_status.h:35: PIPELINE_STATUS_MAX =
DECODER_ERROR_NOT_SUPPORTED,  // Must be equal to
nit: instead of squeezing a comment here I'd stick it on the line before

(the rest of this enum has comments preceeding values, so consistency is nice)

https://codereview.chromium.org/148553003/diff/730001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/148553003/diff/730001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:392: // Note the 'PRESUBMIT_IGNORE_UMA_MAX below,
this silences the PRESUBMIT.py
nit: remove leading ' ... or did you mean to terminate the quote?

if the former, then you should fix up the other comments in
media/audio/audio_output_resampler.cc

[rileya (GONE FROM CHROMIUM), 2014-02-25 19:39:43]

Alright, think I fixed everything.

[scherkus (not reviewing), 2014-02-25 19:41:35]

lgtm!

thanks for tackling this!

[rileya (GONE FROM CHROMIUM), 2014-02-25 21:06:10]

+danakj for TBR of the tiny cc/resources/video_resource_updater.cc change.

[rileya (GONE FROM CHROMIUM), 2014-02-25 21:06:32]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-25 21:08:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/148553003/1040001

[rileya (GONE FROM CHROMIUM), 2014-02-25 21:28:36]

The CQ bit was unchecked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-25 21:30:28]

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

[commit-bot: I haz the power, 2014-02-25 21:30:29]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[danakj, 2014-02-25 21:46:40]

cc LGTM

[rileya (GONE FROM CHROMIUM), 2014-02-25 22:26:25]

+tsepez: Mind having a look at content/common/gpu/gpu_messages.h ?

https://codereview.chromium.org/148553003/diff/1060001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/148553003/diff/1060001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:57: media::VideoFrame::FORMAT_MAX)
Note that VideoFrame::FORMAT_MAX is equal to the last valid value
(IPC_ENUM_TRAITS_MAX_VALUE is inclusive, correct?).

[Tom Sepez, 2014-02-25 23:22:54]

lgtm

https://codereview.chromium.org/148553003/diff/1060001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/148553003/diff/1060001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:57: media::VideoFrame::FORMAT_MAX)
On 2014/02/25 22:26:26, rileya wrote:
> Note that VideoFrame::FORMAT_MAX is equal to the last valid value
> (IPC_ENUM_TRAITS_MAX_VALUE is inclusive, correct?).
Yes, inclusive.

[rileya (GONE FROM CHROMIUM), 2014-02-25 23:30:41]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-25 23:40:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/148553003/1060001

[commit-bot: I haz the power, 2014-02-26 05:34:08]

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

[commit-bot: I haz the power, 2014-02-26 05:34:09]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[rileya (GONE FROM CHROMIUM), 2014-02-26 22:39:57]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-26 22:42:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/148553003/1060002

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

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

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

Failed to apply patch for content/renderer/media/webrtc_audio_renderer.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/webrtc_audio_renderer.cc
  Hunk #2 FAILED at 336.
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/media/webrtc_audio_renderer.cc.rej

Patch:       content/renderer/media/webrtc_audio_renderer.cc
Index: content/renderer/media/webrtc_audio_renderer.cc
diff --git a/content/renderer/media/webrtc_audio_renderer.cc
b/content/renderer/media/webrtc_audio_renderer.cc
index
9187983773fc0dfcde351842ea0386c673fb1511..298436998c26d775c34fb324df9b2ca3b9d38c64
100644
--- a/content/renderer/media/webrtc_audio_renderer.cc
+++ b/content/renderer/media/webrtc_audio_renderer.cc
@@ -238,10 +238,10 @@ bool
WebRtcAudioRenderer::Initialize(WebRtcAudioRendererSource* source) {
     DVLOG(1) << "Resampling from 48000 to 192000 is required";
     sample_rate = 48000;
   }
-  media::AudioSampleRate asr = media::AsAudioSampleRate(sample_rate);
-  if (asr != media::kUnexpectedAudioSampleRate) {
+  media::AudioSampleRate asr;
+  if (media::ToAudioSampleRate(sample_rate, &asr)) {
     UMA_HISTOGRAM_ENUMERATION(
-        "WebRTC.AudioOutputSampleRate", asr,
media::kUnexpectedAudioSampleRate);
+        "WebRTC.AudioOutputSampleRate", asr, media::kAudioSampleRateMax + 1);
   } else {
     UMA_HISTOGRAM_COUNTS("WebRTC.AudioOutputSampleRateUnexpected",
                          sample_rate);
@@ -336,7 +336,7 @@ bool
WebRtcAudioRenderer::Initialize(WebRtcAudioRendererSource* source) {
 
   UMA_HISTOGRAM_ENUMERATION("WebRTC.AudioOutputChannelLayout",
                             source_params.channel_layout(),
-                            media::CHANNEL_LAYOUT_MAX);
+                            media::CHANNEL_LAYOUT_MAX + 1);
   UMA_HISTOGRAM_ENUMERATION("WebRTC.AudioOutputFramesPerBuffer",
                             source_params.frames_per_buffer(),
                             kUnexpectedAudioBufferSize);

[rileya (GONE FROM CHROMIUM), 2014-02-26 22:53:10]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-26 22:58:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/148553003/1080001

[commit-bot: I haz the power, 2014-02-27 00:28:30]

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

[commit-bot: I haz the power, 2014-02-27 00:28:31]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[rileya (GONE FROM CHROMIUM), 2014-02-27 18:12:30]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-27 18:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/148553003/1100001

[commit-bot: I haz the power, 2014-02-27 18:34:22]

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

[commit-bot: I haz the power, 2014-02-27 18:34:23]

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

[rileya (GONE FROM CHROMIUM), 2014-02-28 18:30:42]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-02-28 18:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/148553003/1100001

[commit-bot: I haz the power, 2014-02-28 20:30:46]

Change committed as 254209