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

Added framebuffer and related attributes to XRWebGLLayer #23797

Merged
merged 4 commits into from Jul 20, 2019
Merged

Added framebuffer and related attributes to XRWebGLLayer #23797

merged 4 commits into from Jul 20, 2019

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jul 17, 2019 •
edited

Implement the framebuffer attribute of XRWebGLLayer.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes have tests

This change is Reviewable
@highfive
Copy link

highfive commented Jul 17, 2019

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webidls/XRWebGLLayer.webidl, components/script/dom/xrwebgllayer.rs, components/script/dom/bindings/error.rs, components/script/dom/domexception.rs, components/script/dom/xrsession.rs and 2 more
@highfive
Copy link

highfive commented Jul 17, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 17, 2019

The bit i'm not sure about is

        // Step 8.3. "Allocate and initialize resources compatible with session’s XR device,
        // including GPU accessible memory buffers, as required to support the compositing of layer."
        let texture = context.CreateTexture().ok_or(Error::Operation)?;
        let resolution =
            session.with_session(|session| session.recommended_framebuffer_resolution());
        let mut pixels = CustomAutoRooter::new(None);
        context.TexImage2D(
            constants::TEXTURE_2D,
            0,
            constants::RGBA,
            resolution.width,
            resolution.height,
            0,
            constants::RGBA,
            constants::UNSIGNED_BYTE,
            pixels.root(cx),
        )?;
        context.FramebufferTexture2D(
            constants::FRAMEBUFFER,
            constants::COLOR_ATTACHMENT0,
            constants::TEXTURE_2D,
            Some(&texture),
            0,
        );

which is a bit random. We can't make this up to the device, slightly annoyingly, as it needs to be run in WebGL, not GL, otherwise the GL state and the WebGL state will get out of sync.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 17, 2019

The matching webxr PR is servo/webxr#19
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 17, 2019

r? @Manishearth
@highfive highfive assigned Manishearth and unassigned paulrouget Jul 17, 2019
@asajeffrey asajeffrey force-pushed the asajeffrey:webxr-framebuffer branch from dd23686 to 85905ac Jul 18, 2019
@servo-wpt-sync servo-wpt-sync mentioned this pull request Jul 18, 2019
Closed
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jul 18, 2019

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17915.
@asajeffrey asajeffrey force-pushed the asajeffrey:webxr-framebuffer branch from 85905ac to 21add41 Jul 18, 2019
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jul 18, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17915.
@asajeffrey
Added framebuffer and related attributes to XRWebGLLayer
aa0a72d
@asajeffrey asajeffrey force-pushed the asajeffrey:webxr-framebuffer branch from 21add41 to d6d0416 Jul 18, 2019
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jul 18, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17915.
@asajeffrey asajeffrey marked this pull request as ready for review Jul 18, 2019
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 18, 2019

Added tests. @Manishearth this is ready for review!
@Manishearth
Manishearth approved these changes Jul 18, 2019
View changes
components/script/dom/webidls/DOMException.webidl Outdated
const unsigned short NOT_READABLE_ERR = 26;
const unsigned short OPERATION_ERR = 27;

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Jul 18, 2019

Member

should we be exposing this? I don't see these in Firefox.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 18, 2019

cc @alcooper91 re the webxr WPT tests. I think the changes in d6d0416 are spec-compliant. The only bit I wasn't sure about is the expected semantics of resolution in FakeXRViewInit, which I took to mean the resolution of each eye (i.,e. should be the size of the eye's viewport) not the native resolution of the whole device (https://immersive-web.github.io/webxr/#native-webgl-framebuffer-resolution).
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jul 18, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17915.
asajeffrey added 3 commits Jul 18, 2019
@asajeffrey
Added wpt webxr tests for XR layer's viewports and framebuffer
f2766cf
@asajeffrey
Dead code elimination
c08ef4c
@asajeffrey
Decoupled DOMErrorName from legacy error codes
Loading status checks…
23f15c9
@asajeffrey asajeffrey force-pushed the asajeffrey:webxr-framebuffer branch from d8e87c9 to 23f15c9 Jul 18, 2019
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 18, 2019

@jdm approved the changes to DOMException on IRC: https://mozilla.logbot.info/servo/20190718#c16478495

@bors-servo: r=Manishearth
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jul 18, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17915.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 19, 2019

@bors-servo r=Manishearth
@jdm jdm closed this Jul 19, 2019
@jdm jdm reopened this Jul 19, 2019
@jdm
Copy link
Member

jdm commented Jul 19, 2019

@bors-servo r=Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2019

📌 Commit 23f15c9 has been approved by Manishearth
@servo-wpt-sync servo-wpt-sync mentioned this pull request Jul 19, 2019
Merged
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jul 19, 2019

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17937.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2019

⌛ Testing commit 23f15c9 with merge 39c1429...
bors-servo added a commit that referenced this pull request Jul 19, 2019
@bors-servo
Auto merge of #23797 - asajeffrey:webxr-framebuffer, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
39c1429 
Added framebuffer and related attributes to XRWebGLLayer

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

Implement the `framebuffer` attribute of `XRWebGLLayer`.

---
<!-- 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 have tests

<!-- 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. -->

<!-- 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/23797)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2019

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Jul 19, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2019

⌛ Testing commit 23f15c9 with merge 1128f40...
bors-servo added a commit that referenced this pull request Jul 19, 2019
@bors-servo
Auto merge of #23797 - asajeffrey:webxr-framebuffer, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1128f40 
Added framebuffer and related attributes to XRWebGLLayer

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

Implement the `framebuffer` attribute of `XRWebGLLayer`.

---
<!-- 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 have tests

<!-- 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. -->

<!-- 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/23797)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: Manishearth
Pushing 1128f40 to master...
@bors-servo bors-servo merged commit 23f15c9 into servo:master Jul 20, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees

@Manishearth Manishearth

Labels
A-xr-zomg S-needs-tests
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@asajeffrey @highfive @servo-wpt-sync @jdm @bors-servo @Manishearth @paulrouget
You can’t perform that action at this time.