lvandeve / lodepng

PNG encoder and decoder in C and C++.
zlib License
2.06k stars 421 forks source link

1-bit RGB assumed in colorConvertTest, wrong in getPixelColorRGBA8 #58

Closed kornelski closed 6 years ago

kornelski commented 6 years ago

In the unit tests there's one test with "1-bit RGB" color:

colorConvertTest("11111111 11111111 11111111 00000000 00000000 00000000", LCT_RGB, 1, "10", LCT_GREY, 1);

I'm not sure if that test is valid. AFAIK PNG doesn't allow 1bpp in RGB mode.

There's disagreement between lodepng_get_raw_size() which calculates size for 1bpp, and getPixelColorRGBA8 which does not (the implementation assumes that only 8 and 16 are valid)

if(mode->colortype == LCT_RGB)
  {
    if(mode->bitdepth == 8)
    {
      *r = in[i * 3 + 0]; *g = in[i * 3 + 1]; *b = in[i * 3 + 2];
      if(mode->key_defined && *r == mode->key_r && *g == mode->key_g && *b == mode->key_b) *a = 0;
      else *a = 255;
    }
    else
    {
      *r = in[i * 6 + 0];
      *g = in[i * 6 + 2]; // out of bounds read?
      *b = in[i * 6 + 4];
      if(mode->key_defined && 256U * in[i * 6 + 0] + in[i * 6 + 1] == mode->key_r
         && 256U * in[i * 6 + 2] + in[i * 6 + 3] == mode->key_g
         && 256U * in[i * 6 + 4] + in[i * 6 + 5] == mode->key_b) *a = 0;
      else *a = 255;
    }
  }
lvandeve commented 6 years ago

You're correct, 1-bit RGB does not exist in PNG and is not supported by lodepng either since it only supports the valid PNG color type / bit depth combinations.

In the unit test, the 1 there was a typo, it actually had to be 8, and the test passed by coincidence instead (since it was treating it as 16 bit in the else case and the bit pattern must have matched something valid by coincidence)

Extra error handling in the lodepng_convert and lodepng_get_raw_size functions would be redundant, the error checking for valid color type / bit depth combination already happens elsewhere in the main decoding and encoding flow

Thanks for reporting! I fixed the test to use 8, and 16, instead of the wrong 1 bits.