libvips / lua-vips

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

NYI messages from LuaJIT #27

Closed kleisauke closed 3 years ago

kleisauke commented 6 years ago

Hi John,

Running the soak.lua or the units tests with -jv outputs a few NYI messages. NYI messages indicate that this has not yet been implemented in the in LuaJIT's JIT compiler. See: http://wiki.luajit.org/NYI.

I think it will be good to solve these problems (where possible), especially in performance-critical code (such as voperation and gvalue and vobject).

NYI messages:

  1. [NYI: bytecode 50 at voperation.lua:102] Fixed with: https://github.com/kleisauke/lua-vips/commit/f898808373d567e6dbd501cf941f777442d50acb.
  2. [NYI: bytecode 51 at voperation.lua:118] Fixed with: https://github.com/jcupitt/lua-vips/pull/28
  3. [NYI: bytecode 51 at voperation.lua:188] Fixed with: https://github.com/kleisauke/lua-vips/commit/8c46fac10444e987a00d401011b9b0213b11ec2d.
  4. [NYI: bytecode 72 at voperation.lua:214] Not fixable, we must iterate with pairs here.
  5. [NYI: bytecode 50 at voperation.lua:265] Fixed with: https://github.com/kleisauke/lua-vips/commit/c4d87b599583ab88c87d76278036c8a4e6efabf1.
  6. [NYI: unsupported C type conversion at vobject.lua:89] Fixed with: https://github.com/kleisauke/lua-vips/commit/f898808373d567e6dbd501cf941f777442d50acb
  7. [NYI: bytecode 51 at Image_methods.lua:771] Not fixable, we can't circumvent the vargargs closure.
  8. [NYI: unsupported C type conversion at Image_methods.lua:448] Fixed with: https://github.com/kleisauke/lua-vips/commit/f898808373d567e6dbd501cf941f777442d50acb.
  9. [NYI: unsupported C type conversion at gvalue.lua:134], see footnote 1.
  10. [NYI: bytecode 51 at voperation.lua:91] Fixed with: https://github.com/kleisauke/lua-vips/commit/6dd0e10d7ad2179aea437bbe2230c8f5c958fd0c

Footnotes:

  1. I think there might be ints in the array (because I don't see these messages for the array_int_type gtype).

I'll make a PR in the course of next week.

kleisauke commented 6 years ago

Update: Number 3 and 5 are resolved with https://github.com/kleisauke/lua-vips/commit/8c46fac10444e987a00d401011b9b0213b11ec2d and https://github.com/kleisauke/lua-vips/commit/c4d87b599583ab88c87d76278036c8a4e6efabf1. Number 4 and 7 are not fixable. I'm not sure if we can fix number 1 and 6, I will investigate that further.

Also, I've removed vips.gvalue.new() and switched to the default constructor (vips.gvalue()) with https://github.com/kleisauke/lua-vips/commit/0d30c81a01e3b08da0f07959873711dfd8911fb1.

jcupitt commented 6 years ago

Huh I didn't know about -jv, that's useful.

kleisauke commented 6 years ago

Update: Number 10 is resolved with https://github.com/kleisauke/lua-vips/commit/6dd0e10d7ad2179aea437bbe2230c8f5c958fd0c. It seems that number 1, 6 and 8 can be solved with:

diff --git a/vips/Image_methods.lua b/vips/Image_methods.lua
index 1111111..2222222 100644
--- a/vips/Image_methods.lua
+++ b/vips/Image_methods.lua
@@ -445,10 +445,11 @@ function Image_method:get(name)
 end

 function Image_method:set_type(gtype, name, value)
-    local gv = gvalue.new()
-    gv:init(gtype)
-    gv:set(value)
-    vips_lib.vips_image_set(self.vimage, name, gv)
+    local pgv = gvalue.newp()
+    pgv[0]:init(gtype)
+    pgv[0]:set(value)
+    vips_lib.vips_image_set(self.vimage, name, pgv)
+    gobject_lib.g_value_unset(pgv[0])
 end

 function Image_method:set(name, value)

diff --git a/vips/vobject.lua b/vips/vobject.lua
index 1111111..2222222 100644
--- a/vips/vobject.lua
+++ b/vips/vobject.lua
@@ -86,10 +86,11 @@ local vobject_mt = {
                 return false
             end

-            local gv = gvalue.new()
-            gv:init(gtype)
-            gv:set(value)
-            gobject_lib.g_object_set_property(self, name, gv)
+            local pgv = gvalue.newp()
+            pgv[0]:init(gtype)
+            pgv[0]:set(value)
+            gobject_lib.g_object_set_property(self, name, pgv)
+            gobject_lib.g_value_unset(pgv[0])

             return true
         end

but this is awful (the __gc function from the GValue class is then no longer used).