Conversation
PBfordev
left a comment
PBfordev
left a comment
There was a problem hiding this comment.
Thanks Maarten for the thankless tasks of finishing the PR.
Aside to specific nitpicks, it seems there are no checks for failed memory allocations (nor errors when reading from streams)? I am not sure what is wxWidgets policy for checking out-of-memory errors, in places with large allocations in particular...
vadz
left a comment
vadz
left a comment
There was a problem hiding this comment.
Thanks a lot for working on this!
I don't see any big problems, but there are, as you can see, quite a few small remarks (some of which I had already left in the original PR, but completely forgot about them...) — and I also agree with the comments already left by @PBfordev. I think I can count on you dealing with them when you have time but if you'd like any help with it (or disagree with any of them), please let me know.
FWIW I don't think it's critical to have this in 3.3.0, as it's a pure addition, we could add it in 3.3.1 too, but it would be nice to have it there, of course.
Thanks again!
|
Thanks @PBfordev and @vadz for the reviews. I'll address all the issues. But if 3.3.0-RC is planned for this month it'll probably not make it. The only thing I would probably need help with (if we want this), is adding support to build the built-in version with configure. Similar to how it builds libtiff. |
There was a problem hiding this comment.
I think I addressed all of the review comments.
Questions:
- Should I rename
wxWEBPHandlertowxWebPHandler? All image handlers have capitalized names, but all WebP classes don't. - Where should the submodule go?
src/webpor3rdparty/webp? - I added
wxBITMAP_TYPE_WEBP_RESOURCEbecause all formats have this. Is this fine?
Some things that I won't be adding (unless absolutely required):
- Saving animated webp
- Using built-in webp from configure (or msvs projects)
.gitmodules
Outdated
| branch = wx | ||
| [submodule "src/webp"] | ||
| path = src/webp | ||
| url = https://github.com/webmproject/libwebp.git |
There was a problem hiding this comment.
Yes, if you could make that would be great.
Thanks a lot!
I guess we should keep this name for consistency, even although I find
I don't have strong preferences here, but I prefer
Hmm, this is not going to be implemented anywhere is it? |
vadz
left a comment
vadz
left a comment
There was a problem hiding this comment.
Thanks a lot once again!
This globally looks good to merge to me. Ideal would probably be to build libwebp when using MSVS, just as we do with the other libraries, but this is not a hard requirement, whoever is interested in using it can always implement this later.
MaartenBent
left a comment
MaartenBent
left a comment
There was a problem hiding this comment.
I don't have strong preferences here, but I prefer 3rdparty.
I'll use this location then, once the fork is created.
Hmm, this is not going to be implemented anywhere is it?
No. I just copy-paste all the wxBITMAP_TYPE_GIF/wxBITMAP_TYPE_GIF_RESOURCE and added the same for WebP.
wxBITMAP_TYPE_GIF_RESOURCE also doesn't have any use.
Ideal would probably be to build libwebp when using MSVS, just as we do with the other libraries, but this is not a hard requirement, whoever is interested in using it can always implement this later
Yes, I can try to add it after this PR. Adding the files shouldn't be too difficult. Usually it us dealing with the generated config.h file.
|
I removed the libwebp msvc project, I'll add it to #25339 instead. I moved the submodule to |
|
I've reset |
WebP sub-project searches for OpenGL, and sets the OpenGL variables to NOT_FOUND. Make sure all OpenGL variables are updated when OpenGL dependency is set manually.
Add function to get the WebP library version. Add LoadAnimation function to get all frame images, background colour and duration. Use WebPAnimDecoder to decode frames other than the default (-1) frame. This ensures all returned frames will have the same image size. WebPDemuxGetFrame can return frames smaller than the full image size, but there is no way to get the offset of the frame. Reduce code duplication when creating rgba image. Add enum to store image format (undefined/mixed, lossy or lossless). Add lossless and lossy quality options to wxImage for saving WebP. Add wxBITMAP_TYPE_WEBP_RESOURCE because all other image handlers have this too.
Improve high-dpi support.
Run upmake, bakefile_gen, makeprojects.py, update-setup-h, autogen.
|
@vadz this and #25339 are now ready. The last commit (Make wxImage::SetDataRGBA argument const) is not in the other PR, so I propose to merge this PR first and then #25339 (instead of only #25339). |
|
Thanks a lot once again for all your work! I didn't realize configure was modified in the other PR too, so I've slightly changed it by adding commits here and so got conflicts when merging the other one later, but hopefully I didn't break anything resolving it. In any case, I'm merging both PRs now. |
4 participants
This extends #24479 by addressing some of the remarks made there, adding libwebp as a built-in submodule, and adding WebP animation support.
Also see #23489
webp.animation.mp4
I think it is mostly finished. Except for updating all the build files (only CMake is up-to-date currently). And documentation. And addressing some more remarks from the linked PR. And maybe adding support for saving WebP animations.