Opened 15 years ago

Closed 15 years ago

Last modified 9 months ago

#8341 closed patch

Making output frequency a config option

Reported by: eriktorbjorn Owned by: eriktorbjorn
Priority: normal Component: Audio
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)

output-freq.diff (3.9 KB) - added by eriktorbjorn 15 years ago.
Patch against a May 20 CVS snapshot
output-freq.2.diff (11.7 KB) - added by eriktorbjorn 15 years ago.
Updated patch against a May 25 CVS snapshot
output-freq-alt.diff (10.5 KB) - added by eriktorbjorn 15 years ago.
Alternative patch, without setOuputSampleRate()

Download all attachments as: .zip

Change History (16)

Changed 15 years ago by eriktorbjorn

Attachment: output-freq.diff added

Patch against a May 20 CVS snapshot

comment:1 Changed 15 years ago by fingolfin

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 Changed 15 years ago by fingolfin

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 Changed 15 years ago by eriktorbjorn

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 Changed 15 years ago by fingolfin

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 Changed 15 years ago by eriktorbjorn

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 Changed 15 years ago by eriktorbjorn

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.

Changed 15 years ago by eriktorbjorn

Attachment: output-freq.2.diff added

Updated patch against a May 25 CVS snapshot

comment:7 Changed 15 years ago by fingolfin

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>

Changed 15 years ago by eriktorbjorn

Attachment: output-freq-alt.diff added

Alternative patch, without setOuputSampleRate()

comment:8 Changed 15 years ago by eriktorbjorn

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 Changed 15 years ago by eriktorbjorn

So what is the status of this? Was the last version of the
patch more to your liking?

comment:10 Changed 15 years ago by fingolfin

Looks good to me!

comment:11 Changed 15 years ago by eriktorbjorn

I've commited the patch, then.

comment:12 Changed 15 years ago by eriktorbjorn

Owner: set to eriktorbjorn
Status: newclosed

comment:13 Changed 9 months ago by digitall

Component: Audio
Note: See TracTickets for help on using tickets.