spngsave: fix 8bpp palette save with transparency

[kleisauke]

Test case:

$ curl -LO https://github.com/lovell/sharp/raw/main/test/fixtures/expected/embed-4-into-4.png
$ vips copy embed-4-into-4.png x.png[palette]

[jcupitt]

Looking at this again, I was puzzled by the way the alpha was split off. Looking at spng.c:

https://github.com/randy408/libspng/blob/master/spng/spng.c#L3887-L3890

It looks like the indexes of spng_plte and spng_trns must match.

How about somethnig like:

                VipsPel *p = (VipsPel *) VIPS_IMAGE_ADDR( im_palette, 0, 0 );
                struct spng_plte_entry *entry = &plte.entries[0];
                uint8_t *type3_alpha =  &trns.type3_alpha[0];
                gboolean has_transparency = FALSE;

                for( i = 0; i < palette_count; i++ ) {
                        entry[i].red = p[0];
                        entry[i].green = p[1];
                        entry[i].blue = p[2];
                        type3_alpha[i] = p[3];

                        if( p[3] != 255 )
                                has_transparency = TRUE;

                        p += 4;
                }

                plte.n_entries = palette_count;
                if( has_transparency )
                        trns.n_type3_entries = palette_count;

[kleisauke]

The code you linked is for loading PNG images (spng_decode_image). For saving PNG images, it will use these paths:
https://github.com/randy408/libspng/blob/3c640a075dbb17d2d03f01422a03168fea1de285/spng/spng.c#L5654-L5661
https://github.com/randy408/libspng/blob/3c640a075dbb17d2d03f01422a03168fea1de285/spng/spng.c#L4250-L4253

Especially the memcpy(ctx->trns.type3_alpha, trns->type3_alpha, trns->n_type3_entries); would cause incorrect behavior if the indexes of spng_plte and spng_trns match.

[kleisauke]

Oh, I read your code wrong. The memcpy would be fine with that.

[kleisauke]

FWIW, there are no visual differences with both approaches from my tests.

Details

$ curl -LO https://github.com/randy408/libspng/raw/master/tests/images/basn6a08.png
$ vips copy basn6a08.png x.png[palette]
# With the above patch
$ vips copy basn6a08.png x-patch.png[palette]
# Sanity check average
$ vips avg x.png
127.694336
$ vips avg x-patch.png
127.694336
# Sanity check alpha band
$ python3 band_stats.py x.png
opaque = 8.0
translucent = 984.0
transparent = 32.0
$ python3 band_stats.py x-patch.png
opaque = 8.0
translucent = 984.0
transparent = 32.0
# pngcheck -vv diff
$ git diff --no-index x.png x-patch.png
diff --git a/x.png b/x-patch.png
index 1111111..2222222 100644
--- a/x.png
+++ b/x-patch.png
@@ -1,13 +1,13 @@
-File: x.png (1975 bytes)
+File: x-patch.png (1979 bytes)
   chunk IHDR at offset 0x0000c, length 13
     32 x 32 image, 8-bit palette, non-interlaced
   chunk PLTE at offset 0x00025, length 768: 256 palette entries
-  chunk tRNS at offset 0x00331, length 252: 252 transparency entries
-  chunk pHYs at offset 0x00439, length 9: 1000x1000 pixels/meter (25 dpi)
-  chunk IDAT at offset 0x0044e, length 853
+  chunk tRNS at offset 0x00331, length 256: 256 transparency entries
+  chunk pHYs at offset 0x0043d, length 9: 1000x1000 pixels/meter (25 dpi)
+  chunk IDAT at offset 0x00452, length 853
     zlib: deflated, 32K window, default compression
     row filters (0 none, 1 sub, 2 up, 3 avg, 4 paeth):
       0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
       0 0 0 0 0 0 0 (32 out of 32)
-  chunk IEND at offset 0x007af, length 0
-No errors detected in x.png (6 chunks, -92.9% compression).
+  chunk IEND at offset 0x007b3, length 0
+No errors detected in x-patch.png (6 chunks, -93.3% compression).

Where band_stats.py is:

#!/usr/bin/env python3

import sys
import pyvips

im = pyvips.Image.new_from_file(sys.argv[1])
hist = im[3].hist_find()

def band_stats(hist, val):
    mask = (pyvips.Image.identity() > val) / 255
    return (hist * mask).avg() * 256

total = im.width * im.height
alpha0 = band_stats(hist, 0)
alpha1 = band_stats(hist, 254)

# Metrics on how many pixels are opaque (no alpha),
# translucent (some alpha), and transparent (100% alpha)
print(f"opaque = {alpha1}")
print(f"translucent = {alpha0 - alpha1}")
print(f"transparent = {total - alpha0}")

Only the file size can increase a bit if there are not so many transparency entries. See for example:

# FIXME: This doesn't work on master for some reason (`vips_colourspace: no known route from 'histogram' to 'b-w'`)
$ vips identity x.png
$ vips bandjoin_const x.png x2.png 255
$ vips embed x2.png x3.png[palette] 0 0 257 1
# With the above patch
$ vips embed x2.png x3-patch.png[palette] 0 0 257 1
$ stat -c "%s %n" x3.png x3-patch.png
1080 x3.png
1315 x3-patch.png

[jcupitt]

I fixed the "no route" error, spngsave was messed up.

[jcupitt]

Ah I think I found it. I tried adding:

                        printf( "%d) %d %d %d %d\n", 
                                i, p[0], p[1], p[2], p[3] ); 

To the loop that copies the palette from libimagequant to spng, and it seems it always puts the transparent palette entries first. That's why PNG write works --- the indexes will always match in this case. I used this test prog:

vips identity bw.png
vips relational_const bw.png alpha.png more 128
vips linear alpha.png alpha2.png 1 50
vips colourspace bw.png rgb.png srgb
vips bandjoin "rgb.png alpha2.png" x3.png[palette]

The generated palette looks like this:

generated palette:
0) 58 58 58 50
1) 103 103 103 50
2) 32 32 32 50
3) 31 31 31 50
...
127) 96 96 96 50
128) 0 0 0 50
129) 255 255 255 255
130) 157 157 157 255
...
254) 191 191 191 255
255) 192 192 192 255

If you swap "more" for "less" on line 2 you can change the ordering of the transparent pixels in the file, but the generated palette keeps the transparent elements first. I tried with quantizr and it seems to have the same behaviour.

Is this guaranteed to always be the case? I can't see it in the docs, but if it is, then you're right, we can save a smaller transparency table.

@DarthSim can I ping you on this? Is this palette ordering a library feature?

[DarthSim]

Quantizer sorts the palette by the alpha chanel. AFAIK LIQ does the same.

[jcupitt]

OK, then let's merge, my worries are unfounded.

[kleisauke]

When I investigated issue lovell/sharp#3395, I noticed this assertion failure:

**
VIPS:ERROR:../libvips/foreign/spngsave.c:413:vips_foreign_save_spng_write: assertion failed: (i == 0 || p[3] >= p[-1])
Bail out! VIPS:ERROR:../libvips/foreign/spngsave.c:413:vips_foreign_save_spng_write: assertion failed: (i == 0 || p[3] >= p[-1])
Aborted (core dumped)

(added in commit 2af2ca5)

This can also reproduced with:

vips identity bw.png
vips linear bw.png alpha.png 1 50
vips colourspace bw.png rgb.png srgb
vips bandjoin "rgb.png alpha.png" x3.png[palette]

Thresholding the alpha channel seems to fix this.

--- a/libvips/foreign/spngsave.c
+++ b/libvips/foreign/spngsave.c
@@ -387,7 +387,7 @@ vips_foreign_save_spng_write( VipsForeignSaveSpng *spng, VipsImage *in )
 			spng->Q, 
 			spng->dither, 
 			spng->effort, 
-			FALSE ) )
+			TRUE ) )
 			return( -1 );
 
 		/* PNG is 8-bit index only.

And the output looks OK after that. But I'm not sure if this is the correct solution. Any ideas?

[DarthSim]

I would not do this. Alpha threshold works when the resultant format supports binary transparency only (transparent/opaque). PNG on the other hand supports semi-transparent pixels, and thresholding the alpha channel may reduce transparency quality dramatically.

It seems like I was wrong and LIQ doesn't sort the palette by alpha 😓 I prepared a PR that fixes the issue: #3074

(BTW, everything works fine with Quantizr 😉)

[kleisauke]

Great, thanks! That would correspond to what is done in the libpng saver.

libvips/libvips/foreign/vipspng.c

Lines 1211 to 1213 in df9f5d3

	png_trans[i] = p[3];
	if( p[3] != 255 )
	trans_count = i + 1;