Adding WebPImporter folder #121
Conversation
|
in this new commit, I have made changes in accordance with my TODO list from the last commit. Finding libwebp Module I have created a FindWebP.cmake file by adapting it from other projects. It turns out the previous compiling errors were not caused by not linking libraries (they were already linked) but by libwebp header files belonging to separate libraries which had to be detected by FindWebP.cmake file individually. I am not sure if this file needs improvement or any correction. I have also made changes in existing CMakeLists.txt files to integrate WebPImporter in the project. Remove Remaining References to PngImporter I had missed some data previously, which have been replaced with equivalent WebPImporter references. Also some naming mistakes (e.g. "Webp" instead of "WebP" have been corrected. The Importing Code I have used advanced decoding to extract uncompressed data. As far as I have understood from the information I got about webp, the color depth (bit depth) is always 8 bits per channel. Hence, I have omitted the code in PngImporter checking other bits and configuring accordingly. As for colour spaces, in webp it seems to be user specific, not inherent to the WebP data (bit-stream), hence not returned by WebPGetInfo (decode) API. That's why I have merely checked if the image had alpha and used colour spaces MODE_RGB or MODE_RGBA accordingly while decompressing. There is another feature in WebP bitstream; it can be found out from WebPGetFeatures() function, whether it is a lossy or lossless bitstream. I don't know how to approach this information, if I should add a different code for decompression of lossy images. I couldn' t find anything on documents about it except for a comment, saying that in a lossy bitstream, pixels are stored in format YUV or YUVA depending on alpha being present. And since I couldn't see YUV or YUVA format in PixelFormat.h, I have just commented out the decoding function using YUV format. I haven't done anything in terms of testing yet. I thought I would start that after getting a feedback on this last commit, since I am not sure if I have everything covered in WebPImporter. |
There was a problem hiding this comment.
Hi, welcome and thank you for the work so far!
I commented just on the error handling and one easy-to-forget corner case, the rest looks great for an initial draft.
About lossy and YUV -- I hope there's a way to tell the decoder to always output an 8-bit RGB(A) (maybe by setting some values in config.input before passing it to WebPGetFeatures()?), or hopefully maybe it's even done like that by default? Maybe just try with a lossy file first and if the current code works, that's it, no further work needed :) In any case, when you get to writing tests, there should definitely be a test for both a lossy and lossless file.
About YUV in PixelFormat -- such formats are supported in Vulkan at least, but handling of those is extremely complex due to different resolution of chroma and luma planes and not what regular users would expect anyway, so unconditionally expanding them to RGB makes sense. I might have a look at this again in the future when I get to video support (where those mainly make sense), but it definitely isn't something you should worry about right now.
|
Thank you for the enlightening review @mosra. I hope I've made all necessary changes in error handling code.
Telling the decoder which format to output is not just possible but seemingly the only way to decode. Apparently the colour depth mode is not something to be obtained from the image, rather the user decides in which mode to decode by assigning that information as a feature of config.output which is the decoder output buffer. And since webp bitstreams have always 8-bit colour depth per channel, it outputs always in 8-bit format. Thus, I made no changes in the code for decompressing: it simply decodes in 8-bit RGBA when the bitstream has alpha, and in 8-bit RGB format when it doesn't. I have tried the code with both lossy and lossless files, it works without any problem. Lastly, I have added code to release memory used by WebPData structure, which I missed before. Put that code also inside the error handling blocks. Though I am not sure, if i should have done that.
"row stride" here means stride in bytes, right? not in pixels? The webp decoder expects a stride in bytes in its configuration, so I wanted to make sure that I understand correctly. |
Okay, awesome. (I checked the API to see what all decoding options it provides and discovered there's even a way to scale the image during import, interesting feature :D)
Yes, bytes.
Looks like you forgot to push those changes tho, I see only a new test file being added :) |
|
Hi @mosra,
Sorry, I have committed and pushed the wrong file. I corrected it now.
Should I add any other feature to the webp importer or improve it any other way? Or shall I directly start testing? |
Nono, sorry, it's fine like this. Just collecting ideas for potential future additions. |
Okay, thank you :) Then I can start on testing. |
|
I have created small webp images (3x3, 7x3) in lossy and lossless format, with/without alpha value and added the first new tests: The first one is complete, I thought of completing the latter after asking for a review. I have imitated the relative png importer tests. I have kept tests like Small changes (corrections) have also been made in because I got an error about it ( I am not sure how to be creative in testing, so I am open to ideas about adding new tests :) |
|
I get notified for every push and comment after you tag me, plus I'm watching the repository so I literally see your every step :D Just didn't have time to go through the code yet. Hopefully later today, or tomorrow. |
Good to know, thank you :D |
There was a problem hiding this comment.
Hey @melikebatihan!
Thanks for working on this, you're off to a great start! I added a few suggestions, overall, it's looks to be going in the right direction! You may want to remove some of the obsolete files, which may make the diff a bit easier to read for us for the reviews.
|
Thank you for the last review. Here are the changes and additions in this last commit: CMakeLists I restored the initial CMakeLists.txt as per the comment above. I had made those changes because the option
seemed to be not working when I tried to build and run the project/executables on CLion. But now I see it works fine also after restoring the CMakeLists file :) The build error I got back then was obviously caused by another factor.
Thank you, apparently it will take a long time for me to learn CMake :) FindWebP.cmake
I have adapted FindWebP.cmake file from three different projects I've found. Initially I used the file on this link: https://fossies.org/linux/tiff/cmake/FindWebP.cmake Later on, I made changes on it by removing some parts and adding new blocks of code from the files on these two links: https://github.com/WebKit/webkit/blob/main/Source/cmake/FindWebP.cmake Lastly, for this commit, I changed it again by removing the code about finding and comparing the version via pkg-config. README.md In this file I included the information on how I generated input files as requested. I find its content a bit too simple, but the methods I used to created those files were simple too :) It might need improvement together with the input files maybe. Changes in WebPImporter & Addition of New Tests I included an error case in the importer source code for animated webp files and added a test case regarding it. Also tests including corrupt webp files have been added together with RGBA tests. Pngimporter References & Other Changes As for old pngimporter files and references to pngimporter, I had already removed them but failed to commit their removal, which caused confusion I guess. I included those changes in the last commit, also updated the documentation referring to pngimporter, removed the redundant webp header files in WebPImporterTest.cpp and corrected the comments in WebPImporter.cpp according to the Corrade Coding Style and the relevant comment above. |
There was a problem hiding this comment.
Hey @melikebatihan!
It's starting to look pretty great! I added a few more comments, after which, let's get a review from @mosra.
Regarding the animation support, no need to do that for this initial PR 👍
The Continuous Integration (CI) is currently failing on most platforms, maybe have a look at the logs and check if you can fix some issues there. You will probably need to add to the CI scripts in package/ci such that it downloads and builds the libwebp source code.
After that, some might be failing due to differences in compiler (GCC vs clang vs MSVC).
Best, Jonathan
|
|
||
| Containers::Optional<ImageData2D> WebPImporter::doImage2D(UnsignedInt, UnsignedInt) { | ||
|
|
||
| /* Create the structure that contains the WebP image data. */ |
There was a problem hiding this comment.
Being very picky: usually there's no final . in comments in magnum
|
Hello, thank you for the last review @Squareys. Apart from the modifications I made according to your comments above, I have focused on configuration in this last commit -in order to have a better understanding, figure out why integration tests fail - and made changes in certain configuration files. FindMagnumPlugins.cmake Here I added “WebPImporter” as a member of _MAGNUMPLUGINS_PLUGIN_COMPONENTS on line 158, so that it can be used as a static plugin or as a dependency of another plugin. CircleCI Files
steps:
- install-base-linux:
extra: libfaad-dev libfreetype6-dev libjpeg-dev libopenal-dev libpng12-dev libdevil-dev libharfbuzz-dev libassimp-dev wget libwebp-dev
changelog-plugins-old.dox, line 223: FindWebP.cmake
|
melikebatihan
force-pushed
the
webp-importer
branch
from
ae6df13
to
5ac8f97
Compare
melikebatihan
force-pushed
the
webp-importer
branch
2 times, most recently
from
5ac8f97
to
4cc0b36
Compare
| } | ||
|
|
||
| /* Structure and configuration for decoding */ | ||
| WebPDecBuffer* const outputBuffer = &config.output; |
There was a problem hiding this comment.
This could be WebPDecBuffer& outputBuffer = config.output;. You can then use . instead of -> in the following section
| /* Create external memory pointed by outputBuffer buffer */ | ||
| Containers::Array<char> outData{NoInit, outputBuffer->u.RGBA.size}; | ||
| auto* rgbaBuffer = reinterpret_cast<uint8_t*>(outData.data()); | ||
| outputBuffer->u.RGBA.rgba = rgbaBuffer; |
There was a problem hiding this comment.
You may want to assign directly to the reinterpret_cast, a pointer variable somewhere usually indicates "extra caution necessary" to me when reading the code, but that's not the case here :)
| See @ref building-plugins, @ref cmake-plugins, @ref plugins and | ||
| @ref file-formats for more information. | ||
|
|
||
| @section Trade-WebPImporter-behavior Behavior and limitations |
There was a problem hiding this comment.
You may want to document that the animations/videos (?) are not supported?
There was a problem hiding this comment.
Our endless comments must be overwhelming, sorry about that 😅 I tried to focus mainly on what could be useful for your future adventures in general, so here it is.
I still need to check what's up with the CI (it worked for everyone before, so hopefully just a temporary hiccup), and I still want to get back with a suggestion about fuzzy image compare so you don't need to painstakingly write down the pixel values for lossy files.
Thank you for your patience!
|
|
||
| /* Decompression of the image */ | ||
| if(WebPDecode(image.bytes, image.size, &config) != VP8_STATUS_OK) { | ||
| Error{} << "Image couldn't be decompressed. Might be a corrupt file."; |
There was a problem hiding this comment.
The error message doesn't need to say much, but it definitely should include the actual error returned by the library. The VP8StatusCode enum has just a few values, ideal would be to switch over them and print a human-readable equivalent. Such as this, together with prefixing suggested by Squareys above:
const VP8StatusCode status = WebPDecode(image.bytes, image.size, &config);
if(status != VP8_STATUS_OK) {
Error err;
err << "Trade::WebPImporter::image2D(): decoding error:";
switch(status) {
case VP8_STATUS_OUT_OF_MEMORY:
err << "out of memory";
break;
case VP8_STATUS_INVALID_PARAM:
err << "invalid parameter";
break;
...
case VP8_STATUS_OK:
CORRADE_INTERNAL_ASSERT_UNREACHABLE();
}
return {};
}The OK status can't happen because it was checked for in the if, so there's an assert macro for that -- it both helps the compiler remove code that can't be reached (thus no break is needed after) and makes the code more robust in case stuff would get fatally broken somehow. I'm saving the Error printer to a variable to make it print a newline only once at the end and not also after "decoding error:".
A useful thing to do here is to not add any default: case, as then the compiler will helpfully warn you in case you forget to handle some enum value (or to notify for example when a new version of WebP would add a new error case).
There was a problem hiding this comment.
I did this also for another function (WebPGetFeatures()), which was checking VP8 Status. But I am not sure if it looks good using such a long switch block as duplicate code. Should I use something like std::map or define a function (named e.g. statusInfo()) containing that switch block to get rid of the duplication?
There was a problem hiding this comment.
A helper function would be a good idea, yes. Compared to a std::map or just about anything else, a switch and returning a const char* is the fastest way to do this. The actual branching in the switch is a code you don't ever see or step through in a debugger, so the compiler is free to do any crazy optimization it can think of. Often it ends up being compiled into some sort of a table lookup.
What's a bit hard to get right at first is the unreachable state, to avoid a compiler error saying that a return is missing somewhere. This should do it, if you keep the VP8_STATUS_OK in the switch (so all known cases are handled), but put the unreachable macro outside:
namespace {
const char* vp8StatusCodeString(VP8StatusCode status) {
switch(status) {
case VP8_STATUS_OUT_OF_MEMORY: return "out of memory";
...
case VP8_STATUS_OK: ;
}
CORRADE_INTERNAL_ASSERT_UNREACHABLE();
}
}Put the function just above doImage2D() so it's close to where it's used, the namespace { makes it a hidden symbol to avoid conflicting with similarly named functions that could be elsewhere.
|
The sudden CI error seems to be that you're apparently building the forked repository via your own CircleCI profile, instead of the build being done via my profile as a PR. Not sure what led to that, but I see two options:
|
| /* Same image with 90% image quality (lossy bitstream) */ | ||
| case 1: | ||
| CORRADE_COMPARE_AS(image->data(), Containers::arrayView<char>({ | ||
| '\x4d', '\x55', '\x24', |
There was a problem hiding this comment.
Testing for bit-exact equality of lossy-compressed images is quite a headache .. as you probably already found out :) And it's not unlikely that the output would change slightly when a new WebP version comes out.
So let's try a fuzzy comparison using DebugTools::CompareImage instead, where you can say how much can the pixels differ from the lossless ground truth. Those thresholds are best put into the instance data to not have to branch on testCaseInstanceId() again:
constexpr struct {
const char* name;
const char* filename;
Float maxThreshold, meanThreshold;
} RgbData[]{
{"lossless", "rgb-lossless.webp", 0.0f, 0.0f},
{"lossy with 90% image quality", "rgb-lossy-90.webp", 10.0f, 2.0f},
};The lossless image should be exactly the pixel values we expect, which is why the zeros. For the lossy I picked 10.0f and 2.0f just at random, when you run the test it'll tell you what's the actual threshold and a (slightly larger) value is then what should go here instead.
I also don't think we need to test that many variants, so just a losless and the 90% quality could suffice -- again, our goal is to just make sure the library is used correctly by the plugin, it's the WebP authors' job to ensure the library gives reasonable output for various quality settings.
The test would then look like this, I hope it's not much of a handholding this way :) Since it uses the DebugTools library now, you have to find it in CMake and link the test to it (look into BasisImporter/Test/CMakeLists.txt, there it's also being used).
#include <Magnum/DebugTools/CompareImage.h>
…
void WebPImporterTest::rgb() {
auto&& data = RgbData[testCaseInstanceId()];
setTestCaseDescription(data.name);
Containers::Pointer<AbstractImporter> importer = _manager.instantiate("WebPImporter");
CORRADE_VERIFY(importer->openFile(Utility::Path::join(WEBPIMPORTER_TEST_DIR, data.filename)));
Containers::Optional<Trade::ImageData2D> image = importer->image2D(0);
CORRADE_VERIFY(image);
/* Format and size checked by CompareImage, the image should also have
four-byte aligned rows */
CORRADE_COMPARE(image->storage().alignment(), 4);
const char expected[]{
'\x1e', '\x6e', '\x1e',
'\x1e', '\x6e', '\x1e',
'\x1e', '\x6e', '\x1e', 0, 0, 0,
'\xef', '\x91', '\x91',
'\xef', '\x91', '\x91',
'\xef', '\x91', '\x91', 0, 0, 0,
'\x52', '\x52', '\xbe',
'\x52', '\x52', '\xbe',
'\x52', '\x52', '\xbe', 0, 0, 0,
};
CORRADE_COMPARE_WITH(*image,
(ImageView2D{PixelFormat::RGB8Unorm, {3, 3}, expected}),
(DebugTools::CompareImage{data.maxThreshold, data.meanThreshold}));
}And similar for the RGBA test.
There was a problem hiding this comment.
This definitely looks much cleaner and better :)
when you run the test it'll tell you what's the actual threshold and a (slightly larger) value is then what should go here instead.
In my first commit after determining the threshold, I got CI errors because the values were not large enough for other some Linux versions. Hence, I increased them a bit more, now they are all 0.2 larger than actual threshold.
There was a problem hiding this comment.
the values were not large enough for other some Linux versions
That's quite interesting, and telling something about this format. When I said "it's not unlikely that the output would change slightly when a new WebP version comes out" above, I didn't expect that to happen so soon and so dramatically that it breaks even fuzzy comparison :D Other lossy codecs such as JPEG put a lot of effort into ensuring that the decoded output of the same file is bit-exact across versions, and if it changes, it's viewed as a critical bug. A bit sad to see that WebP doesn't live up to that standard.
It's like a Butterfly Effect, even a single-bit change can result in large differences down the line if the image is further recompressed or if it's used as a source for texture baking operations. And then a minor game patch that was meant just to improve image loading times is suddenly several gigabytes large.
There was a problem hiding this comment.
Yes, I thought it was interesting but had no idea about its consequences. Could it be prevented by forcing the installation of a specific WebP version and check via CMake configuration if the correct version is installed?
There was a problem hiding this comment.
Possibly, but staying at a fixed version has other downsides such as unpatched security issues. More like just something to keep in mind, that unexplainable minor differences in rendered/baked output might be due to a WebP upgrade. A radical suggestion might be to not store your source assets in WebP and treat it only as an export format on which nothing else depends.
Vaguely related -- I came across a similar issue when upgrading a 3rd party glTF parser once, somehow it started parsing transformations slightly differently and led to unexpected test failures. Wasted quite some time trying to find the root cause.
|
Hello @mosra, thank you very much for the useful and educative reviews. I haven't written yet any comment on my last commit about changes in program files as per your last review. I thought of writing after having some tests run successfully and I have just seen your review on tests, thank you :) I will reply to them individually. |
|
I hope I made the changes correctly and didn't miss anything, especially with the coding style :)
Thank you for the advice @mosra, I did both options just in case :)) |
There was a problem hiding this comment.
Okay, amazing, I think we're done here :) Just three minor bits plus if you could make the error printing into a helper function as we discussed above, and then I'm happy to merge.
The last thing that still needs to be done is finding the WebP dependency in FindMagnumPlugins, but that's hard to get right in a PR because nothing uses the Find module here. So I'll do that post-merge, once I can test locally.
Thank you!
WebPImporter with the latest changes Cmake and other config files got changed back. The file .gitignore has been added. Last minor changes have been made. Additions in the source file for error handling as per VP8 status and changes in the test file for image comparison were made. Adjustments in treshold values for image comparison CircleCI configuration got changed to include webp installation, build and tests of WebPImporter. CI configuration for Windows CI configuration removed for android latest CI configuration as per the last review Helper function for VP8 status output added. Rebase conflicts solved.
melikebatihan
force-pushed
the
webp-importer
branch
from
6411e88
to
7d5e9e5
Compare
|
Thank you! And sorry for the conflicts, had to update the CI yesterday because they deprecated old Ubuntu images... |
It is good practice for me to get the hang of Git :) I hope I didn't miss anything in my last commit. |
|
Even if you did, you suffered more than enough already :D My turn now, I'll do a final check locally and merge this, together with other busywork like packages, detection in I hope you had at least some fun with this :) |
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
==========================================
- Coverage 96.14% 96.07% -0.08%
==========================================
Files 121 124 +3
Lines 12072 12145 +73
==========================================
+ Hits 11607 11668 +61
- Misses 465 477 +12
Continue to review full report at Codecov.
|
No problem at all :D |
|
Merged as 392c702, thanks a lot for your contribution :) Together with 4100de3 and mosra/magnum@389ee37, the glTF importer can load files with WebP-encoded textures now, such as CesiumBoxWebp.gltf 🎉 (I had a lot of fun with wiring this through, too.)
Two post-merge change worth mentioning -- I don't think I could catch either of those before looking at the coverage report or before compiling the code locally, so you're not to blame for anything here:
|
|
Looks great, thank you :)
Yes, i noticed that too but I simply thought there might be very specific cases, where it could make a difference. Thanks for the information :) |
Hey @mosra, @Squareys,
here's the work in progress for importing *.webp images with libwebp.
I'll let you know once it's ready for the first review.
Best,
Melike
Progress so far:
TODOs