[WIP] First version of WebP converter #140

melikebatihan · 2023-07-03T23:36:16Z

Hey @mosra, @Squareys,
here is the first version of WebP converter.

1- I have used Advanced Encoding API instead of the Simple Encoding API, which has several simple functions for encoding:

(For lossy encoding)
size_t WebPEncodeRGB(const uint8_t* rgb, int width, int height, int stride, float quality_factor, uint8_t** output);
size_t WebPEncodeRGBA(const uint8_t* rgba, int width, int height, int stride, float quality_factor, uint8_t** output);

(For lossless encoding)
size_t WebPEncodeLosslessRGB(const uint8_t* rgb, int width, int height, int stride, uint8_t** output);
size_t WebPEncodeLosslessRGBA(const uint8_t* rgba, int width, int height, int stride, uint8_t** output);

However, the settings used by these functions disable exact feature which preserves the exact RGB values under transparent area when enabled. When this feature is disabled like in the functions above, these RGB values in transparent areas gets discarded for better compression. Hence, I have used WebPConfig structure in Advanced Encoding API to configure advanced encoding parameters to achieve the highest quality, lossless encoding, and the main encoding function below:

WEBP_EXTERN int WebPEncode(const WebPConfig* config, WebPPicture* picture);

My WebPConfig configuration might not be ideal though, considering that a lossless conversion with 100% quality decreases the conversion speed (quality/speed trade-off).

2- I have adapted most of the tests in WebPImageConverterTest.cpp from the tests of other plugins 'PngImageConverter' and 'JpegImageConverter'. However, I couldn't properly understand the purpose of the methods like

PixelStorage{}.setSkip({0, 1, 0}).setRowLength(3)

So, I can't be sure if the conversions in tests like rgb(), rgba16(), grayscale() are correct.

mosra

Ooo, didn't expect this! :) I was contemplating writing a WebP exporter once I get bored, but the boredom didn't happen so far so I didn't.

Random comments on the essential things first, I'll have a closer look at the rest in the next iteration.

mosra · 2023-07-04T11:11:39Z

src/MagnumPlugins/WebPImageConverter/Test/WebPImageConverterTest.cpp

+constexpr const char OriginalRgbData16[80] = {
+    /* Skip */
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+
+    10, 20, 30, 10, 20, 30, 0, 0, 0, 0,
+    40, 50, 60, 40, 50, 60, 0, 0, 0, 0,
+    70, 80, 90, 70, 80, 90, 0, 0, 0, 0
+};
+
+const ImageView2D OriginalRgb16{PixelStorage{}.setSkip({0, 1, 0}),
+    PixelFormat::RGB16Unorm, {2, 3}, OriginalRgbData16};


PNG supports 16-bit-per channel images, which is what the *16() tests were for. Here you however turned the data to 8-bit, while the pixel format still says RGB16. That doesn't make sense, and since -- as far as I know -- WebP supports only 8 bits per channel, you don't need any of the *16() tests. A dedicated test for (8-bit) RGBA needs to be there however.

Similarly for grayscale images, given that WebP supports only RGB and RGBA I don't think it produces anything remotely correct for single-channel input.

The plugin code should check that the PixelFormat is one of the allowed formats, and fail the conversion if not. In general, formats accepted by the converter should match the formats produced by WebPImporter.

mosra · 2023-07-04T11:13:13Z

src/MagnumPlugins/WebPImageConverter/Test/WebPImageConverterTest.cpp

+    100, 78, 74, 100, 78, 74, 0, 0,
+    100, 84, 41, 100, 84, 41, 0, 0,
+    100, 87, 25, 100, 87, 25, 0, 0
+};


Wait but given you said the conversion is meant to be lossless and exact, this doesn't really look exact :)

mosra · 2023-07-04T11:23:58Z

src/MagnumPlugins/WebPImageConverter/Test/WebPImageConverterTest.cpp

+constexpr const char ConvertedRgba8Data[] = {
+    70, 80, 90, 100, 70, 80, 90, 100,
+    40, 50, 60, 90, 40, 50, 60, 90,
+    10, 20, 30, 80, 10, 20, 30, 80


It also looks like the converter doesn't Y-flip on export. The WebPImporter does (all importer plugins do), which means the converter has to flip as well, to arrive back at the ~~Y up~~ same orientation.

For decoding there was a flip option, the encoder options don't seem to have anything similar. But maybe passing a pointer to the last row instead of the first and a negative stride could work instead?

Scan line order from the webp sepcification:

A processing order of pixels (left to right and top to bottom), starting from the left-hand-top pixel. Once a row is completed, continue from the left-hand column of the next row.

If that does indeed need to be Y-flipped, flipping the image like PngImageConverter does before encoding would work, right?

That comment may have been confusing, sorry, updated it. WebP stores images Y down, while the in-memory data in Magnum is Y up (because they're all imported that way, currently). So the exporter has to flip back, yes.

In case of libpng it's easier because the encoder supports supplying data row-by-row. WebP doesn't, and doesn't have any explicit option for this, so it has to be either

[...] maybe passing a pointer to the last row instead of the first and a negative stride could work

as I said above, or making a copy of the input data and running Utility::flipInPlace() on it before passing it to libwebp.

mosra · 2023-07-04T11:30:45Z

src/MagnumPlugins/WebPImageConverter/WebPImageConverter.cpp

+    const std::pair<Math::Vector2<std::size_t>, Math::Vector2<std::size_t>> dataProperties = image.dataProperties();
+    auto inputData = Containers::arrayCast<const unsigned char>(image.data()).exceptPrefix(dataProperties.first.sum());


Ugh, I have to clean this up in the PngImageConverter and other plugins, the dataProperties() API is just horrific, sorry. It's far easier to access the data properties through the image.pixels() view than this. I'll let you know once that's done so you can mirror that change here.

This is cleaned up as of a3c01d5. Hopefully significantly easier to grasp this way.

mosra · 2023-07-04T11:35:10Z

src/MagnumPlugins/WebPImageConverter/WebPImageConverter.cpp

+        if(flags() & ImageConverterFlag::Quiet)
+            Error() << "Trade::WebPImageConverter::convertToData(): invalid WebPPicture size";
+        else
+            Warning{} << "Trade::WebPImageConverter::convertToData(): width or height of WebPPicture structure exceeds the maximum allowed:" << WEBP_MAX_DIMENSION;
+
+        return {};


I don't understand this logic. Given the conversion fails in any case, why is it a warning if the Quiet flag is not set, and an error if it is? Why one of the messages is more verbose than the other?

I think this whole check shouldn't need to be here at all, it's libwebp's responsibility to check that image dimensions are in allowed bounds.

mosra · 2023-07-04T11:42:32Z

src/MagnumPlugins/WebPImageConverter/WebPImageConverter.cpp

+    int width = image.size().x();
+    int height = image.size().y();
+    UnsignedInt channel_count = pixelFormatChannelCount(image.format());
+    const std::size_t stride = width * channel_count;  // in pixels units, not bytes.


Gotta admit I don't really understand what this comment in libwebp's encode.h means :D But you're using the stride in WebPPictureImport*(), where it is meant to be bytes I think.

To handle the input image stride correctly, ~~it has to be be dataProperties.second.x() instead (for now, until I do the modifications to use image.pixels() instead -- then it would simply be a stride of the view)~~, with a change similar to a3c01d5 it'd be now pixels.stride()[0].

I couldn't properly understand the purpose of the methods like setSkip, setRowLength

To answer this, these are usually for taking slices of large images. For example you have a 64x64 image and then you want the top-right 32x32 corner. So you skip the first 32 rows (assuming Y up coordinate system, with origin bottom left), the first 32 pixels of the first row, and then you have extra 32 pixels between each row, which means you set the row length to 64.

In the tests those are tested with the extra zeros in the image data. Also, this API isn't really great (same as the dataProperties() was), it's much easier to understand with sliced strided array views. It's a legacy from GL that I still have to clean up.

mosra · 2023-07-04T11:51:32Z

src/MagnumPlugins/WebPImageConverter/WebPImageConverter.cpp

+        return {};
+    }
+
+    pic.use_argb = (channel_count == 4) ? 1 : 0;


IIUC, if this is set to 0, then you're supposed to pass in a YUV input or something? That's not what's done here, it's RGB(A) always, so I think this should be 1 always.

mosra · 2023-07-04T11:56:55Z

src/MagnumPlugins/WebPImageConverter/CMakeLists.txt

@@ -0,0 +1,70 @@
+#


I'd like to have the CIs compile & run the new code, can you make them do it? It'd be mainly this:

adding a MAGNUM_WITH_WEBPIMAGECONVERTER option to the root CMakeLists.txt, next to the WebPImporter option

a corresponding add_subdirectory() in src/MagnumPlugins/CMakeLists.txt, again next to the similar code that does it for WebPImporter

and then in all package/ci/*.bat and package/ci/*.sh files, adding a -DMAGNUM_WITH_WEBPIMAGECONVERTER (again next to the WebPImporter flag) that's ON if WebPImporter is ON, and OFF if it's OFF

If you then look at the CI builds in the PR status, they should be building & running tests for the new plugin if everything is alright.

Other places (docs, CMake Find modules, packages...) I can do myself after.

mosra · 2023-07-04T12:01:42Z

src/MagnumPlugins/WebPImageConverter/WebPImageConverter.cpp

+    }
+
+    auto output = writer.mem;
+    Containers::Array<char> fileData{reinterpret_cast<char *>(output), writer.max_size};


This means Array will call delete[] on the output. I wonder if that's correct, given the docs say WebPMemoryWriterClear() should be used.

You'll probably need to allocate a new (NoInit'd) Array instead and then Utility::copy() to it.

Also I think it should be writer.size, not writer.max_size? Which sounds a bit similar to the Acropalypse kind of bugs, and would make sense to explicitly test for by having a ground truth output file to compare against. Such test unfortunately wasn't done for the PNG/JPEG converters, but as of 386fb15 it is. Hopefully this change is easy enough to do, the expected output files can be saved with --save-diagnostics when running the test.

mosra · 2023-07-04T12:07:56Z

src/MagnumPlugins/WebPImageConverter/WebPImageConverter.cpp

+    auto output = writer.mem;
+    Containers::Array<char> fileData{reinterpret_cast<char *>(output), writer.max_size};
+
+    WebPPictureFree(&pic);


To avoid leaks in early returns above, use a Containers::ScopeGuard. Same for the writer.

…tests to the allowed pixel formats

…ion to avoid memory leaks

melikebatihan · 2023-09-23T21:13:54Z

Hi @mosra, @Squareys,

I've made all the fixes and changes mentioned in the previous review, hopefully in an ideal way. After fixing some memory leak issues, all the CI checks seem to be successful, except for AppVeyor, which is still pending as I am writing.

mosra · 2024-03-31T01:13:38Z

This is integrated in a2f8acf, together with recognizing WebP images in AnyImageConverter in mosra/magnum@a2f8acf and support in GltfSceneConverter in 730133d.

Because the plugin now additionally exposes various options for controlling whether the output is lossy or lossless, quality / compression level knobs etc., the code and tests are quite different from what was originally in this PR. Nevertheless, you're kept as the original author. Thanks for doing the first 90% of the work!

mosra reviewed Jul 4, 2023

View reviewed changes

mosra added this to the 2023.0a milestone Jul 4, 2023

melikebatihan added 2 commits September 23, 2023 20:48

First version of WebP converter

5b385e0

Flip the input pixels, fix the converter confiurations and limit the …

d2d28dd

…tests to the allowed pixel formats

melikebatihan force-pushed the webp-converter branch from 655b823 to d2d28dd Compare September 23, 2023 18:58

melikebatihan added 3 commits September 23, 2023 21:20

Remove Quiet Data struct and fix the sizeError() test

29ef21c

Remove redundant lines of code in the test file, add ScopeGuard funct…

0d56a66

…ion to avoid memory leaks

Fix memory leak issue

8616841

mosra added the changelog mention added label Mar 30, 2024

mosra closed this Mar 31, 2024

		const std::pair<Math::Vector2<std::size_t>, Math::Vector2<std::size_t>> dataProperties = image.dataProperties();
		auto inputData = Containers::arrayCast<const unsigned char>(image.data()).exceptPrefix(dataProperties.first.sum());

[WIP] First version of WebP converter #140

[WIP] First version of WebP converter #140

Uh oh!

Conversation

melikebatihan commented Jul 3, 2023

Uh oh!

mosra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosra Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosra Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosra Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melikebatihan commented Sep 23, 2023

Uh oh!

mosra commented Mar 31, 2024

Uh oh!

Uh oh!

mosra Jul 4, 2023 •

edited

Loading

mosra Jul 4, 2023 •

edited

Loading

mosra Jul 4, 2023 •

edited

Loading