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

webgl: Add getParameter(GL_FRAMEBUFFER_BINDING) support #12852

Closed
anholt opened this issue Aug 13, 2016 · 1 comment
Closed

webgl: Add getParameter(GL_FRAMEBUFFER_BINDING) support #12852

anholt opened this issue Aug 13, 2016 · 1 comment

Comments

@anholt
Copy link
Contributor

@anholt anholt commented Aug 13, 2016

I've been working on adding support for this getter. Lack of support for it is currently producing a bunch of INVALID_ENUM in unrelated webgl tests due to its use in some shared utility code in the tests. I'm very new to Rust programming, so it's going slowly.

Current code is at:

(ignore the DO NOT PUSH change in servo, I've been rebasing it out before submission of other patches)

Current questions:

  • How does the webrender_traits change get merged safely given that it will produce compile errors in servo until the new enum cases are included?
  • What's the right way to wrap my HashMap so that it's mutable at CreateFramebuffer()/DeleteFramebuffer() time, but can still store JS values?
  • Is this even the right approach? My HashMap is storing JS<> values, which I think means that they'll keep the WebGLFramebuffers alive for as long as they're present in the hash. The intent of WebGL seems to be that they'd be destroyed by the JS GC (once they're also unbound from their binding points in the GL) This hasn't impacted our other refs because they're only to the current thing in the binding point.

Actually, on the last point: I don't need to be able to map any FramebufferId to a framebuffer, I need to be able to map the current bound framebufferid to a framebuffer. I could track the framebuffer like we do for the current bound program/texture2d/etc!

@emilio
Copy link
Member

@emilio emilio commented Aug 13, 2016

How does the webrender_traits change get merged safely given that it will produce compile errors in servo until the new enum cases are included?

Servo has a locked dependencies, so it won't break the build.

What's the right way to wrap my HashMap so that it's mutable at CreateFramebuffer()/DeleteFramebuffer() time, but can still store JS values?

Wrapping in a DOMRefCell and calling borrow_mut() is the usual way of doing that.

Is this even the right approach? My HashMap is storing JS<> values, which I think means that they'll keep the WebGLFramebuffers alive for as long as they're present in the hash. The intent of WebGL seems to be that they'd be destroyed by the JS GC (once they're also unbound from their binding points in the GL) This hasn't impacted our other refs because they're only to the current thing in the binding point.

Yeah, that's true... DeleteFramebuffer would need to have some kind of manual refcounting, or something, and that seems like too much churn for this.

Actually, on the last point: I don't need to be able to map any FramebufferId to a framebuffer, I need to be able to map the current bound framebufferid to a framebuffer. I could track the framebuffer like we do for the current bound program/texture2d/etc!

Actually, that sounds a lot more easy, and reasonable :-)

anholt added a commit to anholt/servo that referenced this issue Aug 14, 2016
To do this, we need to keep a map of GL names (encapsulated as
WebGLFramebufferId) to WebGLFramebuffer objects so that we can return
the right type.

Fixes servo#12852
bors-servo added a commit that referenced this issue Aug 14, 2016
Implement GL_FRAMEBUFFER_BINDING (and fix a VertexAttrib1fv typo)

<!-- Please describe your changes on the following line: -->
This PR fixes many webgl conformance test failures due to missing support for a getParameter() call in the webgl test utils.

---
<!-- 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] These changes fix #12852

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- 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/12857)
<!-- Reviewable:end -->
anholt added a commit to anholt/servo that referenced this issue Aug 14, 2016
To do this, we need to keep a map of GL names (encapsulated as
WebGLFramebufferId) to WebGLFramebuffer objects so that we can return
the right type.

Fixes servo#12852
bors-servo added a commit that referenced this issue Aug 14, 2016
Implement GL_FRAMEBUFFER_BINDING (and fix a VertexAttrib1fv typo)

<!-- Please describe your changes on the following line: -->
This PR fixes many webgl conformance test failures due to missing support for a getParameter() call in the webgl test utils.

---
<!-- 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] These changes fix #12852

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- 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/12857)
<!-- Reviewable:end -->
samuknet added a commit to samuknet/servo that referenced this issue Sep 6, 2016
To do this, we need to keep a map of GL names (encapsulated as
WebGLFramebufferId) to WebGLFramebuffer objects so that we can return
the right type.

Fixes servo#12852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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