#8341 closed patch
Making output frequency a config option
| Reported by: | eriktorbjorn | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Audio |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This patch is an attempt to make the output sample rate configurable at runtime; something that's been on the TODO list since forever. The patch looks pretty obvious to me, but I don't want to apply it without any feedback.
There are things to be done, of course. I haven't even tried to add a GUI for it in the options window.
Two things I noticed while testing the patch:
My debug message never seems to be displayed unless I change it to debug(0, ...).
When I accidentally triggered a call to error(), the program terminated with a "pure virtual method called" message. Seems our debugging functions don't work in all cases.
Oh well. None of that matters for the patch itself.
Ticket imported from: #957544. Ticket imported from: patches/446.
Attachments (3)
Change History (16)
by , 20 years ago
| Attachment: | output-freq.diff added |
|---|
comment:1 by , 20 years ago
last time I played with the output freqs, the code in scumm/ player_v*.cpp had some issues with it (IIRC in a few cases there were hard coded assumptions about the frequency; in one case, using 44100 Hz caused some sort of overflow). Not sure if those are still there, but you might want to test if they work fine with various output rates (let's say, 11025, 22050, and 44100 Hz).
comment:2 by , 20 years ago
Another thing: I am not sure if its a good idea to add a (pure virtual) OSystem::setOutputSampleRate method for this... * All backends will have to implement that * If it is added, then we must define what happens if the selected frequencies is not available... fall back? Crash & burn? * Current implemention is incorrefct if first an audio callback is set, and then later setOutputSampleRate() is called; from that point on, getOutputSampleRate() returns an incorrect
Considering the backends already are allowed to query the config system (and do so), the SDL backend could simply directly query the output frequency... I see no drawback doing that at this point.
comment:3 by , 20 years ago
Good point about player_v* - I had forgotten about those. Hmm... they seem frequency-aware enough to sound fine with Maniac Mansion (classic and enhanced), Zak (enhanced) and EGA Loom, which I think covers the v1 and v2 cases. I have no idea about v2a and v3a, or ym2612 though.
And, of course, "it sounds fine" (or, in some of my test cases, terrible but in an expected way) is no guarantee against buffer overflows...
> I am not sure if its a good idea to add a (pure virtual) > OSystem::setOutputSampleRate() method for this...
I don't know enough about C++ to understand the bit about pure virtual methods, so I'll trust your judgement on that. I simply tried to mimic the getOutputSampleRate() method. If there's a way of making it optional, so much the better. The way I imagine it, it could safely be made a no-op until a backend maintainer decides to support it.
> what happens if the selected frequencies is not available... > fall back? Crash & burn?
Preferably the former. At least that's what I tried to do in the patch: ask SDL for a frequency, and check which one was obtained. (That's one reason I didn't want to consult the config file to find out the frequency later.)
> Current implementation is incorrect if first an audio callback is > set, and then later setOutputSampleRate() is called
That's the documented behaviour. :-)
Seriously though, do we want to make it possible to change output frequency in mid-game, or just at startup. If the latter, there are several options on how to fix it.
comment:4 by , 20 years ago
I would make it a non-GUI option. 99% of our users should be happy with the default. The selected few who want to use their pure 44.1khz FLAC encoded CD rips can live with setting a config file setting manually once (which should be documented in the README / doc.tex, of course).
Anything else only will lead to a lot of headache, from reseting the mixer to other annoying problem which would be caused by allowing changing the sample rate on the fly (don't get me wrong, it would all be possible, but I don't think it's worth the effort).
comment:5 by , 20 years ago
Setting the sample rate at startup is fine by me. I just wasn't sure about you after you pointed out what happens if you try to call the function after setting the callback.
By the way, the most noticeable improvement to audio quality for me isn't the CD audio - in fact, I couldn't tell the difference. There was a noticeable difference in at least one piece of BASS AdLib music, though. Though that may be mostly of academic interest. The games where I do use AdLib instead of real MIDI don't seem to have that kind of problem.
Anyway, I could make the setOutputSampleRate() function ignore the request if it's called too late. As long as it's a non-GUI option, I don't see any reason to explicitly guard against "stupid" rates, as long as we document what the "sensible" ones are. Other than that, the behaviour seems well-enough defined to me.
The only thing I'm not sure about is that "pure virtual" thing you mentioned. Could you elaborate a bit? I don't have any C++ textbook within reach, I'm afraid.
comment:6 by , 20 years ago
I've updated the patch a bit:
* Added an --output-rate command-line option. * Made setOutputSampleRate() do nothing unless audio is stopped. (Or, in this case, not yet started.) * Made desired.samples depend on sample rate. (Somewhat experimental.) * Added documentation.
comment:7 by , 20 years ago
A pure virtual method is a method of the form
class Foo { virtual void bar() = 0; };
This method is *not* implemented by class Foo; hence you can't instantiate this class.
So as a consequence, the way you added setOutputSampleRate() means all non-SDL backends will be rendered uncompileable by this patch. Which isn't a surprise, since your patch actively tries to push the sample rate setting from the frontend/loader to the backend. Such a problem would not occur, OTOH, if we made the setting "passive", i.e. leave it up to the backend to read out the config setting or not. <shrug>
by , 20 years ago
| Attachment: | output-freq-alt.diff added |
|---|
Alternative patch, without setOuputSampleRate()
comment:8 by , 20 years ago
Ah. Ok, here's an alternative version of the patch that doesn't have a setOutputSampleRate() function.
Good thing I'm usually too chicken to make sweeping changes without discussion, eh? ;-)
comment:9 by , 20 years ago
So what is the status of this? Was the last version of the patch more to your liking?
comment:12 by , 20 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:13 by , 6 years ago
| Component: | → Audio |
|---|
Patch against a May 20 CVS snapshot