SDL3 _audio module
#3657
SDL3 _audio module
#3657
Conversation
Starbuck5
added
New API
📝 WalkthroughWalkthroughAdds an SDL3-targeted audio subsystem: new type stubs, a C extension exposing device/stream state, a Python high-level wrapper, and Meson build updates to include the module and adjust mixer targets for SDL3. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant PyAPI as Python Audio API (_audio.py)
participant CExt as C Backend (_base_audio.c)
participant SDL3 as SDL3 Audio Subsystem
Note over App,SDL3: Device discovery & opening
App->>PyAPI: get_playback_devices()
PyAPI->>CExt: pg_audio_get_playback_device_states()
CExt->>SDL3: query output devices
SDL3-->>CExt: device list
CExt-->>PyAPI: device state list
PyAPI-->>App: AudioDevice[]
App->>PyAPI: device.open(spec)
PyAPI->>CExt: open device with AudioSpec
CExt->>SDL3: open device handle
SDL3-->>CExt: device handle
CExt-->>PyAPI: LogicalAudioDevice
PyAPI-->>App: LogicalAudioDevice
sequenceDiagram
actor App as Application
participant PyAPI as Python Audio API (_audio.py)
participant CExt as C Backend (_base_audio.c)
participant SDL3 as SDL3 Audio Subsystem
Note over App,SDL3: Stream lifecycle & data flow
App->>PyAPI: AudioStream(src_spec, dst_spec)
PyAPI->>CExt: create AudioStreamState
CExt->>SDL3: create audio stream
SDL3-->>CExt: stream handle
CExt-->>PyAPI: AudioStream
App->>PyAPI: stream.bind(device)
PyAPI->>CExt: bind stream to device
CExt->>SDL3: bind stream to device handle
SDL3-->>CExt: result
CExt-->>PyAPI: bound / error
App->>PyAPI: stream.put_data(bytes)
PyAPI->>CExt: queue data
CExt->>SDL3: feed stream data
SDL3-->>CExt: queued bytes
CExt-->>PyAPI: ack
PyAPI-->>App: queued bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-09-01T20:18:57.500ZApplied to files:
📚 Learning: 2025-10-17T08:53:32.449ZApplied to files:
📚 Learning: 2025-09-01T20:22:31.010ZApplied to files:
📚 Learning: 2025-08-17T17:23:06.787ZApplied to files:
📚 Learning: 2025-10-08T08:15:01.411ZApplied to files:
📚 Learning: 2025-08-13T12:39:19.130ZApplied to files:
📚 Learning: 2025-08-30T21:11:00.240ZApplied to files:
🧬 Code graph analysis (2)src_c/_base_audio.c (1)
src_py/_audio.py (2)
🪛 Flake8 (7.3.0)src_py/_audio.py[error] 339-339: undefined name 'weakref' (F821) 🪛 Ruff (0.14.8)src_py/_audio.py79-81: Avoid specifying long messages outside the exception class (TRY003) 84-84: Avoid specifying long messages outside the exception class (TRY003) 128-131: Avoid specifying long messages outside the exception class (TRY003) 174-176: Avoid specifying long messages outside the exception class (TRY003) 178-180: Avoid specifying long messages outside the exception class (TRY003) 194-196: Avoid specifying long messages outside the exception class (TRY003) 250-252: Avoid specifying long messages outside the exception class (TRY003) 258-260: Avoid specifying long messages outside the exception class (TRY003) 282-284: Avoid specifying long messages outside the exception class (TRY003) 290-292: Avoid specifying long messages outside the exception class (TRY003) 305-305: Avoid specifying long messages outside the exception class (TRY003) 316-318: Avoid specifying long messages outside the exception class (TRY003) 339-339: Undefined name (F821) 353-356: Avoid specifying long messages outside the exception class (TRY003) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
src_c/_base_audio.c (1)
57-61: Potential type mismatch foridmember.
SDL_AudioDeviceIDis defined asUint32(unsigned 32-bit), butPy_T_INTis a signed int. This could cause issues if SDL returns device IDs with the high bit set, as they would appear as negative values in Python.Consider using
Py_T_UINTinstead:static PyMemberDef adevice_state_members[] = { - {"id", Py_T_INT, offsetof(PGAudioDeviceStateObject, devid), Py_READONLY, + {"id", Py_T_UINT, offsetof(PGAudioDeviceStateObject, devid), Py_READONLY, NULL}, {NULL} /* Sentinel */ };buildconfig/stubs/pygame/_audio.pyi (1)
1-2: Unused imports.
CallableandTypeVarare imported but currently unused since the callback-related type aliases (lines 26-29) are commented out.Consider removing these imports until the callback functionality is implemented:
-from collections.abc import Callable -from typing import TypeVar - from pygame.typing import FileLikesrc_py/_audio.py (2)
22-22: ConsiderClassVarannotation for mutable class attribute.The
format_num_to_instancedict is intentionally class-level and shared. AddingClassVarclarifies intent and satisfies type checkers.+from typing import ClassVar + class AudioFormat: # ... - format_num_to_instance = dict() + format_num_to_instance: ClassVar[dict[int, "AudioFormat"]] = {}
7-10: Acknowledged: TODO items for documentation, tests, and quit safety.The noted concerns about making
quit()safe are valid - SDL objects become invalid after subsystem quit. Consider implementing reference tracking or invalidation callbacks before exposingquit()publicly.Would you like me to help design a pattern for safely invalidating Python wrapper objects when the audio subsystem is quit?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buildconfig/stubs/pygame/_audio.pyi(1 hunks)src_c/_base_audio.c(1 hunks)src_c/meson.build(1 hunks)src_py/_audio.py(1 hunks)src_py/meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3219
File: src_c/window.c:11-13
Timestamp: 2025-10-17T08:53:32.449Z
Learning: SDL3's first stable release is version 3.2.0 (released January 21, 2025). SDL 3.0.x and 3.1.x were preview/pre-release versions not intended for production use, so version checks for SDL3 features in pygame-ce should use SDL_VERSION_ATLEAST(3, 2, 0) to target the first stable release.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Learnt from: ankith26
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:520-529
Timestamp: 2025-08-17T17:23:06.787Z
Learning: In pygame-ce's mixer.c, when fixing reference leaks in mixer_quit, the cleanest approach is to first call Mix_ChannelFinished(NULL) to disable the callback and prevent race conditions, then manually handle Py_XDECREF calls for channeldata[i].sound and channeldata[i].queue after the Mix_HaltGroup calls.
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_py/meson.buildsrc_c/meson.build
📚 Learning: 2025-10-17T08:53:32.449Z
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3219
File: src_c/window.c:11-13
Timestamp: 2025-10-17T08:53:32.449Z
Learning: SDL3's first stable release is version 3.2.0 (released January 21, 2025). SDL 3.0.x and 3.1.x were preview/pre-release versions not intended for production use, so version checks for SDL3 features in pygame-ce should use SDL_VERSION_ATLEAST(3, 2, 0) to target the first stable release.
Applied to files:
src_py/meson.build
🧬 Code graph analysis (2)
src_c/_base_audio.c (1)
src_c/base.c (2)
import_pygame_base(2294-2297)import_pygame_rwobject(2343-2346)
buildconfig/stubs/pygame/_audio.pyi (1)
src_py/_audio.py (57)
init(358-359)get_init(367-368)get_current_driver(371-372)get_drivers(375-376)get_playback_devices(402-406)AudioDevice(139-172)get_recording_devices(409-413)load_wav(416-421)AudioSpec(99-136)AudioFormat(13-76)bitsize(31-32)bytesize(35-36)is_float(39-40)is_int(43-44)is_big_endian(47-48)is_little_endian(51-52)is_signed(55-56)is_unsigned(59-60)name(63-64)name(167-168)silence_value(67-68)format(117-118)channels(121-122)frequency(125-126)framesize(129-130)open(145-159)LogicalAudioDevice(175-192)is_playback(162-163)channel_map(171-172)pause(176-177)resume(179-180)paused(183-184)gain(187-188)gain(191-192)gain(323-324)gain(327-331)AudioStream(195-355)src_spec(266-269)src_spec(272-288)dst_spec(291-301)dst_spec(304-320)bind(216-223)device(262-263)unbind(225-227)clear(229-230)flush(232-233)num_available_bytes(236-237)num_queued_bytes(240-241)get_data(243-244)put_data(248-249)pause_device(251-252)resume_device(254-255)device_paused(258-259)frequency_ratio(334-335)frequency_ratio(338-342)lock(344-345)unlock(347-348)
🪛 Ruff (0.14.8)
src_py/_audio.py
22-22: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
102-104: Avoid specifying long messages outside the exception class
(TRY003)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
153-155: Avoid specifying long messages outside the exception class
(TRY003)
198-200: Avoid specifying long messages outside the exception class
(TRY003)
202-204: Avoid specifying long messages outside the exception class
(TRY003)
218-220: Avoid specifying long messages outside the exception class
(TRY003)
274-276: Avoid specifying long messages outside the exception class
(TRY003)
282-284: Avoid specifying long messages outside the exception class
(TRY003)
306-308: Avoid specifying long messages outside the exception class
(TRY003)
314-316: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: Pyodide build
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-latest)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: i686
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: x86_64
- GitHub Check: aarch64
🔇 Additional comments (4)
src_py/meson.build (1)
31-34: LGTM!The conditional installation of
_audio.pyfor SDL3 is correctly implemented and follows the existing pattern used elsewhere in the build system.src_c/meson.build (2)
423-433: LGTM!The new
_base_audioextension module is correctly defined following the established pattern for SDL3-specific modules.
435-457: LGTM!The nested guard correctly disables the legacy mixer targets when building for SDL3, aligning with the new audio subsystem approach.
buildconfig/stubs/pygame/_audio.pyi (1)
17-17: Return type ofload_wavneeds verification against implementation.The stub at line 17 declares the return type as
tuple[AudioSpec, bytes]. Verify whether the implementation insrc_py/_audio.pyactually returns a tuple or a list, and align the stub accordingly if there is a mismatch.
Starbuck5
force-pushed
the
sdl3_mixer_api3
branch
from
b552c40 to
3cf92aa
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src_c/_base_audio.c (2)
472-500: Guard against oversized buffers before castingPy_ssize_ttoint
pg_audio_put_audio_stream_datanow correctly usesPy_ssize_t len, but it still blindly casts tointwhen callingSDL_PutAudioStreamData. Very large buffers (>INT_MAX) will silently truncate and could misbehave.Consider an explicit range check:
- if (!SDL_PutAudioStreamData(stream, buf, (int)len)) { + if (len > INT_MAX) { + Py_DECREF(bytes); + return RAISE(pgExc_SDLError, "audio buffer too large"); + } + if (!SDL_PutAudioStreamData(stream, buf, (int)len)) {This keeps the SDL call well‑defined and surfaces a clear Python‑level error.
1029-1065: Tighten error handling for type creation to avoid small leaks
pg_audio_execcorrectly checks bothPyType_FromModuleAndSpeccalls for NULL, but ifPyModule_AddTypefails you return-1without DECREF’ing the just-created type object or clearing state. This only leaks a small reference during import failure, but it’s easy to clean up:- if (PyModule_AddType(module, - (PyTypeObject *)state->audio_device_state_type) < 0) { - return -1; - } + if (PyModule_AddType(module, + (PyTypeObject *)state->audio_device_state_type) < 0) { + Py_CLEAR(state->audio_device_state_type); + return -1; + } ... - if (PyModule_AddType(module, - (PyTypeObject *)state->audio_stream_state_type) < 0) { - return -1; - } + if (PyModule_AddType(module, + (PyTypeObject *)state->audio_stream_state_type) < 0) { + Py_CLEAR(state->audio_stream_state_type); + return -1; + }Not urgent, but it aligns module init with the rest of pygame‑ce’s defensive patterns. Based on learnings, consistency with existing C modules is usually preferred.
src_py/_audio.py (2)
23-30: Annotate the registry dict as a class variable and consider safer lookup
AudioFormat._format_num_to_instanceis a mutable class attribute populated from__init__and later used by_audio_spec_from_ints. Two suggestions:
- Make its intent explicit and satisfy Ruff (RUF012) by annotating it as a
ClassVar:from typing import ClassVar class AudioFormat: _format_num_to_instance: ClassVar[dict[int, "AudioFormat"]] = {}
- For
_audio_spec_from_ints, consider turning the bare index into a clearer error or a fallback:- format_inst = AudioFormat._format_num_to_instance[format_num] + try: + format_inst = AudioFormat._format_num_to_instance[format_num] + except KeyError as exc: + raise pygame.error(f"Unknown audio format {format_num!r}") from excNot mandatory, but it will produce a more helpful error if SDL ever returns an unexpected format code.
Also applies to: 383-401
336-344: Clarify frequency ratio validation messageThe check
if value <= 0 or value > 10:correctly enforces0 < ratio <= 10, but the message could be more direct and matches Ruff’s TRY003 suggestion better if shortened, e.g.:- if value <= 0 or value > 10: - raise ValueError( - "Frequency ratio must be between 0 and 10. (0 < ratio <= 10)" - ) + if value <= 0 or value > 10: + raise ValueError("Frequency ratio must be > 0 and <= 10.")Purely cosmetic, but improves readability.
buildconfig/stubs/mypy_allow_list.txt (1)
31-32: Escape the dot in the allowlist regex for consistencyThe new allowlist entry is
pygame._audio, but these patterns are regexes and existing entries escape the module dot (e.g.pygame\.ftfont). To avoid matching unintended names, consider:-pygame._audio +pygame\._audioFunctionally this likely makes no difference today, but it keeps the pattern style consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buildconfig/stubs/meson.build(1 hunks)buildconfig/stubs/mypy_allow_list.txt(1 hunks)buildconfig/stubs/pygame/_audio.pyi(1 hunks)src_c/_base_audio.c(1 hunks)src_py/_audio.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
📚 Learning: 2025-10-17T08:53:32.449Z
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3219
File: src_c/window.c:11-13
Timestamp: 2025-10-17T08:53:32.449Z
Learning: SDL3's first stable release is version 3.2.0 (released January 21, 2025). SDL 3.0.x and 3.1.x were preview/pre-release versions not intended for production use, so version checks for SDL3 features in pygame-ce should use SDL_VERSION_ATLEAST(3, 2, 0) to target the first stable release.
Applied to files:
buildconfig/stubs/mypy_allow_list.txt
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
buildconfig/stubs/mypy_allow_list.txtsrc_c/_base_audio.c
📚 Learning: 2025-09-01T20:22:31.010Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.
Applied to files:
buildconfig/stubs/mypy_allow_list.txt
📚 Learning: 2025-08-17T17:23:06.787Z
Learnt from: ankith26
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:520-529
Timestamp: 2025-08-17T17:23:06.787Z
Learning: In pygame-ce's mixer.c, when fixing reference leaks in mixer_quit, the cleanest approach is to first call Mix_ChannelFinished(NULL) to disable the callback and prevent race conditions, then manually handle Py_XDECREF calls for channeldata[i].sound and channeldata[i].queue after the Mix_HaltGroup calls.
Applied to files:
buildconfig/stubs/mypy_allow_list.txtsrc_c/_base_audio.c
📚 Learning: 2025-10-08T08:15:01.411Z
Learnt from: ankith26
Repo: pygame-community/pygame-ce PR: 3602
File: buildconfig/stubs/pygame/locals.pyi:601-1198
Timestamp: 2025-10-08T08:15:01.411Z
Learning: The repository pygame-community/pygame-ce is for pygame-ce (pygame community edition), not the original pygame project. Always refer to it as pygame-ce in reviews and analyses.
Applied to files:
buildconfig/stubs/mypy_allow_list.txt
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/_base_audio.c
🧬 Code graph analysis (2)
src_c/_base_audio.c (2)
src_py/_audio.py (7)
name(64-65)name(169-170)gain(189-190)gain(193-194)gain(325-326)gain(329-333)device(264-265)src_c/base.c (2)
import_pygame_base(2294-2297)import_pygame_rwobject(2343-2346)
src_py/_audio.py (3)
src_c/base.c (1)
base(2116-2277)src_c/_base_audio.c (1)
_base_audio(1102-1121)buildconfig/stubs/pygame/base.pyi (1)
get_sdl_byteorder(18-18)
🪛 Ruff (0.14.8)
src_py/_audio.py
23-23: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
103-105: Avoid specifying long messages outside the exception class
(TRY003)
108-108: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
200-202: Avoid specifying long messages outside the exception class
(TRY003)
204-206: Avoid specifying long messages outside the exception class
(TRY003)
220-222: Avoid specifying long messages outside the exception class
(TRY003)
276-278: Avoid specifying long messages outside the exception class
(TRY003)
284-286: Avoid specifying long messages outside the exception class
(TRY003)
308-310: Avoid specifying long messages outside the exception class
(TRY003)
316-318: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
342-344: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
buildconfig/stubs/pygame/_audio.pyi (1)
1-164: Stub surface matches implementation; unused imports are acceptableThe
_audiostubs line up with the current Python API (classes, methods, andload_wavreturn type).Callable/TypeVarare only used in commented-out callback signatures, which is harmless and keeps future plans documented.No changes needed here.
buildconfig/stubs/meson.build (1)
1-6: Stub install gating for_audio.pyilooks correctThe new
pg_stub_excludeslist andif sdl_api != 3guard ensure_audio.pyiis only installed for SDL3 builds while still excluding.flake8. This matches the SDL3‑only nature of the new audio API.No changes needed.
Also applies to: 8-14
src_c/_base_audio.c (2)
215-259: AudioFormat objects passed from Python will causePyLong_AsIntto failIn
pg_audio_open_audio_device,pg_audio_create_audio_stream, andpg_audio_set_audio_stream_formatyou callPyLong_AsInton arguments that the Python layer currently passes asAudioFormatinstances, notints.PyLong_AsIntdoes not honor__index__, so these calls will raiseTypeErrorinstead of extracting the underlying format number, breakingAudioDevice.open()andAudioStreamcreation/format setters.Consider one of:
- In C: accept any indexable object:
PyObject *fmt_obj = args[1]; PyObject *idx = PyNumber_Index(fmt_obj); if (idx == NULL) { return NULL; } spec.format = PyLong_AsInt(idx); Py_DECREF(idx); if (spec.format == -1 && PyErr_Occurred()) { return NULL; }(and similarly for the other format fields), or
- In Python: pass
int(spec.format)instead ofspec.formateverywhere you call into these C APIs.Right now, without this adjustment, the core stream/device-opening API can't be used successfully.
Also applies to: 299-344, 563-603
762-788: No error handling needed forSDL_GetNumAudioDrivers()
SDL_GetNumAudioDrivers()does not return negative values on error. According to SDL documentation, it returns the count of available audio drivers or 0 if none are compiled in. UnlikeSDL_GetNumAudioDevices(), this function has no error path that produces a negative return value. The suggested check is unnecessary.Likely an incorrect or invalid review comment.
src_py/_audio.py (1)
146-157: Pass integer format codes into_base_audio, notAudioFormatinstances
AudioDevice.open,AudioStream.__init__, and thesrc_spec/dst_specsetters passAudioFormatobjects (spec.format,value.format) into_base_audio.*calls. The C layer usesPyLong_AsInton these arguments, which only accepts actualintobjects and will raiseTypeErrorforAudioFormatinstances.To keep the
AudioFormatabstraction while making the calls work, convert tointbefore passing to the C backend:- dev_state = _base_audio.open_audio_device( - self._state, spec.format, spec.channels, spec.frequency - ) + dev_state = _base_audio.open_audio_device( + self._state, int(spec.format), spec.channels, spec.frequency + ) ... - self._state = _base_audio.create_audio_stream( - src_spec.format, + self._state = _base_audio.create_audio_stream( + int(src_spec.format), src_spec.channels, src_spec.frequency, - dst_spec.format, + int(dst_spec.format), dst_spec.channels, dst_spec.frequency, ) ... - _base_audio.set_audio_stream_format( - self._state, (value.format, value.channels, value.frequency), None + _base_audio.set_audio_stream_format( + self._state, + (int(value.format), value.channels, value.frequency), + None, ) ... - _base_audio.set_audio_stream_format( - self._state, None, (value.format, value.channels, value.frequency) + _base_audio.set_audio_stream_format( + self._state, + None, + (int(value.format), value.channels, value.frequency), )Also applies to: 197-215, 274-291, 305-322
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src_py/_audio.py (1)
241-277: Consider avoiding duplicateget_audio_stream_formatcallsBoth
src_specanddst_speccall_base_audio.get_audio_stream_format(self._state)separately and then slice the tuple. You could optionally factor this into a small helper (or cache within the getter) that calls into C once and returns both specs, to avoid redundant C calls and tuple allocations, especially if these properties are accessed frequently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
buildconfig/stubs/mypy_allow_list.txt(1 hunks)src_c/_base_audio.c(1 hunks)src_py/_audio.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buildconfig/stubs/mypy_allow_list.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-17T17:23:06.787Z
Learnt from: ankith26
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:520-529
Timestamp: 2025-08-17T17:23:06.787Z
Learning: In pygame-ce's mixer.c, when fixing reference leaks in mixer_quit, the cleanest approach is to first call Mix_ChannelFinished(NULL) to disable the callback and prevent race conditions, then manually handle Py_XDECREF calls for channeldata[i].sound and channeldata[i].queue after the Mix_HaltGroup calls.
Applied to files:
src_c/_base_audio.c
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/_base_audio.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/_base_audio.c
🪛 Flake8 (7.3.0)
src_py/_audio.py
[error] 339-339: undefined name 'weakref'
(F821)
🪛 Ruff (0.14.8)
src_py/_audio.py
79-81: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
128-131: Avoid specifying long messages outside the exception class
(TRY003)
174-176: Avoid specifying long messages outside the exception class
(TRY003)
178-180: Avoid specifying long messages outside the exception class
(TRY003)
194-196: Avoid specifying long messages outside the exception class
(TRY003)
250-252: Avoid specifying long messages outside the exception class
(TRY003)
258-260: Avoid specifying long messages outside the exception class
(TRY003)
282-284: Avoid specifying long messages outside the exception class
(TRY003)
290-292: Avoid specifying long messages outside the exception class
(TRY003)
305-305: Avoid specifying long messages outside the exception class
(TRY003)
316-318: Avoid specifying long messages outside the exception class
(TRY003)
339-339: Undefined name weakref
(F821)
353-356: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Pyodide build
- GitHub Check: dev-check
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
🔇 Additional comments (1)
src_py/_audio.py (1)
63-65: Return type and behavior ofsilence_valuelook correctThe property now returns
bytesand delegates to the C helper, matching the stub and C implementation. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src_c/_base_audio.c (1)
575-616: Consider adding NULL checks forPyTuple_GetItemresults.While the Python wrapper likely validates that
args[1]andargs[2]are tuples with three elements, adding NULL checks afterPyTuple_GetItemcalls (lines 591-593, 601-603) would make the code more defensive and easier to debug if called incorrectly.Example for the first tuple:
if (!Py_IsNone(args[1])) { - src.format = PyLong_AsInt(PyTuple_GetItem(args[1], 0)); - src.channels = PyLong_AsInt(PyTuple_GetItem(args[1], 1)); - src.freq = PyLong_AsInt(PyTuple_GetItem(args[1], 2)); + PyObject *item0 = PyTuple_GetItem(args[1], 0); + PyObject *item1 = PyTuple_GetItem(args[1], 1); + PyObject *item2 = PyTuple_GetItem(args[1], 2); + if (item0 == NULL || item1 == NULL || item2 == NULL) { + return NULL; + } + src.format = PyLong_AsInt(item0); + src.channels = PyLong_AsInt(item1); + src.freq = PyLong_AsInt(item2); src_p = &src;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/_base_audio.c(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
📚 Learning: 2025-08-17T17:23:06.787Z
Learnt from: ankith26
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:520-529
Timestamp: 2025-08-17T17:23:06.787Z
Learning: In pygame-ce's mixer.c, when fixing reference leaks in mixer_quit, the cleanest approach is to first call Mix_ChannelFinished(NULL) to disable the callback and prevent race conditions, then manually handle Py_XDECREF calls for channeldata[i].sound and channeldata[i].queue after the Mix_HaltGroup calls.
Applied to files:
src_c/_base_audio.c
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/_base_audio.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/_base_audio.c
🧬 Code graph analysis (1)
src_c/_base_audio.c (2)
src_py/_audio.py (11)
name(60-61)name(143-144)channel_map(147-148)gain(163-164)gain(167-168)gain(299-300)gain(303-307)device(238-239)frequency_ratio(310-311)frequency_ratio(314-320)silence_value(64-65)src_c/base.c (2)
import_pygame_base(2294-2297)import_pygame_rwobject(2343-2346)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: Pyodide build
🔇 Additional comments (9)
src_c/_base_audio.c (9)
46-59: LGTM: Conditional device closure is correct.The check using
SDL_IsAudioDevicePhysicalensures that only logical devices (returned bySDL_OpenAudioDevice) are closed, while physical device IDs from enumeration and default device constants are not. This correctly follows SDL3 API requirements.
107-144: LGTM: All error paths correctly handle cleanup.The function now properly checks for NULL returns from
PyList_NewandPyLong_FromLong, and ensuresSDL_free(channel_map)is called on all error paths before returning.
222-268: LGTM: SDL device cleanup on allocation failure is correct.The function now properly closes the SDL audio device with
SDL_CloseAudioDevice(logical_id)iftp_allocfails, preventing a resource leak.
307-352: LGTM: SDL stream cleanup on allocation failure is correct.The function now properly destroys the SDL audio stream with
SDL_DestroyAudioStream(stream)iftp_allocfails, preventing a resource leak.
479-513: LGTM: Type safety and reference counting are correct.The function now properly uses
Py_ssize_tfor the length variable, checks for overflow before casting toint, and releases thebytesreference on all paths including the success path.
867-893: LGTM: Reference counting and resource cleanup are correct.The function now uses
"N"format inPy_BuildValueto transfer ownership of thebytesobject, preventing a reference leak. SDL resources are properly freed.
895-929: LGTM: NULL checks after allocation are correct.Both
get_default_playback_device_stateandget_default_recording_device_statenow properly check iftp_allocreturns NULL before dereferencing the pointer.
1041-1078: LGTM: Type creation NULL checks are correct.Both
PyType_FromModuleAndSpeccalls now have NULL checks before proceeding to add the types to the module, preventing potential crashes on allocation failure.
803-833: Missing NULL check forPyList_New.If
PyList_New(num_devices)fails at line 812,device_listwill be NULL and the subsequent loop will attempt to use it inPyList_SetItem, causing undefined behavior.Apply this diff to add the NULL check:
PyObject *device_list = PyList_New(num_devices); + if (device_list == NULL) { + return NULL; + } PGAudioDeviceStateObject *device; for (int i = 0; i < num_devices; i++) {Likely an incorrect or invalid review comment.
Starbuck5
force-pushed
the
sdl3_mixer_api3
branch
from
6e18b27 to
374f237
Compare
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
Thanks for taking this up and working on this 🎉 , I'm sure it was a lot of work 😅
I've given everything a quick scroll and left a few minor reviews, nothing important.
One more point of discussion I wanted to bring up since you mentioned implementing "next gen mixer" on top of this. I feel like having 3 layers of implementation is overcomplicating things to the end user. We don't want to create another draw/gfxdraw situation (this is not the best analogy but you get the idea). Ideally, the python level audio implementation should be easy enough to use that it is essentially the "next gen mixer". Maybe this means that we should consider adding more convenience methods/classes to the current audio implementation. Of course, old mixer compat could be the sole 3rd layer, but that would just be a compat layer with no new features added.
| install: true, | ||
| subdir: pg, | ||
| ) | ||
| endif |
There was a problem hiding this comment.
minor nit: this should be a part of the sdl_mixer_dep.found() if block.
|
|
||
| static PyObject * | ||
| pg_audio_get_audio_device_name(PyObject *module, PyObject *const *args, | ||
| Py_ssize_t nargs) |
There was a problem hiding this comment.
if this always expects exactly one arg why not just use METH_O convention? As of now this just silently ignores extra args, and must have ub for no arg?
I also observe other places in this file where argument checking is non existent or non sufficient. I get why it is the case though, these functions are internal. But still, better error checking does no harm and catches any current/future issues in our python level more gracefully rather than segfaulting
There was a problem hiding this comment.
Yep that should be METH_O. (It’s one of the first functions I wrote and I didn’t adjust it to match the later style). I’m away from my computer for a few days so I will make the change when I am back or you/anyone could push that.
I intentionally omitted all argument error handling, although I used safe/stable ABI operations for all Pyobjects once received. The idea is that if checks can be moved into the Python VM that JIT Python might be smart enough to eliminate them and lead to higher performance. Not that performance is particularly important for this module.
Maybe a simple and cheap check to do would be to check the number of args is as expected?
| def silence_value(self) -> bytes: ... | ||
| def __index__(self) -> int: ... | ||
| def __repr__(self) -> str: ... | ||
|
|
There was a problem hiding this comment.
I think implementing __eq__ would be handy too, so users could say spec.format == U8 or something like that.
There was a problem hiding this comment.
This is handled by Python’s default identity comparison, because AudioFormats are singletons.
| S32: AudioFormat | ||
| F32: AudioFormat | ||
|
|
||
| class AudioSpec: |
There was a problem hiding this comment.
I believe in the first draft it was, but when I actually went to write it I wanted understandable full control over all of the runtime consequences. Like I made the fields immutable. So for the typing and implementation I don’t see an advantage to making it into a dataclass.
Good points to discuss. I believe I did say that (sounds like me), and I’m sorry to mislead, but my plan for the next generation mixer is to build it on top of SDL3_mixer, not on top of this. The audio module here is supposed to be a faithful port of SDL3’s audio module (built in). I worked up this PR so that a future mixer PR would be able to implement stuff like accepting an AudioDevice or AudioSpec parameter. This is why I didn’t put the meson build code for this under mixer-dep-found, because for this it doesn’t matter if it was found. In terms of module proliferation pygame-ce already has an audio module, _sdl2.audio, that gives some of this enhanced functionality, like device enumeration, voice recording or streaming audio. And even though it’s completely undocumented some people still found it and used it, as we saw with a recent new contributor PR to fix something with it. Advantages of having an audio module rather than limping into mixer:
With regards to next generation vs existing mixer APIs I was thinking they would be in the same namespace to the user, probably just implemented in a separate file and imported. |
|
oh yeah fair enough, I always confuse audio and mixer mb. Anyways, to elaborate and correct myself, what I propose is:
Yes, I see that this implies that a |
|
The future module organization doesn’t need to be settled in this PR, this code would work for either strategy. I would like to bring some of this discussion over to #3581 though. |
2 participants
For #3581 mega-issue.
To fully implement next generation mixer, I realized I needed parts of SDL3's audio subsystem to be exposed. Not for a ton of API surface, but it is necessary for full implementation and necessary for a half-decent compatibility layer with the old mixer API. And it will need to be written anyway for full SDL3 support.
So I spent some time and wrote a whole new module, SDL3 only, starting from the type stubs mocked up in #3581.
I experimented with a hybrid approach with C level PyObjects as well as Python level classes. I'm not sure I'm in love with the exact way I did it, but I think it definitely shows promise as an approach. Some of the classes are only in Python, some are wrappers over low-level calls and "state objects". I really like how convenient it is to add error checking or stuff like the AudioDevice weakref dict at the Python level.
Example script:
Next steps (future PRs):