Add support for WebP image file format via libwebp. #24479
Conversation
hoehermann
force-pushed
the
libwebp
branch
3 times, most recently
from
347123b to
b35e205
Compare
|
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'm not familiar with the Chromium repository system, but I do note that the issue you refered to ( https://chromium-review.googlesource.com/c/webm/libwebp/+/4868215 ) states that the change was merged on 9/19/2023 -- so wouldn't the fix be in the current 1.4 release (released 4/12/2024)? |
|
I answered to Randalphwa's question at #23489 (comment) to keep the discussion in one place. |
|
@vadz Is this okay or do I need to improve something? |
|
Sorry, I've completely missed that it was ready, thanks for the ping. I'll try to look at it but, due personal circumstances, it might take some time. Any reviews/tests by others would be very appreciated! |
vadz
left a comment
vadz
left a comment
There was a problem hiding this comment.
Thanks a lot once again for your work, it's great to have support for WebP and globally it looks pretty good.
I would like to simplify the code creating WebPDemux as explained, if possible, please let me know if you can do it or if I should do it myself.
But an even more important consideration is to ensure that this code is actually getting tested, i.e. we need to modify the CI jobs to install libwebp-dev etc, to at least test it under Unix. If we can test it under Windows it would be great too, of course, but I guess this might be more complicated — but let me know if you see a way to do it.
Sorry again for the delay!
| fi | ||
| fi | ||
| if test "$wxUSE_LIBWEBP" = "yes"; then | ||
| dnl no need to check for duplicates since webp is disabled in the our builtin tiff |
There was a problem hiding this comment.
Nitpick of the day
| dnl no need to check for duplicates since webp is disabled in the our builtin tiff | |
| dnl no need to check for duplicates since webp is disabled in our builtin tiff |
| wxBITMAP_TYPE_TGA, | ||
| wxBITMAP_TYPE_MACCURSOR, | ||
| wxBITMAP_TYPE_MACCURSOR_RESOURCE, | ||
| wxBITMAP_TYPE_WEBP, |
There was a problem hiding this comment.
| wxBITMAP_TYPE_WEBP, | |
| wxBITMAP_TYPE_WEBP, ///< Available since wxWidgets 3.3.0. |
| @category{gdi} | ||
| @see wxImage, wxImageHandler, wxInitAllImageHandlers() | ||
| */ |
There was a problem hiding this comment.
| */ | |
| @since 3.3.0 | |
| */ |
| # skip binary files | ||
| case "$file" in | ||
| *.ani | *.bmp | *.chm | *.cur | *.dia | *.gif | *.gz | *.hlp | *.icns | *.ico | *.jpg | *.mo | *.mpg | *.pcx | *.pdf | *.png | *.pnm | *.pyc | *.tga | *.tif | *.ttf | *.wav | *.zip ) | ||
| *.ani | *.bmp | *.chm | *.cur | *.dia | *.gif | *.gz | *.hlp | *.icns | *.ico | *.jpg | *.mo | *.mpg | *.pcx | *.pdf | *.png | *.pnm | *.pyc | *.tga | *.tif | *.ttf | *.webp | *.wav | *.zip ) |
There was a problem hiding this comment.
Oops, another nitpick (they're alphabetically sorted):
| *.ani | *.bmp | *.chm | *.cur | *.dia | *.gif | *.gz | *.hlp | *.icns | *.ico | *.jpg | *.mo | *.mpg | *.pcx | *.pdf | *.png | *.pnm | *.pyc | *.tga | *.tif | *.ttf | *.webp | *.wav | *.zip ) | |
| *.ani | *.bmp | *.chm | *.cur | *.dia | *.gif | *.gz | *.hlp | *.icns | *.ico | *.jpg | *.mo | *.mpg | *.pcx | *.pdf | *.png | *.pnm | *.pyc | *.tga | *.tif | *.ttf | *.wav | *.webp| *.zip ) |
| if (features.has_alpha) | ||
| { | ||
| // image has alpha channel. needs to be decoded, then re-ordered. | ||
| uint8_t * rgba = WebPDecodeRGBA(webp_data->bytes, webp_data->size, &features.width, &features.height); |
There was a problem hiding this comment.
Ideally this would be a unique_ptr<uint8_t, WebPFree>.
| // TODO: Only read data as needed. WebPDemux can operate on partial data. | ||
| // Could save some bandwidth with e.g. DoGetImageCount | ||
| wxStreamBuffer * mosb = mos->GetOutputStreamBuffer(); | ||
| WebPData * webp_data = new WebPData; |
There was a problem hiding this comment.
Does this have to live for as long as WebPDemuxer itself lives or can it be destroyed immediately after calling WebPDemux() (as I suspect)?
In the latter case, we don't need to create it (nor mos) on the heap, which avoids the need to delete it too. In the former, I'd strongly prefer to keep it in a single object with wxMemoryOutputStream and WebPDemuxer* itself rather than hiding them, which are logically part of the same object, inside a custom deleter.
There was a problem hiding this comment.
Does this have to live for as long as WebPDemuxer itself lives
I assume, with "this" you refer to the wxMemoryOutputStream mos is pointing to. As far as I understood, WebPDemux does not create a copy of the input data in all cases. At least that is what I make of the implementation of WebPDemuxInternal. Unfortunately, the documentation is not terribly clear about the expected life-time of the buffers.
To keep the implementation simple for now, I went with the "read entire file into memory" approach. I hope that is okay. IswxInputStream::read() into a wxMemoryOutputStream the way it is done in wxWidgets?
I'd strongly prefer to keep it in a single object
I can do that.
There was a problem hiding this comment.
I meant both WebPData itself and the buffer associated with it.
Looking at the source (thanks for the link), it seems like data is only used inside this function, as it's used to initialize the local MemBuffer variable and so can't be accessed once the function returns. So it really seems like we don't need these heap allocations (and corresponding deletes) and can just make them local variables to simplify things.
There was a problem hiding this comment.
I now remember (sorry, it has been almost half a year, the details are a bit hazy) that I need the WebPDemux in DoGetImageCount, too. I did not want redundant code for creating a WebPDemux struct from a wxInputStream. I do not recall whether DoGetImageCount works for me or not, but I did not take the time to create any tests. My comment indicates that is the only reason why DoGetImageCount is commented out.
As far as I understand, data – or rather the buffer associated with it – is also used later on: The demuxer emits WebPData structs pointing to different sections within data. Instances include the call to WebPGetFeatures for getting the image dimensions. Also the actual decompression with WebPDecodeRGB….
Indeed, it looks like we do not need the heap-allocated WebPData from line 123 after WebPDemux(). However, I am pretty confident that data (aka wxMemoryOutputStream mos) must remain valid during WebPDemux's lifetime. I should verify that when touching the code again.
| #include <memory> | ||
| #include <functional> | ||
|
|
||
| typedef std::unique_ptr<WebPDemuxer, std::function<void(WebPDemuxer*)>> WebPDemuxerPtr; |
There was a problem hiding this comment.
After looking at the code using this, I think it would be much more clear to have a wrapper class rather than using this unique pointer with a very custom deleter, i.e.
class wxWebPDemuxer
{
WebPDemuxer* m_demuxer = nullptr;
wxMemoryOutputStream m_mos;
WebPData m_data;
};
Or, if possible, make the last 2 fields local variables in the function using them and then just use unique_ptr<WebPDemuxer, WebPDemuxDelete>.
| { | ||
| return false; | ||
| } | ||
| return std::string(buffer, 4) == riff && std::string(&buffer[8], 4) == webp; |
There was a problem hiding this comment.
This is probably not a huge optimization but still, why not just do
| return std::string(buffer, 4) == riff && std::string(&buffer[8], 4) == webp; | |
| return memcmp(buffer, "RIFF", 4) == 0 && memcmp(buffer + 8, "WEBP", 4) == 0; |
instead of creating these temporary strings?
There was a problem hiding this comment.
All I can say that I seem to suffer from C++ poisoning and am usually unaware that memcmp even exists. 😅
| : expected24) ); | ||
| wxImage & expected = g_testfiles[i].bitDepth == 8 ? expected8 : expected24; | ||
| int tolerance = 0; | ||
| switch (g_testfiles[i].type) { |
There was a problem hiding this comment.
| switch (g_testfiles[i].type) { | |
| switch (g_testfiles[i].type) | |
| { |
Thank you. ☺
Does the CI on Windows already employ vcpkg by any chance? I think just executed
No problem, however…
…right now, things are rather stressful on my end. I can do some more work here, but not before November 2024, I am afraid. |
I'm afraid it doesn't. It could be a good idea adding a build installing the external libraries using vcpkg, but for now I'd settle for just installing the WebP libraries from the distribution repositories under Linux — this should be trivial and, while it wouldn't test Windows at all, still much better than nothing.
Sorry to hear this!
There is no particular urgency with this, so if you think you'll have time to return to this later, I'd rather leave it to you. But I'd like to release 3.3.0 before the end of the year (even if I'm far from sure that we'll manage to do it), and it would be nice to have WebP support in it. |
It might work in the msys2/mingw64 build, when adding the mingw-w64-x86_64-libwebp package to the install section. When I check the contents of this package it seems to install the required webp cmake files. |
|
Merged in ad6f05a (Merge branch 'webp', 2025-05-11), thanks again for your work on this, Hermann! |
4 participants
This is a pull request for #23489. Please refer to the conceptual discussion there. Discuss suggestions regarding this particular pull-request here.
Update: I consider this ready for merging.