Make explicit the PipelineStatus enum values and warn that they are logged to UMA.

Make explicit the PipelineStatus enum values and warn that they are logged to UMA.

The explicit values should make it easier to avoid accidentally reusing a
previously-used code as obsolete ones are retired.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151667

[Ami GONE FROM CHROMIUM, 2012-08-13 20:24:19]

scherkus: please review
posciak: FYI

This came up while I wrote https://chromereviews.googleplex.com/4714055

[scherkus (not reviewing), 2012-08-13 21:08:59]

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#n...
media/base/pipeline_status.h:15: // Logged to UMA, so never reuse a value,
always add new/greater ones!
is this only logged for GpuVideoDecoder::Initialize()?

how many values are actually used?

[Ami GONE FROM CHROMIUM, 2012-08-13 21:23:54]

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#n...
media/base/pipeline_status.h:15: // Logged to UMA, so never reuse a value,
always add new/greater ones!
On 2012/08/13 21:08:59, scherkus wrote:
> is this only logged for GpuVideoDecoder::Initialize()?

Today, yes.

> how many values are actually used?

Today GVD can return OK (yay!), DECODER_ERROR_NOT_SUPPORTED (HW doesn't support
specific profile/codec), and PIPELINE_ERROR_DECODE (oh crap, something went
wrong; invalid demuxer config, missing stream, etc.).

[scherkus (not reviewing), 2012-08-14 21:40:29]

(crap I forgot to hit send yesterday!)

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#n...
media/base/pipeline_status.h:15: // Logged to UMA, so never reuse a value,
always add new/greater ones!
On 2012/08/13 21:23:54, Ami Fischman wrote:
> On 2012/08/13 21:08:59, scherkus wrote:
> > is this only logged for GpuVideoDecoder::Initialize()?
> 
> Today, yes.
> 
> > how many values are actually used?
> 
> Today GVD can return OK (yay!), DECODER_ERROR_NOT_SUPPORTED (HW doesn't
support
> specific profile/codec), and PIPELINE_ERROR_DECODE (oh crap, something went
> wrong; invalid demuxer config, missing stream, etc.).

Should VideoDecoders have their own status codes / should they be expanded to
include the problems you listed here?

[Ami GONE FROM CHROMIUM, 2012-08-14 21:43:28]

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#n...
media/base/pipeline_status.h:15: // Logged to UMA, so never reuse a value,
always add new/greater ones!
On 2012/08/14 21:40:30, scherkus wrote:
> On 2012/08/13 21:23:54, Ami Fischman wrote:
> > On 2012/08/13 21:08:59, scherkus wrote:
> > > is this only logged for GpuVideoDecoder::Initialize()?
> > 
> > Today, yes.
> > 
> > > how many values are actually used?
> > 
> > Today GVD can return OK (yay!), DECODER_ERROR_NOT_SUPPORTED (HW doesn't
> support
> > specific profile/codec), and PIPELINE_ERROR_DECODE (oh crap, something went
> > wrong; invalid demuxer config, missing stream, etc.).
> 
> Should VideoDecoders have their own status codes / should they be expanded to
> include the problems you listed here?

My rule of thumb is that if you're not going to handle a status code
specifically, it shouldn't exist.
So, no, I don't think we should make these codes more granular than they already
are.

[scherkus (not reviewing), 2012-08-14 21:51:28]

On 2012/08/14 21:43:28, Ami Fischman wrote:
> http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h
> File media/base/pipeline_status.h (right):
> 
>
http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#n...
> media/base/pipeline_status.h:15: // Logged to UMA, so never reuse a value,
> always add new/greater ones!
> On 2012/08/14 21:40:30, scherkus wrote:
> > On 2012/08/13 21:23:54, Ami Fischman wrote:
> > > On 2012/08/13 21:08:59, scherkus wrote:
> > > > is this only logged for GpuVideoDecoder::Initialize()?
> > > 
> > > Today, yes.
> > > 
> > > > how many values are actually used?
> > > 
> > > Today GVD can return OK (yay!), DECODER_ERROR_NOT_SUPPORTED (HW doesn't
> > support
> > > specific profile/codec), and PIPELINE_ERROR_DECODE (oh crap, something
went
> > > wrong; invalid demuxer config, missing stream, etc.).
> > 
> > Should VideoDecoders have their own status codes / should they be expanded
to
> > include the problems you listed here?
> 
> My rule of thumb is that if you're not going to handle a status code
> specifically, it shouldn't exist.
> So, no, I don't think we should make these codes more granular than they
already
> are.

What about the first part of my question?

[Ami GONE FROM CHROMIUM, 2012-08-14 22:03:54]

On Tue, Aug 14, 2012 at 2:51 PM, <scherkus@chromium.org> wrote:

> On 2012/08/14 21:43:28, Ami Fischman wrote:
>
>> http://codereview.chromium.**org/10824283/diff/1/media/**
>>
base/pipeline_status.h<http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h>
>> File media/base/pipeline_status.h (right):
>>
>
>
> http://codereview.chromium.**org/10824283/diff/1/media/**
>
base/pipeline_status.h#**newcode15<http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#newcode15>
>
>> media/base/pipeline_status.h:**15: // Logged to UMA, so never reuse a
>> value,
>> always add new/greater ones!
>> On 2012/08/14 21:40:30, scherkus wrote:
>> > On 2012/08/13 21:23:54, Ami Fischman wrote:
>> > > On 2012/08/13 21:08:59, scherkus wrote:
>> > > > is this only logged for GpuVideoDecoder::Initialize()?
>> > >
>> > > Today, yes.
>> > >
>> > > > how many values are actually used?
>> > >
>> > > Today GVD can return OK (yay!), DECODER_ERROR_NOT_SUPPORTED (HW
>> doesn't
>> > support
>> > > specific profile/codec), and PIPELINE_ERROR_DECODE (oh crap, something
>>
> went
>
>> > > wrong; invalid demuxer config, missing stream, etc.).
>> >
>> > Should VideoDecoders have their own status codes / should they be
>> expanded
>>
> to
>
>> > include the problems you listed here?
>>
>
>  My rule of thumb is that if you're not going to handle a status code
>> specifically, it shouldn't exist.
>> So, no, I don't think we should make these codes more granular than they
>>
> already
>
>> are.
>>
>
> What about the first part of my question?
>
>
http://codereview.chromium.**org/10824283/<http://codereview.chromium.org/108...
>


The same answer applies: since we wouldn't handle decoder-specific status
codes in a different way, there's no point to adding them.

[scherkus (not reviewing), 2012-08-15 01:03:43]

CL typo: valeus -> values

LGTM w/ mini-rant!

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

http://codereview.chromium.org/10824283/diff/1/media/base/pipeline_status.h#n...
media/base/pipeline_status.h:15: // Logged to UMA, so never reuse a value,
always add new/greater ones!
On 2012/08/14 21:43:28, Ami Fischman wrote:
> On 2012/08/14 21:40:30, scherkus wrote:
> > On 2012/08/13 21:23:54, Ami Fischman wrote:
> > > On 2012/08/13 21:08:59, scherkus wrote:
> > > > is this only logged for GpuVideoDecoder::Initialize()?
> > > 
> > > Today, yes.
> > > 
> > > > how many values are actually used?
> > > 
> > > Today GVD can return OK (yay!), DECODER_ERROR_NOT_SUPPORTED (HW doesn't
> > support
> > > specific profile/codec), and PIPELINE_ERROR_DECODE (oh crap, something
went
> > > wrong; invalid demuxer config, missing stream, etc.).
> > 
> > Should VideoDecoders have their own status codes / should they be expanded
to
> > include the problems you listed here?
> 
> My rule of thumb is that if you're not going to handle a status code
> specifically, it shouldn't exist.
> So, no, I don't think we should make these codes more granular than they
already
> are.

Think about it from WMPI's perspective and http://crbug.com/126070

Today Pipeline converts all VideoRenderer initialization error codes into
PIPELINE_ERROR_COULD_NOT_RENDER, so DECODER_ERROR_NOT_SUPPORTED seems ripe for
removal / moved into a VideoDecoder-specific set of enums that VideoRendererBase
knows to handle, reducing the # of errors that Pipeline/WMPI need to concern
themselves with

My general comment is that we seem to use PipelineStatus[CB] more out of
convenience than anything else -- should we change that?

[Ami GONE FROM CHROMIUM, 2012-08-15 03:53:30]

>
> My general comment is that we seem to use PipelineStatus[CB] more out of
> convenience than anything else -- should we change that?
>

Yes, yes we should.
But that's a separate job from this CL, which seeks to document values
we've already been sending to UMA.
When we address that bug we'll need to preserve the specific numeric values
used now as we break the one enum into smaller enums, which is all the more
reason to document them now.

[commit-bot: I haz the power, 2012-08-15 03:54:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/10824283/1

[scherkus (not reviewing), 2012-08-15 04:13:12]

On 2012/08/15 03:53:30, Ami Fischman wrote:
> >
> > My general comment is that we seem to use PipelineStatus[CB] more out of
> > convenience than anything else -- should we change that?
> >
> 
> Yes, yes we should.
> But that's a separate job from this CL, which seeks to document values
> we've already been sending to UMA.
> When we address that bug we'll need to preserve the specific numeric values
> used now as we break the one enum into smaller enums, which is all the more
> reason to document them now.

Yeah that's fine -- wanted to make sure we're on the same long-term page

[commit-bot: I haz the power, 2012-08-15 06:22:20]

Change committed as 151667