Conversation
jopadan
changed the title
|
Optional Future improvement: switching from global link_libraries() / include_directories() to target_link_libraries(osl PRIVATE ...) would avoid leaking flags to consumers. Only a nice-to-have, not a blocker. |
| lib/libpspmath/src/vfpu_quaternion_sample_hermite.c | ||
| lib/libpspmath/src/vfpu_quaternion_hermite_tangent.c | ||
| ) | ||
| set(GIFLIB_SOURCES |
There was a problem hiding this comment.
Good call removing the vendored giflib. Just one concern, if we switch to external giflib, we must ensure CMake fails explicitly when giflib is not found, rather than silently disabling GIF support. Suggest find_package(GIF REQUIRED) when GIF is part of OSL_IMAGE_FORMAT_LOADERS.
| list(APPEND OSL_IMAGE_FORMATS_SUPPORTED ${IMAGE_FORMAT}) | ||
| if(${IMAGE_FORMAT} IN_LIST OSL_IMAGE_FORMAT_LOADERS) | ||
| add_compile_definitions(-DOSL_IMAGE_LOADER_${IMAGE_FORMAT}) | ||
| include_directories(${${IMAGE_FORMAT}_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Nice move dynamically wiring loaders/writers per format. But GIF is missing in the wiring — I see PNG/JPEG handled, but no conditional block for giflib (e.g. ${GIF_INCLUDE_DIRS}, ${GIF_LIBRARIES}). If OSL_IMAGE_FORMAT_LOADERS contains GIF, we should add both add_compile_definitions(...) and target_link_libraries(... GIF::GIF or ${GIF_LIBRARIES}). Otherwise we risk unintentional loss of GIF support.
| PSP_FW_VERSION=371 | ||
|
|
||
| CFLAGS = -O2 -g -G0 -Wall | ||
| DEFINES := \ |
There was a problem hiding this comment.
All samples now explicitly link PNG/JPEG (-lpng -ljpeg -lz) 👍.
However GIF is missing (-DOSL_IMAGE_LOADER_GIF and -lgif). From a user perspective this looks like GIF support got dropped. I recommend either:
adding GIF defines + -lgif to the relevant samples, or providing at least one dedicated GIF example to prove the loader is still supported.
| $(LIB_DIR)/libintraFont/src/intraFont.o \ | ||
| $(LIB_DIR)/libintraFont/src/libccc.o | ||
|
|
||
| GIFLIBOBJS := \ |
There was a problem hiding this comment.
We removed GIFLIBOBJS and vendored includes, good cleanup. But -DOSL_IMAGE_LOADER_GIF is still defined while no -lgif is linked. This may cause linker errors or silently disable GIF. Suggest detecting giflib availability (via pkg-config or manual flag) and only enabling the macro + linking -lgif when it’s actually found.
There was a problem hiding this comment.
Yes, implementing IMG.cmake function for autotools or using an external image format library package like recoil, sail, or libvips should do the job.
| $(SOURCE_DIR)/splash/oslShowSplashScreen1.o \ | ||
| $(SOURCE_DIR)/splash/oslShowSplashScreen2.o \ | ||
| $(SOURCE_DIR)/mem/oslGetRamStatus.o | ||
| # $(SOURCE_DIR)/image/format/oslLoadImageFileGIF.o |
There was a problem hiding this comment.
Why is $(SOURCE_DIR)/image/format/oslLoadImageFileGIF.o commented out?
Just confirming this isn't accidental.
There was a problem hiding this comment.
Probably an intermediate state that should be fix by implementing IMG.cmake for autotools Makefile or an external image format library.
|
We no longer need the OSL_IMAGE_* macros. It's not the developer's responsibility to declare these in their projects, they are used only for internal validation of image support. |
2 participants
nonstringattribute-DOSL_IMAGE_LOADER_[FORMAT]flags-DOSL_IMAGE_WRITER_[FORMAT]flags