Adds support for loading parameters

[davidbyttow]

Inspired by: #155

However, this doesn't use libvips options yet. My intent is to likely switch to using VImage options (rather than C vararg functions) for import and export parameters, but I wanted to get this exposed and working first.

[coveralls]

Coverage decreased (-0.2%) to 78.485% when pulling 35b05f5 on import_params into 94826ab on master.

[davidbyttow]

WDYT, @tonimelisma ?

[tonimelisma]

Looks sweet. However export parameters are called ExportParams and these are called LoadParams, should they be ImportParams? On the export side we also don't allow for generic export parameters, but we do have generic import parameters, is that cool?

A small detail: FailOnError should probably be FailOnWarning

[tonimelisma]

I guess we could compromise, same as with export stuff, we could have a generic import function and format-specific ones? Perhaps we would need a function to identify the format of a file (or buffer?) so you could switch on it.

If we also want to change the new export parameters and functions to align them with this import params function that would be fine as well as we haven't released a new version with them.

I think generally it would be just easiest if the import and export params worked in a similar fashion.

[davidbyttow]

Good comments, let me share a few thoughts here:

• I'm not thrilled with the PR for a few reasons but it does work so maybe that's good enough (I'll explain why below)
• The way I thought about this vs export is: in the common case you often don't care what the input format is, you just want the thing to load. If you do know the type then I agree it's useful to have specific ones. Perhaps we could have a generic load and then type-specific loads, as you mention. I'm OK with switching to that.
• I took out the "FromFile and FromReader" methods because IMO they should never have been a part of the image API as they just wrap FromBuffer and, for example, ForReader is kind of misleading because you might expect it's streaming but it just reads the entire thing into memory, which is what the user could do themselves (or we have separate utility methods for). Same with FromFile kind of, the vision of these was to pass the file through to Vips to load from file (which can be faster in many cases for sequential reads).
• Naming, I agree maybe symmetrical naming is better (e.g., Import) but it feels weird to call it "Import." I thought of it kind of like for Photoshop where you "Open a file" but you "Export" to a specific type. Maybe the principle here is just being consistent (e.g., Import/Export)

OK, so my problem with this (and export) is our use of defaults. I don't like defining defaults in govips at all, and I don't like that we always pass these values through (e.g., scale=1.0) even when they're unset. I'd much prefer a model where we only set these parameters in vips if the user specifies them. To do that, we need to do two things:

1. Switch away from C varargs and to the libvips GObject or C++ APIs to be able to conditionally set parameters
2. Come up with a good solution for optional arguments in govips (e.g., the With* pattern or explicit Setters that set pointers or something like that)

So, I'm stuck right now between which direct to go.

[tonimelisma]

Hey David, good analysis.

I think the root cause of the problem is the lack of proper undefined values / null safety in Go. Go is so much fun and nice to write, but some of the design choices are pretty horrible.

For point 1), how would we accomplish this? How do we connect to the GObject API from Go, or to a C++ interface for that matter? On a quick browse, I couldn't find a good GObject library, and even if there is, I doubt it's very idiomatic Go.

For point 2) regarding the ergonomics of the govips API, I think the current With* pattern is much more economical than separate Setters. Imagine having to set five different parameters for an export. That's at least 7 lines of code - create the ExportParams, set the five parameters and do the export.

The cleanest option from a Go lang perspective would be to use variadic arguments for a list of Export Params. Define a generic export parameter type ExportParam, and create a set of methods which return these, which can be passed as a list for the Export function (or for the Import function similarly as well).

Similar to imaging: https://github.com/disintegration/imaging/blob/v1.6.2/io.go#L174

[tonimelisma]

Also, I hope you voted in the annual Go language survey and noted that the lack of null safety is a big problem for you 😉

[davidbyttow]

Yeah, I agree with your points too.

PS, don't upgrade to an M1 chip as your dev machine if you can help it, I sunk 2 hours into trying to get govips to build/run on my M1 Mac mini yesterday and still not out of the woods, about to just go back to my 16" MBP for that. I did manage to get one of my other projects (a Unity/Swift hybrid game) to run on my M1 last night though, so partial success.

[davidbyttow]

OK, I mostly just posted what I'm thinking. If this works, then I'll probably change Export to use the same pattern.

The ergonomics are a bit different, and we can still add variadic go constructors to make it easier, but basically you have to explicitly set parameters via its API. E.g., params.autorotate.Set(true) on the user side. I figure they can create wrappers if they want.

WDYT @tonimelisma?

[thomasritz]

In #56 @toaster added access loading options, but they are not included anymore in the code. It looks like they are still supported by libvips. What do you think about adding support for access mode again, @davidbyttow.

[davidbyttow]

@thomasritz i can't recall why that was removed, but yes, if it makes sense we can certainly bring it back. I didn't do it in this PR since this didn't remove it, but should be easy to add.