Saving a greyscale tiff pyramid fails to add predictor tags at lower levels

[manthey]

Bug report

Describe the bug
If I have a greyscale image and run vips tiffsave <infile> <out>.tif --tile --tile-width 256 --tile-height 256 --pyramid --compression lzw --predictor horizontal, the first IFD says it uses horizontal prediction, but all lower resolution IFDs DO NOT SET the predictor tag (0x13D), but DO use horizontal prediction.

To Reproduce
Steps to reproduce the behavior:

1. Use any greyscale image big enough to have multiple levels when converted to a pyramid.
2. vips tiffsave <infile> <out>.tif --tile --tile-width 256 --tile-height 256 --pyramid --compression lzw --predictor horizontal
3. View IFDs 1 or onward and they look wrong because the predictor flag is not set, so they are the difference between pixels rather than pixel values.

Expected behavior
The predictor tag to be set on all IFDs

Actual behavior
The predictor tag is missing on all IFDs except the first one.

Environment

• OS: Tested in Ubuntu 22.04 and Windows 10. The Windows test was done on the release binaries. The Ubuntu test was done with a locally built version.
• Vips: The error is in 8.15.1 but not in 8.14.5.

Additional context
A git bisect with my local build looks like the error was introduced in 04819e1.

The removal of these lines is the problem:
04819e1#diff-4f72fc0bfd65090427bffb48f08db90829bfa4278fbd8d2cb57b3bf8d7faea38L2383-L2386

Further, this line:

04819e1#diff-4f72fc0bfd65090427bffb48f08db90829bfa4278fbd8d2cb57b3bf8d7faea38R687-R688

breaks outputting greyscale images to jpeg compression with high Q values as setting to RGB color space will fail on a 1 band image.

[manthey]

A fix might be this diff

diff --git a/libvips/foreign/vips2tiff.c b/libvips/foreign/vips2tiff.c
index 3d8ccc06e..b6ad200b8 100644
--- a/libvips/foreign/vips2tiff.c
+++ b/libvips/foreign/vips2tiff.c
@@ -684,7 +684,7 @@ wtiff_compress_jpeg_header(Wtiff *wtiff,
        jpeg_set_defaults(cinfo);

        // use RGB mode (no chroma subsample) for high Q
-       if (wtiff->Q >= 90)
+       if (wtiff->Q >= 90 && image->Bands == 3)
                jpeg_set_colorspace(cinfo, JCS_RGB);

        /* Set compression quality. Must be called after setting params above.
@@ -2335,6 +2335,11 @@ wtiff_copy_tiff(Wtiff *wtiff, TIFF *out, TIFF *in)
         * we copy raw tiles.
         */

+       if ((wtiff->compression == COMPRESSION_ADOBE_DEFLATE ||
+                       wtiff->compression == COMPRESSION_LZW) &&
+               wtiff->predictor != VIPS_FOREIGN_TIFF_PREDICTOR_NONE)
+               TIFFSetField(out, TIFFTAG_PREDICTOR, wtiff->predictor);
+
        /* We can't copy profiles or xmp :( Set again from wtiff.
         */
        if (wtiff_embed_xmp(wtiff, out) ||

But I'm not understanding why writing the prediction tag was removed for the webp and zstd compression formats, so don't know if that should be changed back, too.

[jcupitt]

Hi @manthey,

Thanks for reporting this. I think I thought those were pseudotags rather than real tags, so they were no longer necessary now we copy compressed tiles. You're right, they should go back.

I'll add a test for this case as well.

[jcupitt]

It seems TIFFTAG_PREDICTOR is the only non-pseudotag there, so we can just add a CopyField() for it. The other tags (webp, zstd) are pseudotags and cannot be copied (and don't need to be).

This will be in 8.15.2. I credited you in the changelog, I hope that's OK.

[jcupitt]

(and thanks again for reporting this dumb thing!)

[manthey]

Thank you.

[kleisauke]

IIUC, I think the first part of the patch mentioned in comment #3808 (comment) is still needed.

@@ -684,7 +684,7 @@ wtiff_compress_jpeg_header(Wtiff *wtiff,
        jpeg_set_defaults(cinfo);

        // use RGB mode (no chroma subsample) for high Q
-       if (wtiff->Q >= 90)
+       if (wtiff->Q >= 90 && image->Bands == 3)
                jpeg_set_colorspace(cinfo, JCS_RGB);

        /* Set compression quality. Must be called after setting params above.

(perhaps wtiff->Q >= 90 && space == JCS_RGB is clearer?)

[dgutman]

John thanks for responding / fixing this so quickly.. and your definition of dumb is not exactly the universe's definition of dumb.. :-)

[jcupitt]

You're right, the JCS_RGB flag probably shouldn't be set for non-RGB images. Nice!