Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1054593006:
Adding non-deterministic images to blacklist (Closed)
DescriptionAdding non-deterministic images to blacklist
BUG=skia:3653
Committed: https://skia.googlesource.com/skia/+/2f273a1ed098fbd7a8af86b349dbd247de609a2a
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase and merge #Messages
Total messages: 17 (5 generated)
msarett@google.com changed reviewers: + djsollen@google.com
This should make triaging less repetitive. I do have a question about how to use the blacklist: It seems that the blacklist should certainly be used for images that fail to decode or produce non-deterministic outputs. I have also previously added to the blacklist images that decode "wrong" on SkImageDecoder. Also, clearly I have missed some images that fall into this category. So now we are in a situation where some images that decode "wrong" on SkImageDecoder are blacklisted and others are marked as negative in the triaging process. Should we be consistent on where these belong? Any thoughts on where they do belong?
msarett@google.com changed reviewers: + scroggo@google.com
Adding Leon to the conversation
https://codereview.chromium.org/1054593006/diff/1/tools/dm_flags.py File tools/dm_flags.py (right): https://codereview.chromium.org/1054593006/diff/1/tools/dm_flags.py#newcode74 tools/dm_flags.py:74: blacklist.extend('_ image pal4rletrns.bmp'.split(' ')) you'll need to rebase and after rebaseing this line should look like... _ image decode pal4rletrns.bmp
https://codereview.chromium.org/1054593006/diff/1/tools/dm_flags.py File tools/dm_flags.py (right): https://codereview.chromium.org/1054593006/diff/1/tools/dm_flags.py#newcode74 tools/dm_flags.py:74: blacklist.extend('_ image pal4rletrns.bmp'.split(' ')) On 2015/04/03 18:33:03, djsollen wrote: > you'll need to rebase and after rebaseing this line should look like... > > _ image decode pal4rletrns.bmp Gotcha
On 2015/04/03 18:24:50, msarett wrote: > This should make triaging less repetitive. > > I do have a question about how to use the blacklist: > > It seems that the blacklist should certainly be used for images that fail to > decode or produce non-deterministic outputs. > > I have also previously added to the blacklist images that decode "wrong" on > SkImageDecoder. Also, clearly I have missed some images that fall into this > category. So now we are in a situation where some images that decode "wrong" on > SkImageDecoder are blacklisted and others are marked as negative in the triaging > process. Should we be consistent on where these belong? Any thoughts on where > they do belong? I tend to think that if SkImageDecoder decodes something incorrectly, we should just mark it as Negative in Gold. Non-deterministic decodes are a special evil though - they'll keep generating new images for us to triage. So to prevent that, blacklisting seems reasonable. I do not think it's worth it to untriage images which you already triaged as Negative (after all they are wrong). I do think you should file a bug for the bad images, though. We don't necessarily have to fix it, but it will be nice to track it as long as we are still supporting SkImageDecoder - maybe we'll simply mark as fixed/obsolete once we delete SkImageDecoder, but I would rather do that than just assume we do not need to fix it.
I added it to the gold ignores list for now. so this CL may not be needed. https://gold.skia.org/2/ignores
On 2015/04/03 20:00:31, djsollen wrote: > I added it to the gold ignores list for now. so this CL may not be needed. > > https://gold.skia.org/2/ignores Landing this to make Valgrind bot happier. See skbug.com/3653
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1054593006/20001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/04/06 15:31:53, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. LGTM
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1054593006/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/2f273a1ed098fbd7a8af86b349dbd247de609a2a |
