brendan-duncan / image

Dart Image Library for opening, manipulating, and saving various different image file formats.
MIT License
1.17k stars 265 forks source link

Wrong small ICO thumbnail made from `buck_8.png` #648

Closed yegor-pelykh closed 4 months ago

yegor-pelykh commented 4 months ago

I tried to make ICO thumbnail from two different PNG images: buck_24.png and buck_8.png. When I convert from buck_24.png (located in your test/_data/png folder), everything is fine. But when I try to apply these convertions to buck_8.png (located in the same folder), the result is broken.

I think this is because of using palette in buck_8.png.

Please could you have a look?

Test code:

test('buck_8.png', () {
  final bytes = File('test/_data/png/buck_8.png').readAsBytesSync();
  var image = decodePng(bytes)!;

  image = copyResize(
    image,
    width: 150,
  );

  vignette(
    image,
  );

  final ico = encodeIco(image);
  File('$testOutputPath/png/buck_8.ico')
    ..createSync(recursive: true)
    ..writeAsBytesSync(ico);
});

Result image (unfortunately, I can't attach an ICO file here): result

brendan-duncan commented 4 months ago

Yeah that doesn't look right. I'll try to get to that soon.

brendan-duncan commented 4 months ago

Oh, it's not the ICO encoding that is failing, it's the vignette, that doesn't work on palette images, and instead of not doing anything, or converting the image, it's doing the wrong thing.

I would have to fix it by having vignette convert it to a 24-bit image, in which case on your side you would have to do: image = vignette(image); because the image could be copied during the vignette.

You could fix it now with:

test('buck_8.png', () {
  final bytes = File('test/_data/png/buck_8.png').readAsBytesSync();
  var image = decodePng(bytes)!;

  image = copyResize(
    image,
    width: 150,
  );
  image = image.convert(numChannels: 3); //<-- convert the image to 24-bit
  vignette(
    image,
  );

  final ico = encodeIco(image);
  File('$testOutputPath/png/buck_8.ico')
    ..createSync(recursive: true)
    ..writeAsBytesSync(ico);
});
yegor-pelykh commented 4 months ago

Thank you for the clarification, this is a good solution for now.

Do you have any ideas to support vignette for images with a palette?

brendan-duncan commented 4 months ago

It wouldn't ever support palette images, what it would have to do is do that conversion to 24-bit automatically inside the vignette and other filter functions. Those functions manipulate color, and a palette doesn't have enough colors to do what it needs to do. That also means you should always get the image returned by the filter functions, as image = vignette(image);.

yegor-pelykh commented 4 months ago

Got it, thanks for the explanation! So, feel free to close this issue.

brendan-duncan commented 4 months ago

I pushed a change to have the filter functions like vignette auto convert palette images. They will return the new converted image in that case. I'll go ahead and close this issue with that change.