Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color inversion for signed gray images #129

Closed
fintelia opened this issue Apr 18, 2021 · 4 comments
Closed

Color inversion for signed gray images #129

fintelia opened this issue Apr 18, 2021 · 4 comments

Comments

@fintelia
Copy link
Contributor

From @TomasKralCZ in #125 (comment)

One thing I'm not sure about is why @fintelia removed color inversion for signed formats. Is that non-standard behaviour ? I remember there was a comment about how libtiff interprets that in a certain way but it got removed.

#128 wasn't intended to change this behavior, but looking more closely it seems that the library wasn't being consistent prior to the change. In particular, i32 and i64 grayscale images never did inversion, while i8 and i16 would if WhiteIsZero was specified.

We should figure out which is correct behavior and then do it consistently. This comment suggests that inversion might be correct:

image-tiff/src/decoder/mod.rs

Lines 1037 to 1038 in 9b94e9d

// The following conversions interpret the image as in libtiff.
// In particular, MIN is white and MAX is black and not Zero as the name would imply.

@to-mas-kral
Copy link
Contributor

I tried figuring out how libtiff does this, but this is the only relevant code I found:

/*
 * Construct a mapping table to convert from the range
 * of the data samples to [0,255] --for display.  This
 * process also handles inverting B&W images when needed.
 */
static int
setupMap(TIFFRGBAImage* img)
{
    int32_t x, range;

    range = (int32_t)((1L << img->bitspersample) - 1);

    /* treat 16 bit the same as eight bit */
    if (img->bitspersample == 16)
        range = (int32_t)255;

    img->Map = (TIFFRGBValue*)_TIFFmalloc((range + 1) * sizeof(TIFFRGBValue));
    if (img->Map == NULL) {
        TIFFErrorExt(img->tif->tif_clientdata, TIFFFileName(img->tif),
            "No space for photometric conversion table");
        return (0);
    }
    if (img->photometric == PHOTOMETRIC_MINISWHITE) {
        for (x = 0; x <= range; x++)
            img->Map[x] = (TIFFRGBValue)(((range - x) * 255) / range);
    } else {
        for (x = 0; x <= range; x++)
            img->Map[x] = (TIFFRGBValue)((x * 255) / range);
    }
    if (img->bitspersample <= 16 && (img->photometric == PHOTOMETRIC_MINISBLACK || img->photometric == PHOTOMETRIC_MINISWHITE)) {
        /*
	 * Use photometric mapping table to construct
	 * unpacking tables for samples <= 8 bits.
	 */
        if (!makebwmap(img))
            return (0);
        /* no longer need Map, free it */
        _TIFFfree(img->Map);
        img->Map = NULL;
    }
    return (1);
}

What also puzzles me, is that we try to invert floating point values,

fn process_photometry_f32(buffer: &mut [f32]) {
for datum in buffer.iter_mut() {
// FIXME: assumes [0, 1) range for floats
*datum = 1.0 - *datum
}
}

but again, I haven't found any code in libtiff which would manipulate floating-point data in such a way.
I also tried creating a copy of a 32-bit floating-point encoded file where I changed the PhotometricInterpretation tag from BlackIsZero to WhiteIsZero. Both of the files are displayed the same in GIMP, Krita fails to display the WhiteIsZero version, which seems to suggest that this inversion might be wrong.

But take this with a grain of salt. The libtiff codebase is a bit hard to read, so it's possible that I have missed something.

@to-mas-kral
Copy link
Contributor

to-mas-kral commented May 14, 2021

OK, the way libtiff handles these conversions is weird.

libtiff has a few different interfaces(man pages):

  • Raw IO - returns undecoded image data
  • Encoded IO - returns decoded data, swaps bytes to native byte order if needed
  • RGBA IO - decodes image data into 32-bit packed RGBA

The TIFFRGBAImage function (man pages) only supports 1, 2, 4, 8, 16 bit formats, which would partially explain why we weren't handling i32s and i64s consistently. The man pages also explain, that it's possible for users to define functions for converting from other uncommon formats to RGBA.

I also created two test images.
signed-minblack-minwhite-test.zip

I tried loading them with the TIFFRGBAImage function and it does inverse the data but treats the data as unsigned integers. It seems strange that libtiff doesn't handle signed format by default, but it would also explain why most graphics software on my machine (Gwenview, Krita) display these files badly. The only program that renders it correctly is GIMP, which presumably uses the more low-level libtiff functions.

Therefore it's questionable if we should do implicit conversions at all.
One solution would be to not do these conversions implicitly, but provide users with some standard procedures for inverting the colors. These procedures should also take SMinSampleValue and SMaxSampleValue into account.

@fintelia
Copy link
Contributor Author

We only ever do color inversion if the file specifies "white is zero". A lot of the weirdness comes from seeing that tag on files where that doesn't really make sense.

@fintelia
Copy link
Contributor Author

Looked through the liftiff source as well and came to a similar conclusion as @TomasKralCZ. I don't see anything that indicates our current strategy is clearly wrong. I'd say we leave things as they are.

We can revisit later if anyone comes across a more definitive answer or some software that creates images with different assumptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants