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

Assert vertex attr locations exist in GLDevice. #193

Closed
wants to merge 2 commits into from

Conversation

@toolness
Copy link
Contributor

toolness commented Jun 8, 2019

As @benpye discovered in #183 (comment), Pathfinder crashes on systems with Nvidia drivers at least in part because it's looking up locations for vertex attributes that don't exist in some shaders. In the spirit of "crash early, crash often", this PR adds assertions that ensure the vertex attribute and uniform locations we return are valid in GLDevice. This doesn't fix any problems, of course, it just makes it easier to be aware of their existence on non-Nvidia systems, and makes them easier to debug.

That said, I'm not sure if this actually the best solution: perhaps Device::get_vertex_attr() and Device::get_uniform() should actually return Option-wrapped values, so that code calling it can change its behavior based on whether the attributes/uniforms exist or not? I guess I'm not familiar enough with Pathfinder's codebase to know the best answer. Regardless, no worries if you decide not to merge this.

}; ck();
GLVertexAttr { attr }
if attr_int == -1 {

This comment has been minimized.

@toolness

toolness Jun 8, 2019

Author Contributor

We could wrap this if (and the corresponding one in get_uniform) in a #[cfg(debug_assertions)] if we're concerned about the performance. I figured maybe it'd be OK not to do so since presumably these locations don't have to be looked up too often, but I dunno.

@toolness
Copy link
Contributor Author

toolness commented Jun 8, 2019

One thing I noticed while running canvas_minimal with these assertions enabled is that GLSL uniforms which are declared in code but unused--and therefore optimized away during compilation--will trigger the assertion.

Currently this is happening when SolidTileMulticolorProgram is trying to retrieve the location of the PaintTextureSize uniform from tile_solid_multicolor.vs.glsl. It's defined, it's just optimized out because it's not actually used (well, at least with Nvidia drivers, perhaps others will keep them around?).

}; ck();
GLVertexAttr { attr }
if attr_int == -1 {
panic!("Vertex attribute '{}' does not exist in program", name);

This comment has been minimized.

@toolness

toolness Jun 8, 2019

Author Contributor

Oops, I didn't realize that we were actually munging the names before passing them to OpenGL... I'm not sure if it'd be more helpful to panic with the name of the munged or un-munged name here.

@pcwalton
Copy link
Collaborator

pcwalton commented Jun 11, 2019

So we probably shouldn't be checking uniforms, because -1 is a valid uniform location (though it does nothing). However, for attributes we certainly should.

I think these checks should be debug mode only debug_assert!(), etc.

@toolness toolness changed the title Assert uniform/vertex attr locations exist in GLDevice. Assert vertex attr locations exist in GLDevice. Jun 11, 2019
@toolness
Copy link
Contributor Author

toolness commented Jun 11, 2019

Oh wow, I didn't realize that -1 is a valid uniform location. Cool, I've just modified the PR to only pay attention to vertex attributes, and use debug_assert!()!

@pcwalton
Copy link
Collaborator

pcwalton commented Jun 12, 2019

So it turns out this change came for free with some of the Metal work I've been doing, and I cherry-picked it over to master here: dbf02fb

This PR is obsolete now, but thanks for letting me know about the issue and putting it together :)

@pcwalton pcwalton closed this Jun 12, 2019
@toolness toolness deleted the toolness:assert-locations-exist branch Jun 13, 2019
@toolness
Copy link
Contributor Author

toolness commented Jun 13, 2019

Oh cool, sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.