Re-enable advanced blend with a blacklist

Committed: https://skia.googlesource.com/skia/+/1dd0542ca37cf1b4a2ab0ea41f8009ded097fd47

[Chris Dalton, 2015-05-29 02:48:48]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, mjk@nvidia.com

[Chris Dalton, 2015-05-29 02:48:48]

Hey guys, I'm uploading this but also wanted to open a discussion about how we
handle it.

One approach is to blacklist broken drivers inside of Skia (what this patch
does). It adds a lot of complexity, especially since an actual driver might not
be behind the interface.

Another approach could be that if Skia (the library) sees this extension, it
uses it. Period. Then Chrome (the app) is responsible to blacklist the extension
if it wants more robustness. They already manage a lot of black lists, so this
would just be one more thing.

I think I prefer #2 (so not what this patch does), but wanted feedback from you
guys as well. We could push a change to Chrome that completely blacklists
advanced blend, and then happily turn it back on inside of Skia. And once NVIDIA
drivers start rolling out with the necessary fixes, we can start letting them
back through the command buffer in Chrome.

Either way, the NVIDIA branching is extremely complex, so determining whether a
driver has a bugfix is unfortunately not as simple as testing the build number
against a threshold :-( We would need a mini-database listing all the broken
driver numbers.

[nv_mark, 2015-05-29 03:20:56]

lgtm

[bsalomon, 2015-05-29 13:53:20]

On 2015/05/29 02:48:48, Chris Dalton wrote:
> Hey guys, I'm uploading this but also wanted to open a discussion about how we
> handle it.
> 
> One approach is to blacklist broken drivers inside of Skia (what this patch
> does). It adds a lot of complexity, especially since an actual driver might
not
> be behind the interface.
> 
> Another approach could be that if Skia (the library) sees this extension, it
> uses it. Period. Then Chrome (the app) is responsible to blacklist the
extension
> if it wants more robustness. They already manage a lot of black lists, so this
> would just be one more thing.
> 
> I think I prefer #2 (so not what this patch does), but wanted feedback from
you
> guys as well. We could push a change to Chrome that completely blacklists
> advanced blend, and then happily turn it back on inside of Skia. And once
NVIDIA
> drivers start rolling out with the necessary fixes, we can start letting them
> back through the command buffer in Chrome.
> 
> Either way, the NVIDIA branching is extremely complex, so determining whether
a
> driver has a bugfix is unfortunately not as simple as testing the build number
> against a threshold :-( We would need a mini-database listing all the broken
> driver numbers.

Unfortunately, we have to do both. :( Skia is used outside of Chrome so we need
something for that use case. On the other hand, Chrome doesn't let Skia see the
real vendor/renderer GL strings so any switching we do based on them wont work
in that context. So I think we will need a Chromium blacklist before we can land
a Skia change that enables them.

WRT to the specifics of this change, could we change the
advancedBlendEquationSupport() function into a table lookup rather than adding
the two mode blacklist bool? It would be more extensible if we find other errors
with specific modes on other devices. Something like bool
blendEquationSupported(GrBlendEquation)?

This only applies if you want to pursue a partial blacklist on the Chromium
side: The GL extension string based blacklist won't provide the granularity to
blacklist specific modes. We could expose overrides in GrContextOptions as a
table and Chromium code populate it on a mode by mode basis.

[Chris Dalton, 2015-05-30 01:45:40]

I've verified quite extensively that this blacklist works and the non-coherent
images are all correct on 337.00+

[egdaniel, 2015-06-05 18:18:04]

Did you add some blacklist already in chrome to make sure this isn't used?

https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:234: if (kNVIDIA_GrGLDriver == ctxInfo.driver()) {
Outside of intel which I believe you had said had major bugs before, why not
turn on for other drivers? I think most worked before and I'd rather blacklist a
driver if it ends up being problematic. I think we could leave Dodge and Burn
blacklisted on all because I do remember those showing artifacts on others as
well (not surprising since the implementation in the original spec was
numerically unstable and I assume most just copied it). Thoughts?

[nv_mark, 2015-06-05 19:09:38]

lgtm

see my notes below advocating the Dodge & Burn blacklisting is not really worth
it; Skia guys ultimately should make the call; I'm just providing facts and
assessment since Greg asked for thoughts

https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:234: if (kNVIDIA_GrGLDriver == ctxInfo.driver()) {
On 2015/06/05 18:18:04, egdaniel wrote:
> Outside of intel which I believe you had said had major bugs before, why not
> turn on for other drivers? I think most worked before and I'd rather blacklist
a
> driver if it ends up being problematic. I think we could leave Dodge and Burn
> blacklisted on all because I do remember those showing artifacts on others as
> well (not surprising since the implementation in the original spec was
> numerically unstable and I assume most just copied it). Thoughts?

I question if blacking listing the individual Dodge and Burn modes (lines
245-249) is really worth it.

I've expedited pushing the fix for Dodge and Burn into public drivers sooner.

I think we should be smart about how much black listing we do.  In this
particular case, the actual issue is extremely limited in scope and severity

Excessive blacklisting logic has its own pitfalls as it makes testing that much
harder and can bite you later.  Pragmatically, this is a case where I think we
are better served avoiding blacklisting and letting driver updates fix the
problem.  I fully expect in the very term you aren't going to want Skia
cluttered with blacklisting for this minor issue.

My rationale thinking this:  Very little real content uses Dodge or Burn.  Also
the NVIDIA GPUs that are affected are very new and these are devices where
driver updates are pushed quickly done by end-users.  And it has very low
severity; it isn't a crash or non-deterministic rendering issue.  The
numerically unstable math occurs in a narrow range of the domain of the 4D blend
mode function input space for Dodge and Burn.  The problem region is when the
source & destination alpha are approximately equal.  Skia's test is structured
to exercise the problem range well (kudos on the good testing coverage) but the
vast 4D range of Dodge and Burn works fine.

I think we'd be better off not implementing blacklisting for this particular bug
because it is extremely limited in scope, very low severity, and the driver fix
is coming quickly.  To be clear, I'm advocating dropping out the check in lines
245-249.

To be clear, the 238-243 blacklisting I do support.

[Chris Dalton, 2015-06-05 19:32:08]

On 2015/06/05 18:18:04, egdaniel wrote:
> Did you add some blacklist already in chrome to make sure this isn't used?

Yeah, it was technically a whitelist for the NVIDIA driver, so it did block
Chrome. I'll covnert to a blacklist though and block Chrome explicitly until we
can resolve the driver issues in there.

[egdaniel, 2015-06-05 19:47:58]

On 2015/06/05 19:09:38, nv_mark wrote:
> lgtm
> 
> see my notes below advocating the Dodge & Burn blacklisting is not really
worth
> it; Skia guys ultimately should make the call; I'm just providing facts and
> assessment since Greg asked for thoughts
> 
> https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp
> File src/gpu/gl/GrGLCaps.cpp (right):
> 
>
https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp...
> src/gpu/gl/GrGLCaps.cpp:234: if (kNVIDIA_GrGLDriver == ctxInfo.driver()) {
> On 2015/06/05 18:18:04, egdaniel wrote:
> > Outside of intel which I believe you had said had major bugs before, why not
> > turn on for other drivers? I think most worked before and I'd rather
blacklist
> a
> > driver if it ends up being problematic. I think we could leave Dodge and
Burn
> > blacklisted on all because I do remember those showing artifacts on others
as
> > well (not surprising since the implementation in the original spec was
> > numerically unstable and I assume most just copied it). Thoughts?
> 
> I question if blacking listing the individual Dodge and Burn modes (lines
> 245-249) is really worth it.
> 
> I've expedited pushing the fix for Dodge and Burn into public drivers sooner.
> 
> I think we should be smart about how much black listing we do.  In this
> particular case, the actual issue is extremely limited in scope and severity
> 
> Excessive blacklisting logic has its own pitfalls as it makes testing that
much
> harder and can bite you later.  Pragmatically, this is a case where I think we
> are better served avoiding blacklisting and letting driver updates fix the
> problem.  I fully expect in the very term you aren't going to want Skia
> cluttered with blacklisting for this minor issue.
> 
> My rationale thinking this:  Very little real content uses Dodge or Burn. 
Also
> the NVIDIA GPUs that are affected are very new and these are devices where
> driver updates are pushed quickly done by end-users.  And it has very low
> severity; it isn't a crash or non-deterministic rendering issue.  The
> numerically unstable math occurs in a narrow range of the domain of the 4D
blend
> mode function input space for Dodge and Burn.  The problem region is when the
> source & destination alpha are approximately equal.  Skia's test is structured
> to exercise the problem range well (kudos on the good testing coverage) but
the
> vast 4D range of Dodge and Burn works fine.
> 
> I think we'd be better off not implementing blacklisting for this particular
bug
> because it is extremely limited in scope, very low severity, and the driver
fix
> is coming quickly.  To be clear, I'm advocating dropping out the check in
lines
> 245-249.
> 
> To be clear, the 238-243 blacklisting I do support.

Yeah not blacklisting burn and dodge doesn't bother me too much. My only request
for going this route is that it is well documented in the code somewhere that we
are aware of the possible numerical issues around dodge and burn that may cause
streaking, yada yada yada.

[egdaniel, 2015-06-05 20:00:39]

On 2015/06/05 19:47:58, egdaniel wrote:
> On 2015/06/05 19:09:38, nv_mark wrote:
> > lgtm
> > 
> > see my notes below advocating the Dodge & Burn blacklisting is not really
> worth
> > it; Skia guys ultimately should make the call; I'm just providing facts and
> > assessment since Greg asked for thoughts
> > 
> >
https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp
> > File src/gpu/gl/GrGLCaps.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1166513002/diff/60001/src/gpu/gl/GrGLCaps.cpp...
> > src/gpu/gl/GrGLCaps.cpp:234: if (kNVIDIA_GrGLDriver == ctxInfo.driver()) {
> > On 2015/06/05 18:18:04, egdaniel wrote:
> > > Outside of intel which I believe you had said had major bugs before, why
not
> > > turn on for other drivers? I think most worked before and I'd rather
> blacklist
> > a
> > > driver if it ends up being problematic. I think we could leave Dodge and
> Burn
> > > blacklisted on all because I do remember those showing artifacts on others
> as
> > > well (not surprising since the implementation in the original spec was
> > > numerically unstable and I assume most just copied it). Thoughts?
> > 
> > I question if blacking listing the individual Dodge and Burn modes (lines
> > 245-249) is really worth it.
> > 
> > I've expedited pushing the fix for Dodge and Burn into public drivers
sooner.
> > 
> > I think we should be smart about how much black listing we do.  In this
> > particular case, the actual issue is extremely limited in scope and severity
> > 
> > Excessive blacklisting logic has its own pitfalls as it makes testing that
> much
> > harder and can bite you later.  Pragmatically, this is a case where I think
we
> > are better served avoiding blacklisting and letting driver updates fix the
> > problem.  I fully expect in the very term you aren't going to want Skia
> > cluttered with blacklisting for this minor issue.
> > 
> > My rationale thinking this:  Very little real content uses Dodge or Burn. 
> Also
> > the NVIDIA GPUs that are affected are very new and these are devices where
> > driver updates are pushed quickly done by end-users.  And it has very low
> > severity; it isn't a crash or non-deterministic rendering issue.  The
> > numerically unstable math occurs in a narrow range of the domain of the 4D
> blend
> > mode function input space for Dodge and Burn.  The problem region is when
the
> > source & destination alpha are approximately equal.  Skia's test is
structured
> > to exercise the problem range well (kudos on the good testing coverage) but
> the
> > vast 4D range of Dodge and Burn works fine.
> > 
> > I think we'd be better off not implementing blacklisting for this particular
> bug
> > because it is extremely limited in scope, very low severity, and the driver
> fix
> > is coming quickly.  To be clear, I'm advocating dropping out the check in
> lines
> > 245-249.
> > 
> > To be clear, the 238-243 blacklisting I do support.
> 
> Yeah not blacklisting burn and dodge doesn't bother me too much. My only
request
> for going this route is that it is well documented in the code somewhere that
we
> are aware of the possible numerical issues around dodge and burn that may
cause
> streaking, yada yada yada.

The one issue is if we see a lot of issues on other devices with burn and dodge
(especially with android since driver updates are much slower) the blacklist is
going to be needed.

[Chris Dalton, 2015-06-05 21:42:27]

This one switches the whitelist to a blacklist, and removes the blacklist for
dodge and burn as discussed.

It looks like we also need to block Adreno4xx, based on
https://codereview.chromium.org/1154953004

[Chris Dalton, 2015-06-11 03:43:24]

Turn per-equation blacklists back on

[bsalomon, 2015-06-11 15:33:12]

lgtm. Chris showed me the artifacts on the xfermodes2 GM. It looks like they
appear for ranges of alpha in either the src or the dst (depending on dodge vs
burn) and are fairly significant.

[bsalomon, 2015-06-11 17:37:26]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-06-11 17:37:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mjk@nvidia.com
Link to the patchset: https://codereview.chromium.org/1166513002/#ps120001
(title: "Get the base right")

[commit-bot: I haz the power, 2015-06-11 17:37:45]

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

[commit-bot: I haz the power, 2015-06-11 17:38:47]

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

[commit-bot: I haz the power, 2015-06-11 17:38:48]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)

[Mark Kilgard, 2015-06-11 22:23:45]

markkilgard@gmail.com changed reviewers:
+ markkilgard@gmail.com

[Mark Kilgard, 2015-06-11 22:23:46]

figure out the GrBlend.h issue

https://codereview.chromium.org/1166513002/diff/140001/include/gpu/GrCaps.h
File include/gpu/GrCaps.h (right):

https://codereview.chromium.org/1166513002/diff/140001/include/gpu/GrCaps.h#n...
include/gpu/GrCaps.h:13: #include "GrBlend.h"
the build failed unable to locate GrBlend.h

should src/gpu/GrBlend.h actually be in include/gpu perhaps?

[Chris Dalton, 2015-06-12 15:27:26]

The CQ bit was checked by cdalton@nvidia.com to run a CQ dry run

[Chris Dalton, 2015-06-12 15:27:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
mjk@nvidia.com
Link to the patchset: https://codereview.chromium.org/1166513002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-12 15:27:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1166513002/160001

[commit-bot: I haz the power, 2015-06-12 15:30:47]

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

[commit-bot: I haz the power, 2015-06-12 15:30:48]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[Chris Dalton, 2015-06-12 15:39:03]

The CQ bit was checked by cdalton@nvidia.com to run a CQ dry run

[Chris Dalton, 2015-06-12 15:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
mjk@nvidia.com
Link to the patchset: https://codereview.chromium.org/1166513002/#ps180001
(title: "fix msvc")

[commit-bot: I haz the power, 2015-06-12 15:39:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1166513002/180001

[commit-bot: I haz the power, 2015-06-12 15:55:58]

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

[commit-bot: I haz the power, 2015-06-12 15:55:59]

Dry run: This issue passed the CQ dry run.

[Chris Dalton, 2015-06-12 16:00:23]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-06-12 16:00:29]

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

[commit-bot: I haz the power, 2015-06-12 16:01:25]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/1dd0542ca37cf1b4a2ab0ea41f8009ded097fd47