search

libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.99k stars 1.84k forks source link

SDL_getenv where SDL_GetHint should be used instead #7702

Closed icculus closed 3 months ago

icculus commented 1 year ago

src/audio/disk/SDL_diskaudio.c has this (and other things, too)...

const char *envr = SDL_getenv(DISKENVR_IODELAY);

It's not urgent atm, but these should probably be SDL_hints, which will still be usable as environment variables. It's probably worth grepping for "getenv" through the codebase before 3.2.0 and seeing which of these should change.

slouken commented 5 months ago

It turns out there's quite a few of these. I think for the most part we want to turn these into hints or remove them.

icculus commented 5 months ago

I think for the most part we want to turn these into hints or remove them.

Agreed. A lot of this is leftovers from 1.2, I suspect.

I'm going to assign myself, but feel free to steal this if you want to get to it first.

icculus commented 4 months ago

This is (probably) the list where we have to check into this. I've eliminated things that are probably meant to be environment variables (HOME on unix, DISPLAY on x11, etc). The test framework uses one that should stay as a environment var.

SDL_ASSERT should stay as one, as it's intended to automate tests.

Things like ngage and vita might just leave them as-is, as they are probably there for specific reasons (maybe?).

65 total.

src/video/x11/SDL_x11window.c:513:               !SDL_getenv("SDL_VIDEO_X11_VISUALID")) {
src/video/x11/SDL_x11opengl.c:175:        path = SDL_getenv("SDL_OPENGL_LIBRARY");
src/video/x11/SDL_x11modes.c:250:            const char *scale_str = SDL_getenv("GDK_SCALE");
src/video/x11/SDL_x11modes.c:267:    const char *visual_id = SDL_getenv("SDL_VIDEO_X11_VISUALID");
src/video/x11/SDL_x11video.c:493:    return (SDL_getenv("SDL_VIDEO_X11_NODIRECTCOLOR") == NULL);
src/video/x11/SDL_x11keyboard.c:172:        const char *env_xmods = SDL_getenv("XMODIFIERS");
src/video/x11/SDL_x11vulkan.c:61:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/x11/SDL_x11vulkan.c:110:        const char *libX11XCBLibraryName = SDL_getenv("SDL_X11_XCB_LIBRARY");
src/video/ngage/SDL_ngageframebuffer.cpp:157:    if (SDL_getenv("SDL_VIDEO_NGAGE_SAVE_FRAMES")) {
src/video/kmsdrm/SDL_kmsdrmvulkan.c:59:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/SDL_egl.c:199:    ext_override = SDL_getenv(ext);
src/video/SDL_egl.c:344:    path = SDL_getenv("SDL_VIDEO_GL_DRIVER");
src/video/SDL_egl.c:404:        path = SDL_getenv("SDL_VIDEO_EGL_DRIVER");
src/video/cocoa/SDL_cocoavulkan.m:64:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/cocoa/SDL_cocoaopengl.m:232:        path = SDL_getenv("SDL_OPENGL_LIBRARY");
src/video/SDL_vulkan_utils.c:212:    chosenDisplayId = SDL_getenv("SDL_VULKAN_DISPLAY");
src/video/uikit/SDL_uikitvulkan.m:62:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/offscreen/SDL_offscreenframebuffer.c:65:    if (SDL_getenv("SDL_VIDEO_OFFSCREEN_SAVE_FRAMES")) {
src/video/offscreen/SDL_offscreenvulkan.c:77:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/vita/SDL_vitagles_pvr.c:38:    char *override = SDL_getenv("VITA_MODULE_PATH");
src/video/vita/SDL_vitagles_pvr.c:39:    char *skip_init = SDL_getenv("VITA_PVR_SKIP_INIT");
src/video/vita/SDL_vitagl_pvr.c:49:    char *override = SDL_getenv("VITA_MODULE_PATH");
src/video/vita/SDL_vitagl_pvr.c:50:    char *skip_init = SDL_getenv("VITA_PVR_SKIP_INIT");
src/video/vita/SDL_vitatouch.c:49:    disableFrontPoll = SDL_getenv("VITA_DISABLE_TOUCH_FRONT");
src/video/vita/SDL_vitatouch.c:50:    disableBackPoll = SDL_getenv("VITA_DISABLE_TOUCH_BACK");
src/video/vita/SDL_vitavideo.c:128:    if (SDL_getenv("VITA_PVR_OGL") != NULL) {
src/video/vita/SDL_vitavideo.c:173:    char *res = SDL_getenv("VITA_RESOLUTION");
src/video/vita/SDL_vitavideo.c:264:        if (SDL_getenv("VITA_PVR_OGL") != NULL) {
src/video/vita/SDL_vitavideo.c:278:        if (SDL_getenv("VITA_PVR_OGL") != NULL) {
src/video/android/SDL_androidvulkan.c:52:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/dummy/SDL_nullframebuffer.c:64:    if (SDL_getenv("SDL_VIDEO_DUMMY_SAVE_FRAMES")) {
src/video/windows/SDL_windowsopengl.c:113:        path = SDL_getenv("SDL_OPENGL_LIBRARY");
src/video/windows/SDL_windowsvulkan.c:52:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/wayland/SDL_waylandvideo.c:385:    if (!SDL_getenv("WAYLAND_DISPLAY")) {
src/video/wayland/SDL_waylandvideo.c:386:        const char *session = SDL_getenv("XDG_SESSION_TYPE");
src/video/wayland/SDL_waylandwindow.c:1874:        const char *activation_token = SDL_getenv("XDG_ACTIVATION_TOKEN");
src/video/wayland/SDL_waylandmouse.c:335:        const char *xcursor_size = SDL_getenv("XCURSOR_SIZE");
src/video/wayland/SDL_waylandmouse.c:372:            xcursor_theme = SDL_getenv("XCURSOR_THEME");
src/video/wayland/SDL_waylandvulkan.c:57:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/video/wayland/SDL_waylandshmbuffer.c:83:        xdg_path = SDL_getenv("XDG_RUNTIME_DIR");
src/video/SDL_blit_N.c:881:        char *override = SDL_getenv("SDL_ALTIVEC_BLIT_FEATURES");
src/video/SDL_video.c:4195:    start = SDL_getenv(extension);
src/video/vivante/SDL_vivantevulkan.c:50:        path = SDL_getenv("SDL_VULKAN_LIBRARY");
src/core/freebsd/SDL_evdev_kbd_freebsd.c:268:        if (SDL_getenv("SDL_INPUT_FREEBSD_KEEP_KBD") == NULL) {
src/core/linux/SDL_ime.c:47:    const char *im_module = SDL_getenv("SDL_IM_MODULE");
src/core/linux/SDL_ime.c:48:    const char *xmodifiers = SDL_getenv("XMODIFIERS");
src/core/linux/SDL_evdev_kbd.c:477:        if (SDL_getenv("SDL_INPUT_LINUX_KEEP_KBD") == NULL) {
src/dynapi/SDL_dynapi.c:354:        const char *env = SDL_getenv_REAL("SDL_DYNAPI_LOG_CALLS");
src/audio/SDL_audio.c:1547:        const char *env = SDL_getenv("SDL_AUDIO_FREQUENCY");  // !!! FIXME: should be a hint?
src/audio/SDL_audio.c:1558:        const char *env = SDL_getenv("SDL_AUDIO_CHANNELS");
src/audio/SDL_audio.c:1568:        const SDL_AudioFormat val = ParseAudioFormatString(SDL_getenv("SDL_AUDIO_FORMAT"));
src/audio/sndio/SDL_sndioaudio.c:235:    const char *audiodev = SDL_getenv("AUDIODEV");
src/audio/dummy/SDL_dummyaudio.c:39:    const char *envr = SDL_getenv(DUMMYENVR_IODELAY);
src/audio/disk/SDL_diskaudio.c:101:    const char *devname = SDL_getenv(recording ? DISKENVR_INFILE : DISKENVR_OUTFILE);
src/audio/disk/SDL_diskaudio.c:112:    const char *envr = SDL_getenv(DISKENVR_IODELAY);
src/audio/SDL_audiodev.c:90:    audiodev = SDL_getenv("SDL_PATH_DSP");
src/audio/SDL_audiodev.c:92:        audiodev = SDL_getenv("AUDIODEV");
src/audio/alsa/SDL_alsa_audio.c:231:        const char *device = SDL_getenv("AUDIODEV"); // Is there a standard variable name?
src/audio/alsa/SDL_alsa_audio.c:429:    if (SDL_getenv("SDL_AUDIO_ALSA_DEBUG")) {
src/audio/SDL_wave.c:1790:    envchunkcountlimit = SDL_getenv("SDL_WAVE_CHUNK_LIMIT");
src/render/opengl/SDL_render_gl.c:1722:    hint = SDL_getenv("GL_ARB_texture_non_power_of_two");
src/hidapi/SDL_hidapi.c:1143:    if (SDL_getenv("SDL_HIDAPI_JOYSTICK_DISABLE_UDEV") != NULL) {
src/hidapi/SDL_hidapi.c:1161:    if (SDL_getenv("SDL_HIDAPI_DISABLE_LIBUSB") != NULL) {
src/haptic/SDL_haptic.c:543:    env = SDL_getenv("SDL_HAPTIC_GAIN_MAX");
src/joystick/hidapi/SDL_hidapijoystick.c:582:        if (SDL_getenv("SDL_HIDAPI_JOYSTICK_DISABLE_UDEV") != NULL) {
smcv commented 3 months ago

I believe these X11, Wayland and freedesktop.org interactions should probably stay plain environment variables, like HOME and PATH, because that's their standard behaviour across all libraries:

src/video/x11/SDL_x11keyboard.c:172:        const char *env_xmods = SDL_getenv("XMODIFIERS");
src/video/wayland/SDL_waylandvideo.c:385:    if (!SDL_getenv("WAYLAND_DISPLAY")) {
src/video/wayland/SDL_waylandvideo.c:386:        const char *session = SDL_getenv("XDG_SESSION_TYPE");
src/video/wayland/SDL_waylandwindow.c:1874:        const char *activation_token = SDL_getenv("XDG_ACTIVATION_TOKEN");
src/video/wayland/SDL_waylandmouse.c:335:        const char *xcursor_size = SDL_getenv("XCURSOR_SIZE");
src/video/wayland/SDL_waylandmouse.c:372:            xcursor_theme = SDL_getenv("XCURSOR_THEME");
src/core/linux/SDL_ime.c:48:    const char *xmodifiers = SDL_getenv("XMODIFIERS");

This also seems to be somewhat common for OSS audio (typically /dev/dsp) if we still care about that in 2024: according to Debian code search e.g. GStreamer, Python and Wine also read it, with indications that it might be originally a Solaris/SunOS thing:

src/audio/SDL_audiodev.c:92:        audiodev = SDL_getenv("AUDIODEV");

However, these seem suspicious to me, because if AUDIODEV=/dev/dsp is a sensible thing to do on OSS systems, then it seems wrong for the same environment variable to be checked by other audio APIs like ALSA (Linux) and SNDIO (originally OpenBSD I think), which don't have the same API to devices or even the same device naming:

src/audio/sndio/SDL_sndioaudio.c:235:    const char *audiodev = SDL_getenv("AUDIODEV");
src/audio/alsa/SDL_alsa_audio.c:231:        const char *device = SDL_getenv("AUDIODEV"); // Is there a standard variable name?

Probably ALSA should be using some different environment variable instead, either a standard one from libasound if one exists, or a SDL-specific hint instead if not? Looking for getenv in alsa-lib, and in a couple of other audio abstraction layer libraries (openal-soft and portaudio19), I don't see any obvious environment variables jump out at me as "this is probably meant to be the default device name".

And maybe SNDIO should be using a different variable, too? For the original implementation, I'm afraid you'd have to ask someone who uses OpenBSD; but there's a port of libsndio to Linux available in Debian, and that seems to use AUDIORECDEVICE, AUDIOPLAYDEVICE and AUDIODEVICE.

smcv commented 3 months ago

Probably ALSA should be using some different environment variable instead, either a standard one from libasound if one exists, or a SDL-specific hint instead if not?

Ah, I didn't see a getenv because ALSA's interactions with environment variables are part of its configuration language. According to https://wiki.archlinux.org/title/Advanced_Linux_Sound_Architecture there are various environment variables that alter the meaning of pcm.hw, pcm.default and similar aliases - but maybe it would be better for SDL to default to using one of those aliases, and outsourcing all the interactions with environment variables to the ALSA client library.

slouken commented 3 months ago

I believe these X11, Wayland and freedesktop.org interactions should probably stay plain environment variables, like HOME and PATH, because that's their standard behaviour across all libraries:

src/video/x11/SDL_x11keyboard.c:172:        const char *env_xmods = SDL_getenv("XMODIFIERS");
src/video/wayland/SDL_waylandvideo.c:385:    if (!SDL_getenv("WAYLAND_DISPLAY")) {
src/video/wayland/SDL_waylandvideo.c:386:        const char *session = SDL_getenv("XDG_SESSION_TYPE");
src/video/wayland/SDL_waylandwindow.c:1874:        const char *activation_token = SDL_getenv("XDG_ACTIVATION_TOKEN");
src/video/wayland/SDL_waylandmouse.c:335:        const char *xcursor_size = SDL_getenv("XCURSOR_SIZE");
src/video/wayland/SDL_waylandmouse.c:372:            xcursor_theme = SDL_getenv("XCURSOR_THEME");
src/core/linux/SDL_ime.c:48:    const char *xmodifiers = SDL_getenv("XMODIFIERS");

Yep, I left these alone.

This also seems to be somewhat common for OSS audio (typically /dev/dsp) if we still care about that in 2024: according to Debian code search e.g. GStreamer, Python and Wine also read it, with indications that it might be originally a Solaris/SunOS thing:

src/audio/SDL_audiodev.c:92:        audiodev = SDL_getenv("AUDIODEV");

Yep, I left this alone as well.

However, these seem suspicious to me, because if AUDIODEV=/dev/dsp is a sensible thing to do on OSS systems, then it seems wrong for the same environment variable to be checked by other audio APIs like ALSA (Linux) and SNDIO (originally OpenBSD I think), which don't have the same API to devices or even the same device naming:

src/audio/sndio/SDL_sndioaudio.c:235:    const char *audiodev = SDL_getenv("AUDIODEV");
src/audio/alsa/SDL_alsa_audio.c:231:        const char *device = SDL_getenv("AUDIODEV"); // Is there a standard variable name?

I agree, and removed these.

Probably ALSA should be using some different environment variable instead, either a standard one from libasound if one exists, or a SDL-specific hint instead if not? Looking for getenv in alsa-lib, and in a couple of other audio abstraction layer libraries (openal-soft and portaudio19), I don't see any obvious environment variables jump out at me as "this is probably meant to be the default device name".

I added SDL_HINT_AUDIO_ALSA_DEFAULT_DEVICE because SDL internally will map to surround sound devices if the default is selected but 4 or 6 channel audio is requested.

And maybe SNDIO should be using a different variable, too? For the original implementation, I'm afraid you'd have to ask someone who uses OpenBSD; but there's a port of libsndio to Linux available in Debian, and that seems to use AUDIORECDEVICE, AUDIOPLAYDEVICE and AUDIODEVICE.

Yep, SDL will open the device with SIO_DEVANY, and sndio will check these environment variables internally.

smcv commented 3 months ago

SDL will open the device with SIO_DEVANY, and sndio will check these environment variables internally

That seems ideal: for backends that have an API for "please open your default device, whatever that is", calling into it (when feasible) seems like a cleaner approach than having SDL second-guess how the device is meant to be chosen.