Update DM to allow Src's to have optional options.

Update DM to allow Src's to have optional options.

Committed: https://skia.googlesource.com/skia/+/54416de52381ac11dd755100a8025debf595873a

[djsollen, 2015-04-02 17:23:45]

djsollen@google.com changed reviewers:
+ mtklein@google.com

[djsollen, 2015-04-02 17:25:07]

thoughts?

https://codereview.chromium.org/1059513002/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1059513002/diff/1/dm/DM.cpp#newcode138
dm/DM.cpp:138: const char* options;
I'm thinking of storing an SkString on this struct that contains <tag>-<options>
so that when we want to print or check the blacklist we can use that field.

[mtklein, 2015-04-02 17:48:09]

Let's also update dm_flags.py in this CL so we don't get confused.  Lots of
lines to delete now that the source is uniformly "image".

https://codereview.chromium.org/1059513002/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1059513002/diff/1/dm/DM.cpp#newcode138
dm/DM.cpp:138: const char* options;
On 2015/04/02 17:25:07, djsollen wrote:
> I'm thinking of storing an SkString on this struct that contains
<tag>-<options>
> so that when we want to print or check the blacklist we can use that field.

Let's leave them apart for now, given that we're not doing that yet.

https://codereview.chromium.org/1059513002/diff/1/dm/DM.cpp#newcode146
dm/DM.cpp:146: static void push_src(const char* tag, Src* s, const char* options
= nullptr) {
Let's pass this as the second argument and explicitly pass "" for everyone else.

https://codereview.chromium.org/1059513002/diff/1/dm/DM.cpp#newcode199
dm/DM.cpp:199: push_src("image", new ImageSrc(path), "image");     // Decode
entire image
Might want this "image" to be "default" so we don't get confused about
image/image?

[mtklein, 2015-04-02 17:51:38]

https://codereview.chromium.org/1059513002/diff/1/dm/DMJsonWriter.cpp
File dm/DMJsonWriter.cpp (right):

https://codereview.chromium.org/1059513002/diff/1/dm/DMJsonWriter.cpp#newcode58
dm/DMJsonWriter.cpp:58: result["options"]["source_options"] =
gBitmapResults[i].sourceOptions.c_str();
This has to be part of key or gold will explode.

[djsollen, 2015-04-02 20:03:23]

comments addressed, but it still feels a little dirty to expose an options field
that only effects some sources.

[mtklein, 2015-04-02 20:11:59]

Better run
 $ python tools/dm_flags.py test
to see if this makes the command lines we expect.

You may need to
 $ apt-get install python-coverage
first.

https://codereview.chromium.org/1059513002/diff/40001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1059513002/diff/40001/dm/DM.cpp#newcode29
dm/DM.cpp:29: DEFINE_string(srcOptions, "decode subset codec scanline",
Seems like we could skip this.  You're not planning on having bots change this,
are you?

https://codereview.chromium.org/1059513002/diff/40001/dm/DM.cpp#newcode491
dm/DM.cpp:491: if (strcmp(task.src.options, "") == 0) {
!= ?

[djsollen, 2015-04-02 20:24:15]

https://codereview.chromium.org/1059513002/diff/40001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1059513002/diff/40001/dm/DM.cpp#newcode29
dm/DM.cpp:29: DEFINE_string(srcOptions, "decode subset codec scanline",
On 2015/04/02 20:11:58, mtklein wrote:
> Seems like we could skip this.  You're not planning on having bots change
this,
> are you?

Done.

https://codereview.chromium.org/1059513002/diff/40001/dm/DM.cpp#newcode491
dm/DM.cpp:491: if (strcmp(task.src.options, "") == 0) {
On 2015/04/02 20:11:58, mtklein wrote:
> != ?

Done.

[mtklein, 2015-04-02 20:38:13]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-04-02 20:38:14]

lgtm

[commit-bot: I haz the power, 2015-04-02 20:39:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059513002/60001

[commit-bot: I haz the power, 2015-04-02 20:39:05]

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

[commit-bot: I haz the power, 2015-04-02 20:39:06]

Failed to apply patch for tools/dm_flags.py:
While running git apply --index -3 -p1;
  error: patch failed: tools/dm_flags.py:55
  Falling back to three-way merge...
  Applied patch to 'tools/dm_flags.py' with conflicts.
  U tools/dm_flags.py

Patch:       tools/dm_flags.py
Index: tools/dm_flags.py
diff --git a/tools/dm_flags.py b/tools/dm_flags.py
index
8b20540e39cd42f768c1745030c1e3b5fbc7fa6b..37e2e309f5cfbbdcc70bc51f14a5c35a4adf4727
100755
--- a/tools/dm_flags.py
+++ b/tools/dm_flags.py
@@ -55,54 +55,55 @@ def get_args(bot):
 
   blacklist = []
   # This image is too large to be a texture for many GPUs.
-  blacklist.extend('gpu _ PANO_20121023_214540.jpg'.split(' '))
-  blacklist.extend('msaa _ PANO_20121023_214540.jpg'.split(' '))
+  blacklist.extend('gpu _ _ PANO_20121023_214540.jpg'.split(' '))
+  blacklist.extend('msaa _ _ PANO_20121023_214540.jpg'.split(' '))
 
   # Several of the newest version bmps fail on SkImageDecoder
-  blacklist.extend('_ image pal8os2v2.bmp'.split(' '))
-  blacklist.extend('_ image pal8v4.bmp'.split(' '))
-  blacklist.extend('_ image pal8v5.bmp'.split(' '))
-  blacklist.extend('_ image rgb16-565.bmp'.split(' '))
-  blacklist.extend('_ image rgb16-565pal.bmp'.split(' '))
-  blacklist.extend('_ image rgb32-111110.bmp'.split(' '))
-  blacklist.extend('_ image rgb32bf.bmp'.split(' '))
-  blacklist.extend('_ image rgba32.bmp'.split(' '))
-  blacklist.extend('_ image rgba32abf.bmp'.split(' '))
-  blacklist.extend('_ image rgb24largepal.bmp'.split(' '))
-  blacklist.extend('_ image pal8os2v2-16.bmp'.split(' '))
-  blacklist.extend('_ image pal8oversizepal.bmp'.split(' '))
-  blacklist.extend('_ subset rgb24largepal.bmp'.split(' '))
-  blacklist.extend('_ subset pal8os2v2-16.bmp'.split(' '))
-  blacklist.extend('_ subset pal8oversizepal.bmp'.split(' '))
+  blacklist.extend('_ image decode pal8os2v2.bmp'.split(' '))
+  blacklist.extend('_ image decode pal8v4.bmp'.split(' '))
+  blacklist.extend('_ image decode pal8v5.bmp'.split(' '))
+  blacklist.extend('_ image decode rgb16-565.bmp'.split(' '))
+  blacklist.extend('_ image decode rgb16-565pal.bmp'.split(' '))
+  blacklist.extend('_ image decode rgb32-111110.bmp'.split(' '))
+  blacklist.extend('_ image decode rgb32bf.bmp'.split(' '))
+  blacklist.extend('_ image decode rgba32.bmp'.split(' '))
+  blacklist.extend('_ image decode rgba32abf.bmp'.split(' '))
+  blacklist.extend('_ image decode rgb24largepal.bmp'.split(' '))
+  blacklist.extend('_ image decode pal8os2v2-16.bmp'.split(' '))
+  blacklist.extend('_ image decode pal8oversizepal.bmp'.split(' '))
+  blacklist.extend('_ image subset rgb24largepal.bmp'.split(' '))
+  blacklist.extend('_ image subset pal8os2v2-16.bmp'.split(' '))
+  blacklist.extend('_ image subset pal8oversizepal.bmp'.split(' '))
 
   # New ico files that fail on SkImageDecoder
-  blacklist.extend('_ image Hopstarter-Mac-Folders-Apple.ico'.split(' '))
+  blacklist.extend('_ image decode Hopstarter-Mac-Folders-Apple.ico'.split('
'))
 
   # Leon doesn't care about this, so why run it?
   if 'Win' in bot:
-    blacklist.extend('_ image _'.split(' '))
-    blacklist.extend('_ subset _'.split(' '))
+    blacklist.extend('_ image decode _'.split(' '))
+    blacklist.extend('_ image subset _'.split(' '))
 
   # Certain gm's on win7 gpu and pdf are never finishing and keeping the test
   # running forever
   if 'Win7' in bot:
-    blacklist.extend('msaa16 gm colorwheelnative'.split(' '))
-    blacklist.extend('pdf gm fontmgr_iter_factory'.split(' '))
+    blacklist.extend('msaa16 gm _ colorwheelnative'.split(' '))
+    blacklist.extend('pdf gm _ fontmgr_iter_factory'.split(' '))
 
   # Drawing SKPs or images into GPU canvases is a New Thing.
   # It seems like we're running out of RAM on some Android bots, so start off
   # with a very wide blacklist disabling all these tests on all Android bots.
   if 'Android' in bot:  # skia:3255
-    blacklist.extend('gpu skp _ gpu image _ gpu subset _'.split(' '))
-    blacklist.extend('msaa skp _ msaa image _ gpu subset _'.split(' '))
+    blacklist.extend('gpu skp _ _ msaa skp _ _'.split(' '))
+    blacklist.extend('gpu image decode _ msaa image decode _'.split(' '))
+    blacklist.extend('gpu image subset _ msaa image subset _'.split(' '))
 
   if 'Valgrind' in bot:
     # PDF + .webp -> jumps depending on uninitialized memory.  skia:3505
-    blacklist.extend('pdf _ .webp'.split(' '))
+    blacklist.extend('pdf _ _ .webp'.split(' '))
     # These take 18+ hours to run.
-    blacklist.extend('pdf gm fontmgr_iter'.split(' '))
-    blacklist.extend('pdf _ PANO_20121023_214540.jpg'.split(' '))
-    blacklist.extend('pdf skp tabl_worldjournal.skp'.split(' '))
+    blacklist.extend('pdf gm _ fontmgr_iter'.split(' '))
+    blacklist.extend('pdf _ _ PANO_20121023_214540.jpg'.split(' '))
+    blacklist.extend('pdf skp _ tabl_worldjournal.skp'.split(' '))
 
   if blacklist:
     args.append('--blacklist')

[djsollen, 2015-04-03 12:25:27]

The CQ bit was checked by djsollen@google.com

[djsollen, 2015-04-03 12:25:27]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1059513002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-03 12:25:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059513002/80001

[commit-bot: I haz the power, 2015-04-03 12:33:07]

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

[commit-bot: I haz the power, 2015-04-03 12:33:08]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[djsollen, 2015-04-03 13:10:44]

djsollen@google.com changed reviewers:
+ scroggo@google.com

[djsollen, 2015-04-03 14:18:35]

The CQ bit was checked by djsollen@google.com

[djsollen, 2015-04-03 14:18:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1059513002/#ps100001
(title: "blacklist bug")

[commit-bot: I haz the power, 2015-04-03 14:18:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059513002/100001

[scroggo, 2015-04-03 14:23:14]

On 2015/04/03 14:18:49, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1059513002/100001

LGTM

[commit-bot: I haz the power, 2015-04-03 14:24:53]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/54416de52381ac11dd755100a8025debf595873a

[jcgregorio, 2015-04-05 14:29:31]

On 2015/04/03 at 14:24:53, commit-bot wrote:
> Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/54416de52381ac11dd755100a8025debf595873a

source_options should have a default value, something like "none", otherwise the
value is just the empty string, like you can see here:


https://pantheon.corp.google.com/project/31977622648/storage/browser/chromium...

That's problematic because it means in Gold you can never search for that
condition.

[mtklein, 2015-04-06 12:17:56]

On 2015/04/05 14:29:31, jcgregorio wrote:
> On 2015/04/03 at 14:24:53, commit-bot wrote:
> > Committed patchset #6 (id:100001) as
> https://skia.googlesource.com/skia/+/54416de52381ac11dd755100a8025debf595873a
> 
> source_options should have a default value, something like "none", otherwise
the
> value is just the empty string, like you can see here:
> 
> 
>
https://pantheon.corp.google.com/project/31977622648/storage/browser/chromium...
> 
> That's problematic because it means in Gold you can never search for that
> condition.

Should be fine for now.  We provide explicit source_options for all the source
types we ever set options for, just "image" split into "default", "subset",
"scanlines", "codec" I think.

Think we can show empty fields as <empty> or "" in the UI?  Seems easy to forget
to fill a field.

[jcgregorio, 2015-04-06 12:48:07]

On 2015/04/06 at 12:17:56, mtklein wrote:
> On 2015/04/05 14:29:31, jcgregorio wrote:
> > On 2015/04/03 at 14:24:53, commit-bot wrote:
> > > Committed patchset #6 (id:100001) as
> >
https://skia.googlesource.com/skia/+/54416de52381ac11dd755100a8025debf595873a
> > 
> > source_options should have a default value, something like "none", otherwise
the
> > value is just the empty string, like you can see here:
> > 
> > 
> >
https://pantheon.corp.google.com/project/31977622648/storage/browser/chromium...
> > 
> > That's problematic because it means in Gold you can never search for that
> > condition.
> 
> Should be fine for now.  We provide explicit source_options for all the source
types we ever set options for, just "image" split into "default", "subset",
"scanlines", "codec" I think.


The file I linked has all empty source_options.

> 
> Think we can show empty fields as <empty> or "" in the UI?  Seems easy to
forget to fill a field.

I'd rather we have good data going into the system rather than trying to fix it
up in perf and gold. Let's pick a good name for what source_options should be if
it isn't set.

[mtklein, 2015-04-06 12:53:14]

On 2015/04/06 12:48:07, jcgregorio wrote:
> On 2015/04/06 at 12:17:56, mtklein wrote:
> > On 2015/04/05 14:29:31, jcgregorio wrote:
> > > On 2015/04/03 at 14:24:53, commit-bot wrote:
> > > > Committed patchset #6 (id:100001) as
> > >
> https://skia.googlesource.com/skia/+/54416de52381ac11dd755100a8025debf595873a
> > > 
> > > source_options should have a default value, something like "none",
otherwise
> the
> > > value is just the empty string, like you can see here:
> > > 
> > > 
> > >
>
https://pantheon.corp.google.com/project/31977622648/storage/browser/chromium...
> > > 
> > > That's problematic because it means in Gold you can never search for that
> > > condition.
> > 
> > Should be fine for now.  We provide explicit source_options for all the
source
> types we ever set options for, just "image" split into "default", "subset",
> "scanlines", "codec" I think.
> 
> 
> The file I linked has all empty source_options.

Right... that file does not contain any image config results.

> 
> > 
> > Think we can show empty fields as <empty> or "" in the UI?  Seems easy to
> forget to fill a field.
> 
> I'd rather we have good data going into the system rather than trying to fix
it
> up in perf and gold. Let's pick a good name for what source_options should be
if
> it isn't set.

It's options.  Being empty should be an option.  Optional...

[mtklein, 2015-04-06 12:54:51]

> Right... that file does not contain any image config results.

Err, sorry, was looking for the wrong thing.  Here's an image result with
source_options set:


  {
         "key" : {
            "config" : "8888",
            "name" : "02_empty_filter.JPG",
            "source_options" : "decode",
            "source_type" : "image"
         },
         "md5" : "cf406b892536e434d49a86b05f05da14",
         "options" : {
            "ext" : "png"
         }
      },

[mtklein, 2015-04-06 12:55:55]

What do you think about not putting in in the file at all if empty?

[jcgregorio, 2015-04-06 13:07:07]

On 2015/04/06 at 12:55:55, mtklein wrote:
> What do you think about not putting in in the file at all if empty?

That's what we normally do with things in "options", but source_options is part
of the key.

I actually found this issue because keys for these traces look weird, for
example:

  Arm7:GCC:8888:Debug:CPU:NEON:Daisy:aarectmodes:ChromeOS::gm

Note the "::gm", the source_options goes between those last two colons.

[mtklein, 2015-04-06 13:12:22]

On 2015/04/06 13:07:07, jcgregorio wrote:
> On 2015/04/06 at 12:55:55, mtklein wrote:
> > What do you think about not putting in in the file at all if empty?
> 
> That's what we normally do with things in "options", but source_options is
part
> of the key.
> 
> I actually found this issue because keys for these traces look weird, for
> example:
> 
>   Arm7:GCC:8888:Debug:CPU:NEON:Daisy:aarectmodes:ChromeOS::gm
> 
> Note the "::gm", the source_options goes between those last two colons.

Not all things have to have the same shape key, right?  Can't we leave out the
entire `"source_options": "",` pair when source_options don't apply?

[jcgregorio, 2015-04-06 13:17:06]

On 2015/04/06 at 13:12:22, mtklein wrote:
> On 2015/04/06 13:07:07, jcgregorio wrote:
> > On 2015/04/06 at 12:55:55, mtklein wrote:
> > > What do you think about not putting in in the file at all if empty?
> > 
> > That's what we normally do with things in "options", but source_options is
part
> > of the key.
> > 
> > I actually found this issue because keys for these traces look weird, for
> > example:
> > 
> >   Arm7:GCC:8888:Debug:CPU:NEON:Daisy:aarectmodes:ChromeOS::gm
> > 
> > Note the "::gm", the source_options goes between those last two colons.
> 
> Not all things have to have the same shape key, right?  Can't we leave out the
entire `"source_options": "",` pair when source_options don't apply?

No. 

Simply pick a word for the value of source_options when no options are set.

[jcgregorio, 2015-04-06 13:21:47]

On 2015/04/06 at 13:17:06, jcgregorio wrote:
> On 2015/04/06 at 13:12:22, mtklein wrote:
> > On 2015/04/06 13:07:07, jcgregorio wrote:
> > > On 2015/04/06 at 12:55:55, mtklein wrote:
> > > > What do you think about not putting in in the file at all if empty?
> > > 
> > > That's what we normally do with things in "options", but source_options is
part
> > > of the key.
> > > 
> > > I actually found this issue because keys for these traces look weird, for
> > > example:
> > > 
> > >   Arm7:GCC:8888:Debug:CPU:NEON:Daisy:aarectmodes:ChromeOS::gm
> > > 
> > > Note the "::gm", the source_options goes between those last two colons.
> > 
> > Not all things have to have the same shape key, right?  Can't we leave out
the entire `"source_options": "",` pair when source_options don't apply?
> 
> No. 
> 
> Simply pick a word for the value of source_options when no options are set.

OK, just talked to Derek, and learned that source_options only appears for
things in the image corpus? If that's correct then yes, just drop source_options
from the JSON if it is the empty string.