Fix: Audio crash when trying to detect BGM_DISABLED

[Archez]

The introduction of #3080 exposed a bug where we were improperly detecting the BGM_DISABLED sequence.

The bug previously was obscured if you didn't have any custom sequences loaded, as BGM_DISABLED & 0xFF is 255 and there was only like 110 sequences in sequenceMap meaning that AudioLoad_GetFontsForSequence would return null. If you had custom sequences loaded, it was possible that the 255 would then load whatever font data exists for the sequence in that location (undefined behavior), but in Audio_PlayFanfare the font data is never read, only that it is or isn't null, so nothing bad ever happened.

With the voice additions from #3080, without custom sequences loaded, sequenceMap now has a size of 255, meaning that we were attempting to read the font data for BGM_DISABLED & 0xFF which lead to a invalid pointer de-reference and crashing.

When introducing custom sequences we removed one of the & 0xFF from Audio_PlayFanfare and added a check for BGM_DSIABLED to prevent a audio crash. Although removing the other & 0xFF from here will resolve the new audio crash in develop today, there is an incorrect sequence look up happening.

In reviewing all uses of Audio_PlayFanfare there is no such fanfare ID above u8 even though sequence ID types are u16, however sometimes the game will append 0x900 to fanfare requests (Item Gets, opening big treasure chests, etc). This explains the original reason for & 0xFF from the original game as the 0x900 needs to be stripped off before looking up the sequence ID's font data.

In this case, I have opted to bring back the original & 0xFF that we removed, and instead updated of catch for BGM_DISABLED in AudioLoad_GetFontsForSequence to account for seq ID being stripped to u8 range.

I think this is the better approach to resolving the crashes and accounting for authentic behavior.

(I also synced some var names with decomp documentation while updating this)

Build Artifacts

• soh.otr.zip
• soh-linux-compatibility.zip
• soh-linux-performance.zip
• soh-mac.zip
• soh-switch.zip
• soh-wiiu.zip
• soh-windows.zip

[Archez]

Alternatively, we can move the & 0xFF to be inside AudioLoad_GetFontsForSequence before looking up the replacement seqID, if we think that would be cleaner solution. I just chose to go the decomp match route first.

[Malkierian]

How would this approach affect the addition of new sfx/fanfares in the future?

[Malkierian]

I guess I'm not sure what the masking for that lookup is actually doing.

[Archez]

How would this approach affect the addition of new sfx/fanfares in the future?

This would only need to change if we added the ability to have new request IDs, but that is something that has not been brought up before that I know of, and if we got to that point, I imagine there would need to be a lot of changes elsewhere as there many parts of the audio code that assume a max amount of seq IDs being u8.

[garrettjoecox]

Alternatively, we can move the & 0xFF to be inside AudioLoad_GetFontsForSequence before looking up the replacement seqID, if we think that would be cleaner solution.

Either seem better than the current approach. Let’s just plan to keep it like this and revisit if we run into different issues with this new solution.

[garrettjoecox]

Going to go ahead and merge this ahead of the approval timeline as it fixes a common crash