Emulator

lunaboy0, thanks for this investigation. Would you please submit a patch to https://github.com/dolphin-emu/dolphin/pulls with the suggested fix? I would guess we add a condition using acc_pb->audio_addr.sample_format to determine whether to compare with or without the ~1.

Suggested fix sounds good to me, the explanation makes a lot of sense.

I think there was another bug open about SoA being out of pitch, can
someone confirm it's related and dedup?

After taking a closer look at the sampledata reading code, it turns out that the loop pointers are always per-sample based, even for 16 bit samples.
When reading 16 bit samples, the read pointer is multiplied by two, so for 16-bit PCM audio it is guaranteed that ARAM reads are multiples of two bytes.
This means that forcing the lower bit of the loop pointers to zero will always force them to be multiples of two, regardless the sampleformat.

The multiple-of-two-bytes loop pointers commit looks like to be an attempt to fix the 1:32 hours audio-crackling bug (related to issue 6160).
Since this bug was caused by the mixercode and has nothing to do with instrument sampleloops, it looks like this commit didn't fix anything at all.
Revision a1822a3acaf112dd37ed98818acdc0bc5014caba is the real fix of the 1:32 hours audio-crackling bug.

As Delroth said, there is another bugreport (issue 6175) about pitch detuning, but that one is related to DspLLE, while my issue is related to DspHLE.
The pitch detuning of the Horteka instruments is also present in DspLLE.

I've been able to fix the Horteka instruments by replacing the line 'if ((Address & ~1) == (EndAddress & ~1))' with the original 'if (Address >= EndAddress)'.
Replacing this line in AXVoice.h fixes the detuning on DspHLE (this issue)
Replacing this line in DSPAccelerator.cpp fixes the detuning on DspLLE (issue 6175)

Beside Skies of Arcadia, I've also tested several other games (including games that uses PCM16 audio like MarioKart Double Dash and Super Mario Galaxy 2) and I didn't hear any loop errors.

It looks like it is safe to use the original end-of-loop check in both AXVoice.h and DSPAccelerator.cpp and get per-sample looping back.
Do you devs agree with me?

Issue 6175 has been merged into this issue.

The ~1 is for the capcom music loop bug (Issue 3998). I am quite sure that >= is not correct, but == is. Using == might work now with the audio latency fix in place. i.e.

if (Address == EndAddress)

Issue 3998 is the one that needs to be retested. MKDD and SMG2 do not present the bug described in Issue 3998.

Using "if (Address == EndAddress)", "FAST: Racing League" plays random sounds in the in-game music.

With "if (Address >= EndAddress)", the PN03 in-game music cuts out after playing through once.

Tested on Dolphin 4.0-1546.

Other games that this code affects are:

Resident Evil 3 (cutscenes)
Muramasa: The Demon Blade (high pitched squeal in scene transitions)

Maybe it's time to start doing some proper RE of the accelerator with
hardware testing.

lunaboy0, would you please check if the Skies of Arcadia music instruments are in tune after applying this patch? The patch applies to Dolphin 4.0-2793. Also, would you please re-upload the gci save file somewhere? The link on zippyshare has expired.

==========================
diff --git a/Source/Core/Core/DSP/DSPAccelerator.cpp b/Source/Core/Core/DSP/DSPAccelerator.cpp
index c330d40..2dd1a2b 100644
--- a/Source/Core/Core/DSP/DSPAccelerator.cpp
+++ b/Source/Core/Core/DSP/DSPAccelerator.cpp
@@ -111,6 +111,7 @@ u16 dsp_read_accelerator()
const u32 EndAddress = (g_dsp.ifx_regs[DSP_ACEAH] << 16) | g_dsp.ifx_regs[DSP_ACEAL];
u32 Address = (g_dsp.ifx_regs[DSP_ACCAH] << 16) | g_dsp.ifx_regs[DSP_ACCAL];
u16 val;

• u8 audio_size = 0;

  // let's do the "hardware" decode DSP_FORMAT is interesting - the Zelda
  // ucode seems to indicate that the bottom two bits specify the "read size"
  @@ -122,17 +123,20 @@ u16 dsp_read_accelerator()
  {
  case 0x00: // ADPCM audio
  val = ADPCM_Step(Address);

•   	audio_size = 1;
    	break;
    case 0x0A:  // 16-bit PCM audio
    	val = (DSPHost::ReadHostMemory(Address*2) << 8) | DSPHost::ReadHostMemory(Address*2 + 1);
    	g_dsp.ifx_regs[DSP_YN2] = g_dsp.ifx_regs[DSP_YN1];
    	g_dsp.ifx_regs[DSP_YN1] = val;
  

•   	audio_size = 1;
    	Address++;
    	break;
    case 0x19:  // 8-bit PCM audio
    	val = DSPHost::ReadHostMemory(Address) << 8;
    	g_dsp.ifx_regs[DSP_YN2] = g_dsp.ifx_regs[DSP_YN1];
    	g_dsp.ifx_regs[DSP_YN1] = val;
  

•   	audio_size = 0;
    	Address++;
    	break;
    default:
  

@@ -151,7 +155,7 @@ u16 dsp_read_accelerator()
// Somehow, YN1 and YN2 must be initialized with their "loop" values,
// so yeah, it seems likely that we should raise an exception to let
// the DSP program do that, at least if DSP_FORMAT == 0x0A.

• if ((Address & ~1) == (EndAddress & ~1))

• if ((Address) == (EndAddress + audio_size))
  {
  // Set address back to start address.
  Address = (g_dsp.ifx_regs[DSP_ACSAH] << 16) | g_dsp.ifx_regs[DSP_ACSAL];
  diff --git a/Source/Core/Core/HW/DSPHLE/UCodes/AXVoice.h b/Source/Core/Core/HW/DSPHLE/UCodes/AXVoice.h
  index 73f20f4..c791f4c 100644
  --- a/Source/Core/Core/HW/DSPHLE/UCodes/AXVoice.h
  +++ b/Source/Core/Core/HW/DSPHLE/UCodes/AXVoice.h
  @@ -149,12 +149,16 @@ void AcceleratorSetup(PB_TYPE* pb, u32* cur_addr)
  u16 AcceleratorGetSample()
  {
  u16 ret;

• u8 audio_size = 1;

• if (acc_pb->audio_addr.sample_format == 0x19)

•   audio_size = 0;
  

  // Have we reached the end address?
  //
  // On real hardware, this would raise an interrupt that is handled by the
  // UCode. We simulate what this interrupt does here.

• if ((*acc_cur_addr & ~1) == (acc_end_addr & ~1))

• if ((*acc_cur_addr) == (acc_end_addr + audio_size))
  {
  // loop back to loop_addr.
  *acc_cur_addr = acc_loop_addr;

==========================

Skidau,

Sorry for the late reaction, but Google was grouping conversation messages to one line in my inbox, causing new messages to slip through unnoticed.
When I read about this fix in the September progress report, I began testing immediately.

I've tested Horteka and several other places in Skies of Arcadia and I didn't hear any audio detuning.
Maybe it's not needed anymore, but here is a re-upload of the GCI save file on Zippyshare:
http://www41.zippyshare.com/v/37002723/file.html

Thank you for fixing this issue. Excellent work!

lunaboy0, would you please test this version and let me know if the instruments are in tune?

https://dl.dolphin-emu.org/prs/pr-1245-dolphin-latest-x64.7z