wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
5.99k stars 1.75k forks source link

WEBP support for wxImage #23489

Open beENABLED opened 1 year ago

beENABLED commented 1 year ago

Would there by interest for an image handler for webp?

I have written the actual image handler based on Googles webp library, but i have no idea how to integrate the webp library into the wx build process.

vadz commented 1 year ago

Sure, it would be welcome, this is a reasonably popular format (much more so than some of the other ones we support...) and it would be nice to have it.

For libwebp, we have 2 choices:

  1. Handle it as an external library, i.e. rely on it being available somewhere on the system. This is perfectly reasonable under Linux, but can also be done under MSW, see e.g. LZMA library example (notably the installation instructions).
  2. Include it as a submodule in wx itself. It's probably not worth doing it, at least not initially, but if you can integrate it with wx in the same way as libpng or libjpeg are, it could be considered as well.

The main drawback of (1) is that webp support would have to be disabled under MSW by default.

beENABLED commented 1 year ago

Thanks for the response. I looked into the configure script and the cmake files (which i both have little to none experience with), and i'm afraid this may be above my current knowledge level. I wouldn't even know where to start.

vadz commented 1 year ago

For configure it should be as simple as adding PKG_CHECK_MODULES(LIBWEBP) similar to any other existing check (e.g. LIBCURL one). For CMake it would involve using a FindWebp.cmake but I don't know much about it myself.

hoehermann commented 6 months ago

For reference, I did something similar here. I have no idea if I got the code-style right. Support for writing an image with alpha channel is missing, but something which people want. With a bit of luck, some day I have enough energy and time to implement that. Additionally I probably need to create more tests. Also find a way to build and run on MacOS which I do not have a license to.

vadz commented 6 months ago

@hoehermann Nice! If you could please make a PR out of this, we'd be happy to include this in wxWidgets.

hoehermann commented 5 months ago

I added support for saving an image with alpha channel and wrote a bunch of tests.

I tried adding the handler to the code-base at https://github.com/hoehermann/wxWidgets/commit/a277f43. Unfortunately, I have some troubles with the build system. I used hand-written Makefiles back in the day and then switched directly to CMake. I never used autotools. I started manipulating the files directly until I realized, wxWidgets uses an unreleased version of bakefile to generate the configure.in and Makefile.in which are then used by autotools. I managed to build and execute the bkl tool, but failed at finding out how to generate the top-level configure.in. I need to put the libwebp header and library detection as well as the linker flags and compiler defines somewhere. I know of https://www.wxwidgets.org/develop/coding-guidelines/ and https://www.wxwidgets.org/develop/how-to-submit-patches/, but neither page answers that specific question. Can you give me some pointers?

vadz commented 5 months ago

I added support for saving an image with alpha channel and wrote a bunch of tests.

Great, thanks!

failed at finding out how to generate the top-level configure.in

First of all, just to check: if you're making the PR against master, there should be no configure.in any more, but configure.ac. And this file (whatever the extension) is not generated, it is written manually and while the syntax is indeed pretty arcane (it's shell+m4, what's not to like in a combination of such perfectly readable languages), it hopefully shouldn't be too difficult to copy an existing fragment using PKG_CHECK_MODULES() and adjust it to add the flags for webp (see e.g. the one for LIBSECRET) -- this should be enough for Unix systems.

How to handle it under Windows is a different question, but we could leave this for later...

Also, if you have too much trouble with autotools, you could consider modifying just the CMake files (under build/cmake) for now, this will already allow to use CI to run the tests, and then I could mirror the changes there in configure.

Good luck!

hoehermann commented 5 months ago

Good luck!

Thank you. 🙂 Indeed, I made some progress at https://github.com/hoehermann/wxWidgets/tree/libwebp.

I was able to make it work with the CMake files in build/cmake on arch and Ubuntu 22.04. However, current distributions (Debian stable, arch) do not ship a FindWebP.cmake file. We could place one in build/cmake/modules/FindWebP.cmake, for example the one from WebKit. I am not a lawyer. I have no idea if the licenses are compatible. More reasonably, we can use the one from libtiff – that is, if the bundled libtiff is upgraded to a more recent version.

I also could get it to work on arch and Ubuntu 22.04 by modifying the configure.ac and the Makefile.in. Though the latter should be generated by the bakefile system. I would be grateful if you could check if I modified the .bkl files correctly.

I did not try building on Windows at all.

While expanding the enum, I noticed there are _RESOURCE variants of some formats, but not all. Is that something relevant to the WebP handler?

I also added some tests. Not only the loaded WebP image is checked, but JPEG as well (using the same function).

Is this looking reasonably good so far? I could squash the commits and open a pull request. Alternatively, I could prepare some more tests and implement loading single frames from animations, too. Or do one pull request now and another one later.

vadz commented 5 months ago

Good luck!

Thank you. 🙂 Indeed, I made some progress at https://github.com/hoehermann/wxWidgets/tree/libwebp.

Great, thanks!

I was able to make it work with the CMake files in build/cmake on arch and Ubuntu 22.04. However, current distributions (Debian stable, arch) do not ship a FindWebP.cmake file. We could place one in build/cmake/modules/FindWebP.cmake, for example the one from WebKit. I am not a lawyer. I have no idea if the licenses are compatible. More reasonably, we can use the one from libtiff – that is, if the bundled libtiff is upgraded to a more recent version.

Unfortunately updating libtiff is non-trivial (there are many conflicts with our own changes), but we'll do it sooner or later, of course. Still, for now, I can't imagine there would be a problem with putting this file in build/modules/cmake, we already have several very similar ones there.

I also could get it to work on arch and Ubuntu 22.04 by modifying the configure.ac and the Makefile.in. Though the latter should be generated by the bakefile system. I would be grateful if you could check if I modified the .bkl files correctly.

It would be more convenient to discuss this when there is a PR with these changes and, in any case, I can fix/tweak bakefile changes myself, I know that it's a pain to deal with.

For now I'd just like to ask: is there no pkg-config support for webp? I'd prefer to use it in configure if possible.

I did not try building on Windows at all.

It would be nice to have at least the same level of support for WebP under Windows as we have for LZMA as described in the "MSW manual setup" section of docs/doxygen/mainpages/liblzma.h.

While expanding the enum, I noticed there are _RESOURCE variants of some formats, but not all. Is that something relevant to the WebP handler?

Normal handlers load data from files, "resource" ones load it from Windows resources, i.e. data embedded into the executable. I think we can live without the latter one, at least for now.

I also added some tests. Not only the loaded WebP image is checked, but JPEG as well (using the same function).

Thanks!

Is this looking reasonably good so far? I could squash the commits and open a pull request.

Yes, please do.

Alternatively, I could prepare some more tests and implement loading single frames from animations, too. Or do one pull request now and another one later.

I'd prefer to separate this in two (or more) PRs, having even basic WebP support should already be useful and more things can always be added later.

TIA!

hoehermann commented 4 months ago

I'd prefer to use it [pkg-config] in configure

I adjusted the configure.ac to use pkg-config for auto-detecting libwebp.

It would be nice to have at least the same level of support for WebP under Windows

I tried building on Windows. It works relatively painless with CMake and with configure. I did not try anything involving the bakefile tool or msbuild, though.

I prepared this guide. Where to put the file so it is picked up by doxygen?

/////////////////////////////////////////////////////////////////////////////
// Name:        TODO.h
// Purpose:     Documentation of the use of libwebp with wxWidgets
// Author:      Hermann Höhne <hoehermann@gmx.de>
// Licence:     wxWindows licence
/////////////////////////////////////////////////////////////////////////////

/**

@page page_build_libwebp wxImage support for the WebP image format with libwebp

To make use of the wxWEBPHandler, libwebp library is required when building wxWidgets. 
libwebp is available under a BSD license.

This guide explains how to obtain libwebp and let wxWidgets build system use it. 
The exact steps depend on the operating system and compiler used, please refer to the
section appropriate for your environment below.

@section page_build_libwebp_unix Unix-like operating system

Under many Unix or Unix-like systems, libwebp is available as a system package 
and the simplest thing to do is to just install it using the system-specific tool 
(apt, pacman, …). Just note that you may need to install the libwebp-dev 
package in order to obtain the library headers and not just the library itself.

Manually building libwebp from source is not part of this guide.

@subsection page_build_libwebp_configure Use libwebp with configure

Provided, @c pkg-config is available and the appropriate files are in the default
locations, @c ./configure will automatically detect the availability of libwebp.

@subsection page_build_libwebp_cmake Use libwebp with CMake

When using CMake, add @c -DwxUSE_LIBWEBP=ON to CMake command line to enable
using libwebp.

Note: At time of writing, the @c WebPConfig.cmake file supplied by libwebp
is affected by a <a href="https://chromium-review.googlesource.com/c/webm/libwebp/+/4868215">bug</a>.
You may need to add the line <tt>set(WebP_INCLUDE_DIRS ${WebP_INCLUDE_DIR})</tt> 
locally.

Note: Not all distributions offer support for CMake in their version of libwebp (e.g. Ubuntu 22.04).
Use the method @ref page_build_libwebp_configure or find a @c WebPConfig.cmake appropriate
for your system.

@subsection page_build_libwebp_msw_vcpkg_msvc MSW with vcpkg and MSVC

You can use Microsoft vcpkg tool (see https://github.com/Microsoft/vcpkg) to 
build and install libwebp:

@code
    > git clone https://github.com/Microsoft/vcpkg.git
    > .\vcpkg\bootstrap-vcpkg.bat
    > .\vcpkg\vcpkg.exe install libwebp:x64-windows-static
@endcode

You can then proceed to configure wxWidgets with CMake for building with MSVC, 
see @ref page_build_libwebp_cmake. Obtaining and using libwebp in any other way 
is possible, but not covered in this guide.

Note: Dynamic builds have not been tested. x86 builds have not been tested.

@subsection page_build_libwebp_msw_vcpkg_clang MSW with vcpkg and clang

Refer to @ref page_build_libwebp_msw_vcpkg_msvc, but select @c x64-mingw-static
instead of @c x64-windows-static . Then proceed as documented in 
@ref page_build_libwebp_cmake or @ref page_build_libwebp_configure . CMake can work
as well as configure since vcpkg provides libwebp with support for both systems.
*/

To the curious: This is the libwebp upstream change we are waiting for, then CMake support should be flawless. Until then, I decided to leave it at "OFF" by default.

I did not touch anything which documents dependencies. In case wxWidgets is dynamically built against libwebp, there should be an additional package-level dependency documented somewhere. I am thinking of the maintainers of packages for Debian, Arch, etc.

All tests are passing on Windows. They also keep passing if libwebp is not available (I forgot that on the first iteration). On Linux (with GTK), there are not more errors. When pushing on github, the tests on Mac are failing. I cannot see how that relates to my changes.

PS: I am pretty sure, I already wrote this comment in the past. Apparently, I forgot to post it.

vadz commented 4 months ago

Thanks for your work on this! The overview could go to docs/doxygen/mainpages/libwebp.h and it should be linked to from wxImage docs in interface/wx/image.h.

hoehermann commented 4 months ago

@Randalphwa Thank you for your interest in this feature.

so wouldn't the fix be in the current 1.4 release

Thank you for letting me know the new version has been released. When I was preparing the pull request, it was not yet ready and I did not check libwebp's release schedule. libwebp 1.4 has been made available in vcpkg and arch just last week. I did not tend to this pull-request since then. Nothing on Debian, yet.

Because this requires a library that is not built by wxWidgets itself, I'm assuming that anyone wanting to use this functionality will not be able to use any of the pre-built binaries? Or are you envisioning that all the pre-built binaries should have webp support turned on by default? If the pre-built binaries aren't going to have webp support, then it should be documented that the user must build wxWidgets from source to get this functionality.

I cannot answer any of these questions. Can the wxWidgets project legally include libwebp? Similar to how libtiff is included? I do not know. For that reason, it is implemented as an external dependency right now.

In case of the Windows build… I am not a lawyer and I do not want to become one. We need to find someone who can tell us – with the necessary certainty – that libwebp is okay with being included. Then I may give it another go and have it as a subproject.

On Linux, is up to the person who packages the binaries. The wxWidgets package will gain an additional dependency and off you go. Some folks however, are hesitant of introducing new dependencies. This is an organisational question more then a technical question.