Browser fixes #479

dy · 2018-05-04T00:03:55Z

The intention of this PR is to make sure Firefox/IE11/Safari and other non-webkits pass tests.

rreusser

Presumably everything is logically and functionally sound and it just prevents an undefined-behavior corner case from erroring out? If so, seems reasonable to me.

dy · 2018-05-04T21:27:35Z

lib/texture.js

@@ -1314,12 +1333,15 @@ module.exports = function createTextureSet (

      tempBind(texture)
      for (var i = 0; texture.mipmask >> i; ++i) {
+        var _w = w >> i
+        var _h = h >> i
+        if (!_w || !_h) break


Firefox warns when w/h is not POT, and it is not when _w === 0

dy · 2018-05-04T21:29:35Z

lib/texture.js

@@ -828,6 +842,10 @@ module.exports = function createTextureSet (
      gl.copyTexImage2D(
        target, miplevel, format, info.xOffset, info.yOffset, width, height, 0)
    } else {
+      if (!data) {


Firefox requires texture to be init with data before subimage call, otherwise gives warning.

rreusser · 2018-05-14T20:53:05Z

v1.3.1 has seven failures in safari (some given with just a bit of context since the console is too slow to fetch stack traces):

not ok 3084 read float fbo ok
not ok 3085 read float fbo, reuse buffer, ok
...
ok 3130 ndarray-like input.type
ok 3131 ndarray-like input.dtype
ok 3132 ndarray-like input.dimension
ok 3133 ndarray-like input.usage
ok 3134 ndarray-like input.byteLength
not ok 3135 error ok
...
ok 3160 manual properties buffer type
ok 3161 manual properties buffer usage
ok 3162 manual properties buffer byteLength
ok 3163 manual properties buffer dimension
not ok 3164 error ok
...
not ok 20533 stencil renderbuffer - implicit
  ---
    operator: deepEqual
    expected: |-
      [ 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 255, 0, 0, 255, 0, 255, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0 ]
    actual: |-
      [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]
    stack: |-
...
ok 37739 buffer update ok
not ok 37740 error ok
  ---
    operator: equal
    expected: 0
    actual:   1281
...
# elements update
ok 37741 elements update ok
not ok 37742 error ok
  ---
    operator: equal
    expected: 0
    actual:   1281

Of concern is a new error in master:

not ok 16630 subimage simple pixels: error code is ok
  ---
    operator: equal
    expected: 0
    actual:   1282

And that tests halt at 18629 with:

dy · 2018-05-14T21:06:43Z

@rreusser

not ok 16630 subimage simple pixels: error code is ok
  ---
    operator: equal
    expected: 0
    actual:   1282

this error is not critical - some browsers naturally give WebGL warnings but keep functioning, and that assertion checks if there are none.
Also I run running short batches of tests to avoid console blow :)

rreusser · 2018-05-14T21:11:27Z

Yup. Currently digging in a bit more. The stencil renderbuffer - implicit failure is not new, but it's a bit stateful (adjacent test fails depending on if you comment it out). Unfortunately the cause is rather opaque.

rreusser · 2018-05-14T21:23:16Z

Looks like the half float oes bug is Safari-~10-11(?)-specific and has been encountered by a few different projects and that it results in objects rendering black.

Unity encountered this but unfortunately there's no helpful information on the issue.
Babylon's fix is a vendor-specific hack that switches to float if half float doesn't work

I think this is not new but is actually the result of checks that were not previously implemented: 75730b6#diff-6d2e0d9813eb0b2330521d6a391021d3R169

I think the immediate fix here is to have the test fail but not bring all tests to a halt.

rreusser · 2018-05-14T21:53:59Z

After patching over the framebuffer-parse test to report an error without halting, I believe the above list is complete for Safari 11.1 on OS X 10.13.4.

Of course fixing the tests is a different matter.

rreusser · 2018-05-14T22:20:04Z

Apologies this is all a bit scattered. Summarizing what I know about the test failures for Safari:

not ok 3084 read float fbo ok, not ok 3085 read float fbo, reuse buffer, ok (test/read.js:109)
- Fails to read from a floating point fbo. WebGL: INVALID_ENUM: readPixels: invalid type in Safari 11.1. Perhaps it's just disabled or not implemented in Safari? I hit this brick wall once before where I wanted to read data on a mobile device and it simply wasn't supported/allowed and this was the error I got.
ok 3134 ndarray-like input.byteLength: ndarray-like input.byteLength
- The result of a zero-size data array. Fixed by Browser fixes #479, I think.
elements update ok (test/elements-update.js:54)
- I think this is just the same zero-length-data issue
not ok 20533 stencil renderbuffer - implicit
- reason for failure not yet clear.
not ok 16630 subimage simple pixels: error code is ok
- A warning. Seems fine, I guess?
test/framebuffer-parse.js:523: fbo + half float
- Seems like a safari bug people have just been working around
- See: Browser fixes #479 (comment)

dy · 2018-05-14T22:30:19Z

lib/texture.js

-        if (Dtype) {
-          data = new Dtype(width * height * channels)
-        }
+        data = pool.allocType(type, width * height * channels)


We should really scope this to browsers that throw warning on null texture data. How do we do so?

dy · 2018-05-14T22:30:42Z

test/texture2d.js

-          0, 0, 0, 255, 0, 0, 0, 255, 0, 255, 0, 255, 0, 255, 0, 255, 0, 0, 0, 0,
-          0, 0, 0, 255, 0, 0, 0, 255, 0, 255, 0, 255, 0, 255, 0, 255, 0, 0, 0, 0,
-          0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
+          255, 0, 0, 255, 255, 255, 255, 255, 0, 0, 0, 255, 0, 0, 0, 255, null, null, null, null,


null signifies insignificance

Noted in the comment above, so sounds good 👍

dy · 2018-05-14T22:33:31Z

lib/texture.js

-        if (Dtype) {
-          data = new Dtype(width * height * channels)
-        }
+        data = pool.allocType(type, width * height * channels)


We should really scope this to browsers that throw warning on null texture data. How do we do so?

Is there any precedent for feature-detection in regl? Ideally I think we'd just feature-detect if it accepts null data since the result probably doesn't change.

Unfortunately that is not an error but warning, there is no way to detect that via gl.getError or alike https://stackoverflow.com/questions/26400009/webgl-get-error-warning-message-text-as-a-string, so forcing null-data every time via zero pool seems reasonable solution. Is that indeed?

dy · 2018-05-15T00:19:53Z

lib/util/pool.js

+module.exports = createPool()
+
+// zero pool for initial zero data
+module.exports.zero = createPool()


Reserved for zero-data inits.

dy · 2018-05-16T22:27:12Z

@rreusser seems that Safari tests pass on my side. Could you possibly run them once again to make sure all except for read float is fixed? Then it will be up to IE.

rreusser · 2018-05-16T22:45:48Z

The two read float tests fail in Safari 11.1, and also the stencil renderbuffer test at framebuffer-depth-stencil.js:

not ok 20558 stencil renderbuffer - implicit
  ---
    operator: deepEqual
    expected: |-
      [ 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 255, 0, 0, 255, 0, 255, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 255, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0 ]
    actual: |-
      [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]
    stack: |-
      assert@http://172.31.99.184:9969/browser.js:16493:63
      bound@http://172.31.99.184:9969/browser.js:16345:37
      same@http://172.31.99.184:9969/browser.js:16689:17
      bound@http://172.31.99.184:9969/browser.js:16345:37
      testDraw@http://172.31.99.184:9969/browser.js:22500:11
      http://172.31.99.184:9969/browser.js:22509:15
      scope
      REGLCommand@http://172.31.99.184:9969/browser.js:17620:28
      testFBO@http://172.31.99.184:9969/browser.js:22508:11
      http://172.31.99.184:9969/browser.js:22560:12
      bound@http://172.31.99.184:9969/browser.js:16345:37
      run@http://172.31.99.184:9969/browser.js:16364:13
      bound@http://172.31.99.184:9969/browser.js:16345:37
      next@http://172.31.99.184:9969/browser.js:16140:18
      run@http://172.31.99.184:9969/browser.js:12963:19
      drainQueue@http://172.31.99.184:9969/browser.js:12933:45

dy · 2018-05-17T17:32:16Z

Ok, fixed stencil renderbuffer - implicit - it was somehow related to initial renderbuffer setup, as if framebuffer did not redefine renderbuffer stencil/depth in Safari, depth should've been enabled beforehead.

dy · 2018-05-17T18:42:43Z

lib/read.js

+  if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) !== gl.FRAMEBUFFER_COMPLETE) return false
+
+  gl.viewport(0, 0, 1, 1)
+  gl.clearColor(1.0, 0.0, 0.0, 1.0)


I wonder if that can lose previous clear color, if there is any. That should not be an issue, since regl.clear or regl._refresh fully reinit the state.

This check is run on startup, right? I think it'd be fine then as long as the state is refreshed/initialized afterwards.

dy · 2018-05-17T18:44:09Z

lib/read.js

+  gl.bindTexture(gl.TEXTURE_2D, null)
+  if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) !== gl.FRAMEBUFFER_COMPLETE) return false
+
+  gl.viewport(0, 0, 1, 1)


That should not affect previous render/framebuffer, should that? At the moment of this call gl does not have any fbo/renderbuffer attached.

Oh, and apologies, but just curious? What does this really do? On platforms where it's not supported, does it currently fail silently? Throw an error? A warning? After this, what is the result?

This uses own regl ‘check’ function to throw an error if reading floats is not supported, as discussed in the adjacent PR.

Apologies, but I forgot: does it already throw an error without regl itself detecting and throwing an error?

#487 (comment)

instead of silent failure

never mind. got it 👍

dy · 2018-05-17T18:44:51Z

test/read.js

@@ -92,7 +93,7 @@ tape('read pixels', function (t) {
  })

  // now do it for an float fbo
-  if (regl.hasExtension('oes_texture_float')) {
+  if (regl.hasExtension('oes_texture_float') && !safari) {


Blindly disable safari. Is there any sense to double-check if reading floats is supported here instead of this?

Seems okay to me. Could feature-detect it independently from regl, but I think the main goal is just to have safari reasonably report pass/fail on the tests so that regl as a whole is easier to maintain. This accomplishes it, though it'd be nice if there were a pending state or something so that it'd report without failing. But short of that, 👍, I guess?

I tend to agree on that point. What about exposing ‘regl.supports’ set to let user manually check supported features rather than throwing an error? Or mb just reusing ‘regl.limits’?

Seems reasonable if regl detects it anyway. limits is strictly builtin getParameter results (right?) so seems nice to keep that clean.

Side note: Does safari really not throw an exception for unsupported types? 😞 See: https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/readPixels#Exceptions

Saw the commit. limits seems fine too 👍

It does not. It just silently returns 0, as in this demo.

Perfect. Getting a bit lost in the number of issues being tackled here. Got it. 👍 👍

dy · 2018-05-17T22:27:27Z

Ok, now all browser tests pass, merge-ready.

It is up to reviewing and confirming/discussing feature-detection and browser-scoping tests decisions.

@rreusser hope for your help with that :)

rreusser · 2018-05-17T22:47:00Z

So just to reiterate the approach to vendor-specific issues (correct me if I'm wrong):

In the sense that regl only wraps the webgl API, it's nice to avoid caring about what works or doesn't work, but I think some minimal feature detection, if only for the sake of warning/failing when something would otherwise fail silently (and possibly just in debug mode?) is reasonable.

Could split hairs all day on all the little points, but things here look good to me 👍

etpinard · 2018-05-18T14:15:14Z

Big shoutout to @dy and @rreusser for this PR 🎉 🍾

dy added 2 commits May 3, 2018 20:02

Fix safari 0-data buffer setup

beee719

Fix extra logging

5339bf4

rreusser reviewed May 4, 2018

View reviewed changes

dy self-assigned this May 4, 2018

Preinit texture with data

291fab9

dy commented May 4, 2018

View reviewed changes

Remove extra comment

fc8c624

dy commented May 4, 2018

View reviewed changes

dy added 3 commits May 4, 2018 17:52

Fix undefined Dtype

d8b91f5

Add firefox interval timeout

0b87f34

Fix IE context creation

9a7d9e5

dy mentioned this pull request May 14, 2018

Publish a new version? #484

Closed

Merge branch 'master' into browser-fixes

0703765

rreusser mentioned this pull request May 14, 2018

Report a safari-specific test failure without halting all tests #485

Merged

Use pool to preallocate texture

16b7628

dy commented May 14, 2018

View reviewed changes

dy added 2 commits May 14, 2018 18:43

Fix test

34d4b04

Add zero-pool for texture data

50afc01

dy commented May 15, 2018

View reviewed changes

dy added 3 commits May 14, 2018 20:20

Remove insignificant checks

462f27c

Move browser entry to tests

be3326d

Move browser entry to test

d18b56a

Unroll tests

fc418cc

Fix safari framebuffer test

0a82ba4

Display warning reading floats in contexts not supporting the feature

6afda7c

dy commented May 17, 2018

View reviewed changes

dy added 6 commits May 17, 2018 16:01

Rename mikolalysenko with regl-project

ad1c6ac

Fix IE shader issues

1a33343

Fix IE none colorspace test

3199462

Add pow2 cube texture check

617ff84

Fix cube FBO test

87e2ebf

Fix rest of IE tests

b458147

Clean up

8addf24

dy mentioned this pull request May 17, 2018

Preinitialize texture data for the user #454

Closed

dy added 4 commits May 17, 2018 20:10

Move feature-detection to limits

7745514

Add API lines

0b31a64

Rename cubeNpot to npotCubeTexture

71bba6c

Rename npotCubeTexture to npotTextureCube

67d4cdd

dy merged commit 28fbf71 into master May 18, 2018

dy deleted the browser-fixes branch May 18, 2018 14:14

This was referenced May 18, 2018

IE does not support not power of 2 cube textures #490

Closed

Cross browser testing #143

Open

etpinard mentioned this pull request May 22, 2018

Add null-data buffer fix for parcoords plotly/plotly.js#2612

Merged

etpinard mentioned this pull request Jun 5, 2018

Bump regl to 1.3.6 plotly/plotly.js#2694

Merged

Browser fixes #479

Browser fixes #479

Conversation

dy commented May 4, 2018 • edited Loading

rreusser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented May 14, 2018

dy commented May 14, 2018 • edited Loading

rreusser commented May 14, 2018

rreusser commented May 14, 2018 • edited Loading

rreusser commented May 14, 2018 • edited Loading

rreusser commented May 14, 2018 • edited by dy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy commented May 16, 2018 • edited Loading

rreusser commented May 16, 2018 • edited Loading

dy commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser May 18, 2018 • edited Loading

Choose a reason for hiding this comment

rreusser May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser May 17, 2018 • edited Loading

Choose a reason for hiding this comment

dy May 17, 2018 • edited Loading

Choose a reason for hiding this comment

rreusser May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy commented May 17, 2018 • edited Loading

rreusser commented May 17, 2018 • edited Loading

etpinard commented May 18, 2018

dy commented May 4, 2018 •

edited

Loading

dy commented May 14, 2018 •

edited

Loading

rreusser commented May 14, 2018 •

edited

Loading

rreusser commented May 14, 2018 •

edited

Loading

rreusser commented May 14, 2018 •

edited by dy

Loading

dy commented May 16, 2018 •

edited

Loading

rreusser commented May 16, 2018 •

edited

Loading

rreusser May 18, 2018 •

edited

Loading

rreusser May 18, 2018 •

edited

Loading

rreusser May 17, 2018 •

edited

Loading

dy May 17, 2018 •

edited

Loading

rreusser May 18, 2018 •

edited

Loading

dy commented May 17, 2018 •

edited

Loading

rreusser commented May 17, 2018 •

edited

Loading