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

Refine GifExportParams #417

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Refine GifExportParams #417

merged 4 commits into from
Mar 28, 2024

Conversation

n0vad3v
Copy link
Contributor

@n0vad3v n0vad3v commented Mar 18, 2024

In response to #359 (comment)

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
Copy link

coveralls commented Mar 18, 2024

Coverage Status

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

@tonimelisma
Copy link
Collaborator

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

@n0vad3v
Copy link
Contributor Author

n0vad3v commented Mar 28, 2024

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

@tonimelisma
Copy link
Collaborator

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
Copy link
Contributor Author

n0vad3v commented Mar 28, 2024

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

@tonimelisma tonimelisma merged commit d2d2a1f into davidbyttow:master Mar 28, 2024
1 check passed
@tonimelisma
Copy link
Collaborator

Thank you sir. Your contributions are always welcome!

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

Successfully merging this pull request may close these issues.

3 participants