search

libvips / lua-vips

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

Avoid FFI sizeof trick and image copy. #36

Closed ImagicTheCat closed 4 years ago

ImagicTheCat commented 4 years ago

Not all pointers "carry" the size of the memory they point on (just like in C), thus it was restricting the usage to LuaJIT allocated memory (which can be confusing and has limitations without the GC64 mode).

Note: this way even allows a string to be passed as the data pointer.

Hi, I was using lua-vips for processing and I encountered this limitation. FFI is low level, so it makes sense to not abstract away the pointer/size.

(I edited example/array.lua by mistake, fixed it, but the pull request is not updated)

ImagicTheCat commented 4 years ago

Should the same thing be done for write_to_memory since the ffi.sizeof would not give a meaningful size anymore ?

jcupitt commented 4 years ago

Yes, the test fails will need fixing.

ImagicTheCat commented 4 years ago

I think about write_to_memory_nocopy as a suitable name for the alternative of write_to_memory.

jcupitt commented 4 years ago

I don't think there's an API change in write_to_memory, is there? I think the name can stay, unless I've misunderstood.

ImagicTheCat commented 4 years ago

I thought since it doesn't return an FFI data anymore the behavior to get the size changed. But most uses will probably be about accessing the pointer and process the data using the image width, height, bands and format.

jcupitt commented 4 years ago

Ooop, of course you are right, sorry. I've not looked at this for a while.

Sure, _nocopy sounds like a reasonable name.

ImagicTheCat commented 4 years ago

No problem, I will do that and try to write the corresponding tests.

ImagicTheCat commented 4 years ago

I used malloc for the unit test (and made the mistake of being lazy to test it locally) for clarity (compared to a simple cast). It should be fine now.

jcupitt commented 4 years ago

This looks like a nice improvement, thank you for working on this, @ImagicTheCat!

I'll add a note to the CHANGELOG, bump the version and update the rock.

ImagicTheCat commented 4 years ago

Oh, I wanted to rebase into a single commit after your approval, so the commits were very simple, I suppose they will do.

Thank you for maintaining libvips, it is very useful. I may try to improve the OpenEXR support one day.

jcupitt commented 4 years ago

Yes, it's a simple change, I think it's OK.

Yes, the EXR loader is pretty bad :( It's needs modernising.