pnggroup / libpng

LIBPNG: Portable Network Graphics support, official libpng repository
http://libpng.sf.net
Other
1.3k stars 628 forks source link

Gamma correction corner case #309

Open randy408 opened 5 years ago

randy408 commented 5 years ago

This was first submitted on sourceforge but no one uses that site anymore.

libpng has a corner case with the last pixel of 16-bit grayscale images when they're decoded to 8-bit RGBA using gamma correction.

When basn0g16.png is decoded without gamma correction both pixels at 0,0 and 31,31 are decoded to RGBA 0 0 0 255. Using gamma correction it's 0 0 0 255 at 0,0 and 19 19 19 255 at 31,31.

The code below reads basn0g16.png and decodes it without gamma correction and prints the first and last pixel's RGBA values. If "g" is passed as an argument it will decode using gamma correction.

This is reproducible with libpng 1.6.37 libspng bug report

./gamma.o
no gamma correction
image size: 4096
first pixel: 0 0 0 255, last pixel: 0 0 0 255

./gamma.o g
doing gamma correction
image size: 4096
first pixel: 0 0 0 255, last pixel: 19 19 19 255
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>

#include <png.h>

struct buf_state
{
    unsigned char *data;
    size_t bytes_left;
};

void libpng_read_fn(png_structp png_ptr, png_bytep data, png_size_t length)
{
    struct buf_state *state = png_get_io_ptr(png_ptr);

    if(length > state->bytes_left)
    {
        png_error(png_ptr, "read_fn error");
    }

    memcpy(data, state->data, length);
    state->bytes_left -= length;
    state->data += length;
}

int main(int argc, char **argv)
{
    char *filename = "basn0g16.png";
    FILE *png;
    char *pngbuf;
    png = fopen(filename, "r");

    if(png==NULL)
    {
        printf("error opening input file %s\n", filename);
        return 1;
    }

    int gamma_correct = 0;
    if(argc > 1 && argv[1][0] == 'g') gamma_correct = 1;

    if(gamma_correct) printf("doing gamma correction\n");
    else printf("no gamma correction\n");

    fseek(png, 0, SEEK_END);

    long siz_pngbuf = ftell(png);
    rewind(png);

    if(siz_pngbuf < 1) return 1;

    pngbuf = malloc(siz_pngbuf);
    if(pngbuf==NULL)
    {
        printf("malloc() failed\n");
        return 1;
    }

    if(fread(pngbuf, siz_pngbuf, 1, png) != 1)
    {
        printf("fread() failed\n");
        return 1;
    }

    png_infop info_ptr;
    png_structp png_ptr;
    struct buf_state state;

    state.data = pngbuf;
    state.bytes_left = siz_pngbuf;

    unsigned char *image = NULL;
    png_bytep *row_pointers = NULL;

    png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL);
    if(png_ptr == NULL)
    {
        printf("libpng init failed\n");
        return 1;
    }

    info_ptr = png_create_info_struct(png_ptr);
    if(info_ptr == NULL)
    {
        printf("png_create_info_struct failed\n");
        return 1;
    }

    if(setjmp(png_jmpbuf(png_ptr)))
    {
        printf("libpng error\n");
        png_destroy_read_struct(&png_ptr, &info_ptr, NULL);
        if(image != NULL) free(image);
        if(row_pointers !=NULL) free(row_pointers);
        return 1;
    }

    png_set_read_fn(png_ptr, &state, libpng_read_fn);

    if(png_sig_cmp(pngbuf, 0, 8))
    {
        printf("libpng: invalid signature\n");
        return 1;
    }

    png_read_info(png_ptr, info_ptr);

    png_uint_32 width, height;
    int bit_depth, colour_type, interlace_type, compression_type;
    int filter_method;

    if(!png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth,
                     &colour_type, &interlace_type, &compression_type, &filter_method))
    {
        printf("png_get_IHDR failed\n");
        return 1;
    }

    if(gamma_correct)
    {
        double gamma;
        if(png_get_gAMA(png_ptr, info_ptr, &gamma))
            png_set_gamma(png_ptr, 2.2, gamma);
    }

    png_set_gray_to_rgb(png_ptr);

    png_set_filler(png_ptr, 0xFF, PNG_FILLER_AFTER);

    /* png_set_palette_to_rgb() + png_set_tRNS_to_alpha() */
    png_set_expand(png_ptr);

    png_set_strip_16(png_ptr);

    png_set_interlace_handling(png_ptr);
    png_read_update_info(png_ptr, info_ptr);

    size_t rowbytes = png_get_rowbytes(png_ptr, info_ptr);

    size_t image_size = height * rowbytes;
    printf("image size: %zu\n", image_size);

    image = malloc(image_size);
    if(image == NULL)
    {
         printf("libpng: malloc() failed\n");
         png_destroy_read_struct(&png_ptr, &info_ptr, NULL);
         return 1;
    }

    row_pointers = malloc(height * sizeof(png_bytep));
    if(row_pointers == NULL)
    {
        printf("libpng: malloc() failed\n");
        free(image);
        png_destroy_read_struct(&png_ptr, &info_ptr, NULL);
        return 1;
    }

    int k;
    for(k=0; k < height; k++)
    {
        row_pointers[k] = image + k * rowbytes;
    }

    png_read_image(png_ptr, row_pointers);

    png_read_end(png_ptr, info_ptr);

    free(row_pointers);

    png_destroy_read_struct(&png_ptr, &info_ptr, NULL);

    printf("first pixel: %u %u %u %u, last pixel: %u %u %u %u",
            image[0], image[1], image[2], image[3],
            image[4092], image[4093], image[4094], image[4095]);

    return 0;
}
randy408 commented 5 years ago

More tests that produce similar errors: https://gist.github.com/randy408/5a0b8cf8bea56bb6ee82fce7958c1b91, files are from https://github.com/glennrp/libpng/tree/libpng16/contrib/testpngs.

jbowler commented 3 months ago

There is a bad (horrible) bug in 1.6 and prior versions where at least one combination of transforms results in transforms being performed in the wrong order, e.g. gamma correction after 16->8.

This was fixed in my version of 1.7 (by completely rewriting the transform code) but major versions of libpng were required by the maintainer to be "binary" compatible with image transforms; so fixing a bug were the transform is bogus was not permitted without a major release (and even then with much screaming and yelling.)

That doesn't seem to be this case because the observed behaviour is inconsistent with tthe required behaviour that a pixel at one place gets transformed identically to a pixel at another.

There might be a problem in the particular transforms used:

    png_set_gray_to_rgb(png_ptr);

    png_set_filler(png_ptr, 0xFF, PNG_FILLER_AFTER);

    /* png_set_palette_to_rgb() + png_set_tRNS_to_alpha() */
    png_set_expand(png_ptr);

    png_set_strip_16(png_ptr);

png_set_filler and png_set_tRNS_to_alpha cannot be used together; the result is undefined (do you get the filler or the alpha?)

Since this is a poorly defined combination of transforms (in the code PNG_FILLER|PNG_EXPAND_tRNS) it would not surprise me if it revealed a bug.

The basic rule is to check the properties of the input PNG and only apply transforms that make sense.

This is not a case which would be detected by any of the tests since it is asking for incompatible transforms.

I suggest using the simplified API, then you can blame me if it doesn't work :-)