Add `from_canvas()` to `Surface` for Wasm #76

daxpedda · 2023-02-28T21:17:21Z

Added methods to create Surface from a HtmlCanvasElement or OffscreenCanvas following wgpu's example https://github.com/gfx-rs/wgpu/blob/v0.15.1/wgpu/src/lib.rs#L1570-L1640

My use case required this to render in a web worker.

Edit: See this for a future alternative implementation rust-windowing/raw-window-handle#102

This PR addresses the issues in #74 and additionally handles an error when using OffscreenCanvas, which has a different context type: OffscreenCanvasRenderingContext2d. It was assumed that it inherits from CanvasRenderingContext2d, which it does not, see the MDN documentation.

I spoke to @Toniman575 after we discussed some issues, he told me to replace his PR instead.

Replaces #74.

src/lib.rs

src/web.rs

daxpedda · 2023-02-28T21:24:07Z

src/web.rs

+#[wasm_bindgen]
+extern "C" {
+    #[wasm_bindgen(js_name = OffscreenCanvasRenderingContext2D)]
+    pub type OffscreenCanvasRenderingContext2d;
+
+    #[wasm_bindgen(catch, method, structural, js_class = "OffscreenCanvasRenderingContext2D", js_name = putImageData)]
+    fn put_image_data(
+        this: &OffscreenCanvasRenderingContext2d,
+        imagedata: &ImageData,
+        dx: f64,
+        dy: f64,
+    ) -> Result<(), JsValue>;
+
+    #[wasm_bindgen(method, structural, js_class = "OffscreenCanvasRenderingContext2D")]
+    pub fn commit(this: &OffscreenCanvasRenderingContext2d);
+}


web-sys doesn't currently support OffscreenCanvasRenderingContext2D, a PR has already been made and merged, but it will require a new version release: rustwasm/wasm-bindgen#3319.

Let's wait for that, then

Alright, will put it into draft mode then.

daxpedda · 2023-02-28T21:26:53Z

CI failures are because sudo apt-get update && sudo apt-get install gcc-multilib fails on Linux.

ids1024 · 2023-02-28T22:37:59Z

CI seems to pass after rerunning the failed jobs.

daxpedda · 2023-02-28T22:38:51Z

Thanks 🎉.

notgull · 2023-03-01T14:47:28Z

src/web.rs

+    fn from_canvas(canvas: HtmlCanvasElement) -> Result<Self, SoftBufferError>;
+
+    /// Creates a new instance of this struct, using the provided [`HtmlCanvasElement`].
+    fn from_offscreen_canvas(offscreen_canvas: OffscreenCanvas) -> Result<Self, SoftBufferError>;


I'd still like to avoid having web_sys as a public dependency if possible. I know winit does it, but I'd like to keep the number of public dependencies in this project as low as possible.

Would it be possible to just pass in the ID/XPath of the canvas into these functions instead?

I also want to add that wgpu does that too: https://github.com/gfx-rs/wgpu/blob/v0.15.1/wgpu/src/lib.rs#L1570-L1640.

It would be possible for HtmlCanvasElement, but not for OffscreenCanvas, because it's not a DOM element.
The best alternative I can come up with is using wasm_bindgen::JsValue.

Another unsafe alternative I can think of is using a pointer, basically. See wasm_bindgen::convert::IntoWasmAbi.

Completely different direction: we could maybe hide this behind a feature called wasm-bindgen instead of just hiding it behind target_arch = wasm32, if this alleviates some of your concerns.

I want to point out though, that specifically with the wasm-bindgen ecosystem there is not much gained from avoiding making it a public dependency. The thing is that if a library uses wasm-bindgen, the library can't actually work without downstream users having to use wasm-bindgen-cli, so switching to a different ecosystem would still require a breaking change. Even having different versions is not allowed, wasm-bindgen-cli will error out if you do.

At least to me, versioning and the inability to change a dependency without a breaking change, are the big reasons to keep public dependencies low.

EDIT: For more context: this issue with wasm-bindgen and being locked into it's ecosystem is not going to go away anytime soon. The problem is that to make wasm-bindgen work, a JS shim has to be created that provides all the necessary imports, this is done by wasm-bindgen-cli by actually parsing the compiled Wasm file and then extracting the necessary information. Without running wasm-bindgen-cli your Wasm file is unusable. Not only does it generate the JS shim, but it actually modifies the Wasm file to actually make it work, this includes removing garbage that is inserted during the compilation by wasm-bindgen and properly hooking up imports and what wasm-bindgen calls intrinsics, basically things like referencing JsValues, copying arrays and what not.

The only salvation in sight is the Component Model Proposal, which is still at Phase 1.

But when it eventually gets accepted, we won't need wasm-bindgen anymore and hopefully we can move to ecosystem neutral Wasm libraries that don't force downstream users in one or the other ecosystem.

That sounds reasonable to me, I'd be fine with the public dep on web-sys in this case.

daxpedda · 2023-04-06T16:02:23Z

Rebased after #65.

daxpedda · 2023-04-06T17:47:46Z

Rebased after #77.

daxpedda · 2023-04-26T09:59:50Z

Rebased after #88.

notgull · 2023-05-07T16:36:52Z

Would it be possible to split this PR into one that adds the ability to use a HtmlCanvasElement and one for OffscreenCanvas? We can do the first one now but we need to wait until rustwasm/wasm-bindgen#3319 for the second one.

daxpedda · 2023-05-08T10:13:25Z

Cargo.toml

+[package.metadata.docs.rs]
+all-features = true
+rustdoc-args = ["--cfg", "docsrs"]
+targets = ["x86_64-unknown-linux-gnu", "wasm32-unknown-unknown"]


The other targets don't have any target-specific APIs, should I still add them to the list?

I would say yes; I think they've expected at this point.

Only added 64-bit targets, say the word if I should add anything else.

daxpedda · 2023-05-08T10:24:31Z

Would it be possible to split this PR into one that adds the ability to use a HtmlCanvasElement and one for OffscreenCanvas? We can do the first one now but we need to wait until rustwasm/wasm-bindgen#3319 for the second one.

Done: #103.

notgull

LGTM aside from minor nitpicks

src/web.rs

daxpedda requested a review from john01dav as a code owner February 28, 2023 21:17

daxpedda force-pushed the web-canvas branch from 5efc5d5 to fd9c142 Compare February 28, 2023 21:21

daxpedda commented Feb 28, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

daxpedda commented Feb 28, 2023

View reviewed changes

src/web.rs Outdated Show resolved Hide resolved

daxpedda commented Feb 28, 2023

View reviewed changes

daxpedda mentioned this pull request Feb 28, 2023

Fix multi-threaded Wasm #77

Merged

Toniman575 mentioned this pull request Mar 1, 2023

added from_canvas and from_offscreen_canvas methods to Surface #74

Closed

notgull reviewed Mar 1, 2023

View reviewed changes

daxpedda marked this pull request as draft March 1, 2023 15:41

daxpedda force-pushed the web-canvas branch from 8da79d1 to 044a7f9 Compare April 6, 2023 16:01

daxpedda force-pushed the web-canvas branch from 044a7f9 to e12f3db Compare April 6, 2023 17:47

daxpedda force-pushed the web-canvas branch from e12f3db to 206ded7 Compare April 26, 2023 09:49

Added from_canvas methods to Surface

2360138

daxpedda force-pushed the web-canvas branch from 206ded7 to 2360138 Compare April 26, 2023 09:55

notgull mentioned this pull request May 5, 2023

Add @daxpedda as web maintainer #102

Merged

Remove OffscreenCanvas support

206bd17

daxpedda force-pushed the web-canvas branch from 5bc0dee to 206bd17 Compare May 8, 2023 10:08

Document Wasm on docs.rs

d862add

daxpedda commented May 8, 2023

View reviewed changes

daxpedda mentioned this pull request May 8, 2023

Add OffscreenCanvas support #103

Merged

daxpedda marked this pull request as ready for review May 8, 2023 10:24

daxpedda requested a review from notgull May 8, 2023 10:30

notgull approved these changes May 8, 2023

View reviewed changes

src/web.rs Outdated Show resolved Hide resolved

Address review

ce20d55

daxpedda requested a review from notgull May 8, 2023 13:33

notgull approved these changes May 8, 2023

View reviewed changes

daxpedda merged commit fcda747 into rust-windowing:master May 8, 2023
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `from_canvas()` to `Surface` for Wasm #76

Add `from_canvas()` to `Surface` for Wasm #76

daxpedda commented Feb 28, 2023 •

edited

Loading

daxpedda Feb 28, 2023

madsmtm Mar 1, 2023

daxpedda Mar 1, 2023

daxpedda commented Feb 28, 2023

ids1024 commented Feb 28, 2023

daxpedda commented Feb 28, 2023

notgull Mar 1, 2023

daxpedda Mar 1, 2023 •

edited

Loading

madsmtm Mar 2, 2023 •

edited

Loading

daxpedda commented Apr 6, 2023

daxpedda commented Apr 6, 2023

daxpedda commented Apr 26, 2023

notgull commented May 7, 2023

daxpedda May 8, 2023

notgull May 8, 2023

daxpedda May 8, 2023

daxpedda commented May 8, 2023

notgull left a comment

Add from_canvas() to Surface for Wasm #76

Add from_canvas() to Surface for Wasm #76

Conversation

daxpedda commented Feb 28, 2023 • edited Loading

daxpedda Feb 28, 2023

Choose a reason for hiding this comment

madsmtm Mar 1, 2023

Choose a reason for hiding this comment

daxpedda Mar 1, 2023

Choose a reason for hiding this comment

daxpedda commented Feb 28, 2023

ids1024 commented Feb 28, 2023

daxpedda commented Feb 28, 2023

notgull Mar 1, 2023

Choose a reason for hiding this comment

daxpedda Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

madsmtm Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

daxpedda commented Apr 6, 2023

daxpedda commented Apr 6, 2023

daxpedda commented Apr 26, 2023

notgull commented May 7, 2023

daxpedda May 8, 2023

Choose a reason for hiding this comment

notgull May 8, 2023

Choose a reason for hiding this comment

daxpedda May 8, 2023

Choose a reason for hiding this comment

daxpedda commented May 8, 2023

notgull left a comment

Choose a reason for hiding this comment

Add `from_canvas()` to `Surface` for Wasm #76

Add `from_canvas()` to `Surface` for Wasm #76

daxpedda commented Feb 28, 2023 •

edited

Loading

daxpedda Mar 1, 2023 •

edited

Loading

madsmtm Mar 2, 2023 •

edited

Loading