Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for unwritten gl_PointSize. #2822

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kdashg
Copy link
Contributor

@kdashg kdashg commented Mar 5, 2019

No description provided.

@kdashg
Copy link
Contributor Author

kdashg commented Mar 5, 2019

The full quote from ES3 is:

The point size is taken from the shader built-in gl_PointSize written by the vertex shader, and clamped to the implementation-dependent point size range. If the value written to gl_PointSize is less than or equal to zero, or if no value is written, the point size is undefined.

This certainly says that point size is clamped to within the valid range, though it does follow that up by saying that it's undefined-value sometimes. While this is somewhat ambiguous, the reasonable read here is that gl_PointSize should be clamped, even if an undefined value.

@kdashg
Copy link
Contributor Author

kdashg commented Mar 5, 2019

This exposes a bug on ANGLE, but my NV native GL driver on Windows seems fine.

@kdashg
Copy link
Contributor Author

kdashg commented Mar 5, 2019

Curiously, with transform-feedback enabled, ANGLE passes this test.
This sounds a lot like ANGLE is being over-eager to skip draw calls.

@lexaknyazev
Copy link
Member

I fully agree with the "reasonable read" for WebGL that never produces undefined values. However since it doesn't exactly follow the OpenGL spec, I think the "Differences Between WebGL and OpenGL ES" section should mention that.

@lexaknyazev
Copy link
Member

This sounds a lot like ANGLE is being over-eager to skip draw calls.

While this is arguably nitpicking, OpenGL ES rules are:

  1. gl_PointSize is unset - the result is undefined.
  2. gl_PointSize is less than or equal to zero - the result is undefined.
  3. gl_PointSize is greater than zero - clamp to [ALIASED_POINT_SIZE_RANGE[0], ALIASED_POINT_SIZE_RANGE[1]].

Quick tests show that ANGLE always does clamping when gl_PointSize is set (including non-positive values).

Also from ANGLE's StateManager11::syncPrimitiveTopology and RendererD3D::skipDraw

ProgramBinary assumes non-point rendering if gl_PointSize isn't written, which affects varying interpolation. Since the value of gl_PointSize is undefined when not written, just skip drawing to avoid unexpected results.

So, ANGLE is not inherently wrong by skipping draw calls here. I don't know what would be easier to implement for WebGL contexts: requiring gl_PointSize or assuming the default value.

@kenrussell
Copy link
Member

Per comments on #2818: I recall the Chrome team ran into this issue a long time ago, that we attempted to resolve it by initializing gl_PointSize to 1.0 at the beginning of vertex shaders, and that this broke geometry rendering on some platforms. I don't remember the details but we may be able to dig them out of Chromium's issue tracker.

For this reason the only way we could resolve this undefined behavior would be to dynamically recompile the current program if it was used to draw points and if its vertex shader didn't set gl_PointSize. This was a huge amount of complexity to take on and we decided to just leave this corner case undefined.

I'm very concerned that as we are collectively trying to reach conformance again that we are pushing the bar farther and farther out. This particular area has been problematic in the past and would require fairly extensive testing in order to make sure we aren't tickling a buggy area of OpenGL and OpenGL ES drivers by initializing gl_PointSize to 1.0 unilaterally.

How is Firefox currently passing the test in #2778 on Windows?

@lexaknyazev
Copy link
Member

How is Firefox currently passing the test in #2778 on Windows?

Just tested Firefox 65.0.2 on Win10: fails when ANGLE is enabled, passes on NVIDIA 419.17 OpenGL.

@lexaknyazev
Copy link
Member

@jdashg
Given the #3356, should this PR be adjusted for the spec change?

@greggman
Copy link
Contributor

greggman commented Dec 2, 2021

It was suggested to generate INVALID_OPERATION if gl_PointSize is not set and a draw is made with POINTS. This going to break at least one site (mine) where users can make shaders and choose to draw that shader with POINTS, LINES, TRIANGLES. They may or may not have set gl_PointSize.

There's no easy way for a JavaScript app that takes user generated shaders to know beforehand that they shouldn't allow calls with POINTS. To be thorough they'd have to parse the GLSL shaders themselves.

I'm just wondering if no-op is better than error here. Browsers could print a warning.

@lexaknyazev
Copy link
Member

lexaknyazev commented Dec 2, 2021

I'm just wondering if no-op is better than error here.

Assuming that an app does not query last error after every draw call, what's the difference? ANGLE on D3D currently skips the draw and prints a warning to the chrome://gpu page.

@greggman
Copy link
Contributor

greggman commented Dec 2, 2021

The difference is a lingering GL_ERROR

Code:

function draw() {
   gl.drawArrays(userSelectedPrimitive, ....);
   requestAnimationFrame(draw);
}
requestAnimationFrame(draw);

someElement.addEventListener('click', () => {
  // do some WebGL setup of new data
  if (gl.getError()) { throw up }     // This line suddenly gets an error for something
                                      // it didn't do because a new error appeared at draw time
});

kenrussell added a commit to kenrussell/WebGL that referenced this pull request Dec 4, 2021
Failing to set this was relying on currently-undefined behavior and
causing the tests to fail on Apple's M1.

Related to KhronosGroup#2818, KhronosGroup#2822 and KhronosGroup#3356.
kenrussell added a commit to kenrussell/WebGL that referenced this pull request Dec 4, 2021
Failing to set this was relying on currently-undefined behavior and
causing the tests to fail on Apple's M1.

Related to KhronosGroup#2818, KhronosGroup#2822 and KhronosGroup#3356.
kdashg pushed a commit that referenced this pull request Dec 6, 2021
Failing to set this was relying on currently-undefined behavior and
causing the tests to fail on Apple's M1.

Related to #2818, #2822 and #3356.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants