Add Julian progress functions.

[evetion]

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

[yeesian]

Ah I just noticed this! I think it supersedes #352? If so, feel free to close that PR after this PR is submitted

[evetion]

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?

[yeesian]

Thank you for this PR -- it helped me to understand the whole callback mechanism (which I never did in the past). My comments are just to see if we can simplify the end-user API)

[yeesian]

LGTM once tests are passing, thanks! (Trying to look into why they're failing with _progresscallback undefined for now; https://docs.julialang.org/en/v1/manual/calling-c-and-fortran-code/#More-About-Callbacks)

[evetion]

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]

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]

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]

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]

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]

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 #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 🎉

[yeesian]

LGTM, thanks for seeing this all the way through!

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 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

[evetion]

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.