-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Julian progress functions. #353
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I just noticed this! I think it supersedes #352? If so, feel free to close that PR after this PR is submitted
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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... |
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, The cfunction docstring mentions
So it looks like the cfunctions inside functions like |
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. |
Ok, after some tryouts, the best thing here is to change GDAL.jl; to change all calls with pData in them from Like this, but it requires a change in the generator script. The clue is that it's the 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. |
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. |
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 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I've updated the docstrings. Feel free to merge if you're happy with it.
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. |
Should be ever so slightly slower, as some unsafe_conversions are done, but enables a much more Julian interface, using the existing API.