Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.76k stars 313 forks source link

Please support Rust-free `librsvg` #7060

Open barracuda156 opened 4 months ago

barracuda156 commented 4 months ago

Please support Rust-free librsvg, since Rust is broken on a number of platforms with unclear timeline when it gets fixed if ever. And unfortunately, Gnome folks require Rust for the current librsvg versions.

Lawrence37 commented 4 months ago

librsvg 2.52 or higher is required for hi-dpi, so there may not be a good solution.

barracuda156 commented 4 months ago

librsvg 2.52 or higher is required for hi-dpi, so there may not be a good solution.

@Lawrence37 You mean for the output for Retina displays? It is perfectly fine not to have it on systems without Rust (which are likely to be either non-mainstream or not too powerful).

Rather than losing the functionality, it is much preferable to have it reasonably limited. I.e., retain the codepath for earlier librsvg when newer one is not detected.

P. S. Some people may also have security concerns with Rust, since it can only be compiled with itself presently.

Lawrence37 commented 4 months ago

Hi-dpi required some significant changes, so it may not be feasible to make it optional. That's what I meant by "no good solution". @Pandagrapher knows more about it than me.

barracuda156 commented 4 months ago

@Lawrence37 Oh, that would kill RawTherapee for all systems without Rust. That would be really bad…

RJVB commented 1 month ago

Hi-dpi required some significant changes, so it may not be feasible to make it optional. That's what I meant by "no good solution". @Pandagrapher knows more about it than me.

I don't plan to get involved here but I think there must be a possibility to pipe the output of older, normal-res librsvg routines into the hi-res code path, via a scale factor?

Or make the entire support for SVG optional so at least all the other features continue to work? (SVG isn't exactly a photo format in my book...)

Lawrence37 commented 1 month ago

That requires maintaining the old code along with the new code, which is exactly the option that I consider infeasible. RawTherapee doesn't support editing SVG images. We only use SVG for the icons in the GUI.

barracuda156 commented 1 month ago

@Lawrence37 Is it really worth it for the sake of icons in the gui?

If it can be done in a modular way, that will also solve the problem. I don’t care about icons in the gui, can I have conversion engine etc. updated while staying with existing gui?

Lawrence37 commented 1 month ago

Are you saying HiDPI support is not worth it?

barracuda156 commented 1 month ago

@Lawrence37 AFAIK, pretty much all apps using GTK in MacPorts work at the moment on systems without Retina support and without Rust in the same versions which work on the latest macOS. I did not look into how it is done, to be honest. GTK3 has some glitches with GUI on arm64, GTK4 works fine there (it requires manual env setting on legacy macOS, but no special codepath, AFAIK).

I am not the authority to judge what is worth, of course. From my side it looks that this should be doable in some way with minimal effort without a burden of maintaining a dedicated codepath, since this seems to be the case generally.

Regardless of that though, a modular option will be a solution and perhaps a general improvement. As long as core components can be updated separately from GUI, we are good.

Benitoite commented 1 month ago

@Hombre57 authored the switch to svg icons for hidpi support. At the time, five years ago, there were people requesting hidpi support, and no one requesting macosx/ppc support, so... I have to believe there is a way to get rust on ppc macosx. Have you looked at any solutions like https://github.com/rust-lang/rustc_codegen_gcc

Lawrence37 commented 1 month ago

I will let @Pandagrapher explain why RawTherapee needs the minimum version that it asks for, since I do not know the specifics.

A clean separation of the GUI is not possible without a complete redesign due to the tight coupling between the tools' UI and parameters.

RJVB commented 1 month ago

That requires maintaining the old code along with the new code, which is exactly the option that I consider infeasible.

Sorry, what is "that" here?

Did librsvg break or introduce new API when they introduced hi-res support? If not I don't see why you'd need to maintain any old code. As a null approach you could just use whatever librsvg returns (some kind of bitmap imagine I presume), letting people who insist on using an old version put up with icons that are too small.

Actually, that might be exactly what you get when you tweak the build to use an old librsvg version?

We only use SVG for the icons in the GUI.

Doesn't GTk have functions for loading and rendering icons that work as you'd expect regardless of screen resolution?! And that would accept PNG versions too (preferable anyway in my book: disk space is cheap, CPU cycles not).

Benitoite commented 1 month ago

Sorry, what is "that" here?

Going back to png icons.

Pandagrapher commented 1 month ago

Hi-dpi required some significant changes, so it may not be feasible to make it optional. That's what I meant by "no good solution". @Pandagrapher knows more about it than me.

We require librsvg 2.52 or higher for the get_intrinsic_size_in_pixels function (added from this version) that allows retrieving the SVG size in pixels. This function is required even for non-HiDPI screen (we just perform a scaling or not according to HiDPI setting).

Doesn't GTk have functions for loading and rendering icons that work as you'd expect regardless of screen resolution?! And that would accept PNG versions too (preferable anyway in my book: disk space is cheap, CPU cycles not).

RT 5.10 now supports icon themes that means icons can be prodivided in PNG and/or SVG. Such as more and more Gtk app (e.g. Inkscape), we only provide SVG version of icon theme but you still can generate PNG ones for each pixel size.

barracuda156 commented 1 month ago

Hi-dpi required some significant changes, so it may not be feasible to make it optional. That's what I meant by "no good solution". @Pandagrapher knows more about it than me.

We require librsvg 2.52 or higher for the get_intrinsic_size_in_pixels function (added from this version) that allows retrieving the SVG size in pixels. This function is required even for non-HiDPI screen (we just perform a scaling or not according to HiDPI setting).

@Pandagrapher Maybe it is possible to add a fallback just for this one function? librsvg worked somehow without it pre-2.52.

Pandagrapher commented 1 month ago

@Pandagrapher Maybe it is possible to add a fallback just for this one function? librsvg worked somehow without it pre-2.52.

If all the icons in RT icon theme are converted to PNG for different sizes, basically librsvg is not required anymore by RT. However, as some other Gtk app (e.g. Inkscape), we made the choice to only provide SVG only icon theme. As a workaround, you can locally convert the icons to PNG and stub use of RTScalable::loadSurfaceFromIcon function in code

RJVB commented 1 month ago

We require librsvg 2.52 or higher for the get_intrinsic_size_in_pixels function (added from this version) that allows retrieving the SVG size in pixels.

That does sound like something that one could home-roll even if it's at the cost of doing an actual rendering of the graphic. (But then again I don't see what "intrinsic size in pixels" means for a vector drawing so I may misunderstand something completely here.)

as more and more Gtk app (e.g. Inkscape), we only provide SVG version of icon theme

What I was hinting at of course is that Qt has had built-in support for SVG-based icons since just about forever O:^)

Benitoite commented 1 month ago

Intrinsic size is internal to the svg, see https://gnome.pages.gitlab.gnome.org/librsvg/Rsvg-2.0/method.Handle.get_intrinsic_size_in_pixels.html

RJVB commented 1 month ago

Intrinsic size is internal to the svg

As I thought, an unhappy name, it's just the intended size in pixels based on the SVG's intended size in physical units and the rendering density.

I suppose it should be possible to write an equivalent function in C/++, or maybe even translate (de-oxidise?) the existing rusty one.

Are there any constraints on the kind of physical units you are expecting (or would expect) in SVG icons?

How do you handle the case where the function returns false, either because there is no physical size set in the SVG or because it's not an actual physical size but a relative one? In other words, would a dummy

gboolean rsvg_handle_get_intrinsic_size_in_pixels (
  RsvgHandle* , gdouble* , gdouble* )
{
    return FALSE;
}

be a workable fallback?

Pandagrapher commented 1 month ago

It's still possible to add custom code, specific fallback, ... The question is "Should we?"

For the following reasons, IMO we shouldn't develop a fix:

barracuda156 commented 1 month ago

Gtk4 have planned to directly use librsvg to render SVG (in the PR, minimum version is 2.52 for librsvg). When RT will start migrating to Gtk4, use of librsvg rust version would be de facto mandatory

As of now at least, gtk4 works fine with rust-free librsvg. Even if that changes in the future, rust-free gtk4 will continue to work for some years to come. There is no reason for 2024 software to suddenly get broken overnight :)

Using rust for librsvg is a librsvg developers choice

But it is a poor choice, re which quite a bit of people are unhappy (not only legacy macOS users).

RJVB commented 1 month ago

On Friday August 09 2024 08:55:14 Sergey Fedorov wrote:

But it is a poor choice, re which quite a bit of people are unhappy (not only legacy macOS users).

I tend to agree with that if not only given the clear and significant overhead, but I would also argue that the poorer choice was by the GTk developers who accepted the new language dependency. Then again that's not really surprising given the Gnome record of doing things the way they prefer regardless of what others think.

Evidently my suggestion of implementing a fallback function would/could work for GTk too; the most transparent approach would be to add that function to a forked, stainless librsvg (preventing any addition maintenance bundle for dependent software).