webgl: Fix NaN issue

[qjia7]

Fix #6822

Problem
1： On some GPUs, even if a and b are both non-NaN, the value of isNaN in vec4 isNaN = min(vec4(isnan(a)) + vec4(isnan(b)), vec4(1.0)); are still larger than 0., which misleads all values become NAN.
2： After resolving NAN issue, the result is still incorrect. It seems that the isnan_custom is not well supported on the problem GPU. After switching back to builtin isnan, everything works well.

Solution:
Use the bool type bvec4 instead of float type vec4 to calculate isNaN to avoid the the float precision issue when comparing with zero.
Meanwhile, add an env flag WEBGL_ISNAN_CUSTOM to allow user to specify which isnan to use.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

---

This change is 

[qjia7]

@vladmandic @shurshilov
Please have the last test with this PR. I also make it ready for review.
The verified solution #6822 (comment):

vec4 nanValue = a;
bvec4 isNaN = isnan(a);
result.r = isNaN.r ? nanValue.r : result.r;
result.g = isNaN.g ? nanValue.g : result.g;
result.b = isNaN.b ? nanValue.b : result.b;
result.a = isNaN.a ? nanValue.a : result.a;

nanValue = b;
isNaN = isnan(b);
result.r = isNaN.r ? nanValue.r : result.r;
result.g = isNaN.g ? nanValue.g : result.g;
result.b = isNaN.b ? nanValue.b : result.b;
result.a = isNaN.a ? nanValue.a : result.a;

return result;

The solution in this PR:

  bvec4 isNaNA = isnan(a);
  bvec4 isNaNB = isnan(b);
  bvec4 isNaN = bvec4(isNaNA.x || isNaNB.x, isNaNA.y || isNaNB.y, isNaNA.z || isNaNB.z, isNaNA.w || isNaNB.w);
  result.r = isNaN.r ? NAN : result.r;
  result.g = isNaN.g ? NAN : result.g;
  result.b = isNaN.b ? NAN : result.b;
  result.a = isNaN.a ? NAN : result.a;

  return result;

I think the second one should also be workable. But if not, I need to revert this change Use NAN instead of nanValue.xxx

[pyu10055]

@qjia7 thank you for fixing the NaN check, seems there are still some discussion on the validity of the fix? can you add some comment on the snippets to explain why the check needs to be breaking up. thanks!

Reviewed 9 of 9 files at r1, 1 of 1 files at r2, 5 of 5 files at r3, 7 of 7 files at r4, 6 of 6 files at r5, 9 of 9 files at r6.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128)

[qjia7]

@pyu10055 Breaking up the checks won't resolve the problem. And our previous verified solution is a misleading, which used the same test url (which totally removing NAN checking) by accident. So it's reasonable that whether to break up the checks, the result is similar.
After further debugging, it shows that isnan_custom doesn't work well on the problem gpu. But if we switch back to the shader builtin isnan, everything works well. So the current fixing is to allow the user to specify which isnan to use. Please take another look. Thanks.

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128)

[qjia7]

Sorry, please hold on the review. I find another issue. Will ping you when it's ready.

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128)

[qjia7]

It's ready for review. It turns out the previous issue is my test case problem.

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128)

[qjia7]

Kindly ping you guys. Also add @Linchenn in case you are in vacation.

[pyu10055]

thanks

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @Linchenn, and @qjia7)

---

tfjs-backend-webgl/src/flags_webgl.ts line 276 at r8 (raw file):

 * doesn't have the builtin isnan.
 */
ENV.registerFlag('WEBGL_ISNAN_CUSTOM', () => false);

should this default to true to match the previous behavior?

---

tfjs-backend-webgl/src/glsl_version.ts line 78 at r8 (raw file):

      #define isnan(value) isnan_custom(value)
    ` :
                                                             '';

The formatting seems to be weird, is it auto-formatted?

[vladmandic]

should this default to true to match the previous behavior?

if previous behavior is proven to be broken on some gpus, why?

afaik, only reason for isnan_custom is because webgl1 does not have native isnan, so why not enable it for webgl1 and disable otherwise. we should avoid creating new flags that nobody will remember few months from now.

[lina128]

Thank you Jiajia!

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn, @pyu10055, and @qjia7)

---

tfjs-backend-webgl/src/flags_webgl.ts line 276 at r8 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

should this default to true to match the previous behavior?

If it's only useful for webgl2, should this flag be set to true if webgl1 is true?

[qjia7]

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn, @pyu10055, and @vladmandic)

---

tfjs-backend-webgl/src/flags_webgl.ts line 276 at r8 (raw file):

Previously, lina128 (Na Li) wrote…

If it's only useful for webgl2, should this flag be set to true if webgl1 is true?

webgl1 is not controlled by this flag. Since webgl1 doesn't have a builtin isnan, we always use a custom isnan. For webgl2, previously, we use a customized isnan, which is proved to be problem on some GPUs. So I add this flag to use builtin isnan for webgl2 by default. Meanwhile, I keep the original customized isnan for testing if necessary. I agree with @vladmandic's opinion. Maybe I can rename it to WEBGL2_ISNAN_CUSTOM to more reflect the meaning.

---

tfjs-backend-webgl/src/glsl_version.ts line 78 at r8 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

The formatting seems to be weird, is it auto-formatted?

Yes, it's auto-formatted.

[qjia7]

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn, @pyu10055, and @vladmandic)

---

tfjs-backend-webgl/src/flags_webgl.ts line 276 at r8 (raw file):

Previously, qjia7 (Jiajia Qin) wrote…

webgl1 is not controlled by this flag. Since webgl1 doesn't have a builtin isnan, we always use a custom isnan. For webgl2, previously, we use a customized isnan, which is proved to be problem on some GPUs. So I add this flag to use builtin isnan for webgl2 by default. Meanwhile, I keep the original customized isnan for testing if necessary. I agree with @vladmandic's opinion. Maybe I can rename it to WEBGL2_ISNAN_CUSTOM to more reflect the meaning.

Or I can totally remove the customized isnan for webgl2 since we have the builtin isnan ?

[pyu10055]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @Linchenn, @pyu10055, and @vladmandic)

---

tfjs-backend-webgl/src/flags_webgl.ts line 276 at r8 (raw file):

Previously, qjia7 (Jiajia Qin) wrote…

Or I can totally remove the customized isnan for webgl2 since we have the builtin isnan ?

got it, if we believe using build-in isnan will fix the error and not creating any new issues, we can remove this flag.

[qjia7]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @Linchenn, @pyu10055, and @vladmandic)

---

tfjs-backend-webgl/src/flags_webgl.ts line 276 at r8 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

got it, if we believe using build-in isnan will fix the error and not creating any new issues, we can remove this flag.

Thanks Ping. Let's keep it for a while to see if any bug is reported by the builtin isnan. If everything works well, I will submit another PR to totally remove the customized one.

[hujiajie]

@qjia7 It sounds the correctness of the builtin/polyfilled isnan can be programmatically probed with a small snippet and then the right one could be picked up automatically without "creating new flags that nobody will remember few months from now", or no?

[qjia7]

@qjia7 It sounds the correctness of the builtin/polyfilled isnan can be programmatically probed with a small snippet and then the right one could be picked up automatically without "creating new flags that nobody will remember few months from now", or no?

In fact, It's hard to construct a small case to reflect the isnan issue. The current solution is to always use the builtin isnan. I know you may have concern about the comment here Use custom isnan definition to work across differences between implementations on various platforms.. But I think it's compare to the old custom isnan [return (val > 0. || val < 1. || val == 0.) ? false : true; ]. So I prefer we directly use the builtin isnan for simplification.