Add support for Mac OS on M1 #87
Conversation
|
heya, nice, thanks for jumping onto this. as a general comment, the code with these patches applied will probably not run on my machine any more (or on macintosh with intel cpu?). also i want to limit platform specific code to very few places to make our life easier in the future. let me leave a few comments interleaved in the patch. |
src/db/db.c
Outdated
| @@ -454,7 +478,7 @@ int dt_db_read(dt_db_t *db, const char *filename) | |||
| lno++; | |||
|
|
|||
| // scan filename:rating|labels:number | |||
There was a problem hiding this comment.
oh, macintosh thinks 32-bit are long? then probably better to use %"PRIu64" here so it will work on all platforms.
There was a problem hiding this comment.
I just followed compiler warnings, I will see what is going on here, in case ok to switch to PRIu64
src/db/db.h
Outdated
| @@ -4,6 +4,12 @@ | |||
| #include <string.h> | |||
| #include <strings.h> | |||
|
|
|||
| #ifndef __APPLE__ | |||
There was a problem hiding this comment.
this argument swap issue is terrifying. there has got to be a better way (not involving the ifdef around all compare functions above).
There was a problem hiding this comment.
Yes I know, I want just to reach a working/compiling solution and then beutify. 😄
src/db/thumbnails.c
Outdated
| snprintf(cfgfilename, sizeof(cfgfilename), "%s", filename); | ||
| snprintf(deffilename, sizeof(deffilename), "default.%"PRItkn, dt_token_str(input_module)); | ||
| struct stat statbuf = {0}; | ||
| time_t tcfg = 0, tbc1 = 0; | ||
|
|
||
| if(!stat(cfgfilename, &statbuf)) | ||
| #ifndef __APPLE__ |
There was a problem hiding this comment.
okay we probably want to factor this out in something like time_t t = dt_stat(filename) defined in core.h or similar, so the platform specific code is only in one place.
There was a problem hiding this comment.
Yes I will do something like that to put together all plat-spec code. Thank you for the tip 😉
src/pipe/modules/i-raw/main.cc
Outdated
| @@ -314,7 +314,11 @@ void modify_roi_out( | |||
| mod->img_param.whitebalance[3] /= mod->img_param.whitebalance[1]; | |||
| mod->img_param.whitebalance[1] = 1.0f; | |||
|
|
|||
| #ifndef __APPLE__ | |||
There was a problem hiding this comment.
really that doesn't exist? then maybe another wrapper in core.h like dt_isnan
There was a problem hiding this comment.
Yes... Too many things are not in Macos... Painful work..
There was a problem hiding this comment.
i think this patch is obsolete, the test==test performs nan testing without api call.
|
.. about rawspeed. does the vanilla rawspeed git build on M1? i would be surprised if not. anyways it's probably better to use upstream rawspeed for changes. about the blank screen. did you run |
|
Here I am! If you appreciate it, I'm working to make it compile with CMake, which does more platform-specific controls. I also updated ImGUI dependency (I cherry-picked your modifications) to check for the blank screen problem. Thank you for your feedback! |
|
With the new imgui this is the error: Apparently, it cannot set the WindowSize properly... I saw you modified imgui backends for that. |
|
Updates on error state. I set the font size to 16.0 with a literal to go on. Now blocked at: I think this is related to how the dynamic libraries of the modules are compiled. |
|
okay is that error any different to the older imgui at all? not sure i understand how you would put imgui into a gtk3/cairo application. as additional dependency? maybe a saner approach is to use about dynamic libraries. i saw you link there may also be an issue about setting file paths to discover additional data/config files/dsos. i'm highly unlikely to merge cmake build system patches. cmake doesn't do anything for me (it generates makefiles, i have makefiles already), i'm unconvinced it does much for platform independence except mostly a big if around stuff. all variables related to local build environments should go in re:imgui vs glfw functions: some code is c, some is c++ (as minimal as possible, only gui related and interfacing with rawspeed/exiv2). glfw is c, so there are some constraints as to when you can swap things around. |
I'm going to investigate the cause in the following days.
I would like essential ImGui components to control the processing on a single image, then extend for library management like in Ansel. Yes, I would like to get rid of GTK. For sure I will link with
Bad patch due to ramp up on the compilation flow of the project; I will force push the next days.
Thank you for the hint, I will investigate also this.
I will keep it separated. I found it more convenient than hand-written Makefiles for the long-term, but it could be optional for this work. For example, in shared libraries, it automatically handles all the different flags for other OSes.
Here, I need more experience in ImGui development to be helpful. I noticed the patch about |
LucaZulberti
force-pushed
the
macos
branch
6 times, most recently
from
c7f459d to
0663632
Compare
|
Updated with only Makefile modifications. Now the error is: Same error with updated ImGui (HEAD) and old version (HEAD~2). |
|
looks like you're lacking |
LucaZulberti
force-pushed
the
macos
branch
2 times, most recently
from
8322814 to
f93e947
Compare
|
Now Makefiles are more clean. Next error: Same error for old and new ImGui+GLFW. Maybe is related to MoltenVK implementation of Vulkan on macOS? |
|
Running only 
|
|
I rebased on master, GUI is working, |
Makefile
Outdated
| @@ -6,6 +6,8 @@ | |||
| .PHONY:all src clean distclean bin install release cli | |||
| ifeq ($(OS),Windows_NT) | |||
| include bin/config.mk.defaults.w64 | |||
| else ifeq ($(OS),Darwin) | |||
| include bin/config.mk.defaults.darwin | |||
There was a problem hiding this comment.
master has config.mk.defaults.osx instead. probably makes sense to merge these.
Makefile
Outdated
| @@ -53,7 +55,7 @@ endif | |||
|
|
|||
| install-mod: bin Makefile | |||
| mkdir -p $(VKDTDIR)/modules | |||
| rsync -avP --include='**/params' --include='**/connectors' --include='**/*.ui' --include='**/ptooltips' --include='**/ctooltips' --include='**/readme.md' --include='**.spv' --include='**.so' --include '*/' --exclude='**' bin/modules/ ${VKDTDIR}/modules/ | |||
| rsync -avP --include='**/params' --include='**/connectors' --include='**/*.ui' --include='**/ptooltips' --include='**/ctooltips' --include='**/readme.md' --include='**.spv' --include='**.$(SEXT)' --include '*/' --exclude='**' bin/modules/ ${VKDTDIR}/modules/ | |||
There was a problem hiding this comment.
meh. don't really care about macintosh specific file endings. .so works on windows (and macintosh) just fine. i'm manually opening the file / dlopening the symbols anyways.
| endif | ||
| ifeq ($(VKDT_USE_QUAKE), 1) | ||
| RELEASE_FILES+=$(shell cd src/pipe/modules/quake/quakespasm; git ls-files | sed -e 's#^#src/pipe/modules/quake/quakespasm/#') | ||
| RELEASE_FILES+=$(shell cd src/pipe/modules/quake/quakespasm; git ls-files | sed -e 's\#^\#src/pipe/modules/quake/quakespasm/\#') |
There was a problem hiding this comment.
oh does that not work on osx? i need to test this change on linux/windows/my old macos
bin/config.mk.defaults
Outdated
| @@ -101,9 +101,16 @@ VKDT_VULKAN_CFLAGS=$(eval VKDT_VULKAN_CFLAGS := $$(shell pkg-config --cflags vul | |||
| VKDT_VULKAN_LDFLAGS=$(eval VKDT_VULKAN_LDFLAGS := $$(shell pkg-config --libs vulkan))$(VKDT_VULKAN_LDFLAGS) | |||
There was a problem hiding this comment.
this one isn't even loaded on macintosh?
bin/config.mk.defaults.darwin
Outdated
| @@ -0,0 +1,144 @@ | |||
| # build time configuration. | |||
There was a problem hiding this comment.
see above, please merge with config.mk.defaults.osx
bin/config.mk.defaults.osx
Outdated
| @@ -59,7 +59,7 @@ export VKDT_USE_ALSA | |||
|
|
|||
| # build the i-mcraw loader module to load raw video from the motioncam app. | |||
| # it has no additional dependencies but comes with several ten thousand lines of c++ code | |||
| VKDT_USE_MCRAW=1 | |||
| VKDT_USE_MCRAW=0 | |||
There was a problem hiding this comment.
does that not work? newer master should have removed the SSE/x86 code in this module. i don't see a reason why it wouldn't work on arm now.
src/Makefile
Outdated
| @@ -103,26 +103,25 @@ gui/%.o: gui/%.c Makefile $(GUI_H) gui/flat.mk | |||
| # main application | |||
| # ====================== | |||
| ../bin/vkdt: $(GUI_O) $(QVK_O) $(CORE_O) $(SND_O) $(PIPE_O) $(DB_O) Makefile | |||
| $(CC) $(GUI_O) $(QVK_O) $(CORE_O) $(SND_O) $(PIPE_O) $(DB_O) -pie -o $@ \ | |||
There was a problem hiding this comment.
probably needs some mechanism to put it back on none-macintosh systems.
src/core/fs.h
Outdated
| @@ -52,7 +68,7 @@ fs_copy( | |||
| if(sb.st_mode & S_IFDIR) { len = 0; goto copy_error; } // don't copy directories | |||
| if(-1 == (fd1 = open(dst, O_CREAT | O_WRONLY | O_TRUNC, 0644))) goto copy_error; | |||
| do { | |||
| ret = sendfile(fd1, fd0, 0, len); // works on linux >= 2.6.33, else fd1 would need to be a socket | |||
| ret = os_sendfile(fd1, fd0, 0, len); // works on linux >= 2.6.33, else fd1 would need to be a socket | |||
There was a problem hiding this comment.
this code path is not called any more on macintosh.
src/core/fs.h
Outdated
| @@ -206,8 +222,8 @@ fs_basedir( | |||
| snprintf(basedir, maxlen, "%s", path); | |||
| fs_dirname(basedir); | |||
| #elif defined(__APPLE__) | |||
| pid_t pid = getpid(); | |||
| proc_pidpath (pid, basedir, maxlen); | |||
| uint32_t len = maxlen; | |||
src/core/fs.h
Outdated
| @@ -471,8 +487,12 @@ fs_createtime( | |||
| #else | |||
| struct stat statbuf = {0}; | |||
| stat(filename, &statbuf); | |||
| #ifdef __APPLE__ | |||
There was a problem hiding this comment.
i think this is obsolete now, the ifdef is in the else path of another ifdef APPLE, no?
src/db/thumbnails.c
Outdated
| #endif | ||
| else | ||
| { | ||
| char tmp[PATH_MAX]; | ||
| if(snprintf(tmp, sizeof(tmp), "%s/%s", dt_pipe.basedir, deffilename) >= PATH_MAX) | ||
| return VK_INCOMPLETE; | ||
| if(!stat(tmp, &statbuf)) | ||
| #ifdef __APPLE__ |
There was a problem hiding this comment.
i think all these are fine in master already.
src/qvk/qvk.c
Outdated
| @@ -156,6 +159,9 @@ qvk_init(const char *preferred_device_name, int preferred_device_id, int window) | |||
| VkInstanceCreateInfo inst_create_info = { | |||
| .sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO, | |||
| .pApplicationInfo = &vk_app_info, | |||
| #ifdef __APPLE__ | |||
There was a problem hiding this comment.
master has this portability extension already, patch is probably obsolete.
|
Thank you @hanatos for checking everything, I will revise everything singularly next days 👍🏻 |
On M1, arm64 is not recognized by compiler as possible march.
|
Hi @hanatos, I started from master in order to see which changes I had to preserve. Anyway, I had to do this: Otherwise, libvulkan was not found by the loader. |
|
@LucaZulberti looks like the way it should be is you copy the libvulkan to the vkdt bin/lib folder and add RPATH to point to this folder, here is an example that comes with Vulkan, it bundles the shared objects: |
| @@ -80,14 +80,14 @@ install-lib: install-mod Makefile src/core/version.h | |||
| RELEASE_FILES=$(shell echo src/core/version.h; git ls-files --recurse-submodules) | |||
| ifeq ($(VKDT_USE_RAWINPUT), 1) | |||
| RAWSPEED_DIR=$(shell ls -d src/pipe/modules/i-raw/rawspeed-*) | |||
| RELEASE_FILES+=$(shell cd $(RAWSPEED_DIR) && git ls-files | sed -e 's#^#$(RAWSPEED_DIR)/#') | |||
There was a problem hiding this comment.
did this actually lead to problems on your system? i would say these # pounds need no escaping because they are within ' single quotes, and this works as it is on my end.
edit: also see https://stackoverflow.com/questions/63512455/how-to-properly-escape-character-in-a-makefile
There was a problem hiding this comment.
Yes, I have this error on branch master:
$ make clean
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C .. clean
Makefile:48: [ ()]
Makefile:92: [ ()]
Makefile:87: *** unterminated call to function `shell': missing `)'. Stop.
make: *** [clean] Error 2
Line 87:
RELEASE_FILES+=$(shell cd $(MCRAW_DIR) && git ls-files | sed -e 's#^#$(MCRAW_DIR)/#')
There was a problem hiding this comment.
which version of make is this? this even worked on my tests with macos.
There was a problem hiding this comment.
the native version that comes with Mac is 3.81, installing a newer version with brew worked for me.
|
nice, this reduced diff now looks like it supposedly works "out of the box", except for the march switch in the default config. i propose we change it to "march=native" in the default and for package builds/github workflows overwrite it using a custom config.mk (as is the case for the linux version already) |
ah, thanks for the pointer. i suppose the next step in the macintosh journey is creating a disk image which takes care of all these bundling/rpath issues. maybe a good idea to take a look at the darktable one for inspiration. |
Yes, also Ansel repository has a set of scripts to automate the creation of the dmg image: |
Dear all, I appreciate your work here and would like to try porting vkDT to Mac OS.
I'm not an experienced Mac OS developer; I want to give it a try to use the software on my MacBook.
My setup:
Ventura 13.5Sequoia 15.1Dependency (to improve the list):
brew.brew, installllvmandlibompfor custom compilation of therawspeedlibrary.ToDo:
rawspeed: add controls for Apple target in 4894513imgui: cherry-picked modifications from @hanatos fork to upstream in c57af75glfw: use submodule to compile it with ImGuiUp to now, the commits are just a fast try to make it work.
I know there is a lot of rubbish from many points of view of programmers 😄
Thank you again for this software!