Support devicename=None in _sdl2.AudioDevice

[rhendric]

The docstring already claimed that this was supported, but it didn't work.

[coderabbitai]

📝 Walkthrough

Walkthrough

Allow devicename to be None when opening audio devices: pass NULL to SDL_OpenAudioDevice for the default device or a UTF-8 encoded C string when a name is provided; update type hints in stubs to Optional[str]; preserve existing error handling.

Changes

Cohort / File(s)	Summary
Audio device initialization src_c/cython/pygame/_sdl2/audio.pyx	Accept devicename as None or str; convert to a C string pointer (NULL when None, UTF-8 pointer otherwise) and pass it to SDL_OpenAudioDevice. Preserve existing error handling and use the new pointer at the call site.
Type hints / stubs buildconfig/stubs/pygame/_sdl2/audio.pyi	Update typings: import Optional; change AudioDevice.__init__ and AudioDevice.devicename signatures to accept/return Optional[str] instead of str.

Sequence Diagram(s)

sequenceDiagram
    participant Py as Python caller
    participant C as Cython wrapper
    participant SDL as SDL library

    Py->>C: open_audio(devicename)
    alt devicename is None
        C->>C: devicename_ptr = NULL
    else devicename provided
        C->>C: encode devicename -> utf8_ptr
        C->>C: devicename_ptr = utf8_ptr
    end
    C->>SDL: SDL_OpenAudioDevice(devicename_ptr, ...)
    alt success
        SDL-->>C: device_id
        C-->>Py: device object / id
    else failure
        SDL-->>C: NULL / error
        C-->>Py: raise error
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Support devicename=None in _sdl2.AudioDevice" directly and accurately summarizes the main change in the changeset. The modifications to both the Cython implementation and type stubs are specifically about enabling the devicename parameter to accept None in addition to strings, changing its type from str to Optional[str]. The title is concise, clear, and sufficiently specific for a teammate reviewing commit history to understand the primary intent of the change.
Description Check	✅ Passed	The pull request description "The docstring already claimed that this was supported, but it didn't work" is directly related to the changeset and provides meaningful context for the changes. It explains the motivation behind adding None support: there was a documentation-implementation mismatch where the docstring claimed the feature was supported but the code didn't actually implement it. This context is relevant and meaningful, making the description appropriate for the PR objectives.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3dd631d and e0b4055.

📒 Files selected for processing (2)

• buildconfig/stubs/pygame/_sdl2/audio.pyi (3 hunks)
• src_c/cython/pygame/_sdl2/audio.pyx (2 hunks)

🔇 Additional comments (3)

buildconfig/stubs/pygame/_sdl2/audio.pyi (1)

2-2: LGTM! Type annotations correctly reflect optional devicename.

The stub updates properly annotate devicename as Optional[str] in both the constructor parameter and property return type, aligning with the runtime implementation that now supports None.

Also applies to: 29-29, 43-43

src_c/cython/pygame/_sdl2/audio.pyx (2)

134-136: LGTM! Type validation correctly allows None and str.

The updated type check properly validates that devicename is either None or a str, rejecting any other types with a clear error message.

---

145-155: LGTM! Encoding logic and bytes object lifetime are correct.

The implementation properly handles both cases:

• When devicename is None, passes NULL to SDL for the default device
• When devicename is a string, encodes to UTF-8 and maintains the bytes object lifetime correctly

The devicename_bytes local variable ensures the C string pointer remains valid throughout the SDL_OpenAudioDevice call.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src_c/cython/pygame/_sdl2/audio.pyx (1)

145-160: Consider refactoring to reduce duplication.

The SDL_OpenAudioDevice call is duplicated in both branches with only the first parameter differing. You could reduce duplication with:

+        cdef bytes device_name_bytes
+        cdef const char* device_name_ptr
         if self._devicename is None:
-            self._deviceid = SDL_OpenAudioDevice(
-                NULL,
-                self._iscapture,
-                &self.desired,
-                &self.obtained,
-                allowed_changes
-            )
+            device_name_ptr = NULL
         else:
-            self._deviceid = SDL_OpenAudioDevice(
-                self._devicename.encode("utf-8"),
-                self._iscapture,
-                &self.desired,
-                &self.obtained,
-                allowed_changes
-            )
+            device_name_bytes = self._devicename.encode("utf-8")
+            device_name_ptr = device_name_bytes
+
+        self._deviceid = SDL_OpenAudioDevice(
+            device_name_ptr,
+            self._iscapture,
+            &self.desired,
+            &self.obtained,
+            allowed_changes
+        )

Note: Keeping device_name_bytes alive ensures the pointer remains valid during the C call.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3fc1dbe and e42b76c.

📒 Files selected for processing (1)

• src_c/cython/pygame/_sdl2/audio.pyx (2 hunks)

🔇 Additional comments (2)

src_c/cython/pygame/_sdl2/audio.pyx (2)

134-135: LGTM! Type validation correctly supports None.

The type check properly allows None and validates that non-None values are strings. The error message accurately reflects the accepted types.

---

145-160: Correctly implements None support for default device.

The branching logic properly passes NULL to SDL_OpenAudioDevice when devicename is None, which selects the default device per SDL2 API. UTF-8 encoding for non-None device names is appropriate.

[ankith26]

Hello, thanks for contributing to pygame-ce! 🥳

The code changes are looking good, but the type stubs (at buildconfig/stubs/pygame/_sdl2/audio.pyi) are still incorrect, could you fix that too in this PR?
The devicename arg of __init__ and devicename property of the class should both be typed as Optional[str]

[ankith26]

LGTM, thanks again for the PR! 😎

[Starbuck5]

@rhendric Can you provide some details about your use of the _sdl2.audio API? I'm curious. I was unaware anyone used it, besides from the list device names function.

[rhendric]

I'm only using it for the async audio API. I'm playing with generating music dynamically in real time, so I don't have pre-recorded audio files to play, and so getting a callback invoked when SDL needs audio data is much more straightforward than racing to proactively queue up Sounds through the mixer API.

[rhendric]

I should probably also mention that I'm in the very early stages of experimenting with this, and while my first demo script showed promise, in the context of a larger application I'm currently struggling with some skipping issues that might force me to switch to synchronous after all (though there seems to be a different bug in the mixer API that is getting in the way of queuing dynamic Sounds repeatedly). So don't move any mountains based solely on my use case just yet.

[Starbuck5]

@rhendric You might be interested in what I've been working on for SDL3 audio support then, where it will hopefully not be hidden away in a private undocumented module. #3581

though there seems to be a different bug in the mixer API that is getting in the way of queuing dynamic Sounds repeatedly

Are you running into the default limit of 8 sounds playing at once? That can be raised.

[rhendric]

SDL3 audio support

Looks good! The open_stream function in particular would be a match for my use case, I think, provided that something like the overhead of waiting for the GIL on every callback isn't the source of my current woes.

Are you running into the default limit of 8 sounds playing at once?

No, I think I'm running into #531. There's a race condition between the main thread rapidly queuing sound data and the SDL thread moving sounds from the queue field to the sound field for the channel. The root issue seems to be that endsound_callback in mixer.c tests whether queue is NULL before locking on anything. I have a patch that I'm in the process of stress testing.