Skip to content

Conversation

@lexaknyazev
Copy link
Member

Fixes this test on Chrome due to #2818.

Copy link
Contributor

@kdashg kdashg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gl_PointSize is only valid when positive (and within ALIASED_POINT_SIZE_RANGE), so setting gl_PointSize shouldn't matter here, as it would be unsafe for WebGL to treat these as undefined.

WebGL doesn't specify what happens if gl_PointSize is unset, but gl_PointSize should always be being clamped to within the point size range, which does not allow zero.

Also, transform feedback with needs to run on drawArrays(POINTS) regardless of gl_Position or gl_PointSize, so skipping the draw (which is what it sounds like is happening?) here would be invalid.

@lexaknyazev
Copy link
Member Author

I think that this particular test (active vs bound texture) shouldn't depend on an unrelated implementation issue that could be resolved separately.

@lexaknyazev
Copy link
Member Author

gl_PointSize should always be being clamped to within the point size range, which does not allow zero.

GLES doesn't suggest that (from 3.0 section 3.4):

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.

@kdashg
Copy link
Contributor

kdashg commented Mar 5, 2019

I filed #2822 to add an explicit test for this.
Curiously, with transform-feedback enabled, chrome passes this test.
This sounds a lot like Chrome is being over-eager to skip draw calls.

@kdashg
Copy link
Contributor

kdashg commented Mar 5, 2019

Let's talk about this in that PR, but I generally don't like working around implementation bugs in unrelated tests.

@kdashg
Copy link
Contributor

kdashg commented Mar 5, 2019

To keep coverage, I would like to resolve #2822 before merging this.

@kenrussell
Copy link
Member

This test was not intended to test the undefined behavior of failing to write gl_PointSize in a vertex shader used to draw points. Let's continue to discuss that in #2818 and #2822 . Merging this now.

@kenrussell kenrussell merged commit f9e736f into KhronosGroup:master Mar 8, 2019
@lexaknyazev lexaknyazev deleted the fix-texture-active-bind-test branch March 18, 2019 13:22
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 11, 2019
conformance/textures/misc/texture-active-bind.html
was fixed on Windows in
KhronosGroup/WebGL#2819

LUMINANCE/ALPHA emulation fixes in http://crrev.com/c/1545512
fixed these tests on Android Qualcomm:
conformance2/textures/misc/tex-new-formats.html
conformance2/textures/misc/copy-texture-image-luma-format.html
deqp/functional/gles3/textureformat/unsized_2d_array.html
deqp/functional/gles3/textureformat/unsized_3d.html

TBR: kbr@chromium.org
Bug: 931006, 906740, 947236
Change-Id: I5c3a79c17b6a263b1a52f0a9fa646de591e370b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1559756
Reviewed-by: James Darpinian <jdarpinian@chromium.org>
Commit-Queue: James Darpinian <jdarpinian@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649767}
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.

3 participants