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 WebGL2 framebuffer attachments #25784

Merged

Conversation

@mmatyas
Copy link
Contributor

mmatyas commented Feb 17, 2020

Adds an initial implementation for the framebuffer attachments introduced with WebGL2 and the related enums and constrains checks.

Depends on #25765.

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 Feb 17, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webgltexture.rs, components/script/dom/webglframebuffer.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webglrenderbuffer.rs
  • @jgraham: tests/wpt/webgl/meta/conformance2/rendering/framebuffer-completeness-unaffected.html.ini, tests/wpt/webgl/meta/conformance2/rendering/framebuffer-texture-level1.html.ini, tests/wpt/webgl/meta/conformance2/state/gl-object-get-calls.html.ini, tests/wpt/webgl/meta/conformance2/renderbuffers/framebuffer-test.html.ini, tests/wpt/webgl/meta/conformance2/renderbuffers/framebuffer-object-attachment.html.ini and 2 more
  • @KiChjang: components/script/dom/webgltexture.rs, components/script/dom/webglframebuffer.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webglrenderbuffer.rs
@highfive
Copy link

highfive commented Feb 17, 2020

warning Warning warning

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

Manishearth commented Feb 18, 2020

r? @nox or @jdm

I'm less familiar with a lot of the webgl code

@highfive highfive assigned nox and unassigned Manishearth Feb 18, 2020
@jdm jdm assigned jdm and unassigned nox Feb 18, 2020
@nox
Copy link
Member

nox commented Feb 19, 2020

The implementation looks good, but could you please amend the three commits to recreate the expectations from scratch? Your fixes look like they only added failures because the test cases in the Khronos test suites are weirdly numbered programatically, and every time we support new APIs their numbers changes and most of the previous expectations become garbage.

constants::DEPTH_COMPONENT24 |
constants::DEPTH_COMPONENT32F |
constants::DEPTH24_STENCIL8 |
constants::DEPTH32F_STENCIL8 |

This comment has been minimized.

Copy link
@jdm

jdm Feb 19, 2020

Member

I'm concerned that adding these formats here will make them usable from webgl1 contexts when they shouldn't be. Perhaps we should have an argument the caller passes in to indicate which kind of context it is?

This comment has been minimized.

Copy link
@mmatyas

mmatyas Feb 20, 2020

Author Contributor

Hm you might be right, I've added an additional check for this case.

@@ -213,91 +219,138 @@ impl WebGLFramebuffer {
self.size.get()
}

fn check_attachment_constrains(

This comment has been minimized.

Copy link
@jdm

jdm Feb 19, 2020

Member

Should this be check_attachment_constraints instead?

This comment has been minimized.

Copy link
@mmatyas

mmatyas Feb 20, 2020

Author Contributor

Indeed, thanks!

@jdm
Copy link
Member

jdm commented Feb 19, 2020

Sorry, left review comments yesterday and forgot to submit them.

@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_framebuf_attachments branch from d54285b to 1bea2c7 Feb 20, 2020
@mmatyas
Copy link
Contributor Author

mmatyas commented Feb 20, 2020

@nox Ah, I might have accidentally left some of such tests in the commits, will fix it.

@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_framebuf_attachments branch from 1bea2c7 to d7bc0e0 Feb 20, 2020
@mmatyas
Copy link
Contributor Author

mmatyas commented Feb 20, 2020

Also rebased to master, now that the earlier patches have landed.

@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_framebuf_attachments branch from d7bc0e0 to 08b675f Feb 20, 2020
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Feb 20, 2020

Error syncing changes upstream. Logs saved in error-snapshot-1582203730577.

@mmatyas mmatyas force-pushed the szeged:mmatyas__webgl_fns_framebuf_attachments branch from 08b675f to 974b0f3 Feb 20, 2020
@jdm
jdm approved these changes Feb 20, 2020
@jdm
Copy link
Member

jdm commented Feb 20, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2020

📌 Commit 974b0f3 has been approved by jdm

@jdm
Copy link
Member

jdm commented Feb 21, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Testing commit b41805a with merge 05b6726...

bors-servo added a commit that referenced this pull request Feb 21, 2020
…, r=jdm

Add support for WebGL2 framebuffer attachments

Adds an initial implementation for the framebuffer attachments introduced with WebGL2 and the related enums and constrains checks.

<!-- Please describe your changes on the following line: -->
Depends on #25765.

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 Feb 21, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Feb 21, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Testing commit b41805a with merge 947a602...

bors-servo added a commit that referenced this pull request Feb 21, 2020
…, r=jdm

Add support for WebGL2 framebuffer attachments

Adds an initial implementation for the framebuffer attachments introduced with WebGL2 and the related enums and constrains checks.

<!-- Please describe your changes on the following line: -->
Depends on #25765.

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 Feb 21, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Feb 21, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Testing commit b41805a with merge afb1faf...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing afb1faf to master...

@bors-servo bors-servo merged commit afb1faf into servo:master Feb 21, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Feb 21, 2020
…m, r=jdm

Add support for WebGL2 getFramebufferAttachmentParameter

Implements the WebGL2 variant of `getFramebufferAttachmentParameter` and the related parameter checks.

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

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

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 Feb 21, 2020
…m, r=jdm

Add support for WebGL2 getFramebufferAttachmentParameter

Implements the WebGL2 variant of `getFramebufferAttachmentParameter` and the related parameter checks.

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

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

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

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