HarbourMasters / HarbourMasters/Shipwright

aEnvMixerImpl: where do the -4 / -2 wet-path XOR masks come from?

Open
#7,119 0 comments 0 reactions 1 assignee Claimed by @serprex View on GitHub
Dominant language
C
Stars
5.4k
Forks
837
Avg merge
1d 9h
Merged PRs (30d)
72

Description

While looking at the audio mixer I could not find any reference implementation that matches our wet-path negation, and I would rather ask than guess before anyone touches it.

`soh/soh/mixer.c`, `aEnvMixerImpl`:

```c
int16_t negs[4] = { neg_left ? -1 : 0, neg_right ? -1 : 0, neg_3 ? -4 : 0, neg_2 ? -2 : 0 };
...
*dry[j] = clamp16(*dry[j] + samples[j]);
*wet[j] = clamp16(*wet[j] + ((samples[swapped[j]] * vol_wet >> 16) ^ negs[2 + j]));
```

Three implementations, none of which has the wet masks:

| implementation | dry | wet |
| --- | --- | --- |
| mupen64plus-rsp-hle `alist_envmix_exp` | no negation | no negation |
| sm64-port `src/pc/mixer.c` (what this file descends from) | XOR -1 | no negation |
| here | XOR -1 | **XOR -4 / -2** |

mupen64plus's `ENVMIXER` reads only `A_INIT` and `A_AUX` out of the flag byte and ignores the four low bits that `aEnvMixer` packs `x0..x3` into. sm64-port has `negs[2]`, not `negs[4]`, and no negation on the wet path at all.

The history does not settle it: the file arrives in libultraship already in this form (Kenix3/libultraship@e42bd5a3, 2022-02-22, @AloXado320), and the only parent present in that history does not contain the file, so there is no earlier state to diff against. It moved here in #2361 / Kenix3/libultraship#99. blawar/ooot has byte-identical code, but got it from here (`added SoH mixer.c to fix audio issues`, 2022-03-23), so it is not independent corroboration.

Two questions:

1. Is the wet-path negation modelled on something concrete -- microcode disassembly, a hardware comparison, a specific bug it fixed -- or is it an artefact?
2. XOR with `-4` (`0xFFFC`) and `-2` (`0xFFFE`) is not a negation of anything: it flips every bit except the low two and the low one respectively. If the intent was "negate", `~x` (XOR `-1`) is the near miss and `-x` the exact one. For what it is worth, `4` and `2` are exactly the bit values of `x1` and `x2` in the command-word packing, which is the shape a flag/mask mix-up would leave -- but that is speculation and I would not act on it.

I am deliberately **not** proposing a patch. With mupen64plus doing no negation at all and sm64-port doing none on the wet path, "correct" has at least two candidate answers, and switching to a plain `* -1` would just be a third guess. Measured against the current behaviour, a plain negate differs by up to 4 LSB (mean 2.67) for the `-4` mask and up to 2 LSB for `-2`, so this is inaudible either way -- it is a correctness question, not an audible bug.

Flagging it because the same code is in 2ship2harkinian, Starship, SpaghettiKart, Ghostship and Lighthouse: if it is wrong, it is wrong in all of them.

@Kenix3 @AloXado320 -- do either of you remember where this came from?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.