svgload: introduce rgb128 parameter for SVG 128-bit load support #4523

kstanikviacbs · 2025-05-19T13:26:00Z

With the increasing demand for high quality images - a need of rendering SVG with increased color-depth (more than 8 bit) has also appeared.

Given Cairo introduced support for new data format unlocking this option for libvips - CAIRO_FORMAT_RGBA128F: https://www.cairographics.org/news/cairo-1.17.2/ - this PR is an attempt to introduce rgb128 flag into svgload to enable high-bit SVG rendering if needed.

jcupitt

It seems to work well! I marked a few style things that I think need changing.

jcupitt · 2025-05-19T14:26:44Z

meson.build

@@ -657,6 +664,7 @@ build_summary = {
     'enable Analyze7 load': [get_option('analyze')],
     'enable PPM load/save': [get_option('ppm')],
     'enable GIF load': [get_option('nsgif')],
+     'enable SVG 128-bit rendering': [svg_128bit_support]


This should probably go -- it's not really a build option.

Good point, removed.

meson.build

libvips/foreign/svgload.c

libvips/foreign/cairo.c

libvips/foreign/svgload.c

jcupitt · 2025-05-19T15:08:19Z

I tested with a sample SVG:

$ vips svgload lion.svg x.v --rgb128

And I get:

Very nice!

jcupitt · 2025-05-19T15:16:47Z

... and thank you for doing this work, Konrad, good job.

kstanikviacbs · 2025-05-19T19:56:54Z

Thanks for your review @jcupitt . I did my best to apply all your great suggestions. Please take a look again at your convenience.

kleisauke · 2025-05-20T08:42:38Z

Commit kleisauke@c4d898f has some minor formatting improvements and fixes the compilation when HAVE_128BIT_SVG_RENDERING is undefined, e.g.:

../libvips/foreign/svgload.c: In function ‘vips_foreign_load_svg_load’:
../libvips/foreign/svgload.c:707:13: error: ‘svg’ undeclared (first use in this function)
  707 |         if (svg->rgb128) {
      |             ^~~
../libvips/foreign/svgload.c:707:13: note: each undeclared identifier is reported only once for each function it appears in
../libvips/foreign/svgload.c:708:28: error: ‘class’ undeclared (first use in this function)
  708 |                 vips_error(class->nickname, "128-bit rendering is not supported with this version");
      |                            ^~~~~

Feel free to cherry-pick.

kstanikviacbs · 2025-05-20T08:58:34Z

Commit kleisauke@c4d898f has some minor formatting improvements and fixes the compilation when HAVE_128BIT_SVG_RENDERING is undefined, e.g.:

../libvips/foreign/svgload.c: In function ‘vips_foreign_load_svg_load’:
../libvips/foreign/svgload.c:707:13: error: ‘svg’ undeclared (first use in this function)
  707 |         if (svg->rgb128) {
      |             ^~~
../libvips/foreign/svgload.c:707:13: note: each undeclared identifier is reported only once for each function it appears in
../libvips/foreign/svgload.c:708:28: error: ‘class’ undeclared (first use in this function)
  708 |                 vips_error(class->nickname, "128-bit rendering is not supported with this version");
      |                            ^~~~~

Feel free to cherry-pick.

Thanks @kleisauke ! I cherry-picked whole your commit.

jcupitt · 2025-05-20T09:06:04Z

Great! It's looking rather neat now, I think.

We need two more things:

doc comments near the bottom of svgload.c: you need to add rgb128 to the list of optional arguments for vips_svgload*(), and vips_svgload() needs a paragraph explaining what the flag does, eg. "Setting @rgb128 TRUE enables 128-bit scRGB output."
this is an API change, so we need a line in the main CHANGELOG
don't forget to credit yourself!

jcupitt · 2025-05-20T09:08:53Z

Sorry to bring this up so late, but I wonder if rgb128 is the best name for this flag? It's RGBA, not RGB, and it's 128 bits per pixel not per band element (the size measure we use elsewhere).

Savers use bitdepth to control the write depth of images. Maybe we should have a bitdepth parameter which defaults to 8 but can be set to 32? Can anyone think of another loader with a similar setting?

Does anyone have any thoughts on this?

kstanikviacbs · 2025-05-20T09:27:20Z

Sorry to bring this up so late, but I wonder if rgb128 is the best name for this flag? It's RGBA, not RGB, and it's 128 bits per pixel not per band element (the size measure we use elsewhere).

Savers use bitdepth to control the write depth of images. Maybe we should have a bitdepth parameter which defaults to 8 but can be set to 32? Can anyone think of another loader with a similar setting?

Does anyone have any thoughts on this?

That's a very good point. I was also thinking about it for a while. I did some quick research and haven't found any loaders exposing such option. On one hand - bitdepth seems to follow naming convention touching such aspects and following it seems natural. On the other though - if we have only two options (8 and 32) for now (and probably this won't change soon) keeping it as bool is also a bit less misleading to me, this might be all about the parameter name we should change, perhaps something like: high_bit_depth flag with documented 32-bit value?

I'm happy to refactor it to be bitdepthanyway if that's what you guys prefer.

kstanikviacbs · 2025-05-20T09:28:29Z

Great! It's looking rather neat now, I think.

We need two more things:

* doc comments near the bottom of `svgload.c`: you need to add `rgb128` to the list of optional arguments for `vips_svgload*()`, and `vips_svgload()` needs a paragraph explaining what the flag does, eg. "Setting @rgb128 TRUE enables 128-bit scRGB output."

* this is an API change, so we need a line in the main CHANGELOG

* don't forget to credit yourself!

Thanks! I'd postpone applying this for a while till we have a conclusion re: rgb128 :)

jcupitt · 2025-05-20T09:14:33Z

cplusplus/include/vips/VImage8.h

@@ -5784,6 +5784,7 @@ class VImage : public VObject {
 	 *   - **access** -- Required access pattern for this file, VipsAccess.
 	 *   - **fail_on** -- Error level to fail on, VipsFailOn.
 	 *   - **revalidate** -- Don't use a cached result for this operation, bool.
+	 *   - **rgb128** -- Enable 128-bit rendering


These doc comments are generated automatically, so you don't need to include them in a PR.

This text will be replaced by the _("Enable 128-bit SVG rendering") in the ARG declaration next time this script runs:

https://github.com/libvips/libvips/blob/master/cplusplus/gen-operators.py

(no need for you to do anything, just a note)

jcupitt · 2025-05-20T09:15:31Z

libvips/foreign/cairo.c

+ * Processes ''n'' pixels in the ''p'' buffer.
+ * The data is assumed to be RGBA (R, G, B, A) 32-bit floats per pixel.
+ */
+void vips__rgba128f_unpremultiplied(float *p, int n) {


This need to go through clang-format. libvips style is:

void vips__rgba128f_unpremultiplied(float *p, int n) { ... }

Sorry, I've just realised this name is wrong :( I should have said this before.

The other converters are named as eg. vips__premultiplied_bgra2rgba(), meaning "from cairo-style premultiplied bgra to libvips rgba".

Your converter is premultiplied rgba128 to libvips scrgba, so it should be called vips__premultiplied_rgb1282scrgba().

svgload: introduce rgb128 parameter for SVG 128-bit load support

42fd9fb

jcupitt reviewed May 19, 2025

View reviewed changes

kstanikviacbs force-pushed the svgload_128bit_support branch 3 times, most recently from 3e74ffb to 9ca1b76 Compare May 19, 2025 19:52

PR feedback addressed.

c6ffae0

kstanikviacbs force-pushed the svgload_128bit_support branch from 9ca1b76 to c6ffae0 Compare May 19, 2025 19:54

Minor improvements

10433b3

jcupitt approved these changes May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

svgload: introduce rgb128 parameter for SVG 128-bit load support #4523

svgload: introduce rgb128 parameter for SVG 128-bit load support #4523

kstanikviacbs commented May 19, 2025

jcupitt left a comment

jcupitt May 19, 2025

kstanikviacbs May 19, 2025

jcupitt commented May 19, 2025

jcupitt commented May 19, 2025

kstanikviacbs commented May 19, 2025

kleisauke commented May 20, 2025

kstanikviacbs commented May 20, 2025

jcupitt commented May 20, 2025

jcupitt commented May 20, 2025

kstanikviacbs commented May 20, 2025 •

edited

Loading

kstanikviacbs commented May 20, 2025

jcupitt May 20, 2025

jcupitt May 20, 2025

jcupitt May 20, 2025

svgload: introduce rgb128 parameter for SVG 128-bit load support #4523

Are you sure you want to change the base?

svgload: introduce rgb128 parameter for SVG 128-bit load support #4523

Conversation

kstanikviacbs commented May 19, 2025

jcupitt left a comment

Choose a reason for hiding this comment

jcupitt May 19, 2025

Choose a reason for hiding this comment

kstanikviacbs May 19, 2025

Choose a reason for hiding this comment

jcupitt commented May 19, 2025

jcupitt commented May 19, 2025

kstanikviacbs commented May 19, 2025

kleisauke commented May 20, 2025

kstanikviacbs commented May 20, 2025

jcupitt commented May 20, 2025

jcupitt commented May 20, 2025

kstanikviacbs commented May 20, 2025 • edited Loading

kstanikviacbs commented May 20, 2025

jcupitt May 20, 2025

Choose a reason for hiding this comment

jcupitt May 20, 2025

Choose a reason for hiding this comment

jcupitt May 20, 2025

Choose a reason for hiding this comment

kstanikviacbs commented May 20, 2025 •

edited

Loading