search

kleisauke / wasm-vips

libvips for the browser and Node.js, compiled to WebAssembly with Emscripten.
https://kleisauke.github.io/wasm-vips/
MIT License
463 stars 25 forks source link

Add SVG support via resvg #40

Closed RReverser closed 1 year ago

RReverser commented 1 year ago

This is still missing text rendering because dealing with fonts is its own problem, but otherwise works well and is enough to pass Sharp tests.

Wanted to submit anyway so that my branch / work isn't lost, but leaving disabled by default because it's both incomplete w/o text, but also because it's a relatively rarely useful feature. More common problem is optimising SVG to SVG, for which people use tools like SVGO, while in libvips SVG can only be rasterised and treated like other raster images.

d3lm commented 1 year ago

Nice, still great to have this! Awesome work 👏

RReverser commented 1 year ago

Just a heads up - might be better to merge https://github.com/kleisauke/libvips/pull/2 before this one to reduce complexity of patches in this PR.

kleisauke commented 1 year ago

Great! I was able to support this as a dynamic module and also added some test coverage for this (by porting libvips' SVG tests). https://github.com/RReverser/wasm-vips/compare/libresvg...kleisauke:wasm-vips:RReverser-libresvg https://github.com/RReverser/libvips/compare/wasm-vips-resvg...kleisauke:RReverser-wasm-vips-resvg?w=1 (?w=1 to hide those changes in commit https://github.com/kleisauke/libvips/commit/0b75df254a478b29580ce5d92ba2ed272446883e :sweat_smile:)

I could push these commits to the branches associated with this PR and https://github.com/kleisauke/libvips/pull/2, if that looks good to you.

FWIW, I'm planning to upstream the Rust patches introduced in commit https://github.com/kleisauke/wasm-vips/commit/94e67b1617fc096d2a65b64f4243a7b64f5f4559, here is the relevant CI failure which is the reasoning behind these patches: https://github.com/kleisauke/wasm-vips/actions/runs/3947805916/jobs/6757635992#step:7:225

RReverser commented 1 year ago

here is the relevant CI failure which is the reasoning behind these patches: https://github.com/kleisauke/wasm-vips/actions/runs/3947805916/jobs/6757635992#step:7:225

That error looks odd. Why is a Windows CI task using nightly-x86_64-unknown-linux-gnu toolchain?

RReverser commented 1 year ago

Or, I suppose it's actually Wasm / Emscripten build, but then the triple should still be Emscripten's I think?

kleisauke commented 1 year ago

Why is a Windows CI task using nightly-x86_64-unknown-linux-gnu toolchain? I suppose it's actually Wasm / Emscripten build, but then the triple should still be Emscripten's I think?

I think Rustup uses the build triple (i.e. x86_64-unknown-linux-gnu) instead of the host triple (i.e. wasm32-unknown-emscripten) as the directory name to install its toolchains in.

I'm not sure why this information about the source code is emitted at all in the binary, I did not expect that in release builds.

$ grep "/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/rand.rs" lib/vips-resvg.wasm 
grep: lib/vips-resvg.wasm: binary file matches

FWIW, it also fails on commit 2eed1c3daa034fcebf23aadc50b41aba11862348, see e.g.: https://github.com/kleisauke/wasm-vips/actions/runs/3942488976/jobs/6746825745#step:7:223

thread '<unnamed>' panicked at 'failed to open /dev/urandom: Os { code: 44, kind: NotFound, message: "No such file or directory" }', /root/.rustup/toolchains/nightly-2023-01-12-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/rand.rs:135:51
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Commit a23b339f3567e1f743ed293e86f6e8566b4d1dac changes that directory to:

/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/
kleisauke commented 1 year ago

... the reason it only fails on Windows is that it does not have /dev/urandom available (this path is only appropriate on Unix-based OSes). I suppose this is not a issue if you do not link with -sNODERAWFS or use WasmFS instead.

kleisauke commented 1 year ago

With this patch:

--- a/build.sh
+++ b/build.sh
@@ -171,6 +171,11 @@ fi
 if [ "$WASM_FS" = "true" ]; then export CFLAGS+=" -DWASMFS"; fi
 if [ "$PIC" = "true" ]; then export CFLAGS+=" -fPIC"; fi

+# Ensure build paths are removed from the resulting binaries
+# https://reproducible-builds.org/docs/build-path/
+export RUSTFLAGS+=" --remap-path-prefix=$(rustc --print sysroot)/= --remap-path-prefix=$DEPS/="
+export CFLAGS+=" -ffile-prefix-map=$DEPS/="
+
 export CXXFLAGS="$CFLAGS"

 export LDFLAGS="$COMMON_FLAGS -L$TARGET/lib -sAUTO_JS_LIBRARIES=0 -sAUTO_NATIVE_LIBRARIES=0"

I now see:

$ grep "/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu" lib/vips-resvg.wasm || echo "no matches"
no matches

I'll commit that after this PR lands. :)

kleisauke commented 1 year ago

Commit https://github.com/kleisauke/wasm-vips/commit/54bce444a611966e885b053b90f57598b8af2eb1 fixes the remaining tests. I just opened PR https://github.com/libvips/libvips/pull/3307 for that.

kleisauke commented 1 year ago

I'm planning to upstream the Rust patches introduced in commit 94e67b1

I just opened draft PR https://github.com/rust-lang/rust/pull/107221 and https://github.com/rust-lang/libc/pull/3087 for this.

RReverser commented 1 year ago

I just opened draft PR rust-lang/rust#107221 and rust-lang/libc#3087 for this.

Nice! Out of curiosity, why do you create those PRs as drafts? They look like should be ready for review straight away?

kleisauke commented 1 year ago

Out of curiosity, why do you create those PRs as drafts? They look like should be ready for review straight away?

The Rust PR depends on the libc PR, so the libc crate needs to be updated first in Rust (i.e. a similar commit as https://github.com/rust-lang/rust/commit/e294640484731dca722f2cc3509a18e8b4a4a20d is needed).

The libc PR was marked as draft because it does not meet this check:

  • [ ] Edit corresponding file(s) under libc-test/semver when you add/remove item(s)

But perhaps I could add a new libc-test/semver/emscripten.txt file in that PR?

RReverser commented 1 year ago

The libc PR was marked as draft because it does not meet this check:

I don't think that's necessary a criteria for draft, it just will be part of review comments? Idk, I just almost never use the draft PR feature so was curious about your usage / rationale.

kleisauke commented 1 year ago

I don't think that's necessary a criteria for draft, it just will be part of review comments?

Ah, sorry I misread your "review straight away" as "merge straight away".

I usually open pull requests as a draft if I think it's not ready to merge yet, these PRs can be reviewed but not merged as-is (because it is dependent on another PR or still being worked on).

Perhaps I should treat draft PRs as non-reviewable rather than non-mergeable? If so, the draft status of these PRs can be removed.

RReverser commented 1 year ago

should treat draft PRs as non-reviewable

Ah that probably is our mismatch - that's how I understood them since they were introduced, yeah, sort of "work in progress, please don't review yet". After all, the button on the draft PRs is called "Ready for review".

atjn commented 1 year ago

From https://github.com/kleisauke/wasm-vips/pull/40#issue-1533328986

[this is] a relatively rarely useful feature

I just wanted to mention that I need this because I want to convert my website's icon into web app manifest icons. The website icon is SVG because that is the best format, but SVG is not supported on all platforms for the manifest, so I need to convert it to PNG. I am very happy to see you beginning work on it - thanks!

kleisauke commented 1 year ago

I just opened draft PR https://github.com/rust-lang/rust/pull/107221 and https://github.com/rust-lang/libc/pull/3087 for this.

It seems that a release of libc with that PR included is taking longer than expected. I'll merge this as-is after the CI passes (the Rust nightly version is pinned, so I don't expect any build failures).