Add wasm32-unknown-unknown/WebGL support

[grovesNL]

Start to make a more maintainable version of my old branch from #1900 (comment). Heavily WIP but maybe somebody wants to start giving some feedback as I work through the remainder of issues.

This PR replaces gl_generator with glow (https://github.com/grovesNL/glow), a experimental crate meant to unify WebGL/OpenGL types and function signatures. Currently unsupported functions will just panic, but I think it makes sense to move some of the version logic and extension fallbacks in there too (removing them from the gl backend). The WebGL types are now just keys (and Copy) which fixes some of the issues I mentioned previously.

This is for the wasm32-unknown-unknown target and uses web-sys.

TODO:

• (major) spirv_cross wrapper on wasm32-unknown-unknown and C++ library through emscripten. For now the quad shaders are hardcoded. See Add wasm32-unknown-unknown support grovesNL/spirv_cross#92. (I'll probably leave the PR open until we work out how the integration between wasm bundles should work)
• Consolidate render loop somehow (see WIP in glow example https://github.com/grovesNL/glow/blob/master/examples/hello/src/main.rs#L118). I need some more ideas on how to work around the 'static bounds for wasm32 here. The basic idea is that wasm32 has to use callbacks and we use spinloops on the native target, so we would ideally like to unify these (at least for use in examples but also simple applications). For wasm32 with the quad example I just render once and quit instead. Currently winit is working through the implementation of "Event Loop 2.0" and there's been some effort to write a stdweb backend for this, which we could later help port to wasm32-unknown-unknown. I think for now we can just keep the wasm render loop path separate in gfx examples, and things will work naturally once winit wasm32-unknown-unknown support is ready.
• Remove all remaining #[cfg(not(target_arch = "wasm32"))] wherever possible to make this more maintainable. There are a few places remaining that can be removed when we move some logic into glow, like version parsing and extension checking. Some other parts like the runloop can't be addressed without winit support for wasm32-unknown-unknown or other workarounds in quad.
• Decide where to put things like index.html for examples (I've excluded it for now) and guide about how to create wasm bundle with wasm-bindgen.
• Consider how logging should work – I don't see how to redirect stdout to console.log so it's a bit manually at the moment. It would be really cool if things like info! just work automatically with wasm32-unknown-unknown. We should be able to use https://github.com/iamcodemaker/console_log for the wasm backend without any major challenges.
• Publish glow Published 0.2.0
• Publish updated spirv_cross Published 0.14.0
• Add wasm-bindgen to CI This has been added

---

This change is 

[grovesNL]

I spoke to the wasm working group:

• it's not planned for stdout redirection to be stabilized in std anytime soon
• macros println! will continue to no-op (probably for a long time, though possibly will have some functionality when a new target is available)
• for log it looks like we can use https://github.com/iamcodemaker/console_log

[grovesNL]

For spirv_cross, I've been able to compile the C++ portion of spirv_cross to a WebAssembly+JavaScript bundle through Emscripten.

The example below loads a SPIR-V shader, passes it to SPIRV-Cross, and compiles to GLSL.

Next we need to redirect the Rust FFI calls. When building for native targets these calls will still go direct to C++, but when building for wasm32-unknown-unknown they should go through the JS bindings provided in the Emscripten bundle (probably just exposed as globals on the window object in JS). This step is a bit unfortunate, because I'd ideally like to keep calls within wasm only, but I don't see a good way around this for now. If anyone knows a better way please let me know 😃

Another slight issue is that the Emscripten bundle will have to be included manually (i.e. into the final HTML file or bundler) by anyone using the wasm32-unknown-unknown target with gfx-backend-gl. This is because we want the Emscripten bundle to be provided on the window by the time gfx-backend-gl loads, and I don't know if there are any hooks (i.e. when building for wasm32-unknown-unknown) that could help us here.

[kvark]

they should go through the JS bindings provided in the Emscripten bundle

I find it weird that you are targeting unknown while still using Emscripten. I thought the whole point was to avoid it?

[minecrawler]

bundle will have to be included manually (i.e. into the final HTML file or bundler)

I don't think it's an issue, because isn't that the norm? It's an inconvenience at most, and might have to be documented or tooling has to be created for it.

The wasm file is a library, not an application. You can compare it to a DLL file on Windows. There are bundlers, like Parcel, which are able to build a web-application with Rust content, though, so maybe something like that could be used as a template. Other than that, as described in the WASM book, linking the wasm file has to be done manually in the HTML file.

[grovesNL]

@kvark

I find it weird that you are targeting unknown while still using Emscripten. I thought the whole point was to avoid it?

The Emscripten build I'm talking about is only the C++ portion of SPIRV-Cross, so we can run SPIRV-Cross in the browser (the part in my screenshot). The idea is to have something like a window.SPIRVCross (the C++ portion, compiled by Emscripten) that will later be referenced by spirv_cross (the Rust portion, compiled as wasm32-unknown-unknown). I'm not sure of a way around this besides rewriting the C++ portion in Rust.

@minecrawler

I don't think it's an issue, because isn't that the norm? It's an inconvenience at most, and might have to be documented or tooling has to be created for it.

Right, but I'm talking about the transitive dependency on the C++ library here: application (Rust, compiled with wasm32-unknown-unknown) -> gfx-backend-gl (Rust) -> spirv_cross (Rust) -> SPIRV-Cross (C++, compiled with Emscripten to JS and wasm)

The application's index.html has to include both the wasm produced by rustc but also the Emscripten bundle so the window.SPIRVCross is already in place by the time the application's wasm runs.

The alternative is that spirv_cross includes the necessary JS and wasm inline somewhere and causes it to be evaluated by the JS engine somehow during initialization of the application, or we could use a callback/return a Future somewhere in spirv_cross. The problem is that I don't know the best way to do this 😄 I can't find much precedent for this either but I expect lots of libraries wrapping C or C++ libraries will have to deal with this. I think it will be even more complicated when using bundlers like Parcel (I just load it manually in the HTML file for now).

[carlosdp]

I think it might be possible to statically link the wasm instead. I'm looking into trying to get SPIRV-Cross to compile directly to wasm using this method.

[grovesNL]

To clarify, I'm able to compile SPIRV-Cross to wasm with Emscripten without a problem. The problem is I'm not sure of the best method to link the Rust wasm (wasm32-unknown-unknown) and C++ wasm (Emscripten) parts together besides the ideas I mentioned above.

It would be great if we could statically link the wasm modules together, but it's not clear how this would be possible without compiling both parts with Emscripten in the first place (at least with the Emscripten output generated from SPIRV-Cross), which we'd like to avoid.

[carlosdp]

The stack overflow link in my last comment walks through a way to link the wasm directly using the binaryen tools

…

On Mon, Jan 14, 2019 at 11:00 PM Josh Groves ***@***.***> wrote: To clarify, I'm able to compile SPIRV-Cross to wasm with Emscripten without a problem. The problem is I'm not sure of the best method to link the Rust wasm (wasm32-unknown-unknown) and C++ wasm (Emscripten) parts together besides the ideas I mentioned above. It would be great if we could statically link the wasm modules together, but it's not clear how this would be possible without compiling both parts with Emscripten in the first place (at least with the Emscripten output generated from SPIRV-Cross), which we'd like to avoid. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2554 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMIU42suASBPMHiEcxRmZ54Sv_3ILOOks5vDXyagaJpZM4Ztzqa> .

[grovesNL]

Ah ok, thanks, I missed that part. I could see something like that being feasible in the future, but it looks a bit too experimental at the moment, unless there is a more stable version of that approach somewhere.

[grovesNL]

If anyone is curious about the raw/ungzipped size of SPIRV-Cross through Emscripten (only the C++ parts), I tried compiling the parts we'll need (the GLSL compiler) with optimization enabled:

• with -O3 it appears be around 670 kilobytes
• with -Oz it appears to be around 523 kilobytes

(see https://kripken.github.io/emscripten-site/docs/optimizing/Optimizing-Code.html for the full list of options)

[grovesNL]

I spent a few hours today looking into how dynamic linking should work in WebAssembly generally, and Rust with WebAssembly. It seems like both of these are still under consideration, so it looks like it's too early to make any progress with more direct linking.

FWIW there is a clang target for wasm32-unknown-unknown (just like rustc has), so one idea could be to generate LLVM IR or LLVM bitcode for SPIRV-Cross, combine this directly with IR/bitcode generated by rustc, and eventually generate a single wasm bundle. However there needs to be improvements in tooling on both the C++ and Rust sides for this, so I don't think I'm going to keep looking into this option for now. https://github.com/yurydelendik/wasmception is a great reference for playing with this target.

[grovesNL]

Another small update: I continued experimenting with the idea of having Rust wasm32-unknown-unknown make calls to JavaScript (through wasm bindgen), where the JavaScript methods are just the Emscripten generated C++ bindings.

So each call into the C++ SPIRV-Cross library roughly looks like Rust (wasm, wasm32-unknown-unknown) -> JavaScript -> C++ (wasm Emscripten). I manually wrote some bindings for the GLSL compiler and it works really well so far:

The screenshot is running a simple Rust binary compiled to wasm32-unknown-unknown which has spirv_cross as a dependency. It includes a SPIR-V file, creates a GLSL compiler using the regular spirv_cross API and compiles SPIR-V to GLSL. I think it's pretty impressive that we can do this without changing the API of spirv_cross at all.

As I mentioned earlier, there are a few downsides to this approach:

• The Emscripten C++ files (one JS file and one wasm file) need to be loaded by the application (in their own HTML file), because I don't see we could easily embed it as part of gfx at the moment. This is conceptually similar to linking to installed libraries. There might be a better way to do this in the future, so that the JS and wasm are included inline somehow (basically JS snippets, which are planned for wasm-bindgen).
• Calling through JS adds overhead, so it would be nice to be able to call directly between both these wasm modules instead.
• The Emscripten bundle is a bit large, so it there could be some improvements by using clang wasm32-unknown-unknown directly when it's more stable. After this step, it would be great to have both Rust and C++ code in the same wasm module.

[carlosdp]

I have your branch running locally now and would like to help. I can try to tackle the render loop consolidation if that helps. Have you started trying to make the spirv_cross shim yet?

[grovesNL]

Sounds great! The render loop consolidation would be useful. I'm mostly interested in an idiomatic callback-based way to create and run a renderloop with a cleanup at the end, and allows moving resources etc. into the cleanup. I played around with the idea wrapping all resources into struct fields that are Option but maybe there are some other ideas.

I'm also wondering whether we could add wasm32/requestAnimationFrame support to winit somehow, if they're ready to support wasm32-unknown-unknown. Then most projects would be able to just switch to winit's callback-driven API instead.

I've started to work on the spirv_cross shim (the screenshot above is running an example), currently I'm just filling out the last few bindings we should need. Calls are made from Rust/wasm32-unknown-unknown -> JS -> C++/Emscripten wasm as mentioned above. So far the spirv_cross work has been mostly adding some helpers to handle pointer offsets and string conversion correctly wherever the previous assumptions made for C FFI aren't valid anymore (i.e. reading a string from the Emscripten heap vs. reading a string from a pointer).

[grovesNL]

spirv_cross as wasm32-unknown-unknown (using the ideas mentioned above) is mostly ready now: see grovesNL/spirv_cross#92. It would be great if somebody could review and consider any possible issues with this approach.

Here's a spirv_cross example with native and wasm32-unknown-unknown running side-by-side:

[grovesNL]

Some more updates:

• Rebased this branch onto master again
• Added console error panic hook (to get stack traces when panics occur) and console log (to write info! etc. to the console) to the quad example
• Confirmed the wasm32-unknown-unknown version of spirv_cross works with this branch, by loading spirv and converting it to ESSL at runtime (in the browser)

[kvark]

Reviewed 7 of 16 files at r1.
Reviewable status: 7 of 16 files reviewed, 14 unresolved discussions (waiting on @grovesNL)

---

examples/Cargo.toml, line 39 at r1 (raw file):

winit = "0.18"
glsl-to-spirv = "0.1.4"
gfx-hal = { path = "../src/hal", version = "0.1" }

these are duplicates of general dependencies

---

examples/quad/main.rs, line 26 at r1 (raw file):

#[cfg(not(target_arch = "wasm32"))]
extern crate glsl_to_spirv;

these wouldn't be needed after rebase as we are on Rust 2018

---

examples/quad/main.rs, line 36 at r1 (raw file):

use wasm_bindgen::prelude::*;

#[cfg_attr(target_arch = "wasm32", wasm_bindgen(start))]

let's just move it a line below, where we are under the cfg and therefore don't need cfg_attr: we can just do #[wasm_bindgen(start)]

---

examples/quad/main.rs, line 38 at r1 (raw file):

#[cfg_attr(target_arch = "wasm32", wasm_bindgen(start))]
#[cfg(target_arch = "wasm32")]
pub fn wasm_main() {

why don't we mark our real main as wasm start? why the need for an extra function?

---

examples/quad/main.rs, line 94 at r1 (raw file):

fn main() {
    #[cfg(target_arch = "wasm32")]
    console_log::init_with_level(log::Level::Debug).unwrap();

is this temporary?

---

examples/quad/main.rs, line 549 at r1 (raw file):

            let glsl = fs::read_to_string("quad/data/quad.vert").unwrap();
            #[cfg(not(target_arch = "wasm32"))]
            let spirv: Vec<u8> = glsl_to_spirv::compile(&glsl, glsl_to_spirv::ShaderType::Vertex)

nit: let's move glsl computation in here to avoid repeating the cfg attributes

---

examples/quad/main.rs, line 671 at r1 (raw file):

        #[cfg(target_arch = "wasm32")] // TODO
        {
            running = false;

can just initialize it as let mut running = cfg!(...);?

---

examples/quad/data/quad.frag.spv, line 0 at r1 (raw file):
why are these different?

---

src/backend/gl/Cargo.toml, line 26 at r1 (raw file):

gfx-hal = { path = "../../hal", version = "0.1" }
smallvec = "0.6"
glow = { version = "0.1", path = "../../../../glow" }

would love to see it under gfx-rs org :)

---

src/backend/gl/src/command.rs, line 754 at r1 (raw file):

            let index = first_binding as usize + i;
            if self.cache.vertex_buffers.len() <= index {
                self.cache.vertex_buffers.resize(index + 1, None);

horray for strong typing 🎉

---

src/backend/gl/src/device.rs, line 64 at r1 (raw file):

    pub fn create_shader_module_from_source(
        &self,
        data: &[u8],

let's accept &str right away?

---

src/backend/gl/src/device.rs, line 213 at r1 (raw file):

        let is_embedded = self.share.info.shading_language.is_embedded;
        compile_options.version = match self.share.info.shading_language.tuple() {
            (4, 60) if !is_embedded => glsl::Version::V4_60,

looks like we should refactor it to move is_embedded check outside

---

src/backend/gl/src/device.rs, line 1028 at r1 (raw file):

        let ptr = if caps.emulate_map {
            let raw = Box::into_raw(vec![0; size as usize].into_boxed_slice()) as *mut u8;

how does it know that 0 is u8 here?

---

src/backend/gl/src/device.rs, line 1397 at r1 (raw file):

            let sync = fence.0.get();
            if let Some(s) = sync {
                if self.share.private_caps.sync && gl.is_sync(Some(s)) {

is_sync should not be getting an option?

[grovesNL]

Reviewable status: 5 of 16 files reviewed, 14 unresolved discussions (waiting on @grovesNL and @kvark)

---

examples/Cargo.toml, line 39 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

these are duplicates of general dependencies

Done

---

examples/quad/main.rs, line 26 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

these wouldn't be needed after rebase as we are on Rust 2018

Will do

---

examples/quad/main.rs, line 36 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's just move it a line below, where we are under the cfg and therefore don't need cfg_attr: we can just do #[wasm_bindgen(start)]

Done

---

examples/quad/main.rs, line 38 at r1 (raw file):
In https://rustwasm.github.io/docs/wasm-bindgen/reference/attributes/on-rust-exports/start.html this (unfortunate) limitation is discussed:

Note that due to various practical limitations today the start section of the executable may not literally point to main, but the main function here should be started up automatically when the wasm module is loaded.

---

examples/quad/main.rs, line 94 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is this temporary?

I'm not sure, I think we can still pass the flag during build, but the examples recommend conditionally setting it for debug builds (https://docs.rs/console_log/0.1.2/console_log/). It's less convenient regardless, because we can't pass the log flag during cargo run for the wasm target :)

---

examples/quad/main.rs, line 549 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: let's move glsl computation in here to avoid repeating the cfg attributes

I was also wondering whether we should use some glsl-to-spirv macro (there are a few crates now) and always create the spirv at build time to avoid cfg entirely

---

examples/quad/main.rs, line 671 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can just initialize it as let mut running = cfg!(...);?

We still want to run the wasm example for the first frame though. If we use let mut running = cfg!(...);) we won't enter while running at all

---

examples/quad/data/quad.frag.spv, line at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why are these different?

We don't normally include the spirv files directly, but we could. The issue is that we can't run glsl_to_spirv in the browser, so to support wasm we either have to create the spirv files at build time, or commit them into the repository.

---

src/backend/gl/Cargo.toml, line 26 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would love to see it under gfx-rs org :)

Sure, we can move it anytime

---

src/backend/gl/src/command.rs, line 754 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

horray for strong typing 🎉

👍
Done

---

src/backend/gl/src/device.rs, line 1028 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

how does it know that 0 is u8 here?

Hmm yeah this may be sized too large. I can specify that at least.

In general this is the hack I mentioned, we'll need to look into how Emscripten emulates buffer mapping

---

src/backend/gl/src/device.rs, line 1397 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is_sync should not be getting an option?

Yeah, I guess the option came from web-sys (https://rustwasm.github.io/wasm-bindgen/api/web_sys/struct.WebGl2RenderingContext.html#method.is_sync). Maybe we should enforce this not to receive Option though

[kvark]

Reviewed 9 of 16 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @grovesNL and @kvark)

---

src/backend/gl/src/info.rs, line 243 at r2 (raw file):

#[derive(Copy, Clone)]
pub enum Requirement<'a> {

should we have a separate variant here for WebGL?

---

src/backend/gl/src/info.rs, line 466 at r2 (raw file):

        // TODO && gl.GenFramebuffers.is_loaded(),
        framebuffer_texture: info.is_supported(&[Core(3, 0)]), //TODO: double check
        buffer_role_change: true || !info.version.is_embedded, // TODO

do we need buffer role changes in WebGL?

---

src/backend/gl/src/native.rs, line 10 at r2 (raw file):

use crate::{Backend, GlContext};

pub type VertexArray = <GlContext as glow::Context>::VertexArray;

as a thought experiment, I wonder how the crate would look like if it was written to be generic over glow::Context to begin with

---

src/backend/gl/src/queue.rs, line 422 at r2 (raw file):

                            view[3] as i32,
                        );
                        #[cfg(not(target_arch = "wasm32"))] // TODO

need a new private capability here?

---

src/backend/gl/src/state.rs, line 242 at r2 (raw file):

            let (color_eq, color_src, color_dst) = map_blend_op(color);
            let (alpha_eq, alpha_src, alpha_dst) = map_blend_op(alpha);
            #[cfg(not(target_arch = "wasm32"))] // TODO

another private cap

---

src/backend/gl/src/window/web.rs, line 107 at r2 (raw file):

        let caps = hal::SurfaceCapabilities {
            image_count: if self.window.get_pixel_format().double_buffer {

this is kinda weird: we'd force 2 images yet always return 0 in acquire? Perhaps, let's return 0, 1, 0, 1, 0, etc instead?

[kvark]

Amazing work, thank you!
Most of my concerns can be addressed later, or not even related directly to your changes, but please at the very minimum let's leave some comments in the code to not forget about them.

Reviewed 15 of 15 files at r5, 18 of 18 files at r6.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @grovesNL)

---

.travis.yml, line 41 at r6 (raw file):

      rust: stable

    # WebAssembly

just totally in love with this line in CI script ❤️

---

examples/Cargo.toml, line 46 at r5 (raw file):

optional = true

[target.'cfg(target_arch = "wasm32")'.dependencies.gfx-backend-gl]

looks like these wasm/!wasm entries could be merged once glutin is no longer a default (i.e. not the only WSI supported)

---

examples/README.md, line 23 at r6 (raw file):

To run the quad example with WebAssembly:

- `cd ..` to move up to the parent directory of the gfx repository.

lets rewrite this to being a large code block with bash syntax? I.e.

cd .. # move to the parent
git clone ... # to clone spirv_cross
...

Might also consider having an entry in our Makefile to do this

---

examples/quad/main.rs, line 547 at r5 (raw file):

        let fs_module = {
            #[cfg(target_arch = "wasm32")]
            let spirv = include_bytes!("./data/quad.frag.spv").to_vec();

I think we are safe to do this on all backends: we can still leave run-time loading for the other examples

---

examples/quad/main.rs, line 655 at r5 (raw file):

        #[cfg(target_arch = "wasm32")] // TODO
        {
            running = false;

nit: could also do running = !cfg!(target_arch = "wasm"); for a one-liner

---

examples/quad/main.rs, line 835 at r5 (raw file):

        frame += 1;

        #[cfg(target_arch = "wasm32")] // TODO

since running==false here, we are going to leave the loop anyway, do we still need this TODO?

---

src/backend/gl/Cargo.toml, line 26 at r5 (raw file):

gfx-hal = { path = "../../hal", version = "0.2" }
smallvec = "0.6"
glow = { git = "https://github.com/grovesNL/glow", rev = "159c495" }

we'll need this published if we want the GL backend to see the light of crates.io

---

src/backend/gl/src/command.rs, line 121 at r5 (raw file):

    CopyImageToSurface(n::ImageKind, n::Surface, command::ImageCopy),

    BindBufferRange(u32, u32, n::RawBuffer, i32, i32),

would be great to see more typedefs instead of raw integer types. Alternatively, we could name things in this enum, i.e.

BindBufferRange {
  ty: u32,
  binding: u32,
  buffer: n::RawBuffer,
  offset: i32,
  size: i32,
}

---

src/backend/gl/src/command.rs, line 168 at r5 (raw file):

    blend_targets: Option<Vec<Option<pso::ColorBlendDesc>>>,
    // Maps bound vertex buffer offset (index) to handle.
    vertex_buffers: Vec<Option<n::RawBuffer>>,

nit: should make this a fixed-size vec (or ArrayVec) now that options are inside

---

src/backend/gl/src/command.rs, line 996 at r5 (raw file):

                                *binding,
                                *buffer,
                                *offset as i32,

somewhat unfortunate that i32 are used for offset and size

---

src/backend/gl/src/device.rs, line 1028 at r1 (raw file):

Previously, grovesNL (Josh Groves) wrote…

It's a cast from *mut [u8] to *mut u8, is there a better way?

hold on, isn't *mut [u8] a pointer to a run-time sized slice? In this case, the first word in memory would be its length, not the first element.

---

src/backend/gl/src/device.rs, line 430 at r5 (raw file):

    mut set_param_int: SetParamInt,
) where
    SetParamFloat: FnMut(u32, f32),

it would be cleaner to have a trait with these methods implemented for texture and sampler objects

---

src/backend/gl/src/device.rs, line 1218 at r5 (raw file):

                        let mut h = h;
                        for i in 0..num_levels {
                            gl.tex_image_2d(

I wonder why we are initializing each level separately like this? AFAIK, drivers allocate the whole mipmap chain when called with level = 0 anyway...

---

src/backend/gl/src/device.rs, line 1167 at r6 (raw file):

        let (int_format, iformat, itype) = match format {
            Format::Rgba8Unorm => (glow::RGBA8, glow::RGBA, glow::UNSIGNED_BYTE),
            Format::Bgra8Unorm => (glow::RGBA8, glow::BGRA, glow::UNSIGNED_BYTE),

This doesn't look correct... The second member in GL is just used in non-storage initializers for specifying the format of the data. Since we aren't providing any data, its basically ignored.
What the user expects here is that the internal format is Bgra8, so unless we can emulate this (for rendering/mapping/copies), we shouldn't announce it.
FTR, the story about internal BGRA8 formats on GL is super complicated, see https://phabricator.services.mozilla.com/D21965 comment starting with "On desktop, BGRA8 is not supported"

---

src/backend/gl/src/device.rs, line 1242 at r6 (raw file):

                        for i in 0..num_levels {
                            gl.tex_image_3d(
                                glow::TEXTURE_2D_ARRAY,

similarly, I don't think we should be initializing every level explicitly

---

src/backend/gl/src/info.rs, line 184 at r5 (raw file):

    /// Can map memory
    pub map: bool,
    /// Whether to emulate map memory

please describe in short why this is needed and in what cases

---

src/backend/gl/src/info.rs, line 257 at r5 (raw file):

        let platform_name = PlatformName::get(gl);
        let version =
            Version::parse(get_string(gl, glow::VERSION).unwrap_or_else(|_| String::from("")))

could we use unwrap_or_default()?

---

src/backend/gl/src/info.rs, line 265 at r5 (raw file):

        .unwrap();
        #[cfg(target_arch = "wasm32")]
        let shading_language = Version::new_embedded(3, 0, String::from(""));

Are we confusing WebGL with wasm32 target through the code? We don't have to support Js, but it would be nice if our code didn't actively prevent this if we change our minds.

---

src/backend/gl/src/info.rs, line 294 at r5 (raw file):

    pub fn is_version_supported(&self, major: u32, minor: u32) -> bool {
        !self.version.is_embedded
            && self.version >= Version::new(major, minor, None, String::from(""))

it is kinda awkward that every check here would involve construction of a new String object... Does it have the same property as Vec::new() for not allocating anything? Alternatively, we could use Cow with static lifetime.

---

src/backend/gl/src/info.rs, line 472 at r5 (raw file):

        // TODO && gl.GenFramebuffers.is_loaded(),
        framebuffer_texture: info.is_supported(&[Core(3, 0)]), //TODO: double check
        buffer_role_change: true || !info.version.is_embedded, // TODO

is this a leftover that still needs to be fixed?

---

src/backend/gl/src/info.rs, line 478 at r5 (raw file):

        program_interface: info.is_supported(&[Core(4, 3), Ext("GL_ARB_program_interface_query")]),
        frag_data_location: !info.version.is_embedded,
        sync: false && info.is_supported(&[Core(3, 2), Es(3, 0), Ext("GL_ARB_sync")]), // TODO

if this isn't supported on WASM, we could at least put something like !info.is_wasm &&

---

src/backend/gl/src/lib.rs, line 15 at r5 (raw file):

pub extern crate glutin;
extern crate smallvec;
extern crate spirv_cross;

pretty sure all those extern declarations (other than macro-use and pub) can be removed now with Rust2018

---

src/backend/gl/src/native.rs, line 10 at r2 (raw file):

Previously, grovesNL (Josh Groves) wrote…

Yeah, I think either would probably be ok

let's leave a TODO here

---

src/backend/gl/src/queue.rs, line 427 at r5 (raw file):

                            view[3] as i32,
                        );
                        if self.share.private_caps.depth_range_f64_precision {

I'm confused. Why aren't we just using f32 version unconditionally?

---

src/backend/gl/src/queue.rs, line 829 at r5 (raw file):

                }

                // `MULTISAMPLE` may not be available for OpenGL ES or WebGL

nit: we should have a private flag for it instead of using is_embedded. Also, match false needs to go :)

---

src/backend/gl/src/state.rs, line 160 at r5 (raw file):

}

pub(crate) fn bind_blend_slot(gl: &GlContainer, slot: ColorSlot, desc: &pso::ColorBlendDesc, supports_draw_buffers: bool) {

alternatively, we could pass Share here that has both the GL context and the private flags

---

src/backend/gl/src/window/glutin.rs, line 219 at r5 (raw file):

                                let mut w = w;
                                let mut h = h;
                                for i in 0..config.image_layers {

this is the same problem I pointed @msiglreith to in their PR: we are iterating wrongly on the image_layers

---

src/backend/gl/src/window/web.rs, line 7 at r5 (raw file):

use glow::Context;

fn get_window_extent(window: &Window) -> image::Extent {

does this need to be fixed? If we have no "proper" way, we should have a public facing constant with the Extent

---

src/backend/gl/src/window/web.rs, line 191 at r5 (raw file):

                                let mut w = w;
                                let mut h = h;
                                for i in 0..config.image_layers {

same problem as #2793 (comment))
Also, we should really be sharing this code with regular texture creation

[msiglreith]

Would it be possible to import glow as gl? I would prefer keeping the current style regarding gl::TEXTURE_2D etc.

[grovesNL]

Would it be possible to import glow as gl?

Sure, I don't have a preference here if everyone is fine with renaming it at the import level. It should reduce the diff slightly too.

[kvark]

I don't have a strong feeling about this. The diff will only be spared on the constant usage (e.g. gl::TEXTURE_2D), while entry points are still different. @msiglreith do you feel strongly? or was it just a suggestion to consider?

[grovesNL]

Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @kvark and @msiglreith)

---

examples/Cargo.toml, line 46 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

looks like these wasm/!wasm entries could be merged once glutin is no longer a default (i.e. not the only WSI supported)

Agreed. I purposely didn't change it in this PR, but I think we could remove it as a default soon.

---

examples/README.md, line 23 at r6 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

lets rewrite this to being a large code block with bash syntax? I.e.

cd .. # move to the parent
git clone ... # to clone spirv_cross
...

Might also consider having an entry in our Makefile to do this

Done.

---

examples/quad/main.rs, line 547 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think we are safe to do this on all backends: we can still leave run-time loading for the other examples

Done.

---

examples/quad/main.rs, line 655 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could also do running = !cfg!(target_arch = "wasm"); for a one-liner

Done.

---

examples/quad/main.rs, line 835 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

since running==false here, we are going to leave the loop anyway, do we still need this TODO?

Done.

---

src/backend/gl/Cargo.toml, line 26 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we'll need this published if we want the GL backend to see the light of crates.io

This is published already (it's building successfully on CI)

---

src/backend/gl/src/command.rs, line 121 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would be great to see more typedefs instead of raw integer types. Alternatively, we could name things in this enum, i.e.

BindBufferRange {
  ty: u32,
  binding: u32,
  buffer: n::RawBuffer,
  offset: i32,
  size: i32,
}

Ageed. IMO these could even be newtyped to have something stronger than a type alias. I started doing this initially but backed it out because it seemed too invasive for this PR.

---

src/backend/gl/src/command.rs, line 168 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: should make this a fixed-size vec (or ArrayVec) now that options are inside

Sure, for now this is still resized dynamically whenever necessary so it might be easier to do it in a follow-up PR. What is the maximum we can rely on safely though?

---

src/backend/gl/src/command.rs, line 996 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

somewhat unfortunate that i32 are used for offset and size

Yeah, I guess we could choose either and clamp.

For now I just matched the WebGL bindings everywhere and they use i32. It's because of the type used in the arguments in https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/bindBufferRange (this happens in some other places too)

---

src/backend/gl/src/device.rs, line 1028 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hold on, isn't *mut [u8] a pointer to a run-time sized slice? In this case, the first word in memory would be its length, not the first element.

I didn't think the representation between *mut [u8] and *mut u8 were different – I thought the difference only existed at the type level but I could be wrong. I tried to look for some references but it's hard to Google :)

At least it doesn't look like the length is being read:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6de0b9edb414697e2d8700f93d3828eb

---

src/backend/gl/src/device.rs, line 430 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

it would be cleaner to have a trait with these methods implemented for texture and sampler objects

Agreed, added a TODO in the comments for now

---

src/backend/gl/src/device.rs, line 1218 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I wonder why we are initializing each level separately like this? AFAIK, drivers allocate the whole mipmap chain when called with level = 0 anyway...

Yeah I'm not sure this is necessary. Is there anywhere in the spec that requires it? I could add a TODO or an issue.

---

src/backend/gl/src/device.rs, line 1167 at r6 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

This doesn't look correct... The second member in GL is just used in non-storage initializers for specifying the format of the data. Since we aren't providing any data, its basically ignored.
What the user expects here is that the internal format is Bgra8, so unless we can emulate this (for rendering/mapping/copies), we shouldn't announce it.
FTR, the story about internal BGRA8 formats on GL is super complicated, see https://phabricator.services.mozilla.com/D21965 comment starting with "On desktop, BGRA8 is not supported"

Agreed, but I don't think the behavior changed in this PR. Should I just add a comment for this? Or I could create an issue?

---

src/backend/gl/src/device.rs, line 1242 at r6 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

similarly, I don't think we should be initializing every level explicitly

Agreed

---

src/backend/gl/src/info.rs, line 184 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

please describe in short why this is needed and in what cases

Done.

---

src/backend/gl/src/info.rs, line 257 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could we use unwrap_or_default()?

Done.

---

src/backend/gl/src/info.rs, line 265 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Are we confusing WebGL with wasm32 target through the code? We don't have to support Js, but it would be nice if our code didn't actively prevent this if we change our minds.

I think so, but it's not clear how best to support both. I could use a feature here but I had some trouble combining feature and target when specifying dependencies. Is there something else we should do here instead?

IIRC at the time I was writing the target checks, there were some problems with crates trying to target both wasm32-unknown-unknown and wasm32-unknown-emscripten.

---

src/backend/gl/src/info.rs, line 294 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

it is kinda awkward that every check here would involve construction of a new String object... Does it have the same property as Vec::new() for not allocating anything? Alternatively, we could use Cow with static lifetime.

Yeah, I think it needs some more rework in general now that we can't assume we're working with &'static str so I took the most straightforward path for this PR. So my longer-term idea is to basically move more of the version and extension checking into glow and rework it at that point. What do you think?

---

src/backend/gl/src/info.rs, line 472 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is this a leftover that still needs to be fixed?

IIRC this is related to #2812 and it happens to work for quad without requiring buffer role change, so I disabled it here to skip the assertion. I think we need to fix #2812 in order to remove this and still have quad work (otherwise I could disable the assertion)

---

src/backend/gl/src/info.rs, line 478 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if this isn't supported on WASM, we could at least put something like !info.is_wasm &&

Done.

---

src/backend/gl/src/lib.rs, line 15 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

pretty sure all those extern declarations (other than macro-use and pub) can be removed now with Rust2018

Done.

---

src/backend/gl/src/native.rs, line 10 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's leave a TODO here

Done.

---

src/backend/gl/src/queue.rs, line 427 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'm confused. Why aren't we just using f32 version unconditionally?

We could do that, I wasn't sure if clamping to f32 would be fine, for example

---

src/backend/gl/src/queue.rs, line 829 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: we should have a private flag for it instead of using is_embedded. Also, match false needs to go :)

Alright, I added a flag for now.

It looks like the match false came from some samples refactoring a long time ago: a7aebbc#diff-40ec989d89402e8df9d92c26c6491ed1R60

---

src/backend/gl/src/state.rs, line 160 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

alternatively, we could pass Share here that has both the GL context and the private flags

Done.

---

src/backend/gl/src/window/glutin.rs, line 219 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

this is the same problem I pointed @msiglreith to in their PR: we are iterating wrongly on the image_layers

Yeah this already existed before the PR, should I add a TODO or create an issue?

---

src/backend/gl/src/window/web.rs, line 7 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does this need to be fixed? If we have no "proper" way, we should have a public facing constant with the Extent

I think it needs to be fixed soon, but we can still make progress until we have a better solution. For now I've been watching winit progress on wasm support and hoping that it becomes our solution for web window handling. Otherwise we can create our own basic windowing this extent won't be hardcoded.

---

src/backend/gl/src/window/web.rs, line 191 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

same problem as #2793 (comment))
Also, we should really be sharing this code with regular texture creation

Yeah, most of the implementation could probably be shared between the swapchain too – see my TODO above :)

[kvark]

Thank you! It's time for this ship to sail :)
bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push