Skip to content

/ servo

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

Implement GL_FRAMEBUFFER_BINDING (and fix a VertexAttrib1fv typo) #12857

Merged
merged 2 commits into from Aug 14, 2016
Merged

Implement GL_FRAMEBUFFER_BINDING (and fix a VertexAttrib1fv typo) #12857

merged 2 commits into from Aug 14, 2016

Conversation

@anholt
Copy link
Contributor

anholt commented Aug 14, 2016 •
edited by larsbergstrom

This PR fixes many webgl conformance test failures due to missing support for a getParameter() call in the webgl test utils.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12852
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@anholt
webgl: Fix a typo in VertexAttrib1fv handling.
a21bbb7 
The VertexAttrib[234]fv compare to the same size as in their function
name.  This wasn't noticed becacuse this function isn't connected from
the .webidl yet.
@highfive
Copy link

highfive commented Aug 14, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webglrenderingcontext.rs
  • @emilio: components/script/dom/webglrenderingcontext.rs
@highfive
Copy link

highfive commented Aug 14, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@anholt
Copy link
Contributor Author

anholt commented Aug 14, 2016

Note: Let me know if including unrelated commits in a PR like this is inappropriate, and I can split it out.
@KiChjang
Copy link
Member

KiChjang commented Aug 14, 2016

r? @emilio
@highfive highfive assigned emilio and unassigned KiChjang Aug 14, 2016
@emilio
Copy link
Member

emilio commented Aug 14, 2016

@bors-servo: r+

Thanks for doing this @anholt! :)
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2016

📌 Commit 17a9ac9 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2016

⌛ Testing commit 17a9ac9 with merge fc101a7...
bors-servo added a commit that referenced this pull request Aug 14, 2016
@bors-servo
Auto merge of #12857 - anholt:webgl-framebuffer-binding-2, r=emilio
Loading status checks…
fc101a7 
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 -->
components/script/dom/webglrenderingcontext.rs
@@ -1788,7 +1812,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
// https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
fn VertexAttrib1fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
if let Some(data_vec) = array_buffer_view_to_vec_checked::<f32>(data) {
if data_vec.len() < 4 {
if data_vec.len() < 1 {

This comment has been minimized.

Sign in to view
@emilio

emilio Aug 14, 2016

Member

Also, good catch here :)
@emilio
Copy link
Member

emilio commented Aug 14, 2016

On 08/13/2016 05:37 PM, Eric Anholt wrote:

Note: Let me know if including unrelated commits in a PR like this is
inappropriate, and I can split it out.

Oh, also, I don't really mind this, it's fine if it doesn't diverge a
lot from the main code in the PR. Other reviewers might be more picky,
but I think it's not worth the overhead of two different PRs for this :)

Thanks again!
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2016

💔 Test failed - mac-rel-wpt
@emilio
Copy link
Member

emilio commented Aug 14, 2016 

Tests with unexpected results:
  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/attribs/gl-disabled-vertex-attrib.html:
  │ FAIL [expected PASS] WebGL test #15: at (0, 0) expected: 0,255,0,255 was 255,255,255,255
  │   → assert_true: at (0, 0) expected: 0,255,0,255 was 255,255,255,255 expected true got false
  │ 
  │ reportTestResultsToHarness/<@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ test@http://web-platform.test:8000/resources/testharness.js:500:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:86:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:152:5
  │ checkCanvasRectColor@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1046:9
  │ checkCanvasRect@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1072:3
  │ checkCanvas@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1095:3
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/attribs/gl-disabled-vertex-attrib.html:27:3

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/default-texture.html:
  │ FAIL [expected PASS] WebGL test #0: at (0, 0) expected: 0,0,0,255 was 0,0,128,191
  │   → assert_true: at (0, 0) expected: 0,0,0,255 was 0,0,128,191 expected true got false
  │

Seems like legit consequences of the new validation an unexpected consequence of the typo fix? I'll double check as fast as I can have a build. Unfortunately the way the wpt and webgl test suite integration can be flaky in this sense (marks not-run tests as passing), but it's in order to minimize pain at the time of updating the test suite.

My intuition is that these tests weren't running without this patch, I'll verify it and if that's the case, I'll r+ once the expectations are updated, thanks!
@anholt
Copy link
Contributor Author

anholt commented Aug 14, 2016 •
edited

gl-disabled-vertex was me dropping an apparently macos-specific failure on subtest 15. Can we flag subtests as os-specific failures?
@emilio
Copy link
Member

emilio commented Aug 14, 2016

@anholt: yes, you can use the if os == "osx": syntax that was in the __DIR__.ini in individual subsets.
@anholt
Copy link
Contributor Author

anholt commented Aug 14, 2016

And the other one was maybe just #11618 :/
@emilio
Copy link
Member

emilio commented Aug 14, 2016

Yep! That seems reasonable. r=me with that expectation change
@anholt
webgl: Add support for getParameter(GL_FRAMEBUFFER_BINDING).
Loading status checks…
6651db8 
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 #12852
@anholt anholt force-pushed the anholt:webgl-framebuffer-binding-2 branch from 17a9ac9 to 6651db8 Aug 14, 2016
@anholt
Copy link
Contributor Author

anholt commented Aug 14, 2016

Now to go apply this expectation syntax to the "enable webgl testing on linux" series.
@emilio
Copy link
Member

emilio commented Aug 14, 2016

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2016

📌 Commit 6651db8 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2016

⌛ Testing commit 6651db8 with merge 8472699...
bors-servo added a commit that referenced this pull request Aug 14, 2016
@bors-servo
Auto merge of #12857 - anholt:webgl-framebuffer-binding-2, r=emilio
Loading status checks…
8472699 
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 -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit 6651db8 into servo:master Aug 14, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Aug 14, 2016
Closed
4 of 5 tasks complete
@anholt anholt deleted the anholt:webgl-framebuffer-binding-2 branch Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

webgl: Add getParameter(GL_FRAMEBUFFER_BINDING) support
5 participants
@anholt @highfive @KiChjang @emilio @bors-servo
You can’t perform that action at this time.