mozjpeg icon indicating copy to clipboard operation
mozjpeg copied to clipboard

Make mozjpeg as its own lib.

Open Jehan opened this issue 5 years ago • 23 comments

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:

  • The various binary will be installed as moz*. I.e.: mozcjpeg, mozdjpeg, mozjpegtran, mozrdjpgcom, mozwrjpgcom man pages have also been renamed.
  • A mozjpeg.pc is installed. It works fine.
  • The shared lib is libmozjpeg.so* and static is libmozjpeg.a.
  • Headers are in 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 of mozjpeg.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.

Jehan avatar Nov 19 '20 20:11 Jehan

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 pkg-config is unheard of. On macOS pkg-config is not available by default either. These platforms will get renamed libraries. That turns MozJPEG from a seamless improvement to being an alternative/fork that applications have to intentionally integrate :(

kornelski avatar Nov 20 '20 22:11 kornelski

I think renaming (or even deleting some) of the demo commands is fine.

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.)?

Specifically, I'm worried about Windows users, where pkg-config is unheard of. On macOS pkg-config is not available by default either.

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,

Jehan avatar Nov 21 '20 10:11 Jehan

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?

zvezdochiot avatar Nov 21 '20 13:11 zvezdochiot

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. pkg-config on Windows is almost as strange as telling Linux distros that they should be using vcpkg.

kornelski avatar Nov 21 '20 17:11 kornelski

A configurable prefix would be great.

What do you mean? A configurable prefix is brought by default with CMake as far as I know, with the CMAKE_INSTALL_PREFIX option. Or do you mean something else?

Jehan avatar Nov 21 '20 17:11 Jehan

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.

kornelski avatar Nov 21 '20 17:11 kornelski

@Jehan say:

What do you mean?

I described everything in detail: https://github.com/mozilla/mozjpeg/pull/383#issuecomment-731581915

zvezdochiot avatar Nov 21 '20 18:11 zvezdochiot

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 avatar Nov 21 '20 18:11 Jehan

@Jehan say:

Then the generic naming libjpeg should be an additional option, not replace libmozjpeg.

This is your and only your opinion. And @kornelski originally made mozjpeg as a complete replacement for libjpeg. Full! True, many, including myself, do not agree with such a replacement. We must look for a compromise. I am very happy with the prefix compromise.

zvezdochiot avatar Nov 21 '20 18:11 zvezdochiot

This is your and only your opinion.

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.). The possibility of having an always-available non-ambiguous naming is a strength.

Jehan avatar Nov 21 '20 18:11 Jehan

@Jehan say:

It's not about opinion.

It is in it. The goal determines the direction of development.

PS: I am on your side, but not by your methods.

zvezdochiot avatar Nov 21 '20 18:11 zvezdochiot

I think this PR closed accidentally and should be opened again.

dofuuz avatar Mar 03 '21 05:03 dofuuz

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.

Jehan avatar Mar 03 '21 09:03 Jehan

@Jehan say:

I will try to work again on it soon, then will repropose a new version

See also #391 .

zvezdochiot avatar Mar 03 '21 10:03 zvezdochiot

Now that Jehan is the new Gimp maintainer this PR can bring mozjpeg en Gimp.

zeroheure avatar Apr 04 '21 18:04 zeroheure

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. 😛

Jehan avatar Apr 05 '21 13:04 Jehan

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:

  1. There is now a WITH_MOZPREFIX option, set to OFF on Windows and ON everywhere else. As you would expect, setting it to OFF is the same as the current situation (everything is generically named "libjpeg").
  2. The other new option WITH_LIBJPEG_PKG_CONFIG (only useful when WITH_MOZPREFIX is ON) installs libjpeg.pc additionally to mozjpeg.pc. This makes that even when using a custom mozjpeg namespace, the library could be detected as libjpeg when using pkg-config.

I have thought long about this comment from @kornelski:

I think renaming (or even deleting some) of the demo commands is fine. Overlap with system-global ones has been causing confusion.

In the end, I went with just renaming all commands (not deleting them) for 3 reasons:

  1. first I figured it would be much easier to backport/update to upstream jpeg-turbo this way (minimal changes).
  2. second if someone only has mozjpeg on their machine, at least they have the full CLI toolset. E.g. even though djpeg may be unmodified by MozJPEG project, it's still useful. At least now it will be properly namespaced (mozdjpeg) when WITH_MOZPREFIX is ON.
  3. Looking closer at unit testing rules, I realized that many tests are actually made of 2 dependent unit tests: one doing a compression using cjpeg, then one doing a decompression using djpeg and finally the result comparing to a given MD5 checksum. I think it's still pretty important for mozjpeg project to do the full roundtrip of tests to make sure no regression sneaks in because of its algorithm.

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).

Jehan avatar Sep 07 '23 16:09 Jehan

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 avatar Sep 07 '23 17:09 Jehan

Jehan, my hero !

zeroheure avatar Sep 08 '23 07:09 zeroheure

@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! :-)

Jehan avatar Oct 13 '23 12:10 Jehan

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! 🙂

Jehan avatar Apr 08 '24 15:04 Jehan