WebP: Fix alpha decoding when source subsampling is used

[wladimirleite]

Analyzing the issue described in #736, I believe it is caused by the way alpha channel may be encoded.

In readAlpha() method (referenced below), the alpha channel is decoded into the alpha raster of destination , which can be smaller than the original image, if source subsampling is used.

TwelveMonkeys/imageio/imageio-webp/src/main/java/com/twelvemonkeys/imageio/plugins/webp/WebPImageReader.java

Line 513 in 29f7547

private void readAlpha(BufferedImage destination, ImageReadParam param, final int width, final int height) throws IOException {

However, in the end of this method, filters may be applied:

TwelveMonkeys/imageio/imageio-webp/src/main/java/com/twelvemonkeys/imageio/plugins/webp/WebPImageReader.java

Lines 551 to 558 in 29f7547

	if (filtering != AlphaFiltering.NONE) {
	for (int y = 0; y < destination.getHeight(); y++) {
	for (int x = 0; x < destination.getWidth(); x++) {
	int predictorAlpha = getPredictorAlpha(alphaRaster, filtering, y, x);
	alphaRaster.setSample(x, y, 0, alphaRaster.getSample(x, y, 0) + predictorAlpha % 256);
	}
	}
	}

Inside the getPredictorAlpha() function, the values from the previous row and/or column are used to calculate the current alpha value, as shown below.
In this case, if subsampling was already applied (as it is today), this function will get a different value (from 2 or more rows/columns before). And as this is an accumulative process, pixels to the right and bottom of the image may have higher errors (as observed in the provided sample images).

TwelveMonkeys/imageio/imageio-webp/src/main/java/com/twelvemonkeys/imageio/plugins/webp/WebPImageReader.java

Lines 561 to 589 in 29f7547

	private int getPredictorAlpha(WritableRaster alphaRaster, int filtering, int y, int x) {
	switch (filtering) {
	case AlphaFiltering.NONE:
	return 0;
	case AlphaFiltering.HORIZONTAL:
	if (x == 0) {
	return y == 0 ? 0 : alphaRaster.getSample(0, y - 1, 0);
	}
	else {
	return alphaRaster.getSample(x - 1, y, 0);
	}
	case AlphaFiltering.VERTICAL:
	if (y == 0) {
	return x == 0 ? 0 : alphaRaster.getSample(x - 1, 0, 0);
	}
	else {
	return alphaRaster.getSample(x, y - 1, 0);
	}
	case AlphaFiltering.GRADIENT:
	if (x == 0) {
	return y == 0 ? 0 : alphaRaster.getSample(0, y - 1, 0);
	}
	else if (y == 0) {
	return alphaRaster.getSample(x - 1, 0, 0);
	}
	else {
	int left = alphaRaster.getSample(x - 1, y, 0);
	int top = alphaRaster.getSample(x, y - 1, 0);
	int topLeft = alphaRaster.getSample(x - 1, y - 1, 0);

So I think the solution would be first decoding the alpha channel in a raster with the original dimensions (including applying any filters), then subsample this alpha raster to the destination raster.

As I am not very familiar with TwelveMonkeys' code, probably this can be achieved with a better implementation.
My main goal was to confirm that the proposed approach solves the issue.

[haraldk]

Thank you very much! 😀

Code looks reasonable! I'll read though it in more detail and hope to merge it soon.

[haraldk]

I'll merge your PR, as it seems to fix the issue. 👍🏻

However, I would really appreciate if you could also create a test method (in the WebPImageReaderTest class) that fails in the current version, but passes with your fix, to avoid future regressions for subsampled reading. Preferably using one of the existing sample files. You should find plenty of examples in the existing test classes. Let me know if you have the time to provide that, or have any questions!

[wladimirleite]

I'll merge your PR, as it seems to fix the issue. 👍🏻

That is great! Thank you!

However, I would really appreciate if you could also create a test method (in the WebPImageReaderTest class) that fails in the current version, but passes with your fix, to avoid future regressions for subsampled reading. Preferably using one of the existing sample files. You should find plenty of examples in the existing test classes. Let me know if you have the time to provide that, or have any questions!

Sure, I can try to do that.
I will let you know if I have any questions, or update here when I am done.

[wladimirleite]

I would really appreciate if you could also create a test method (in the WebPImageReaderTest class) that fails in the current version, but passes with your fix, to avoid future regressions for subsampled reading.

I just added an unit test that passes with the new code but fails with the current version.

Preferably using one of the existing sample files.

Unfortunately, none of the existing sample files have alpha transparency and alpha filtering, which is necessary to trigger the issue, so I had to add a new sample file. Let me know if there is another way to avoid adding it.

While running the existing unit tests and creating the new one, I found two issues in the code I submitted, fixed now by the commits 3eabc59 and 0a2efb9.

[haraldk]

If you have a sample file with smaller dimensions, that would probably be better for test performance. If not, no worries, we can use this one. 😀

[wladimirleite]

If you have a sample file with smaller dimensions, that would probably be better for test performance. If not, no worries, we can use this one. 😀

I couldn't find a small sample image, so I replaced by the smallest I found, with ~15% less pixels than before (and a much smaller file in bytes).

PS: There seems to be something odd with the indentation here.

I tried to follow the existing indentation.
It looks fine for me here...
Not sure if is that what called your attention, buy the TestData above the one I added has multiple dimensions, so its indentation is different, right?

[haraldk]

I'm thinking maybe we should just allow param to be null in the copyIntoRasterWithParams method? Will be more readable.

Code will also read better with a static import of VP8LDecoder.copyIntoRasterWithParams.

[wladimirleite]

Agreed! I made both changes.

[haraldk]

I think we should be able to get away with not creating another full-size temporary raster here. Instead we can do the filtering on the tempRaster below, by creating a writable child raster on line 544, and do the alpha filtering on that.

Something like this:

        WritableRaster alphaRaster = destination.getAlphaRaster();

        switch (compression) {
            case 0:
                readUncompressedAlpha(alphaRaster);
                break;

            case 1:
                // Simulate header
                imageInput.seek(imageInput.getStreamPosition() - 5);

                WritableRaster tempRaster = Raster.createInterleavedRaster(DataBuffer.TYPE_BYTE, width, height, 4, null);
                readVP8Lossless(tempRaster, null, width, height);

                // Copy from green (band 1) in temp to alpha in destination
                WritableRaster alphaChannel = tempRaster.createWritableChild(0, 0, tempRaster.getWidth(), tempRaster.getHeight(), 0, 0, new int[]{1});
                alphaFilter(alphaChannel, filtering);
                alphaRaster.setRect(alphaChannel); // TODO: Replace this with copyIntoRasterWithParams(alphaChannel, alphaRaster, param);
                break;

            default:
                processWarningOccurred("Unknown WebP alpha compression: " + compression);
                opaqueAlpha(alphaRaster);
                break;
        }

...and extracting the current alpha filtering to a separate method:

    private void alphaFilter(WritableRaster alphaRaster, int filtering) {
        if (filtering != AlphaFiltering.NONE) {
            for (int y = 0; y < alphaRaster.getHeight(); y++) {
                for (int x = 0; x < alphaRaster.getWidth(); x++) {
                    int predictorAlpha = getPredictorAlpha(alphaRaster, filtering, y, x);
                    alphaRaster.setSample(x, y, 0, alphaRaster.getSample(x, y, 0) + predictorAlpha % 256);
                }
            }
        }
    }

I'm not sure if alpha filtering is supported for uncompressed alpha, and we don't support it anyway (if you have a sample file with uncompressed alpha, please send it to me! 😀 ).

[wladimirleite]

I think we should be able to get away with not creating another full-size temporary raster here. Instead we can do the filtering on the tempRaster below, by creating a writable child raster on line 544, and do the alpha filtering on that.

Cool! I made the changes you proposed, and it seems to be working fine.

I'm not sure if alpha filtering is supported for uncompressed alpha, and we don't support it anyway (if you have a sample file with uncompressed alpha, please send it to me! 😀 ).

I checked all WebP files I have here, and couldn't find any with uncompressed alpha.
I will try to collect new ones, and let you know if I can find any of these.

By the way, while looking for an uncompressed alpha sample, I found another WebP file that seems to be valid, but it is triggering an exception (it is single file out of a ~5K test set). This time it seems a much simpler problem. I will open another issue, as it is not related to this one.

[wladimirleite]

I'm not sure if alpha filtering is supported for uncompressed alpha, and we don't support it anyway (if you have a sample file with uncompressed alpha, please send it to me! 😀 ).

I checked all WebP files I have here, and couldn't find any with uncompressed alpha. I will try to collect new ones, and let you know if I can find any of these.

Looking at a larger set of WebPs (~40K files), I found 3 that are reported as having "alpha compression = 0".
I am not sure if they actually have uncompressed alpha, or are false detections caused by some issue in the files.
They do enter in readUncompressedAlpha() while decoding.
possibly-uncompressed-alpha-samples.zip

[wladimirleite]

Thanks!
I am taking a closer look in the comments you made, and will try to make the suggested adjustments.

[wladimirleite]

Ok, I think I covered all the requested changes.
Just let me know if something is missing or can be improved.

[haraldk]

Excellent! You have addressed all my concerns. 👍🏻