#9010 closed patch
AdLib: Support fo DOSBox AdLib Emulator
| Reported by: | lordhoto | Owned by: | lordhoto |
|---|---|---|---|
| Priority: | normal | Component: | Audio |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hi,
this WIP patch adds support for the new DOSBox AdLib emulator. This one for example plays a sound effect in Kyrandia 1 fine, while our old FMOPL doesn't. Indy4 intro sounds a bit different too, I don't know which one is more accurate here though, since I don't have any recording from an real AdLib to compare with. The DOSBox emulator code can be disabled via DISABLE_DOSBOX_ADLIB for small devices without floating point support.
ADVANTAGES: - Should have better AdLib emulation overall (I know of at least one sound effect fixed in Kyra1 and in Lands of Lore some sound effects sound different/better too) - We still can use the old emulator, through our new abstract AdLib API.
DISADVANTAGES: - the DOSBox emulator depends on floating point math (as far as I can tell), this prevents it from being used on small devices like NDS.
TODO: - I added a new abstraction layer to support both the old and new emulator, it needs to be documented and maybe moved to sound/fmopl.h again (I would prefer sound/adlib.h though). Currently the new API is in sound/softsynth/adlib/adlib.h. This isn't the best place for that though, since *theoretically* we could even support hardware OPL devices with it now. - there is *no* support for switching the emulators on runtime currently. We need to think about a nice way to do so. Maybe we want to use a config entry for that (and create the GUI to change it). I'm open for comments on this one. - we might want to make all the emulator code plugin based, like we plan/do with MIDI too - currently the DOSBox emulator depends on static variables, this prevents us from creating two instances of it at once and it also bloats binary size. I don't know if the DOSBox guys plan any major changes to the emulator, thus I wouldn't want to touch this area for now. I'll contact them about that soon. - SCI seems to depend heavily on the old FMOPL API, thus I had to include it directly there. I don't know whether SCI just wants an DualOPL2 support though, which could be easily achieved with the DOSBox AdLib emulator. At least the code looked like it creates two OPL instances to play stereo sound, which is exactly what DualOPL2 should do AFAIK. - the code could need an overall cleanup
NOTES: - the old FM OPL code was moved to sound/softsynth/adlib/mame[.cpp/.h] - you can change the AdLib emulator used at bottom of sound/softsynth/adlib/adlib.cpp - there are some more TODOs in the code :-)
I'm open for comments and suggestions.
Ticket imported from: #2773359. Ticket imported from: patches/1115.
Attachments (6)
Change History (29)
by , 16 years ago
| Attachment: | dosbox_adlib_v1.patch added |
|---|
comment:1 by , 16 years ago
BTW. the AdLib::AdLib API could really need some extension / cleanup. For example OPL3 and DualOPL2 would support stereo sound and currently there's no way of telling which OPL mode is initialized (apart from only OPL2 being supported currently ;-).
comment:2 by , 16 years ago
I fixed a bug in AdLib_DOSBox::writeReg and did some formatting. Updated patch is attached.
comment:3 by , 16 years ago
| Owner: | removed |
|---|
comment:4 by , 16 years ago
Is it just me, or doesn't the patch apply to a normal checkout? Maybe because of the file moves?
comment:5 by , 16 years ago
Could very well be, just try to remove the sound/fmopl.* files. I moved those to sound/softsynth/adlib/mame[.cpp/.h], but they were modified.
comment:6 by , 16 years ago
Okay, the patch applies for me now.
And I'm very much in favour of it, since it fixes the Gob2 title music. :)
comment:7 by , 16 years ago
According to the dosbox cvs logs, an integer opl emulator was just added to dosbox too this morning.
comment:8 by , 16 years ago
Yep saw those commit logs in my mails. I will look at integrating them tomorrow.
comment:9 by , 16 years ago
I'd have been positively surprised if you had at least left the actual licensing header intact, especially as you used the lib code otherwise unmodified.
comment:10 by , 16 years ago
Oh just checked it again it's licensed under LGPL not GPL, sorry about that, I'll need to change that.
Actually it's slightly 'modified' (some unused (global) variables commented out, removed header includes in the opl.cpp file and reeanbled the datatype defines again).
comment:11 by , 16 years ago
> Actually it's slightly 'modified'
Only in spots that were supposed to be modified when including it in other projects, or that have no real effect.
Thus I ask you to please use the exact text that is used in the cvs version found in the DOSBox repository.
comment:12 by , 16 years ago
Uploaded a new patch.
Changes: - Used DOSBox source file header for dbopl_fl.h and dbopl_fl.cpp (formerly opl.h and opl.cpp) - Integrated DOSBox integer based AdLib emulator - Moved code from sound/softsynth/adlib/adlib.h to sound/fmopl.h - Moved code from sound/softsynth/adlib/adlib.cpp to sound/fmopl.cpp - Removed sound/softsynth/adlib/adlib.cpp and sound/softsynth/adlib/adlib.h
I guess we could think of dropping support of the floating point based emulator though. I guess for small devices the integer based would be better.
Currently the floating point based one is disabled in sound/softsynth/adlib/dosbox.cpp.
comment:13 by , 16 years ago
New patch (again!).
CHANGES: - Dropped floating point based AdLib emulator, since the integer based one works fine (it's trivial to add again, anyway). - Changed SCI code, thus allowing it to use the DOSBox AdLib emulator.
Still TODO: - Decide whether we really want to have something like AdLib::AdLib - Add way to switch between mame and DOSBox AdLib emulator - Do some speed measurements of the new DOSBox AdLib emulator. - Write documentation of AdLib::AdLib - Maybe add support to Dual OPL2 and OPL3 to the DOSBox AdLib emulator
comment:14 by , 16 years ago
Updated patch.
CHANGES: - Renamed namespace AdLib to OPL - Renamed class AdLib to OPL - Moved sound/softsynth/adlib to sound/softsynth/opl - Added some documentation for OPL::OPL - Moved OPL type parameter from OPL::init to OPL::create
TODO (random order): - Do some speed measurements of the new DOSBox AdLib emulator. - Decide whether we want a GUI for selecting the AdLib emulator or whether we want to make it mutually exclusive on compile time (I would *love* to hear some feedback on this ;-)
comment:15 by , 16 years ago
> Dropped floating point based AdLib emulator, > since the integer based one works fine
First of all there's no floating point based emulator. All use integer calculations, with the exception of opl.cpp which keeps two variables in floating point. Second, the (what you call integer based) dbopl.cpp is NOT supposed to be used yet as it has known deficiencies, has not undergone testing and in its current state has open bugs. But it's of course up to you to decide which one you want to use.
Both opl.cpp and dbopl.cpp can be used for dual opl as done in DOSBox by using the opl3 panning feature.
comment:16 by , 16 years ago
Yeah waltervn noticed some problems with it in SCI games.
The opl.cpp/opl.h emulator has the slight disadvantage of code duplication, when supporting different OPL types and in my tests you were not able to create more than one instance of a specific OPL type, which is currently required by our (current) SCI code. The latter one is easy to fix when we support Dual OPL2 directly, and I don't think any other code uses more than one instance at once too.
Hm my copy of opl.cpp and opl.h is using floating point based functions all over the place. In "change_decayrate", "change_releaserate" etc. for example, thus I thought it's floating point math, but maybe the sample generation is purely integer math based. It seems the first glance of the code didn't reveal the truth :-).
comment:17 by , 16 years ago
> thus I thought it's floating point math
Functions like change_decayrate are called rarely, the speed relevant parts are in the sample generation. One point that IS relevant but which I could not convert yet is operator_output, but this requires getting step_amp and vol being 32bit-integer and keeping the calulations there in the signed 32bit range. Ideas welcome of course.
Two instances of an adlib emulation core are not necessary in the approach Harekiet used in dosbox for dualopl, the only "flaw" is that noise channels are always centered.
comment:18 by , 16 years ago
Yep that multiple instance for Dual OPL2 is only because our OPL emulator doesn't support Dual OPL2 out of the box. I guess that's so far the only point where we create more than one instance at a time, so as I said easy to fix when we support Dual OPL2 directly.
comment:19 by , 16 years ago
Well I guess if it's really only rarely used it might be fine. Some of our target systems don't have any floating point unit at all, thus that "operator_output" could lead to a bit of a problem here, but I don't have any means of testing that.
comment:20 by , 16 years ago
That is a really really nice addition to ScummVM.
Although I would prefer that at least for now the emulator could be selected via GUI, i.e. add "Adlib (MAME)" and just "Adlib" until we will see that newer Adlib is perfect :)
I consider this patch to be ready for inclusion once we're back on so-called "floating point" version.
comment:21 by , 16 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:22 by , 16 years ago
Committed with following changes: - Made MAME OPL default for OPL2 - Support for Dual OPL2 + OPL3 - Switched to 'floating-point' (rather 'compat') DOSBox OPL emu.
comment:23 by , 6 years ago
| Component: | → Audio |
|---|
Patch against r40006