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 initial support for WebGL compressed textures #23226

Merged
merged 1 commit into from May 21, 2019

Conversation

Projects
None yet
5 participants
@mmatyas
Copy link
Contributor

commented Apr 18, 2019

This patch is an initial implementation of WebGL compressed texture support, it contains

  • functions for registering and querying compressed texture extensions
  • initial implementation of CompressedTexImage2D and CompressedTexSubImage2D and their parameter validation
  • implementation of S3TC (DXT1, DXT3, DXT5) and ETC1 extensions as examples

What's still missing:

  • some of the parameter validation steps are missing
  • the pixel comparison tests fail for more complex cases (I'm probably missing something trivial at the GL calls)

Related: #10209 and #20594

cc @jdm @zakorgy


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • Related issues: #10209, #20594
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 18, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/WEBGLCompressedTextureS3TC.webidl, components/script/dom/webgl_extensions/extensions.rs, components/script/dom/webgl_extensions/ext/mod.rs, components/script/dom/webidls/WEBGLCompressedTextureETC1.webidl, components/script/dom/webgltexture.rs and 4 more
  • @jgraham: tests/wpt/webgl/meta/conformance/extensions/webgl-compressed-texture-s3tc.html.ini
  • @KiChjang: components/script/dom/webidls/WEBGLCompressedTextureS3TC.webidl, components/script/dom/webgl_extensions/extensions.rs, components/script/dom/webgl_extensions/ext/mod.rs, components/script/dom/webidls/WEBGLCompressedTextureETC1.webidl, components/script/dom/webgltexture.rs and 4 more
@highfive

This comment has been minimized.

Copy link

commented Apr 18, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@paulrouget paulrouget assigned jdm and unassigned paulrouget Apr 18, 2019

@paulrouget

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

@jdm who could review this?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I have been working on reviewing the changes.

@jdm
Copy link
Member

left a comment

We will also need to deal with:

@mmatyas mmatyas force-pushed the mmatyas:webgl_compressed_textures branch from 10b039f to cbfa90d Apr 30, 2019

@mmatyas

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Thanks, after initializing the texture and fixing some smaller things, now all the tests pass!

I've also moved the compressed texture enums into TexFormat, which simplifies some of the logic. There is an issue however, that the GLenum values are not present in Gleam. Gleam autogenerates the list of GLenums on build, based on the extensions listed here. However, because it works by querying the system, Gleam cannot generate the enums for extensions that are not available on your system. So for now I've hardcoded the enum values for compressed formats.

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I'm glad to see the tests are now passing. Since there are some review comments that have not been addressed yet, I'm removing this from my review queue. Let me know if I should look at it again!

@mmatyas mmatyas force-pushed the mmatyas:webgl_compressed_textures branch from cbfa90d to a791579 Apr 30, 2019

@mmatyas

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Ok, left some comments and fixed the build fail on Travis. Also with ImageInfo::is_compressed_format implemented, I think the "Additions to Chapter 4 of the OpenGL ES 2.0.25 Specification" is also resolved now.

@jdm

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

📌 Commit a791579 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

⌛️ Testing commit a791579 with merge 7c8a703...

bors-servo added a commit that referenced this pull request May 14, 2019

Auto merge of #23226 - mmatyas:webgl_compressed_textures, r=jdm
Add initial support for WebGL compressed textures

This patch is an initial implementation of WebGL compressed texture support, it contains

- functions for registering and querying compressed texture extensions
- initial implementation of `CompressedTexImage2D` and `CompressedTexSubImage2D` and their parameter validation
- implementation of S3TC (DXT1, DXT3, DXT5) and ETC1 extensions as examples

What's still missing:

- some of the parameter validation steps are missing
- the pixel comparison tests fail for more complex cases (I'm probably missing something trivial at the GL calls)

Related: #10209 and #20594

cc @jdm @zakorgy

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] Related issues: #10209, #20594
- [x] There are tests for these changes

<!-- 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/23226)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

💔 Test failed - mac-rel-wpt2

@jdm

This comment has been minimized.

Copy link
Member

commented May 21, 2019

That looks right. Weird that update-manifest wasn't doing the trick :<

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

📌 Commit 12b573d has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

⌛️ Testing commit 12b573d with merge 5330cca...

bors-servo added a commit that referenced this pull request May 21, 2019

Auto merge of #23226 - mmatyas:webgl_compressed_textures, r=jdm
Add initial support for WebGL compressed textures

This patch is an initial implementation of WebGL compressed texture support, it contains

- functions for registering and querying compressed texture extensions
- initial implementation of `CompressedTexImage2D` and `CompressedTexSubImage2D` and their parameter validation
- implementation of S3TC (DXT1, DXT3, DXT5) and ETC1 extensions as examples

What's still missing:

- some of the parameter validation steps are missing
- the pixel comparison tests fail for more complex cases (I'm probably missing something trivial at the GL calls)

Related: #10209 and #20594

cc @jdm @zakorgy

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] Related issues: #10209, #20594
- [x] There are tests for these changes

<!-- 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/23226)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

💔 Test failed - status-taskcluster

@jdm

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Still timing out on mac only. I'll try running the test locally to see if I can reproduce on my macbook.

@jdm jdm force-pushed the mmatyas:webgl_compressed_textures branch from 12b573d to fb1849c May 21, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@bors-servo r+
The test seems to exhibit some pathological line-breaking behaviour on macOS, according to my profiler. I modified the test to only add images to the page when there are failures, so the issue gets hidden.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

📌 Commit fb1849c has been approved by jdm

@jdm jdm force-pushed the mmatyas:webgl_compressed_textures branch from fb1849c to 7f0b820 May 21, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

📌 Commit 7f0b820 has been approved by jdm

bors-servo added a commit that referenced this pull request May 21, 2019

Auto merge of #23226 - mmatyas:webgl_compressed_textures, r=jdm
Add initial support for WebGL compressed textures

This patch is an initial implementation of WebGL compressed texture support, it contains

- functions for registering and querying compressed texture extensions
- initial implementation of `CompressedTexImage2D` and `CompressedTexSubImage2D` and their parameter validation
- implementation of S3TC (DXT1, DXT3, DXT5) and ETC1 extensions as examples

What's still missing:

- some of the parameter validation steps are missing
- the pixel comparison tests fail for more complex cases (I'm probably missing something trivial at the GL calls)

Related: #10209 and #20594

cc @jdm @zakorgy

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] Related issues: #10209, #20594
- [x] There are tests for these changes

<!-- 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/23226)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

⌛️ Testing commit 7f0b820 with merge 123f585...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

☀️ Test successful - arm64, linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 123f585 to master...

@bors-servo bors-servo merged commit 7f0b820 into servo:master May 21, 2019

4 of 5 checks passed

Travis CI - Pull Request Build Created
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
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
You can’t perform that action at this time.