gm: change ErrorBitfield to ErrorType/ErrorCombination

gm: change ErrorBitfield to ErrorType/ErrorCombination
This will enable future GM changes to report errors more completely/consistently, like skdiff.
Committed: https://code.google.com/p/skia/source/detail?r=8344

[epoger, 2013-03-21 17:56:27]

https://codereview.chromium.org/12992003/diff/1/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/12992003/diff/1/gm/gm_error.h#newcode14
gm/gm_error.h:14: typedef int ErrorBitfield;
patchset 1: pull ErrorBitfield definitions into new gm_errors.h file.  No
behavioral changes.

[epoger, 2013-03-21 20:40:17]

https://codereview.chromium.org/12992003/diff/9001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/12992003/diff/9001/gm/gm_error.h#newcode14
gm/gm_error.h:14: /**
patchsets 2+3: instead of an ErrorBitfield (alias for integer) that was less
strictly structured, move to a C++ enumeration of error types (ErrorType) and a
class that can be used to hold a list of error types (ErrorCombination).

https://codereview.chromium.org/12992003/diff/9001/gm/gm_error.h#newcode22
gm/gm_error.h:22: kLast_ErrorType = kWritingReferenceImage_ErrorType
In a future CL, this will enable us to:
1. create a separate fFailedTest array (in gmmain) for each ErrorType
2. programmatically report the totals for each ErrorType, with a loop from 0 to
kLast_ErrorType

https://codereview.chromium.org/12992003/diff/9001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/9001/gm/gmmain.cpp#newcode259
gm/gmmain.cpp:259: rec.fIsPixelError =
errorCombination.includes(kImageMismatch_ErrorType);
I think this syntax is clearer and less error-prone than the old one.

https://codereview.chromium.org/12992003/diff/9001/gm/gmmain.cpp#newcode1408
gm/gmmain.cpp:1408: if (FLAGS_gpuCacheSize.count() > 0) {
I had to move this flag parsing code out of the functions that return
ErrorCombination... previously, they had been returning -1, which wasn't a legal
ErrorBitfield value!  But the old "typedef int" approach didn't catch that,
while the new classes do.

[epoger, 2013-03-21 20:44:55]

Ready for review at patchset 4... but I suggest comparing patchsets 1 and 3 when
doing the review.  That gets to the "meat" of the change.

[epoger, 2013-03-21 21:56:54]

I sent this CL to the Skia_Linux_NoGPU bot (specifying -r 8310) to check for
warnings-as-errors problems, and I was surprised to see an error in
GenerateGMs...

http://108.170.217.252:10117/builders/Skia_Linux_NoGPU_Debug_32_Trybot/builds...
GM: ---- image-surface_8888-deferred: 26128 (of 400000) differing pixels, max
per-channel mismatch R=255 G=255 B=255 A=0
GM: ---- image-surface_565-deferred: not computing max per-channel pixel
mismatch because non-8888

I fired off another instance of that trybot, to see if it was a flake, but it
happened again!

http://108.170.217.252:10117/builders/Skia_Linux_NoGPU_Debug_32_Trybot/builds...
GM: ---- image-surface_8888-deferred: 26128 (of 400000) differing pixels, max
per-channel mismatch R=255 G=255 B=255 A=0
GM: ---- image-surface_565-deferred: not computing max per-channel pixel
mismatch because non-8888

Here's the waterfall bot running as of r8310, without such an error:
http://108.170.217.252:10117/builders/Skia_Linux_NoGPU_Debug_32/builds/1059

Soon, I will try running that test locally and see if I can reproduce the
error... I am really surprised that my change would affect that one test
specifically.

[epoger, 2013-03-22 01:59:20]

On 2013/03/21 21:56:54, epoger wrote:
> Soon, I will try running that test locally and see if I can reproduce the
> error... I am really surprised that my change would affect that one test
> specifically.

As surprising as that is, it really does appear to stem from my change.  I have
checked out another clean tree at r8310, and if I run the following test it
passes in that workspace:

epoger@wpgntat-ubiq141:~/src/skia/blue/trunk$ out/Debug/gm --match surface
--config 8888 ; echo $?
Non-default runtime configuration options:
GM: drawing... image-surface [800 500]
GM: Ran 1 tests: 1 passed, 0 failed, 1 missing reference images
  Leaked SkRefCnt: 4
     Leaked SkWeakRefCnt: 1
         Leaked SkTypeface: 1
     Leaked SkFlattenable: 1
         Leaked SkData: 1
     Leaked SkPathRef: 1
     Leaked ???: 1
0

but in my workspace with this CL, it fails:

epoger@wpgntat-ubiq141:~/src/skia/white/trunk$ out/Debug/gm --match surface
--config 8888 ; echo $?
Non-default runtime configuration options:
GM: drawing... image-surface [800 500]
GM: ---- image-surface_8888-deferred: 26128 (of 400000) differing pixels, max
per-channel mismatch R=255 G=255 B=255 A=0
GM: Ran 1 tests: 0 passed, 1 failed, 1 missing reference images
GM: 		image-surface_8888_-deferred pixel_error
  Leaked SkRefCnt: 4
     Leaked SkWeakRefCnt: 1
         Leaked SkTypeface: 1
     Leaked SkFlattenable: 1
         Leaked SkData: 1
     Leaked SkPathRef: 1
     Leaked ???: 1
255

Investigating...

[epoger, 2013-03-22 02:39:44]

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode875
gm/gmmain.cpp:875: if (errors.isEmpty()) {
Yikes, this "fixes" the "problem" revealed by the trybots... more accurately,
this preserves the earlier programming error that has been hiding -deferred
drawing errors for who-knows-how-long!!!

Before this CL, this code read:
            if (!generate_image(gm, gRec, context, rt, &bitmap, true)) {
                return kEmptyErrorBitfield;
            }

In other words... if generate_image() returned kEmptyErrorBitfield, then exit
early and report success (without comparing the generated image against
referenceBitmap)!

I will send this to a couple of trybots... I think they will succeed now.  By
still hiding these failures. :-(

[epoger, 2013-03-22 02:42:10]

On 2013/03/22 02:39:44, epoger wrote:
> Yikes, this "fixes" the "problem" revealed by the trybots... more accurately,
> this preserves the earlier programming error that has been hiding -deferred
> drawing errors for who-knows-how-long!!!

P.S. I think this is an excellent argument in favor of test-driven development,
especially for tools that are used to test OTHER code.

[epoger, 2013-03-22 03:12:29]

Trybots are looking good so far.  PTAL at patchset 5.

[borenet, 2013-03-22 13:53:29]

I like it!  Hiding the bitwise operations is great for readability.  Just a few
comments.

https://codereview.chromium.org/12992003/diff/21001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gm_error.h#newcode18
gm/gm_error.h:18: kNoGpuContext_ErrorType,
Should this be wrapped in a #ifdef SK_SUPPORT_GPU?

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode395
gm/gmmain.cpp:395: return kNoGpuContext_ErrorType;
Should this be an instance of ErrorCombination, rather than an enum?

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode875
gm/gmmain.cpp:875: if (errors.isEmpty()) {
On 2013/03/22 02:39:44, epoger wrote:
> Yikes, this "fixes" the "problem" revealed by the trybots... more accurately,
> this preserves the earlier programming error that has been hiding -deferred
> drawing errors for who-knows-how-long!!!
> 
> Before this CL, this code read:
>             if (!generate_image(gm, gRec, context, rt, &bitmap, true)) {
>                 return kEmptyErrorBitfield;
>             }
> 
> In other words... if generate_image() returned kEmptyErrorBitfield, then exit
> early and report success (without comparing the generated image against
> referenceBitmap)!
> 
> I will send this to a couple of trybots... I think they will succeed now.  By
> still hiding these failures. :-(

File bugs!

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode1147
gm/gmmain.cpp:1147: int gpuCacheSizeCount);
This was previously hidden behind SK_SUPPORT_GPU.  How much do we care about
keeping it that way?

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode1413
gm/gmmain.cpp:1413: int gpuCacheSizeCount = -1; // -1 means use the default
To me, it feels like these are "escaping" their ifdef prison.  Maybe it's not a
bad thing, and they should run free, but I think it's worth talking about (and
verifying that there are no problems on the NoGPU bot).

[epoger, 2013-03-22 14:31:23]

Thanks, Eric!

https://codereview.chromium.org/12992003/diff/21001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gm_error.h#newcode18
gm/gm_error.h:18: kNoGpuContext_ErrorType,
On 2013/03/22 13:53:29, borenet wrote:
> Should this be wrapped in a #ifdef SK_SUPPORT_GPU?

Done.

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode395
gm/gmmain.cpp:395: return kNoGpuContext_ErrorType;
On 2013/03/22 13:53:29, borenet wrote:
> Should this be an instance of ErrorCombination, rather than an enum?

The compiler knows how to automatically convert it, thanks to the existence of
ErrorCombination::ErrorCombination(ErrorType).

If you prefer, I could make it explicit:
return ErrorCombination(kNoGpuContext_ErrorType)

Let me know if you'd like me to do that.

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode875
gm/gmmain.cpp:875: if (errors.isEmpty()) {
On 2013/03/22 13:53:29, borenet wrote:
> On 2013/03/22 02:39:44, epoger wrote:
> > Yikes, this "fixes" the "problem" revealed by the trybots... more
accurately,
> > this preserves the earlier programming error that has been hiding -deferred
> > drawing errors for who-knows-how-long!!!
> > 
> > Before this CL, this code read:
> >             if (!generate_image(gm, gRec, context, rt, &bitmap, true)) {
> >                 return kEmptyErrorBitfield;
> >             }
> > 
> > In other words... if generate_image() returned kEmptyErrorBitfield, then
exit
> > early and report success (without comparing the generated image against
> > referenceBitmap)!
> > 
> > I will send this to a couple of trybots... I think they will succeed now. 
By
> > still hiding these failures. :-(
> 
> File bugs!

Filed... I will update that bug, and assign it, once this CL is committed.

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode1147
gm/gmmain.cpp:1147: int gpuCacheSizeCount);
On 2013/03/22 13:53:29, borenet wrote:
> This was previously hidden behind SK_SUPPORT_GPU.  How much do we care about
> keeping it that way?

The problem: now that these values are passed as a function parameter, we would
have to put an #ifdef around part of the function declaration to keep them
hidden.  I'm leery of changing the signature of a function based on preprocessor
directives, but if you think that's the right approach I can do it.

I think it's cleaner to parse FLAGS_gpuCacheSize outside of this function, for 2
reasons:
1. I think it's good to validate user flags as early as possible
2. If the user flag validation fails within this function, we need to return
some new ErrorType (kFlagValidation_ErrorType ?)

One approach could be: I create a new GpuCacheDescription class that is used as
the parameter here (instead of the two integers).  If GPU is not supported,
GpuCacheDescription is defined as some class with all private accessors (so the
compiler will barf if the code tries to read it on non-GPU builds).

Should I try that approach, or do you have some other suggestion?

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode1413
gm/gmmain.cpp:1413: int gpuCacheSizeCount = -1; // -1 means use the default
On 2013/03/22 13:53:29, borenet wrote:
> To me, it feels like these are "escaping" their ifdef prison.  Maybe it's not
a
> bad thing, and they should run free, but I think it's worth talking about (and
> verifying that there are no problems on the NoGPU bot).

There's plenty not to like here, including the magical -1 values which I copied
from the existing code. :-(

In general, I think it's best to have as few #ifdefs in the code as possible
(for increased maintainability).  But to date, yes, we have used #ifdefs
liberally so that the gpuCacheSize code is completely turned off on non-GPU
builds.  I'm not sure how to balance out those competing interests.

Here's my proposal: let's see what we can figure out for the
run_multiple_configs() parameters, and then I will put as much as possible of
the gpuCacheSize code back in its prison.

[epoger, 2013-03-22 14:32:54]

On 2013/03/22 14:31:23, epoger wrote:
> Thanks, Eric!

(see diff between patchsets 5 and 6 for my responses to all of your comments,
and small changes made as part of that)

[borenet, 2013-03-22 14:37:12]

LGTM

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode395
gm/gmmain.cpp:395: return kNoGpuContext_ErrorType;
On 2013/03/22 14:31:23, epoger wrote:
> On 2013/03/22 13:53:29, borenet wrote:
> > Should this be an instance of ErrorCombination, rather than an enum?
> 
> The compiler knows how to automatically convert it, thanks to the existence of
> ErrorCombination::ErrorCombination(ErrorType).
> 
> If you prefer, I could make it explicit:
> return ErrorCombination(kNoGpuContext_ErrorType)
> 
> Let me know if you'd like me to do that.

That would be my personal preference, but I'll leave it up to you.

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode1147
gm/gmmain.cpp:1147: int gpuCacheSizeCount);
On 2013/03/22 14:31:23, epoger wrote:
> On 2013/03/22 13:53:29, borenet wrote:
> > This was previously hidden behind SK_SUPPORT_GPU.  How much do we care about
> > keeping it that way?
> 
> The problem: now that these values are passed as a function parameter, we
would
> have to put an #ifdef around part of the function declaration to keep them
> hidden.  I'm leery of changing the signature of a function based on
preprocessor
> directives, but if you think that's the right approach I can do it.
> 

That IS ugly.

> I think it's cleaner to parse FLAGS_gpuCacheSize outside of this function, for
2
> reasons:
> 1. I think it's good to validate user flags as early as possible
> 2. If the user flag validation fails within this function, we need to return
> some new ErrorType (kFlagValidation_ErrorType ?)
> 

Makes sense.

> One approach could be: I create a new GpuCacheDescription class that is used
as
> the parameter here (instead of the two integers).  If GPU is not supported,
> GpuCacheDescription is defined as some class with all private accessors (so
the
> compiler will barf if the code tries to read it on non-GPU builds).
> 
> Should I try that approach, or do you have some other suggestion?

The only other thing I can think of is to make those variables global, which is
arguably worse.  I'll leave it up to you.

[epoger, 2013-03-22 15:41:37]

Eric, PTAL at patchset 7 to make sure you're still on board.

In the meanwhile, I'll sync my workspace to HEAD to see what changes have gone
in over the past day!

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode395
gm/gmmain.cpp:395: return kNoGpuContext_ErrorType;
On 2013/03/22 14:37:12, borenet wrote:
> On 2013/03/22 14:31:23, epoger wrote:
> > On 2013/03/22 13:53:29, borenet wrote:
> > > Should this be an instance of ErrorCombination, rather than an enum?
> > 
> > The compiler knows how to automatically convert it, thanks to the existence
of
> > ErrorCombination::ErrorCombination(ErrorType).
> > 
> > If you prefer, I could make it explicit:
> > return ErrorCombination(kNoGpuContext_ErrorType)
> > 
> > Let me know if you'd like me to do that.
> 
> That would be my personal preference, but I'll leave it up to you.

Done here, and in one other place where it was also automatically being
converted.

https://codereview.chromium.org/12992003/diff/21001/gm/gmmain.cpp#newcode1147
gm/gmmain.cpp:1147: int gpuCacheSizeCount);
On 2013/03/22 14:37:12, borenet wrote:
> The only other thing I can think of is to make those variables global, which
is
> arguably worse.  I'll leave it up to you.

I went with globals, because creating the new GpuCacheDescription class was
gonna get more complicated.  And besides, the flag values are already global, so
it doesn't seem any worse than it was before.

https://codereview.chromium.org/12992003/diff/23003/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/23003/gm/gmmain.cpp#newcode56
gm/gmmain.cpp:56: #define DEFAULT_CACHE_VALUE -1
created macro token instead of magical -1 values throughout

https://codereview.chromium.org/12992003/diff/23003/gm/gmmain.cpp#newcode1036
gm/gmmain.cpp:1036: // Macro magic to convert a numeric preprocessor token into
a string.
Uh-oh, it's magic!

[bsalomon, 2013-03-22 16:03:23]

https://codereview.chromium.org/12992003/diff/21003/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21003/gm/gmmain.cpp#newcode398
gm/gmmain.cpp:398: return ErrorCombination(kNoGpuContext_ErrorType);
Drive-by: This is unreachable because of the loop at 1469 (on the left side)
which removes gpu configs that have no context or too high of a sample count.

[epoger, 2013-03-22 16:06:08]

https://codereview.chromium.org/12992003/diff/21003/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21003/gm/gmmain.cpp#newcode398
gm/gmmain.cpp:398: return ErrorCombination(kNoGpuContext_ErrorType);
On 2013/03/22 16:03:23, bsalomon wrote:
> Drive-by: This is unreachable because of the loop at 1469 (on the left side)
> which removes gpu configs that have no context or too high of a sample count.

Thanks.  I don't think that's a problem, do you?

And how on earth did you notice this???

[epoger, 2013-03-22 17:29:37]

Trybot results look good, so I'm gonna go ahead and commit this.  Eric, if you
have any lingering requests, I'll be happy to deal with them in a separate CL!

[epoger, 2013-03-22 17:29:50]

Committed patchset #8 manually as r8344 (presubmit successful).

[borenet, 2013-03-22 17:42:07]

On 2013/03/22 17:29:37, epoger wrote:
> Trybot results look good, so I'm gonna go ahead and commit this.  Eric, if you
> have any lingering requests, I'll be happy to deal with them in a separate CL!

I think this is fine!

[bsalomon, 2013-03-25 13:01:50]

https://codereview.chromium.org/12992003/diff/21003/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12992003/diff/21003/gm/gmmain.cpp#newcode398
gm/gmmain.cpp:398: return ErrorCombination(kNoGpuContext_ErrorType);
On 2013/03/22 16:06:09, epoger wrote:
> On 2013/03/22 16:03:23, bsalomon wrote:
> > Drive-by: This is unreachable because of the loop at 1469 (on the left side)
> > which removes gpu configs that have no context or too high of a sample
count.
> 
> Thanks.  I don't think that's a problem, do you?
> 
> And how on earth did you notice this???

Heh... I added the code at 1469 recently and just happened to notice this change
go by in the stream.

It doesn't seem like a problem to me.