Adds format-specific export params

[davidbyttow]

FYI @tonimelisma, putting this up so you can see where my head is for the export params. Not yet ready for review

[davidbyttow]

ugh stupid formatter, will fix

[tonimelisma]

Ok, cool. Having a shared struct for all image types when their parameters have different semantics is indeed a bit confusing.

Can you explain what the main benefits of the approach are?

Perhaps we could simplify further - why don't we have export functions for each image type directly with their own arguments? Instead of creating export struct, setting specific export struct members, then calling function we would just directly call image type specific function with whatever parameters we want to?

I'm not sure I have strong opinions either way. I guess being backwards compatible is the main objective.

[davidbyttow]

Agree on backward compatibility, I will make sure that's the case regardless, even if I have to deprecate some stuff.

As for benefit of approach: Different image formats have their own unique parameters although though some common parameters are shared among many formats (like strip and quality). We should expose all options for the image types we support.

There's really two ways to do that:

1. A single export method that takes all of the parameters, in which we'd rely on documentation/comments to say what works for which formats
2. Explicit format-specific export methods that take specific parameters for that type

I think we can agree that #2 is strictly better than #1. The only caveat is if we want to have an export method that just says "export as whatever image type it is based on what was loaded, I don't care", in which case we could expose maybe just a few options that work with all (or most) image types (strip, quality, etc). That's essentially where we are today basically.

As for a struct vs directly calling image type specific functions with parameters, it's a matter of extensibility and future backward compatibility. If we use a struct, then we can add new parameters later if we want without breaking the function signatures. Also it's a matter of practicality because in the case of TIFF that has like 12 parameters, we don't have unnecessarily long functions that you have to call if you only want to set 3 of those parameters, for example (and then what are the defaults?). If this were Scala or something where you can use optionals and named parameters, then I'd consider that model. So to get the best of both worlds, we can have the "New{Format}ExportParams" methods for each type where it sets reasonable default. If we wanted we can add further syntactic sugar methods that wrap it and set various parameters in one shot (e.g., ExportAsJpg(image Image, stripMetadata bool, quality int), but I think clients can set those up in their own codebase easily by using the "New{Format}ExportParams".

Side note, if we want to avoid both type-specific structs and type-specific export methods by having a single export method, we could just create a common interface that all of the param structs derive from and then type-switch ourselves, but I'm not sure how much benefit there is to that.

[tonimelisma]

Thanks @davidbyttow that's well-thought design. Could you provide some guidance and comment on PR #155 on whether to move forward with the old-style import params or implement them in a similar fashion as this PR?

[davidbyttow]

Will do (probably a bit later tonight)

[bozaro]

I try to avoid va_list arguments for ...load_buffer methods to make optional flags in PR #155.

This logic also allow avoid hell like:

  return vips_jpegsave_buffer(in, buf, len,
                              "strip", INT_TO_GBOOLEAN(strip),
                              "Q", quality,
                              "optimize_coding", TRUE,
                              "interlace", INT_TO_GBOOLEAN(interlace),
                              "subsample_mode", VIPS_FOREIGN_JPEG_SUBSAMPLE_ON,
                              NULL);

[davidbyttow]

@bozaro, that's fine but it comes down to where we want defaults. If we want to control all defaults in Go for all params, then we are going to pass them all in anyway. If we want to fall back to the libvip defaults if unset in Go, then using the pattern you have is fine too. Either way its an implementation detail on the C side that we can change later.

[coveralls]

Coverage decreased (-1.8%) to 78.69% when pulling b2f6b21 on export_params into 9d49b8a on master.

[davidbyttow]

This seems good to go, I think @tonimelisma

[bozaro]

The list of parameters depends on the specific version of libvips.
If you pass an undeclared parameter to libvips, an error will be returned.
The only option I see with full control of parameters in Go is to generate methods for a specific version of libvips. But this relationship seems too harsh.

[tonimelisma]

Hey @bozaro each govips version supports and is tested with specific libvips versions so this is not an issue. If/when we see new libvips issues which we want to support we can add them and bump up the libvips dependency. The main determination right now of the libvips version we use is the one that package management on Ubuntu and macOS provide. On macOS we use homebrew and on Ubuntu we use the latest stable version, which I backport to the last two LTS versions in my personal package archive.

[tonimelisma]

Go beauty in its purest form man