Add support for WebGL2 TexStorage2D #26023

mmatyas · 2020-03-24T12:57:44Z

Adds initial support for the WebGL2 TexStorage2D call, adds support for the related texture enums and enables some of the texture tests.

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

cc @jdm @zakorgy

A couple of notes:

Currently tests/wpt/webgl/meta/conformance2/textures is disabled for testing; this patch enables the misc subdirectory and disables the rest, which is why there are so many new test files. The primary test for TexStorage2D is conformance2/textures/misc/tex-storage-2d.html.
Most of the test passes, except the "texSubImage2D should succeed on immutable texture as long as the format is compatible" checks. This produces INVALID_OPERATIONs here because the texture's image info is not set at that level. I'm probably forgetting to call set_image_infos_at_level somewhere but I'm not sure where.
There's a duplication of the internal texture format list with eg. the renderbuffer code, I wonder if there would be a good common place for it.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

highfive · 2020-03-24T12:57:50Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webgltexture.rs, components/script/dom/webglrenderingcontext.rs
@KiChjang: components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webgltexture.rs, components/script/dom/webglrenderingcontext.rs

components/script/dom/webgltexture.rs

mmatyas · 2020-03-26T11:47:13Z

Thanks, I was experimenting with adding a self.initialize step, but unfortunately I still run into the same invalid operation issue. I've included the changes as a separate work-in-progress commit, do you have some ideas about where it fails?

jdm · 2020-03-27T14:34:08Z

components/script/dom/webgltexture.rs

+            height as u32,
+            1,
+            TexFormat::from_gl_constant(internal_format).unwrap(),
+            levels as u32,


I stepped through the test failure in a debugger and realized that we are initializing a single level of the texture, but TexStorage2D expects all levels from 0 to levels to be initialized. See page 137 of https://www.khronos.org/registry/OpenGL/specs/es/3.0/es_spec_3.0.pdf#nameddest=section-3.8.4.

Ah you're right! Indeed I should've read the spec a bit more closely, I'll do some restructuring now.

mmatyas · 2020-04-06T14:33:42Z

So last week I've started restructuring things, which at first looked to be simple, but then I've ended up in the rabbit hole of dealing with texture formats: to store the format in the texture, I had to convert the integer to a TexFormat, but for that I had to add the enums for all the newly acceptable formats, but because the formats were now implemented, they were also accessible from each and every WebGL function, and so I also needed to implement the additional texture format and data type combination checks and update the verification of the texture related functions.

Anyway, there are still quite a few failing test cases, but here's the current WIP status of this patch. I've run a tidy on it but not updated or regenerated all the tests yet.

bors-servo · 2020-04-07T21:46:27Z

☔ The latest upstream changes (presumably #26135) made this pull request unmergeable. Please resolve the merge conflicts.

mmatyas · 2020-04-08T11:50:14Z

Well it keeps getting more and more out of scope, so I'll try to stop here for now. I've cleaned up the debug code, added the tests and rebased to master. There might be still a few misses here and there, but things seem to work mostly fine.

jdm

I agree, I think we should merge what we have before it keeps getting bigger. Thanks for all the work to fix up the missing pieces here!

jdm · 2020-04-08T14:49:42Z

components/canvas_traits/webgl.rs

+    }
+
+    pub fn usable_as_internal(self) -> bool {
+        /*match self {


We should remove this commented code.

jdm · 2020-04-08T14:49:42Z

components/script/dom/webgl_validations/tex_image_2d.rs

+        };
+        if !dimensions_valid {
+            context.webgl_error(InvalidEnum);
+            return Err(TexImageValidationError::InvalidBorder);


I think a bunch of these error values don't match what's actually happening.

Whoops, indeed, fixed.

jdm · 2020-04-08T16:42:26Z

@bors-servo r+

bors-servo · 2020-04-08T16:42:27Z

📌 Commit 1c79147 has been approved by jdm


        Auto merge of #26023 - mmatyas:webgl_fns_texstorage2d, r=jdm

@jdm

Add support for WebGL2 TexStorage2D Adds initial support for the WebGL2 `TexStorage2D` call, adds support for the related texture enums and enables some of the texture tests. See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.6  cc @jdm @zakorgy A couple of notes: - Currently `tests/wpt/webgl/meta/conformance2/textures` is disabled for testing; this patch enables the `misc` subdirectory and disables the rest, which is why there are so many new test files. The primary test for `TexStorage2D` is `conformance2/textures/misc/tex-storage-2d.html`. - Most of the test passes, except the "texSubImage2D should succeed on immutable texture as long as the format is compatible" checks. This produces INVALID_OPERATIONs [here](https://github.com/servo/servo/blob/d5e5414be372a28215c5162b1257765b89fbd619/components/script/dom/webglrenderingcontext.rs#L798) because the texture's image info is not set at that level. I'm probably forgetting to call `set_image_infos_at_level` somewhere but I'm not sure where. - There's a duplication of the internal texture format list with eg. the renderbuffer code, I wonder if there would be a good common place for it. ---  - [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

bors-servo · 2020-04-08T16:42:33Z

⌛ Testing commit 1c79147 with merge 19cbea9...

bors-servo · 2020-04-08T17:24:53Z

💔 Test failed - status-taskcluster


        webgl: Fix support for float/half-float texture formats.


        webgl: Fix tidy


        webgl: Update texture test results


        Update test expectations.


        webgl: Add missing RGB8 texture format combination.


        webgl: Return TEXTURE_IMMUTABLE_FORMAT as a boolean; don't panic on m…

…acOS for TEXTURE_IMMUTABLE_LEVELS.


        Update test results for macOS.

jdm · 2020-04-30T04:22:26Z

@bors-servo r+

bors-servo · 2020-04-30T04:22:27Z

📌 Commit 0c87590 has been approved by jdm

bors-servo · 2020-04-30T04:22:31Z

⌛ Testing commit 0c87590 with merge e544a97...


        Auto merge of #26023 - mmatyas:webgl_fns_texstorage2d, r=jdm

@jdm

Add support for WebGL2 TexStorage2D Adds initial support for the WebGL2 `TexStorage2D` call, adds support for the related texture enums and enables some of the texture tests. See: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.6  cc @jdm @zakorgy A couple of notes: - Currently `tests/wpt/webgl/meta/conformance2/textures` is disabled for testing; this patch enables the `misc` subdirectory and disables the rest, which is why there are so many new test files. The primary test for `TexStorage2D` is `conformance2/textures/misc/tex-storage-2d.html`. - Most of the test passes, except the "texSubImage2D should succeed on immutable texture as long as the format is compatible" checks. This produces INVALID_OPERATIONs [here](https://github.com/servo/servo/blob/d5e5414be372a28215c5162b1257765b89fbd619/components/script/dom/webglrenderingcontext.rs#L798) because the texture's image info is not set at that level. I'm probably forgetting to call `set_image_infos_at_level` somewhere but I'm not sure where. - There's a duplication of the internal texture format list with eg. the renderbuffer code, I wonder if there would be a good common place for it. ---  - [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

bors-servo · 2020-04-30T05:03:02Z

💔 Test failed - status-taskcluster

CYBAI · 2020-04-30T06:24:06Z

  ▶ Unexpected subtest result in /_webgl/conformance/textures/misc/tex-sub-image-2d.html:
  │ FAIL [expected PASS] WebGL test #0: pixel at (1, 0) was (0, 0, 0), should be (1, 1, 1)
  │   → assert_true: pixel at (1, 0) was (0, 0, 0), should be (1, 1, 1) expected true got false
  │ 
  │ reportTestResultsToHarness/<@http://web-platform.test:8000/_webgl/js/js-test-pre.js:123:24
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1988:25
  │ test@http://web-platform.test:8000/resources/testharness.js:535:30
  │ reportTestResultsToHarness@http://web-platform.test:8000/_webgl/js/js-test-pre.js:122:15
  │ testFailed@http://web-platform.test:8000/_webgl/js/js-test-pre.js:249:31
  └ @http://web-platform.test:8000/_webgl/conformance/textures/misc/tex-sub-image-2d.html:107:19

Not sure if this is an intermittent failure, other than it, other failures are #25513 and #23290.

jdm · 2020-04-30T13:40:43Z

@bors-servo retry

#26365 #25513 #23290

bors-servo · 2020-04-30T14:13:20Z

⌛ Testing commit 0c87590 with merge 2b20f04...

bors-servo · 2020-04-30T14:40:26Z

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

highfive assigned jdm Mar 24, 2020

highfive added the S-awaiting-review label Mar 24, 2020

jdm requested changes Mar 24, 2020

View changes

components/script/dom/webgltexture.rs Outdated Show resolved Hide resolved

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 24, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 26, 2020

jdm reviewed Mar 27, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 27, 2020

mmatyas force-pushed the mmatyas:webgl_fns_texstorage2d branch from 31e38ac to 742d27c Apr 6, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 6, 2020

highfive added the S-needs-rebase label Apr 7, 2020

mmatyas force-pushed the mmatyas:webgl_fns_texstorage2d branch from 742d27c to 4bf4cb3 Apr 8, 2020

jdm requested changes Apr 8, 2020

View changes

mmatyas force-pushed the mmatyas:webgl_fns_texstorage2d branch from 4bf4cb3 to 1c79147 Apr 8, 2020

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Apr 8, 2020

jdm and others added 7 commits Apr 9, 2020

webgl: Fix support for float/half-float texture formats.

29ab55d

webgl: Fix tidy

54b100b

webgl: Update texture test results

2e8ef1b

Update test expectations.

24a7f38

webgl: Add missing RGB8 texture format combination.

3876d6d

webgl: Return TEXTURE_IMMUTABLE_FORMAT as a boolean; don't panic on m…

9ce84d9

…acOS for TEXTURE_IMMUTABLE_LEVELS.

Update test results for macOS.

Loading status checks…

0c87590

jdm force-pushed the mmatyas:webgl_fns_texstorage2d branch from 8d1b60e to 0c87590 Apr 30, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Apr 30, 2020

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Apr 30, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 30, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 30, 2020

bors-servo merged commit 2b20f04 into servo:master Apr 30, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 30, 2020

bors-servo mentioned this pull request Apr 30, 2020

Add initial support for BlitFramebuffer #26288

Closed

3 of 3 tasks complete

servo / servo

Add support for WebGL2 TexStorage2D #26023

Add support for WebGL2 TexStorage2D #26023

mmatyas commented Mar 24, 2020

highfive commented Mar 24, 2020

mmatyas commented Mar 26, 2020

This comment has been minimized.

This comment has been minimized.

mmatyas commented Apr 6, 2020

bors-servo commented Apr 7, 2020

mmatyas commented Apr 8, 2020

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Apr 8, 2020

bors-servo commented Apr 8, 2020

bors-servo commented Apr 8, 2020

bors-servo commented Apr 8, 2020

jdm commented Apr 30, 2020

bors-servo commented Apr 30, 2020

bors-servo commented Apr 30, 2020

bors-servo commented Apr 30, 2020

CYBAI commented Apr 30, 2020

jdm commented Apr 30, 2020

bors-servo commented Apr 30, 2020

bors-servo commented Apr 30, 2020

servo / servo

Join GitHub today

Add support for WebGL2 TexStorage2D #26023

Add support for WebGL2 TexStorage2D #26023

Conversation

mmatyas commented Mar 24, 2020

highfive commented Mar 24, 2020

mmatyas commented Mar 26, 2020

This comment has been minimized.

jdm Mar 27, 2020

This comment has been minimized.

mmatyas Mar 30, 2020

mmatyas commented Apr 6, 2020

bors-servo commented Apr 7, 2020

mmatyas commented Apr 8, 2020

jdm left a comment

This comment has been minimized.

jdm Apr 8, 2020

This comment has been minimized.

jdm Apr 8, 2020

This comment has been minimized.

mmatyas Apr 8, 2020

jdm commented Apr 8, 2020

bors-servo commented Apr 8, 2020

bors-servo commented Apr 8, 2020

bors-servo commented Apr 8, 2020

jdm commented Apr 30, 2020

bors-servo commented Apr 30, 2020

bors-servo commented Apr 30, 2020

bors-servo commented Apr 30, 2020

CYBAI commented Apr 30, 2020

jdm commented Apr 30, 2020

bors-servo commented Apr 30, 2020

bors-servo commented Apr 30, 2020