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

Test for having the same read and write buffer using a transform feedback #2349

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

Conversation

greggman
Copy link
Contributor

Firefox passes, Chrome fails

note: there are 2 lines marked as "FIXME" that I believe should be removed
assuming the spec changes so that gl.bindBuffer is less strict

@greggman
Copy link
Contributor Author

This is related to #2346

<!--

/*
** Copyright (c) 2015 The Khronos Group Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

wtu.glErrorShouldBe(gl, gl.NO_ERROR, "linking transform feedback shader should not set an error");

const inLoc = 0;
const outLoc = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

outLoc is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

wtu.glErrorShouldBe(gl, gl.NO_ERROR, "there should be no errors");

const expected = [2, 4, 6];
gl.bindBuffer(gl.ARRAY_BUFFER, dstBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming this is where Chrome fails? According to the current spec restrictions, Chrome generates an INVALID_OPERATION here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is one place where chrome fails if the "FIXME" lines are removed. Otherwise Chrome fails because it allows calling drawArrays even though the same buffers are bound to a VAO and and TF.

wtu.glErrorShouldBe(gl, gl.INVALID_OPERATION, "reading and writing to same buffer");

gl.bindBuffer(gl.ARRAY_BUFFER, dstBuffer);
wtu.checkFloatBuffer(gl, gl.ARRAY_BUFFER, expected, "should be the same as before as nothing has executed");
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it's executed, the result is still the same expected because it's the same program? So this is a useless and misleading check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If incorrectly executed (as it currently is in chrome) the result is 4, 8, 12 because it's manipulating the result from the first * 2.0 operation.

That result is probably GPU dependent since the whole problem is results of having the same input and output buffer is undefined. In any case it shouldn't execute and the buffer should still have 2, 4, 6 in it. It doesn't in Chrome currently on my MBP. Seems like the test is valid


}

function runFeedback(gl, prog, srcVAO, tf, dstBufferInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

dstBufferInfo isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@kenrussell
Copy link
Member

I'd like to hold off merging this until Chrome's behavior has been updated per #2346 .

…back

note: there are 2 lines marked as "FIXME" that I believe should be removed
assuming the spec changes so that `gl.bindBuffer` is less strict
@greggman greggman force-pushed the transform-feedback-same-read-and-write-buffer branch from 8b75c1b to 3ff07f5 Compare March 28, 2017 11:47
@zhenyao
Copy link
Contributor

zhenyao commented Mar 29, 2017

lgtm
but let's hold off until spec is updated as @kenrussell suggested

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