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

Add support for WebGL2 TexImage2D #26787

Merged
merged 1 commit into from Jun 18, 2020
Merged

Add support for WebGL2 TexImage2D #26787

merged 1 commit into from Jun 18, 2020

Conversation

@imiklos
Copy link
Contributor

imiklos commented Jun 4, 2020

Adds initial support for one of the WebGL2 TexImage2D call.

I've enabled the tests/wpt/webgl/tests/deqp/ tests.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix part of #26512
  • There are tests for these changes

@mmatyas @zakorgy @jdm
@highfive
Copy link

highfive commented Jun 4, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webgl2renderingcontext.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl
  • @KiChjang: components/script/dom/webgl2renderingcontext.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl
@imiklos
Copy link
Contributor Author

imiklos commented Jun 4, 2020

I'm getting INVALID_OPERATION from the GL side on every call. I'm stuck and not sure about the current implementation.
Looking for some advice.
@jdm
Copy link
Member

jdm commented Jun 4, 2020

I'm getting INVALID_OPERATION from the GL side on every call. I'm stuck and not sure about the current implementation.
Looking for some advice.

The reason this occurs is the pixel unpack buffer is bound when we invoke glTexImage2d. This causes us to treat the pointer passed as the pixel data buffer to be used as an offset into the bound pixel unpack buffer, instead.

The following patch works around this:

diff --git a/components/canvas/webgl_thread.rs b/components/canvas/webgl_thread.rs
index 9b7b4ed468..da9a502d1d 100644
--- a/components/canvas/webgl_thread.rs
+++ b/components/canvas/webgl_thread.rs
@@ -1586,6 +1586,11 @@ impl WebGLImpl {
                 );

                 gl.pixel_store_i(gl::UNPACK_ALIGNMENT, unpacking_alignment as i32);
+                let mut buffer = [0];
+                unsafe {
+                    gl.get_integer_v(gl::PIXEL_UNPACK_BUFFER_BINDING, &mut buffer);
+                }
+                gl.bind_buffer(gl::PIXEL_UNPACK_BUFFER, 0);
                 gl.tex_image_2d(
                     target,
                     level as i32,
@@ -1597,6 +1602,7 @@ impl WebGLImpl {
                     effective_data_type,
                     Some(&pixels),
                 );
+                gl.bind_buffer(gl::PIXEL_UNPACK_BUFFER, buffer[0] as u32);
             },
             WebGLCommand::TexSubImage2D {
                 target,

However, it would be more efficient to use a different model - instead of retrieving the pixels from the unpack buffer, then sending those pixels to be treated like any other 2d texture, I think we should make the WebGLRenderingContext::tex_image_2d take an enum argument like:

enum TexSource {
    Pixels(TexPixels),
    BufferOffset(i64),
}

and modify WebGLThread to handle that case appropriately.
@jdm jdm assigned jdm and unassigned nox Jun 4, 2020
@imiklos imiklos force-pushed the szeged:texi2d_3 branch from 4ece60d to 896d171 Jun 10, 2020
@imiklos
Copy link
Contributor Author

imiklos commented Jun 10, 2020

@jdm Thank you for your help! I've updated the pull request. A lot of test timeouts on my side while running the test-wpt on tests/wpt/webgl/tests/deqp/. I'm not sure if I should leave enabled the tests for deqp or not, in case if it stay enabled I've updated the test expectations for it.
@jdm
jdm requested changes Jun 10, 2020
View changes
components/canvas/webgl_thread.rs Outdated
let ptr = gl.map_buffer_range(
gl::PIXEL_UNPACK_BUFFER,
offset as isize,
length as isize,
gl::MAP_READ_BIT,
);
let data: &[u8] = unsafe { slice::from_raw_parts(ptr as _, length) };
gl.unmap_buffer(gl::PIXEL_UNPACK_BUFFER);

gl.pixel_store_i(gl::UNPACK_ALIGNMENT, unpacking_alignment as i32);
let mut buffer = [0];
unsafe {
gl.get_integer_v(gl::PIXEL_UNPACK_BUFFER_BINDING, &mut buffer);
}
gl.bind_buffer(gl::PIXEL_UNPACK_BUFFER, 0);

let pixels = prepare_pixels(
internal_format,
data_type,
size,
unpacking_alignment,
alpha_treatment,
y_axis_treatment,
None,
Cow::Borrowed(&data),
);
Comment on lines 1615 to 1640

This comment has been minimized.

Sign in to view
@jdm

jdm Jun 10, 2020

Member

glTexImage2D will automatically use the contents of the pixel unpack buffer if it's bound, so I suggest we modify the tex_image_2d API in sparkle to take an enum argument for the pixels. If the enum value is BufferOffset, then have we can have a debug assert that the pixel unpack buffer is bound and convert the offset to a pointer instead of https://github.com/servo/sparkle/blob/master/src/lib.rs#L120.
@jdm
Copy link
Member

jdm commented Jun 10, 2020

@bors-servo try=wpt
bors-servo added a commit that referenced this pull request Jun 10, 2020
@bors-servo
Auto merge of #26787 - szeged:texi2d_3, r=<try>
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…
19ab499 
[WIP] Add support for WebGL2 TexImage2D

Adds initial support for one of the WebGL2 `TexImage2D` call.

I've enabled the `tests/wpt/webgl/tests/deqp/` tests.

---
<!-- 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 part of #26512
- [x] There are tests for these changes

@mmatyas @zakorgy @jdm
<!-- 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 Jun 10, 2020

⌛ Trying commit 896d171 with merge 19ab499...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2020

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

jdm commented Jun 10, 2020

I suspect that the deqp just take longer than the default harness timeout in a debug build. Using something like --timeout-multiplier=4 locally might help.
@imiklos imiklos force-pushed the szeged:texi2d_3 branch from 896d171 to 9949c5d Jun 11, 2020
@imiklos
Copy link
Contributor Author

imiklos commented Jun 11, 2020

@jdm Thank you! I've updated the pull request with the changes you mentioned, also updated the test expectations according to the ci. This pull request is depends on servo/sparkle#28.
@jdm
jdm approved these changes Jun 11, 2020
View changes
@jdm jdm changed the title [WIP] Add support for WebGL2 TexImage2D Add support for WebGL2 TexImage2D Jun 11, 2020
@jdm
Copy link
Member

jdm commented Jun 11, 2020

@bors-servo r+
@imiklos
Copy link
Contributor Author

imiklos commented Jun 16, 2020

I've rebased the pull request and updated the test expectations.
@jdm
Copy link
Member

jdm commented Jun 16, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

⌛ Trying commit b477129 with merge ae18a22...
bors-servo added a commit that referenced this pull request Jun 16, 2020
@bors-servo
Auto merge of #26787 - szeged:texi2d_3, r=<try>
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…
ae18a22 
Add support for WebGL2 TexImage2D

Adds initial support for one of the WebGL2 `TexImage2D` call.

I've enabled the `tests/wpt/webgl/tests/deqp/` tests.

---
<!-- 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 part of #26512
- [x] There are tests for these changes

@mmatyas @zakorgy @jdm
<!-- 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 Jun 16, 2020

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

jdm commented Jun 16, 2020

I fear the deqp tests may be too inconsistent to enable :/
@imiklos
Add support for WebGL2 TexImage2D
Loading status checks…
6591fa5 
Adds initial support for one of the WebGL2 `TexImage2D` call.
@imiklos imiklos force-pushed the szeged:texi2d_3 branch from b477129 to 6591fa5 Jun 18, 2020
@imiklos
Copy link
Contributor Author

imiklos commented Jun 18, 2020

I've disabled the deqp tests.
@jdm
Copy link
Member

jdm commented Jun 18, 2020

@bors-servo r+
Thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Jun 18, 2020

📌 Commit 6591fa5 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jun 18, 2020

⌛ Testing commit 6591fa5 with merge 6b5455f...
bors-servo added a commit that referenced this pull request Jun 18, 2020
@bors-servo
Auto merge of #26787 - szeged:texi2d_3, r=jdm
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…
6b5455f 
Add support for WebGL2 TexImage2D

Adds initial support for one of the WebGL2 `TexImage2D` call.

I've enabled the `tests/wpt/webgl/tests/deqp/` tests.

---
<!-- 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 part of #26512
- [x] There are tests for these changes

@mmatyas @zakorgy @jdm
<!-- 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 Jun 18, 2020

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

jdm commented Jun 18, 2020

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

bors-servo commented Jun 18, 2020

⌛ Testing commit 6591fa5 with merge 1b55ab5...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 18, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 1b55ab5 to master...
@bors-servo bors-servo merged commit 1b55ab5 into servo:master Jun 18, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@imiklos @highfive @jdm @bors-servo @nox
You can’t perform that action at this time.