-
Notifications
You must be signed in to change notification settings - Fork 417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make mozjpeg as its own lib. #383
base: master
Are you sure you want to change the base?
Conversation
Thanks for the proposal. I think renaming (or even deleting some) of the demo commands is fine. Overlap with system-global ones has been causing confusion. I am worried about backward compatibility for existing users of the library. Specifically, I'm worried about Windows users, where |
Which ones are the demo commands should I remove? I'll remove them from the build system (but leave the code around to simplify upstream merges involving these parts of code). Or you mean all the non-library binaries (even cjpeg, djpeg, etc.)?
They don't use pkg-config as much as a standard as us, but they can still install and use it. And they usually do when they use Free Software born builds (i.e. like here with CMake, or with autotools/meson). At least that's true on Windows for sure. Now if you want, we could actually create the libjpeg.so (symbolic links on Linux/Unix, file copies on Windows) when WITH_LIBJPEG is set. Then it will be totally transparent. Do you mind if we bump the CMake version? I saw a new CMake command which make this easier, it appeared in CMake 3.14, |
Hi @Jehan . ❓ Maybe?: set(PREFIXNAME "moz")
add_library(${PREFIXNAME}jpeg-static STATIC ${JPEG_SOURCES} $<TARGET_OBJECTS:simd> sed -i -e 's|moz|\$\{PREFIXNAME\}|g' CMakeLists.txt And added PREFIXNAME in options? |
I was thinking about deleting all command-line tools except cjpeg and jpegtran. Others have nothing mozjpeg-specific. A configurable prefix would be great. Preferably default off on Windows. |
What do you mean? A configurable prefix is brought by default with CMake as far as I know, with the |
I mean config option to rename build products to mozjpeg-unique or keep them with the same names as plain libjpeg. Not the install prefix directory. |
@Jehan say:
I described everything in detail: #383 (comment) |
Ah yeah well the problem with this approach is that you can only install one or another. I think that mozjpeg should always exist. That makes it the main thing, it gives the lib its existence. Then the generic naming libjpeg should be an additional option, not replace libmozjpeg. The second thing is that even if we somehow tweaked the rule to simply run all the build targets twice (once with a prefix, once without), then we end up building the whole thing twice, which is ridiculous (sure mozjpeg is not huuuge, so it's still fast; still, my perfectionnist self doesn't like this). I'd prefer build it once (and just symlink/copy). But if you absolutely prefer the configurable prefix (no rebuild, just we either build it as libmozjpeg or as libjpeg), it's fine by me. We can go this way if you like. And I will default it to libjpeg on Windows (where the concept of a standard build system and install prefixes and stuff don't mean much anyway) and libmozjpeg on Unix/Linux. |
@Jehan say:
This is your and only your opinion. And @kornelski originally made |
It's not about opinion. It allows the lib "libmozjpeg" to always work as being mozjpeg (unequivocally). And optionally we can have also have it named libjpeg in a generic way for other cases (e.g. if some distributions were to actually provide an option for this, etc.). |
@Jehan say:
It is in it. The goal determines the direction of development. PS: I am on your side, but not by your methods. |
I think this PR closed accidentally and should be opened again. |
Indeed. Looks like it. The commit which closed it looks completely unrelated. Anyway I will try to work again on it soon, then will repropose a new version (because I can't open it again) according to the various comments. Just didn't have much time for it lately. |
Now that Jehan is the new Gimp maintainer this PR can bring mozjpeg en Gimp. |
Ahah that was already the case before I got a "title". 🙂 All I need is to make the time to finish this and then we can add mozjpeg specific support in GIMP. Also I am not the maintainer but a co-maintainer. We are better together than alone. 😛 |
By default, mozjpeg will stay named generically libjpeg on Windows, whereas it will install with its own namespace on other OSes, i.e. libmozjpeg.so|a, mozjpeg.pc, etc. The public header files keep their name but are installed under include/mozjpeg/ (the pkg-config file will take care of providing proper build flags), this way no include gets broken. The new option WITH_MOZPREFIX allows to bypass the default behavior (so that it is still possible to name the lib mozjpeg name on Windows, or libjpeg on other OSes). A second new option WITH_LIBJPEG_PKG_CONFIG add a libjpeg.pc even when WITH_MOZPREFIX is ON (additionally to mozjpeg.pc). Pkg-config is the modern way for software to find their dependencies and correct build/link flags.
c872770
to
b156233
Compare
Sorry that it took that long. Today I decided to have a look again at my MR to get it done with. Anyway so here are the changes in the last version of this patch:
I have thought long about this comment from @kornelski:
In the end, I went with just renaming all commands (not deleting them) for 3 reasons:
For the record, I have compiled my patched version for Linux (on a Debian) and for Windows (cross-compiled from Linux). So I can say that so far, it looks good. I have also made sure that the full unit test set still succeeds (both my Linux and Windows builds). |
This uses MozJPEG from Mozilla and is dependent on this patch to be merged: mozilla/mozjpeg#383 This is an optional option (standard libjpeg API, e.g. with jpeg-turbo, is still used when MozJPEG is not detected at build time). When using MozJPEG, we have access to both the standard algorithm used by libjpeg-turbo, since MozJPEG is a patched version of libjpeg-turbo, and their own encoding algorithm. We can switch from one to another with a single call. Here is what the maintainer says about MozJPEG goal: > The point of MozJPEG is to improve quality/filesize ratio. It's a win-win: you > get better quality for the same file size, or better file size for the same > quality, or both. There is no downside in either quality or file size. MozJPEG > tunes for these two aspects over speed. libjpeg-turbo's maintainer values speed > over the other two variables. > > MozJPEG has a few techniques. Improved splitting of progressive scans gives > smaller file size while being 100% visually identical with libjpeg-turbo. > > But MozJPEG also has trellis quantization and tuned quantization tables that > give better visual quality, but on a microscopic scale they make different > choices than libjpeg-turbo, so some pixels differ. The differences are > relatively small and predictable, so there's no risk of unexpectedly ruining > an image (especially that on average, you get better quality). Cf. mozilla/mozjpeg#382 (comment) Note that after several testing, I could indeed confirm that it seems to always produce smaller files (as far as my testing went) for similarly looking quality, but the speed cost can actually be quite important: on my computer, for some random files where encoding would take 0.7 second, it took 3.5 secs with mozjpeg; for much bigger file (~25MiB) where export with jpeg-turbo takes about 3.9 secs, it takes 30+ seconds with MozJPEG which is a huge difference and can be very frustrating. For small files only, this is less of a problem (I still timed an important difference, but from 0.05 to 0.15 secs is actually bearable). This is why this cannot be an option checked by default. About naming: I hesitated to call it "Export for Web" because it's clearly one of the big use cases (optimizing file size for images on websites), but I just decided to go with a much more explicit name (even though it may resonate less that the basic "Export for Web" which everyone asks for).
For the record, I also prepared a MR in GIMP: https://gitlab.gnome.org/GNOME/gimp/-/merge_requests/1068 I will merge it when this is validated, then we'll likely soon see mozjpeg packaged in various Linux distributions (or in MSYS2 for Windows, etc.). |
Jehan, my hero ! |
@kornelski I saw you just made a release. Cool. Any chance this could be reviewed and be merged? This way, it could even make it to GIMP 3 (by the current activity of things, I think we may make our feature freeze around December). Thanks! :-) |
Any chance for this to be merged? It won't make it to GIMP 3.0 now (because we are already in feature freeze), but I'd love to be able to provide the option to switch encoding engine in future versions of GIMP! 🙂 |
Is this still accurate ? Since the release of Jpegli 0.10, Mozjpeg is behind Jpegli both in encoding speed, compression ratio and quality (but Jpegli is slower to decode). IMHO, Jpegli is also simpler to use. |
This is off-topic. It's not because a new lib does something similar, and maybe even better — I don't know —, that one should stop evolving. Alternatives are always a good thing. |
Actually after our discussion at #382, I thought I might as well demonstrate as it's just so easy to do.
With this commit, mozjpeg would install fully without any system clash:
moz*
. I.e.: mozcjpeg, mozdjpeg, mozjpegtran, mozrdjpgcom, mozwrjpgcomman
pages have also been renamed.mozjpeg.pc
is installed. It works fine.libmozjpeg.so*
and static islibmozjpeg.a
.include/mozjpeg/
and are named the same.There is a new option WITH_LIBJPEG. I expect that distribution will not set it so that they can keep libjpeg-turbo, but if someone were to set it, all it does is add the
libjpeg.pc
file (which is basically a copy ofmozjpeg.pc
). The reason is that modern programs anyway should not look for a dependency by testing if a .so file exists (this is not portable, and this doesn't get you the full build/link flags with dependencies if there are any, etc.). Nowadays you just check for the pkg-config and it gives you the real lib name, where to find the headers, and so on.I have not really bothered too much with WITH_TURBOJPEG option and lib and binaries installed then would still clash obviously. But I assume this option only exists in mozjpeg because you avoid removing too much code (avoid merge conflicts basically), but this would not be a flag which packagers are supposed to use for mozjpeg, right? (then we should probably set it to FALSE)
Now this makes mozjpeg completely package-able with no clash, and software are able to know they are using mozjpeg or turbo, and therefore to propose both the smaller files or faster compression options.
Also you'll notice this commit does not change anything in actual code, not even file renames. Everything is only in the build files. This way, we keep the changes minimum in order to avoid merge conflicts.