[robertleeplummerjr]

Closes #142

The unit test I ported from webgl2, to webgl1, to headlessgl, and then as well to google angle, to see how is would hold up across layers. They are here: https://github.com/proxy/gist.github.com/robertleeplummerjr/3313587571dd2a8fa3c3907e451b5564

[dhritzkiv]

First off, thank you for your hard work! Much appreciated. Looking forward to having this land and include you and your support on this repo. But first…

---

I pulled down the repo, built, and ran the tests locally (after making some quick fixes to the multiple-render-targets test script).

While the draw_buffer and multiple-render-targets tests pass, there are 44 tests that don't seem to pass.

Namely these suites:

• renderbuffers_framebuffer-object-attachment
• rendering_gl-scissor-fbo-test
• state_state-uneffected-after-compositing
• textures_mipmap-fbo

Are you getting these results, or are all your tests passing?

[robertleeplummerjr]

Will resolve this AM.

[robertleeplummerjr]

I started to look at failures from enabling "extensions_webgl-draw-buffers" from conformance tests, and we have a bit to go to satisfy those, fyi.

As far as the other failures, I only get the following failures:

• glsl_variables_glsl-built-ins
  • not ok 5762 should be green
  • not ok 5763 should be green
• not ok 8026 no plan foundegative level

But the tests to seem to die early with:

⨯ fail  3
error Command failed with exit code 1.

[dhritzkiv]

I'm getting errors with the multiple-render-targets test:

not ok 8306 Only 45000 are not 0, expected 144000

in multiple-render-targets.js, outWidth (300) * ouputHeight (150) = 45000, so that makes some sense. How did you arrive at the expectation of 144000? Should it may be be 180000 (45000 pixels * 4 channels)?

[dhritzkiv]

Edit: the test failures may be the result of the wrong results: after outputting the file, I notice that it's not working on my machine:

I wonder how to debug this issue…

[robertleeplummerjr]

Just reproduced, will have a fix momentarily.

[robertleeplummerjr]

It was the inclusion of the gl.NONE, so pulled that back. Test is now passing locally.

[robertleeplummerjr]

The size of the graphical output was simply me trying to find one of the simplest outputs that I hadn't written so to re-emulate. The original "test" was from here: https://stackoverflow.com/questions/51793336/webgl-2-0-multiple-output-textures-from-the-same-program/51798078#51798078 , which was just me counting the pixels, and yes they are width * height * 4, but there is some offset from the edges. I would be very welcome to simplifying the test, but I think I just welcomed getting the PR opened asap.

[dhritzkiv]

Fortunately I am now getting this output:
.
Interestingly, there are no black bars.

Further, I am now getting this error for the test:

not ok 1 Only 168750 are not 0, expected 144000

[robertleeplummerjr]

Interesting! I wonder if foreground or background is being lost? How far do we need to go to get this merged in? Does it have to be full on unit tests compliant, or can we go in stages?

[dhritzkiv]

Doing some basic math:
300 * 150 * 4 (w * h * channels) = 180000 (possible expected value?)
180000 - 168750 (actual result) = 11250 (difference)
11250 - (350*150/4) = 0

…I don't how that's significant (or if it's just coincidence), but I would guess that one of the colour's channels values is 0?

Are you getting this result when you run the tests? (btw - you can write tape.only( instead of tape( to run only the multiple-render-targets test)

[robertleeplummerjr]

If I may propose: This unit test is too complex. We just need a few pixels to know the output, in-fact, was from multi targets. We can then start to address bugs as they are found, like this one.

[robertleeplummerjr]

That is as well a good point, perhaps we should ensure none of them are zero, before checking if they are zero 😛

[dhritzkiv]

How far do we need to go to get this merged in? Does it have to be full on unit tests compliant, or can we go in stages?

Unfortunately for us (but fortunate for the project) we can have no regression in the tests in order to land something in master and push a release. Which means all the tests must pass as the current pass rate is 100%.

Fortunately, it's only a few suites that are failing, and the source of the errors likely lies in a single point of the new code. Find that source, and the tests will likely pass.

[robertleeplummerjr]

That isn't what I mean, of course this PR cannot introduce new bugs. What I mean is, for example, this entire feature isn't being tested as it has been black listed here:

headless-gl/test/extensions.js

Line 6 in e558e99

'extensions_webgl-draw-buffers'

What I should have clarified is: Does that entire blacklisted suite need to pass before this lands?

[dhritzkiv]

btw, I think I found the source of the zeros:

headless-gl/test/multiple-render-targets.js

Line 39 in f55a533

gl_FragData[2] = vec4(.2, .8, .0, 1);

[dhritzkiv]

Does that entire blacklisted suite need to pass before this lands?

Good question. I'll have to think about that. My inclination is that the test should no longer be black listed if we're adding this functionality in. However, it may have been blacklisted for other reasons.

[robertleeplummerjr]

Then I'll get to work on unit tests asap.

[robertleeplummerjr]

Was failing 130 or so tests, now down to 18 with extension tests on, fyi.

[robertleeplummerjr]

For your reference, if you haven't already used it, here are the conformance tests that are used in >headless-gl, for the browser: https://www.khronos.org/registry/webgl/conformance-suites/1.0.3/webgl-conformance-tests.html

In running the webgl-draw-buffers extension, FF and Safari (TP) both fail, but Chrome passes. Curious.

FF passes fine for me! There has to be something I'm missing then.

[robertleeplummerjr]

Ok, I don't think it (broadcasting the color from gl_FragColor) is supported in the version of google angle we are on.

The missing feature implementation from google angle: https://github.com/google/angle/blob/7c37ca1836402b654dbbf5f00007e09bdf2a7051/src/compiler/translator/tree_ops/EmulateGLFragColorBroadcast.cpp

Specifically: https://github.com/google/angle/blob/7c37ca1836402b654dbbf5f00007e09bdf2a7051/src/compiler/translator/tree_ops/EmulateGLFragColorBroadcast.cpp#L114

We could shim in something like this, but ultimately I'd like to keep our stuff as vanilla as possible.

Neither the feature or the unit test are in our version, fyi.

[dhritzkiv]

Yes, let's keep it as-is and not shim it. Hopefully in the future if/when we upgrade the angle implementation, we can expect behaviour more consistent with the spec and have those tests pass.

---

Other than the #ifdef GL_EXT_draw_buffers issue, and the cross-platform implementation differences, is there anything else keeping the draw-buffer extension suite from passing? We can skip the draw-buffer extension suite for this PR/release, ensure all other tests pass, and proceed with final reviews before merging.

---

Also, I believe the multiple render targets test you added needs some fixing up to have the correct expected vs. actual zeros quantities.

[robertleeplummerjr]

Other than the #ifdef GL_EXT_draw_buffers issue, and the cross-platform implementation differences, is there anything else keeping the draw-buffer extension suite from passing? We can skip the draw-buffer extension suite for this PR/release, ensure all other tests pass, and proceed with final reviews before merging.

I don't think so, (also, already built an upgrade script for google angle for later on down the road, he he) looking into other tests and the new test I added. Ty @dhritzkiv !

[robertleeplummerjr]

I run into some funny business with the following tests:

["more_functions_copyTexImage2DBadArgs", require("./more_functions_copyTexImage2DBadArgs")],

["more_functions_copyTexSubImage2D", require("./more_functions_copyTexSubImage2D")],  // This one is blacklisted, should the others?

["more_functions_copyTexSubImage2DBadArgs", require("./more_functions_copyTexSubImage2DBadArgs")],

They cause a Abort trap: 6, but if I comment them out, or blacklist them, I now get all passing unit tests.

[dhritzkiv]

Hmm, I don't have an issue with those suites on my machine…

[robertleeplummerjr]

It is probably either my node version (10) or operating system (osx 10.13.3 (17D47)).

Regards "15". I forgot to account for the missing pixel you pointed out, the one that is "0". Fixing it, will have it updated momentarily.

[dhritzkiv]

Hmm. I'm running the exact same environment. I imagine our GFX cards are different (but that also shouldn't matter). Mine is the NVIDIA GeForce GTX 675MX 1024 MB (from 2011, 2012)

[robertleeplummerjr]

I'm running the Intel stock, don't know the details this moment.

[robertleeplummerjr]

Regardless, it is a separate issue to those raised by this PR. What needs to happen next with this PR to be merge worthy?

[robertleeplummerjr]

My GFX is an: Intel Iris Pro 1536 MB

[dhritzkiv]

I'll do at least one more review of this PR's code, tests, and README over the next few days, and I'll invite the other maintainer of the repo to do so as well. Overall, it's looking to be in a good place.

After that, we'll merge into master, and then cut a new release for npm (along with generating prebuilt files for easy install).

[dhritzkiv]

@mourner Hey! Could you look over this PR quick? I need to do another pass over soon, myself, but overall it looks good.

Summary of the state of this:

• New extension!
• Existing tests pass (locally†)
• New multiple-render targets test testing the extension also passes
• Un-blacklisting the webgl-draw-buffers extension test suite leads to a small handful of assertion tests tests failing, but it's not for a lack of trying: there seem to be implementation differences between the three major browsers themselves, as well as the current version of angle that we're using in headless-gl, as well as graphics card of the user machine. In the end, these differences are what I'd consider edge-case. Ultimately, we can simply keep that suite blacklisted and we'd still get an all-green.††

† Travis-CI is now completely non-functional for this repo, as the connection with Travis-CI is now done through Github integrations/apps. Migrating to the new integration method requires ownership access to the repo. I will contact stackgl personally and make a plea for them to grant access to you and/or me. Thanks to a stackgl org owner, I was able to gain access to the CI, among other things.

†† my rationale for continuing to ignore the webgl-draw-buffers extension suite: 1) this is a non-breaking change; 2) the tests for that extension never worked and are outdated/diverged from the Khronos conformance tests suite (which themselves don't pass in every browser).

---

Robert here has been working super hard on this PR in order to support his awesome gpujs/gpu.js project and by extension BrainJS/brain.js. Both highly successful and impressive works.

He's been working on this since well before this PR's creation. He and I have been talking behind the scenes and he's expressed an interest in becoming a maintainer on this repo. Given his excellent work on this PR, his authorship of the two aforementioned products, and his overall enthusiasm for this project, I would recommend him as a maintainer. Robert also has started on other potential headless-gl improvements, such as updating angle which includes the support of webgl-2.

[dhritzkiv]

@robertleeplummerjr FYI: I added a small stylistic commit to the PR in order to trigger a CI build

[dhritzkiv]

I'll wait until midday tomorrow for the other maintainer to chime in. After that, I'll merge, version bump, and release on npm.

[mourner]

This looks pretty good to me — thanks a lot for working on this! My only nitpick is the amount of repetition with those 0–16 attachments/etc., but I'm not sure how easy it is to DRY up — we can leave that for a follow-up.

[mourner]

@robertleeplummerjr Just invited you as a collaborator. Thanks again for taking on this — I'm excited!

[robertleeplummerjr]

Ty guys. Invite accepted!