Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upDetect more GL limits. #123
Conversation
Don't forward GL parameter gets for constant limits. This avoids IPC traffic for unchanging constants that are determined when the GL context is created. These changes require servo/surfman#123. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20876. - [x] There are tests for these changes <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20884) <!-- Reviewable:end -->
| } | ||
|
|
||
| #[cfg(feature = "serde")] | ||
| impl<'de> Deserialize<'de> for GLLimits { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where D: Deserializer<'de> | ||
| { | ||
| let values = try!(<[_; 3]>::deserialize(deserializer)); | ||
| let values = try!(<[_; 10]>::deserialize(deserializer)); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| .serialize(serializer) | ||
| } | ||
| } | ||
|
|
||
| macro_rules! gl_integer { | ||
| ($gl:ident, $pname:ident) => { |
This comment has been minimized.
This comment has been minimized.
kvark
May 31, 2018
Member
what's a particular reason for this to be a macro as opposed to a function?
This comment has been minimized.
This comment has been minimized.
| let max_texture_image_units = gl_integer!(gl_, MAX_TEXTURE_IMAGE_UNITS); | ||
| let max_vertex_texture_image_units = gl_integer!(gl_, MAX_VERTEX_TEXTURE_IMAGE_UNITS); | ||
|
|
||
| // Based off of https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/dom/canvas/WebGLContextValidate.cpp#523-558 |
This comment has been minimized.
This comment has been minimized.
kvark
May 31, 2018
Member
jeez, I would not want to accept this code if I was reviewing it:
We are now going to try to read GL_MAX_VERTEX_OUTPUT_COMPONENTS
* and GL_MAX_FRAGMENT_INPUT_COMPONENTS, however these constants
* only entered the OpenGL standard at OpenGL 3.2. So we will try
* reading, and check OpenGL error for INVALID_ENUM.
I think it's a bad idea. If a constant was introduced in GL 3.2, wouldn't it be better for us to just check the context version and go from that?
For an example of what can be wrong with this code: an error could be triggered by something else before our fallible check, and we'd think that it occurred due to the check.
This comment has been minimized.
This comment has been minimized.
kvark
May 31, 2018
Member
On a second thought, maybe checking for the context version is not that trivial, since it's returned as string (d'oh, GL).
I still don't like this approach, but I'd not want you to get stuck finding a better way here. At the very least, could we ensure the fallible integer queries expect GL_INVALID_ENUM error and nothing else?
|
Heh, I magically started receiving github bugmail again for this repo :) |
| } | ||
|
|
||
| fn gl_fallible_integer(gl_: &gl::Gl, pname: gl::GLenum) -> Result<u32, ()> { | ||
| let mut val = [0]; |
This comment has been minimized.
This comment has been minimized.
emilio
May 31, 2018
Member
You should probably debug_assert that gl_.get_error() is gl::NO_ERROR here, otherwise you'll reject known values.
| unsafe { | ||
| $gl.get_integer_v(gl::$pname, &mut val); | ||
| } | ||
| assert_eq!($gl.get_error(), gl::NO_ERROR, "Error retrieving {}", stringify!($pname)); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Comments addressed. |
|
Thank you for addressing the concerns! |
Don't forward GL parameter gets for constant limits. This avoids IPC traffic for unchanging constants that are determined when the GL context is created. These changes require servo/surfman#123. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20876. - [x] There are tests for these changes <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20884) <!-- Reviewable:end -->
jdm commentedMay 30, 2018
No description provided.