Add improved qsound_hle core #3819
Conversation
qsound_hle: use enums for most DSP ROM addresses, move them to qsoundhle.h
|
This commit addresses the issues brought up in the previous thread #3821. |
|
I've done a great deal of testing of this PR today. First off, it compiles cleaning with current GIT as of this writing. I've manually ran about a dozen machines with binaries from before and after to listen for changes and while I didn't notice anything standing out good or bad. Also in my regression testing of all 33 (before vs after) there was no breaks in audio output as far as I could determine. I did my testing on the 4 drivers affected (cps1, cps2, zn, vgmplay) and all of them, outside of needed wavdata changes deal with the new registers and commands being processed, worked and sounded fine. The changes appear to be consistent across the board where machines with the same datahash also shared the same datahash after this was applied. The only issue I had was that I needed to make a "qsound_hle" file (duplicate data/dump) from the existing "qsound" because it appears the HLE uses the rom as well as the LLE.. reading data from it to construct data tables. Since there isn't a way for both implementations to share the same "qsound" device, this appears to be valid. |
|
Are there any remaining issues with this PR that would keep it from being merged? |
|
[23:34:14] the type punning has to go |
src/devices/sound/qsoundhle.cpp
Outdated
| } | ||
|
|
||
| } // anonymous namespace | ||
| #define CLAMP(x, low, high) (((x) > (high)) ? (high) : (((x) < (low)) ? (low) : (x))) |
There was a problem hiding this comment.
Please don't use macros when constexpr functions are better (multiple evaluation of expressions, etc.). Also, this can be defined in terms of std::min and std::max.
src/devices/sound/qsoundhle.cpp
Outdated
| memset(m_channel, 0, sizeof(m_channel)); | ||
| // state save | ||
| // PCM registers | ||
| for(int j=0;j<16;j++) // PCM voices |
There was a problem hiding this comment.
Please follow the convention used in existing files - space between control keyword and opening parenthesis of control expression, and spaces after semicolon/comma separators.
src/devices/sound/qsoundhle.h
Outdated
| int m_state_counter; | ||
| int m_ready_flag; | ||
|
|
||
| uint16_t *register_map[256]; |
There was a problem hiding this comment.
Why this approach rather than a flat array of uint16_t for the registers with accessors for the common access patterns?
src/devices/sound/qsoundhle.h
Outdated
|
|
||
| uint16_t *register_map[256]; | ||
|
|
||
| inline uint16_t read_dsp_rom(uint16_t addr) { return m_dsp_rom[addr&0xfff]; } |
There was a problem hiding this comment.
inline qualifier has no effect here and adds clutter - if the body is present at declaration inline is implied.
src/devices/sound/qsoundhle.h
Outdated
| int rvol; // right volume | ||
| } m_channel[16]; | ||
| // sub functions | ||
| inline int16_t get_sample(uint16_t bank,uint16_t address); |
There was a problem hiding this comment.
inline qualifier without body present at declaration generally doesn't produce desired effect. If you want to suppress export of the body, you should put the inline qualifier on the definition.
src/devices/sound/qsoundhle.cpp
Outdated
| } | ||
|
|
||
| // Update the delay read position to match new delay length | ||
| void qsound_hle_device::delay_update(struct qsound_delay *d) |
There was a problem hiding this comment.
Things that operate entirely on an inner structure/class like this should be members of the inner structure/class, otherwise you're passing a useless this pointer for the outer class and need an explicit parameter for the inner structure/class.
src/devices/sound/qsoundhle.cpp
Outdated
| qsound_adpcm temp2 = {}; std::fill(std::begin(m_adpcm),std::end(m_adpcm),temp2); | ||
| qsound_fir temp3 = {}; std::fill(std::begin(m_filter),std::end(m_filter),temp3); | ||
| std::fill(std::begin(m_alt_filter),std::end(m_alt_filter),temp3); | ||
| qsound_delay temp4 = {}; std::fill(std::begin(m_wet),std::end(m_wet),temp4); |
There was a problem hiding this comment.
You really need to provide member initialisation for the qsound_voice etc. structures for this to work properly, and you don't need the locals, as you can construct a temporary in the call, e.g. std::fill(std::begin(m_voice), std::end(m_voice), qsound_voice());
src/devices/sound/qsoundhle.cpp
Outdated
| int16_t qsound_hle_device::get_sample(uint16_t bank,uint16_t address) | ||
| { | ||
| uint32_t rom_addr; | ||
| uint8_t sample_data; |
There was a problem hiding this comment.
In cases like this, please declare variables at initialisation and use const where appropriate to reduce mutable state.
src/devices/sound/qsoundhle.cpp
Outdated
| { | ||
| // unused registers | ||
| for(int i=0;i<256;i++) | ||
| register_map[i] = NULL; |
There was a problem hiding this comment.
Please use nullptr as a null pointer constant. Also, probably a case for std::fill
src/devices/sound/qsoundhle.cpp
Outdated
| m_ready_flag = 0; | ||
| m_out[0] = m_out[1] = 0; | ||
| m_state = 0; | ||
| m_state_counter = 0; |
There was a problem hiding this comment.
Resetting the device would cause the DSP to jump to address zero, causing it to zero its registers and reinitialise.
|
Sorry about the bit of drama, this will still get a fair review once Vas's notes have been dealt with. As-is the translation from the original DSP assembly is a little too literal, or what we used to call 'Achocode'. With the changes addressed that should be much less of a problem. |
This adds the rewritten QSound HLE by ValleyBell and superctr with full spatialization, among other accuracy improvements