libvips / lua-vips

Lua binding for the libvips image processing library
MIT License
125 stars 10 forks source link

Add bindings for misc image methods #46

Closed RiskoZoSlovenska closed 6 months ago

RiskoZoSlovenska commented 2 years ago

This PR adds support for a small handful of misc image methods which are not covered by the binding, such as hasalpha.

Since vips is still a bit of a black box for me, I'm not 100% sure that this is the correct way to add support for these methods, but it looks like pyvips does the same thing.

I'm not sure whether tests are necessary here, and if they are, I'm not sure where to put them.

jcupitt commented 2 years ago

For addalpha, I'd put something like this just after bandsplit (ie. in the convenience method area):

function Image_method:addalpha()
    local max_alpha

    if self:interpretation() == "rgb16" or
        self:interpretation() == "grey16" then
        max_alpha = 65535
    else
        max_alpha = 255
    end

    return self:bandjoin(max_alpha)
end

(I've not written Lua for a long time and that's untested, I hope I have it right)

RiskoZoSlovenska commented 6 months ago

Hi, I've amended this PR to only add the hasalpha and addalpha methods. I've also rebased on master and added tests for the new functionality.

jcupitt commented 6 months ago

Look good!

I guess array.lua needs some local adding. I wonder why lint passed this before?

jcupitt commented 6 months ago

Could you also add a line to changelog? And credit yourself, of course.

jcupitt commented 6 months ago

... libvips now has:

double
vips_interpretation_max_alpha(VipsInterpretation interpretation)
{
    switch (interpretation) {
    case VIPS_INTERPRETATION_GREY16:
    case VIPS_INTERPRETATION_RGB16:
        return 65535.0;
    case VIPS_INTERPRETATION_scRGB:
        return 1.0;
    default:
        return 255.0;
    }
}

but it was only added recently, so you'd need to add a test. It's simpler just to reproduce the code in lua.

RiskoZoSlovenska commented 6 months ago

libvips already provides an addalpha function: https://www.libvips.org/API/current/libvips-conversion.html#vips-addalpha, https://github.com/libvips/libvips/blob/159e8bea25fbc70316e32cd36add0b7c899568b7/libvips/conversion/bandjoin.c#L521-L541. Why don't we simply make the binding call that instead of reimplementing the logic in Lua?

RiskoZoSlovenska commented 6 months ago

Actually, how come the binding doesn't cover addalpha already? I can't really say I understand how the binding works yet but what makes addalpha different from the other conversion methods that are automatically bound (e.g. composite, premultiply, etc.)?

jcupitt commented 6 months ago

libvips is in a few layers:

  1. At the base is GObject, the object system from glib. It's a little like Java, so single-inheritance, multiple inheritance of interfaces, introspection (a mix of runtime and derived from scanning header files), and a signal/slot mechanism (java delegates).
  2. libvips layers VipsObject on that, which adds mostly full run-time introspection and some extra stuff around object lifetime management.
  3. Next is a VipsOperation class which defines operation lifetime (create object, set parameters, build, read results, dispose).
  4. Things like VipsComposite are VipsOperation subclasses that implement image composition (obviously).
  5. lua-vips works by catching references to undefined methods on the image object, then walking the class hierarchy below VipsOperation and looking for a class with that nickname. It's actually pretty simple, just a 150 lines of lua in voperation.lua.

The libvips C API is just a few thin wrappers over this object system. The C++ / ruby / python / php / etc. bindings all work the same way -- in effect, the libvips binding is generated at runtime via introspection. The nice thing is that because it's mostly automatic, the lua binding is tiny, and will work with new versions of libvips with (usually) no changes needed. No maintenance is the best maintenance!

You can use vips -l to show the libvips class hierarchy, it might make it clearer:

$ vips -l
  VipsOperation (operation), operations
    VipsSystem (system), run an external command
    VipsArithmetic (arithmetic), arithmetic operations
      VipsBinary (binary), binary operations
        VipsAdd (add), add two images
        VipsSubtract (subtract), subtract two images
...

vips_addalpha() doesn't use this object system, it's just a simple C function in the C API (which lua-vips does not use), so it won't appear.

You could call it via ffi, but it was added fairly recently, so you'd need something like eg.:

if libvips.version > 8.9 then
  return vips_addalpha(in)
else
  return in:bandjoin(max_alpha)
fi

Since you'll need the lua fallback anyway, and it's trivial, it's probably simpler to just always use that.

jcupitt commented 6 months ago

The gobject docs have an introduction to their object system, if you're curious:

https://docs.gtk.org/gobject/index.html

RiskoZoSlovenska commented 6 months ago

Oh, I see, thanks for the explanation.

It was added fairly recently, so you'd need something like eg.:

Would that really be necessary, though? Why bother polyfilling older versions of the library? Wouldn't it be reasonable to simply throw an error when someone tries to use a function that doesn't exist in their version of libvips?

I'm just concerned that by (re)implementing functions in Lua, we're treading a thin line; whenever the libvips function is updated (such as to add support for scRGB), we'll have to update the Lua implementation, or risk a situation where the binding silently produces results different from libvips (which, IMO, is worse than having the binding fail completely).

Edit: I found out that such a situation is already occurring with pyvips; see https://github.com/libvips/pyvips/issues/457.

jcupitt commented 6 months ago

Yes, it's a bit of a wart in the API.

I didn't want to make a complete operation for addalpha (it's so trivial! why bother!), so I left it as a utility function in the binding. But then of course the (many) bindings have to be synchronised, and any improvements have to be rewritten $n times. With hindsight, it probably would have been better as a proper core operation.

Wouldn't it be reasonable to simply throw an error when someone tries to use a function that doesn't exist in their version of libvips?

You can probably argue it either way. addalpha is just a line of code, so why not have a lua fallback? You already need the if vips.version > 8.9 test, it's almost no more effort to include the fallback as well.

jcupitt commented 6 months ago

I had a go and I think calling the C API for this would be tricky and likely to cause bugs in future. It's much cleaner and safer to have a plain lua implementation.

We could add addalpha as a true libvips operation, that would be another good approach.

RiskoZoSlovenska commented 6 months ago

Right, I didn't realize that calling the C API for addalpha would be less trivial than for e.g. hasalpha, since it creates a new image.

I'm in favour of making addalpha a true libvips operation; I'll try to open a PR if someone doesn't beat me to it. In the meantime, we can probably merge this.

rolandlo commented 6 months ago

Thanks @RiskoZoSlovenska for your contribution. I merged it.