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 support for some more WebGL2 limit parameters #26265

Closed
wants to merge 1 commit into from

Conversation

@mmatyas
Copy link
Contributor

mmatyas commented Apr 22, 2020

Adds support for the following new WebGL2 GetParameter values:

  • MAX_ELEMENT_INDEX
  • MAX_ELEMENTS_INDICES
  • MAX_ELEMENTS_VERTICES
  • MAX_FRAGMENT_INPUT_COMPONENTS
  • MAX_SAMPLES
  • MAX_SERVER_WAIT_TIMEOUT
  • MAX_TEXTURE_LOD_BIAS
  • MAX_VARYING_COMPONENTS
  • MAX_VERTEX_OUTPUT_COMPONENTS

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

cc @jdm @zakorgy


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes
@highfive
Copy link

highfive commented Apr 22, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webgl2renderingcontext.rs
  • @KiChjang: components/script/dom/webgl2renderingcontext.rs
@highfive
Copy link

highfive commented Apr 22, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm
Copy link
Member

jdm commented Apr 22, 2020

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2020

📌 Commit 9799870 has been approved by jdm

@highfive highfive assigned jdm and unassigned nox Apr 22, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2020

Testing commit 9799870 with merge 62c10d6...

bors-servo added a commit that referenced this pull request Apr 22, 2020
Add support for some more WebGL2 limit parameters

Adds support for the following new WebGL2 GetParameter values:

- `MAX_ELEMENT_INDEX`
- `MAX_ELEMENTS_INDICES`
- `MAX_ELEMENTS_VERTICES`
- `MAX_FRAGMENT_INPUT_COMPONENTS`
- `MAX_SAMPLES`
- `MAX_SERVER_WAIT_TIMEOUT`
- `MAX_TEXTURE_LOD_BIAS`
- `MAX_VARYING_COMPONENTS`
- `MAX_VERTEX_OUTPUT_COMPONENTS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Apr 22, 2020

  ▶ CRASH [expected OK] /_webgl/conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
  │ 
  │ _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
  │ called `Option::unwrap()` on a `None` value (thread WebGL thread, at components/canvas/webgl_limits.rs:236)
  │ stack backtrace:
  │    0: <servo::backtrace::Print as core::fmt::Debug>::fmt
  │    1: core::fmt::write
  │    2: std::io::Write::write_fmt
  │    3: servo::backtrace::print
  │    4: servo::main::{{closure}}
  │    5: std::panicking::rust_panic_with_hook
  │    6: rust_begin_unwind
  │    7: core::panicking::panic_fmt
  │    8: core::panicking::panic
  │    9: <canvas_traits::webgl::GLLimits as canvas::webgl_limits::GLLimitsDetect>::detect
  │   10: canvas::webgl_thread::WebGLThread::handle_msg
  │   11: canvas::webgl_thread::WebGLThread::process
  │   12: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   13: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   14: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │   15: std::sys::unix::thread::Thread::new::thread_start
  │   16: _pthread_body
  │   17: _pthread_start

I think we need to use try_get_float instead of get_float for some of the new limits.

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 23, 2020

Hm, yet another graphics issue I can't seem to reproduce... According to the spec, the only float value is MAX_TEXTURE_LOD_BIAS, the rest of them are integers. The only difference I see that might count is that MAX_ELEMENT_INDEX is a GLint64, so probably we should use glGetInteger64v. However, we already have 64bit values queried with get_integer (eg. MAX_UNIFORM_BLOCK_SIZE), so if that would be the problem, then the tests should have failed already too.

I'll see if I can add the 64bit integer method to sparkle, we can then try it out if that's indeed the issue here.

@jdm
Copy link
Member

jdm commented Apr 23, 2020

I checked on my mac, and it's panicking on max_element_index = gl.get_integer(gl::MAX_ELEMENT_INDEX);.

@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_getparam2 branch from 9799870 to 3c81664 Apr 27, 2020
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 27, 2020

Thanks for checking! I've updated the call for MAX_ELEMENT_INDEX and, just in case, the rest of the 64bit values too.

PS. I'm not sure about returning it as UInt32 to the JS side though, should it be eg. DoubleValue instead?

@mmatyas mmatyas mentioned this pull request Apr 27, 2020
3 of 3 tasks complete
@jdm
Copy link
Member

jdm commented Apr 27, 2020

Yes, DoubleValue would be safer.

@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_getparam2 branch from 3c81664 to 68ad98c Apr 27, 2020
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 27, 2020

Ok, updated to use DoubleValues. I've also changed the CLIENT/SERVER_WAIT_TIMEOUTs to it too.

@jdm
Copy link
Member

jdm commented Apr 27, 2020

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2020

📌 Commit 68ad98c has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2020

Testing commit 68ad98c with merge a4e636e...

bors-servo added a commit that referenced this pull request Apr 27, 2020
Add support for some more WebGL2 limit parameters

Adds support for the following new WebGL2 GetParameter values:

- `MAX_ELEMENT_INDEX`
- `MAX_ELEMENTS_INDICES`
- `MAX_ELEMENTS_VERTICES`
- `MAX_FRAGMENT_INPUT_COMPONENTS`
- `MAX_SAMPLES`
- `MAX_SERVER_WAIT_TIMEOUT`
- `MAX_TEXTURE_LOD_BIAS`
- `MAX_VARYING_COMPONENTS`
- `MAX_VERTEX_OUTPUT_COMPONENTS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Apr 27, 2020

Curious; we're still panicking when calling max_element_index = gl.get_integer64(gl::MAX_ELEMENT_INDEX);.

@jdm
Copy link
Member

jdm commented Apr 27, 2020

I suspect the issue is GL_MAX_ELEMENT_INDEX is accepted by pname only if the GL version is 4.3 or greater. from https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glGet.xhtml. https://searchfox.org/mozilla-central/source/dom/canvas/WebGL2ContextState.cpp#102-106 from Firefox has special handling for this attribute. We only request GL 3.2 in WebGLVersion::to_surfman_version for WebGL2.

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 27, 2020

Hm, if I remember correctly we only have extension querying at the moment; can/should we make the same isSupported route to the GL layer and back? Or perhaps we should just return some max value for now? Maybe keep the field as an Option when the GlLimits is created?

@jdm
Copy link
Member

jdm commented Apr 27, 2020

I suggest using try_get_integer64 and unwrap_or(0).

Adds support for the following new WebGL2 GetParameter values:

- `MAX_ELEMENT_INDEX`
- `MAX_ELEMENTS_INDICES`
- `MAX_ELEMENTS_VERTICES`
- `MAX_FRAGMENT_INPUT_COMPONENTS`
- `MAX_SAMPLES`
- `MAX_SERVER_WAIT_TIMEOUT`
- `MAX_TEXTURE_LOD_BIAS`
- `MAX_VARYING_COMPONENTS`
- `MAX_VERTEX_OUTPUT_COMPONENTS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2
@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_getparam2 branch from 68ad98c to e0c4d49 Apr 28, 2020
@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 28, 2020

Ok, updated.

@jdm
Copy link
Member

jdm commented Apr 28, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

📌 Commit e0c4d49 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit e0c4d49 with merge 84e2252...

bors-servo added a commit that referenced this pull request Apr 28, 2020
Add support for some more WebGL2 limit parameters

Adds support for the following new WebGL2 GetParameter values:

- `MAX_ELEMENT_INDEX`
- `MAX_ELEMENTS_INDICES`
- `MAX_ELEMENTS_VERTICES`
- `MAX_FRAGMENT_INPUT_COMPONENTS`
- `MAX_SAMPLES`
- `MAX_SERVER_WAIT_TIMEOUT`
- `MAX_TEXTURE_LOD_BIAS`
- `MAX_VARYING_COMPONENTS`
- `MAX_VERTEX_OUTPUT_COMPONENTS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Apr 29, 2020

Fixed the remaining panic in #26354.

@jdm jdm closed this Apr 29, 2020
bors-servo added a commit that referenced this pull request Apr 30, 2020
Add support for even more WebGL2 GetParameter enums

Adds support for the following new WebGL2 GetParameter values:

- `PACK_ROW_LENGTH`
- `PACK_SKIP_PIXELS`
- `PACK_SKIP_ROWS`
- `RASTERIZER_DISCARD`
- `UNPACK_IMAGE_HEIGHT`
- `UNPACK_ROW_LENGTH`
- `UNPACK_SKIP_IMAGES`
- `UNPACK_SKIP_PIXELS`
- `UNPACK_SKIP_ROWS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

Depends on #26265 because they touch the same test file.

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Apr 30, 2020
Add support for even more WebGL2 GetParameter enums

Adds support for the following new WebGL2 GetParameter values:

- `PACK_ROW_LENGTH`
- `PACK_SKIP_PIXELS`
- `PACK_SKIP_ROWS`
- `RASTERIZER_DISCARD`
- `UNPACK_IMAGE_HEIGHT`
- `UNPACK_ROW_LENGTH`
- `UNPACK_SKIP_IMAGES`
- `UNPACK_SKIP_PIXELS`
- `UNPACK_SKIP_ROWS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

Depends on #26265 because they touch the same test file.

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Apr 30, 2020
Add support for even more WebGL2 GetParameter enums

Adds support for the following new WebGL2 GetParameter values:

- `PACK_ROW_LENGTH`
- `PACK_SKIP_PIXELS`
- `PACK_SKIP_ROWS`
- `RASTERIZER_DISCARD`
- `UNPACK_IMAGE_HEIGHT`
- `UNPACK_ROW_LENGTH`
- `UNPACK_SKIP_IMAGES`
- `UNPACK_SKIP_PIXELS`
- `UNPACK_SKIP_ROWS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

Depends on #26265 because they touch the same test file.

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Apr 30, 2020
Add support for even more WebGL2 GetParameter enums

Adds support for the following new WebGL2 GetParameter values:

- `PACK_ROW_LENGTH`
- `PACK_SKIP_PIXELS`
- `PACK_SKIP_ROWS`
- `RASTERIZER_DISCARD`
- `UNPACK_IMAGE_HEIGHT`
- `UNPACK_ROW_LENGTH`
- `UNPACK_SKIP_IMAGES`
- `UNPACK_SKIP_PIXELS`
- `UNPACK_SKIP_ROWS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

Depends on #26265 because they touch the same test file.

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Apr 30, 2020
Add support for even more WebGL2 GetParameter enums

Adds support for the following new WebGL2 GetParameter values:

- `PACK_ROW_LENGTH`
- `PACK_SKIP_PIXELS`
- `PACK_SKIP_ROWS`
- `RASTERIZER_DISCARD`
- `UNPACK_IMAGE_HEIGHT`
- `UNPACK_ROW_LENGTH`
- `UNPACK_SKIP_IMAGES`
- `UNPACK_SKIP_PIXELS`
- `UNPACK_SKIP_ROWS`

See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

<!-- Please describe your changes on the following line: -->

Depends on #26265 because they touch the same test file.

cc @jdm @zakorgy

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
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

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