davidbyttow / govips

A lightning fast image processing and resizing library for Go
MIT License
1.23k stars 196 forks source link

Refine `GifExportParams` #417

Closed n0vad3v closed 4 months ago

n0vad3v commented 5 months ago

In response to https://github.com/davidbyttow/govips/issues/359#issuecomment-2002981134

Changes:

  1. Added notes on GifExportParams to tell users that different vips version will use different params
  2. StripMetadata is removed as libvips does not support this function on GIFs, I've tried pass it in with VIPS_OBJECT(operation), "strip", params->stripMetadata,, the output image remains the same.
  3. Passed in bitdepth in early vips version

However this PR is not finished, I've found several problems here.

  1. No matter how I change GifExportParams's Dither, Effort or Bitdepth, the output image's sha256sum remains the same
  2. In the current set_gifsave_options, only one parameter can be used with passed in multiple parameters, as below
    // See for argument values: https://www.libvips.org/API/current/VipsForeignSave.html#vips-gifsave
    if (params->gifDither > 1 && params->gifDither <= 10) {
    ret = vips_object_set(VIPS_OBJECT(operation), "dither", params->gifDither, NULL);
    }
    if (params->gifEffort >= 1 && params->gifEffort <= 10) {
    ret = vips_object_set(VIPS_OBJECT(operation), "effort", params->gifEffort, NULL);
    }
    if (params->gifBitdepth >= 1 && params->gifBitdepth <= 8) {
      ret = vips_object_set(VIPS_OBJECT(operation), "bitdepth", params->gifBitdepth, NULL);
    }

    But, as different parameters will result in same output image, I'm not sure why this is happening.

coveralls commented 5 months ago

Coverage Status

coverage: 73.277%. remained the same when pulling 0ed507ffd69d7e988eb176fa9630fa67dca9c5bf on webp-sh:master into e348555c2ce9d5d37997afb638820a12b103cbe4 on davidbyttow:master.

tonimelisma commented 4 months ago

Hey @n0vad3v, thank you very much for the PR. Do you think you will have time finishing it?

n0vad3v commented 4 months ago

Hi @tonimelisma I think this PR is ready to merge for now, however more testing might be needed on GIF images.

tonimelisma commented 4 months ago

It looks like you've removed the StripMetadata parameter. I'm really sorry but we can't have breaking changes in the API. I know it's not optimal to have poorly working or nonfunctional parameters in the API but we can't break users' software.

n0vad3v commented 4 months ago

Got it, now I'm adding StripMetadata option back and added a note for it.

tonimelisma commented 4 months ago

Thank you sir. Your contributions are always welcome!