Games: drive the asynchronous audio interface - #153
Conversation
Kodi accepts a client's registration for the asynchronous audio interface and then never asks it for audio. A client on that interface produces none of its own accord -- it waits to be asked and writes what it has from the thread it was asked on -- so it stays silent for the whole session. Silence is not the only symptom. The frame rate is paced against the audio a client delivers, so one that is never asked also runs as fast as the machine allows rather than at the speed of the console. game.libretro has had the client half of this implemented for years, noting "this function is not part of the Game API yet". Add it to the API and call it once per frame, alongside RunFrame(). Clients on the ordinary synchronous audio path answer GAME_ERROR_NOT_IMPLEMENTED and are unaffected. Adding an entry to KodiToAddonFuncTable_Game changes the layout of a struct shared with add-on binaries, so the game instance version goes to 7.0.0 and the minimum with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This is a small, great, well-contained impactful fix. The only contention is the API bump, as the team wants APIs frozen for B2 release. That said, B2 isn't out yet. And I'm kinda left to my own for game stuff. So any changes to the API we need PRd against master ASAP. |
| char*(__cdecl* GetImageLabel)(const AddonInstance_Game*, unsigned int); | ||
| void(__cdecl* FreeString)(const AddonInstance_Game*, char*); | ||
|
|
||
| /*! |
There was a problem hiding this comment.
Drop the doxy here, instead of documenting the C-level function pointers, we document API functions in the C++ header Game.h. If the doc here introduces new info, maybe combine the docs. But the doc in Game.h looks good.
There was a problem hiding this comment.
Carried over into the upstream PR, xbmc#28964, which is the one that merged — AudioAvailable sits in the C table with no doxy on it, and the documentation lives on the C++ side in Game.h as you asked. Nothing outstanding here; this PR was superseded rather than abandoned.
garbear
left a comment
garbear
left a comment
There was a problem hiding this comment.
The change here looks great. I haven't confirmed by reading the libretro.h API, but testing in the PR description confirms correctness.
We should also test rewind and fast-forward. But I think RetroPlayer silences audio in these situations.
|
Last night I did a round of RetroPlayer test builds that included this change. I haven't runtime tested, but it certainly builds successfully in the RetroPlayer patchset. Specifically I did a test builds with these PRs: I'm kinda swamped with the B2 release and day job, so anything we wanna ship in B2 I'll need help merging upstream. Can you list all PRs with new features or API bumps? (Anything not a fix, which are allowed in RC 1). If you can list all PRs we want in B2, I'll do a combined build (basically what I did last night but also uploaded to the mirrors for distribution on my Releases page). If you can make a list of everything we wanna ship in B2, I'll get it all in the release. |
|
Smashing and yep i'll do that in about 12 hours - Morning for me! |
|
Here's the full list, split by what needs to beat the freeze. Needs to land before B2 — API bump or new feature
Those four are the ones the freeze actually bites. #153 is the only one that moves the API version number; #150 adds callbacks alongside it. Fixes — fine for RC1, no rush
xbmc#347 is worth pulling forward despite being a fix — you mentioned wanting the udev switch before B2, and it's a one-line default change. Not for v22#151 and kodi-game/game.libretro#164 — hardware rendering. Still experimental, carries its own API bump, and I have open defects on it. Not something to rush into a release. On the API bumpYou said API changes need to go against xbmc/xbmc master ASAP. #153 is the only one of mine that does. Say the word and I'll open it there — it's a clean 45-line diff off master with no dependencies on the rest of this, so it can go in on its own. Same for #152 if you want the feature in, though that one touches no API. |
|
This late in the B2 cycle, we can get the RA callbacks in because we have to bump anyway due to the audio fix. I can spin the huge RA patch cuz it only affects games. I'll let the team know, and when I get a chance (could be 24hr) we'll plot a path to merge both game API bumps. |
|
Revised list — three additions since my last, and one removal. Needs to land before B2 — API bump or new feature
Rumble — three PRs, one chain, all fixesRumble has never worked for any controller with any emulator. It takes all three of these; each is inert without the others:
xbmc#169 isn't strictly rumble, but it came out of the same testing: Other fixes — fine for RC1
Removing the achievements work from the B2 listI had #150 and kodi-game/game.libretro#162 down for B2. On reflection I'd pull both, and I'd rather say why than quietly drop them. They're a much larger surface than anything else here — a new dependency, network and login handling, memory-map wiring per core — and unlike #153 they haven't been through upstream review. My testing has been good (82/83 achievements active on Dreamcast, 21/21 memory addresses valid) but it's been my testing, on one box, against a handful of cores. Landing that during a freeze is the kind of thing that produces a bad release rather than a good feature. They'd be much better as the first thing in the next cycle, with room to be reviewed properly. Entirely your call — if you'd rather have them in B2 they're ready to build, and I'll support whichever way you go. Still not for v22#151 and kodi-game/game.libretro#164 — hardware rendering. Experimental, own API bump, open defects. On the API bump#153 is still the only PR of mine that touches the API version. Say the word and I'll open it against xbmc/xbmc master — clean 45-line diff, no dependencies on anything else here. |
|
Ignore the removal in my last comment — I wrote it without having read yours first, and argued against a call you'd already made. Apologies for the noise. You're right that the bump is the expensive part and it's being paid anyway. Here's the corrected list. B2 — the two Game API bumps
xbmc#162 isn't an API bump itself but is the other half of #150; they don't work apart. B2 — feature
B2 — rumble, three PRs forming one chainRumble has never worked for any controller with any emulator. Each of these is inert without the others:
Fixes — fine for RC1
Not for v22#151 and kodi-game/game.libretro#164 — hardware rendering. Experimental, own API bump, open defects. What I'd still flag on the RA workNot as an argument against shipping it — your call and you've made it — just so it's on the record: my testing has been on one box against a handful of cores (82/83 achievements active on Dreamcast, 21/21 memory addresses valid), and it hasn't had upstream review. If anything bites in B2 it's most likely to be there, and I'll be around to fix it quickly. Happy to open #153 and #150 against xbmc/xbmc master whenever you want to plot that path. |
|
We're still plotting, but yeah let's break B2. That's what RC 1 is for, getting in fixes for the cool shit we added in the alpha/beta cycle. |
|
Completed in xbmc#28964. |
2 participants
Kodi accepts a client's registration for the asynchronous audio interface and then never asks it for audio.
A client on that interface produces no audio of its own accord — it waits to be asked, and writes what it has from the thread it was asked on. Since nothing ever asks, it stays silent for the whole session.
Silence isn't the only symptom. The frame rate is paced against the audio a client delivers, so a client that is never asked also runs as fast as the machine allows rather than at the speed of the console. That's how I found it: a Dreamcast core running at roughly double speed, with no sound.
game.libretrohas had the client half implemented for years, with a comment noting "this function is not part of the Game API yet". This adds it to the API and calls it once per frame, alongsideRunFrame().Compatibility
Clients on the ordinary synchronous audio path inherit the default
AudioAvailable(), which returnsGAME_ERROR_NOT_IMPLEMENTED, and are unaffected — no source change needed in any existing add-on.Adding an entry to
KodiToAddonFuncTable_Gamedoes change the layout of a struct shared with add-on binaries, so this bumps the game instance version to 7.0.0 and the minimum with it. Existing add-ons need a rebuild, not a code change. Happy to split the bump out or sequence it behind whatever else is queued for 7.0.0 if you'd rather batch the ABI breaks.Testing
Verified against Flycast via
game.libretro, on the hardware-rendering branch: the client goes from silent to producing audio, and the pacing follows.One caveat worth stating plainly — Flycast needed a separate
game.libretrofix (answeringRETRO_ENVIRONMENT_GET_FASTFORWARDING) before it produced any audio at all, so that core exercised both changes together. The reasoning here stands on its own: the interface is registered and then never driven.