yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
137 stars 25 forks source link

Add Julian progress functions. #353

Closed evetion closed 1 year ago

evetion commented 1 year ago

Should be ever so slightly slower, as some unsafe_conversions are done, but enables a much more Julian interface, using the existing API.

evetion commented 1 year ago

I think the other PR, which this is based on, should be merged, as ArchGDAL (GeoArrays) is not fully functional on Julia >=1.8.2 (can't use Cloud Optimized Geotiff for example).

This PR could warrant some more discussion, because I've tried to be backwards compatible, but that yields some inconsistent naming schemes. We might want to break this part of the API?

evetion commented 1 year ago

I'm clearly missing some understanding of the intricacies of cfunction. If we define the cfunction ArchGDAL._cprogresscallback before.

julia> using ArchGDAL
julia> f = @cfunction(ArchGDAL._progresscallback, Cint, (Cdouble, Cstring, Ptr{Cvoid}))
Ptr{Nothing} @0x00007f7cd4705da0
julia> ccall(Base.unsafe_convert(Ptr{Cvoid}, f), Cint, (Cdouble, Cstring, Ptr{Cvoid}), 1.0, "test", C_NULL)
[ Info: (1.0, Cstring(0x00007f7ce110a238), Ptr{Nothing} @0x0000000000000000)
1

julia> ccall(Base.unsafe_convert(Ptr{Cvoid}, ArchGDAL._cprogresscallback), Cint, (Cdouble, Cstring, Ptr{Cvoid}), 1.0, "test", C_NULL)

signal (11): Segmentation fault
in expression starting at REPL[5]:1
unknown function (ip: 0x7f8bcb10e1e0)
top-level scope at ./REPL[5]:1
Allocations: 3605364 (Pool: 3601633; Big: 3731); GC: 3

Btw, note that while the previous commits fail horribly in CI, they all work on my M1 mac. I've now reverted to my Fedora workstation to develop this further...

visr commented 1 year ago

I don't really understand this PR or how it should fix the problem that not all architectures are supported. Could you explain?

In your example, Base.unsafe_convert(Ptr{Cvoid}, f) does not seem neccessary, ccall will do that for us, in a safer manner I believe.

The cfunction docstring mentions

And that these arguments will be evaluated in global scope during compile-time (not deferred until runtime). Adding a '$' in front of the function argument changes this to instead create a runtime closure over the local variable callable (this is not supported on all architectures).

So it looks like the cfunctions inside functions like copywholeraster! need to have a $ added in front of the first argument to use the local variable, e.g. @cfunction($_progresscallback. But then the not about this not being supported on all architectures applies.

evetion commented 1 year ago

This PR is about making user defined progress functions work in a Julian way. Any function that takes a float and string, while returning a bool should work.

evetion commented 1 year ago

Ok, after some tryouts, the best thing here is to change GDAL.jl; to change all calls with pData in them from Ptr{Cvoid} into Any. This will enable ccall to handle the conversion in a safe(r) manner than I have been trying to do with Ref and GC.@preserve.

Like this, but it requires a change in the generator script. The clue is that it's the Ptr{Cvoid} following pProgressFunc.

diff --git a/src/libgdal.jl b/src/libgdal.jl
index 74afada..4f1c71f 100644
--- a/src/libgdal.jl
+++ b/src/libgdal.jl
@@ -2551,7 +2551,7 @@ function gdalcreatescaledprogress(arg1, arg2, arg3, arg4)
         ccall(
             (:GDALCreateScaledProgress, libgdal),
             Ptr{Cvoid},
-            (Cdouble, Cdouble, GDALProgressFunc, Ptr{Cvoid}),
+            (Cdouble, Cdouble, GDALProgressFunc, Any),
             arg1,
             arg2,
             arg3,
@@ -4567,7 +4567,7 @@ function vsisync(
                 Cstring,
                 Ptr{Cstring},
                 GDALProgressFunc,
-                Ptr{Cvoid},
+                Any,
                 Ptr{Ptr{Cstring}},
             ),
             pszSource,

Works locally on both Linux and M1, so no platform differences.

yeesian commented 1 year ago

Ok, after some tryouts, the best thing here is to change GDAL.jl; to change all calls with pData in them from Ptr{Cvoid} into Any.

I was curious about the claim for this before I found it in https://julialang.org/blog/2013/05/callback/#passing_closures_via_pass-through_pointers.

evetion commented 1 year ago

Yeah that has been my reference indeed. Crazy to see it's now 10 years old, but the Any conversion actually works and is the cleanest solution. It's like https://github.com/yeesian/ArchGDAL.jl/pull/349, in that we pass most control to ccall itself.

My own tryouts with Ref always led to segmentation faults for some cases, even though it worked 99% of the time (100% of the time on my M1, which threw me off for a while). So my guess would be that the GC cleaned up my Ref or even the function pointer. Note that you can't take a pointer to a Function directly in Julia (immutable things are moved around).

I'll make a PR for GDAL. Happy new year 🎉

evetion commented 1 year ago

My remaining nit is to include the function signature of progressfunc inside the respective docstrings, otherwise end-users don't have a way to know what types of functions would be valid.

I've updated the docstrings. Feel free to merge if you're happy with it.

I guess one downside of this approach is users can't plug in GDALTermProgress, but I guess we can provide our own port (or better, maybe e.g. via progress meter.jl) in the future

You actually can! Although it starts to feel like pingpong a bit:


progressfunc = (progress, message) -> GDAL.gdaltermprogress(progress, message, C_NULL)
0...10...20...30...40...50...60...70...80...90...100 - done.