MiSTer-devel / TurboGrafx16_MiSTer

TurboGrafx-16 CD / PC Engine CD for MiSTer
94 stars 57 forks source link

CD Audio Glitch - CDDA FIFO has samples in it, but should be empty #206

Closed dentnz closed 3 months ago

dentnz commented 3 months ago

Example of issue: Rayxamber 2, maybe others?

Video of the audio glitch happening and a core containing this PR with audio glitch removed: https://www.youtube.com/watch?v=Lm8VBf3Q6so

After dying in the game, CD audio would fade out, and upon playing the audio track from the beginning, there would be a momentary section of audio playback. This audio sounded to me like some audio samples that were left in the CDDA FIFO buffer. Upon checking the logic, it appears that FIFO buffer is assumed to be empty or drained, particularly when muting and unmuting CD audio. I believe that to not always be the case.

This PR attempts to resolve the problem by enabling the SCLR port of the SCFIFO megafunction, setting it high to flush the CDDA FIFO forcefully. It does this when RST_N is low or CD_STOP_CD_SND is high.

dshadoff commented 3 months ago

I wasn't aware that there were still games with this FIFO issue - but I confirmed even with my own copy that this does occur.

This appears to be related to issue #146 , but may be a slightly different entry case (issue 146 was pause before fade was complete).

My fix at the time was to let the FIFO empty itself while muted, which wasn't ideal - I would have chosen a clear if the FIFO had a CLR signal at the time, I hadn't thought to regenerate the FIFO.

Assuming this PR has no other side-effects, I will go back and amend my fix to 146 after this is accepted.

Regarding the additional "pops" which you identify in your video (which this PR doesn't address), I don't know what those are; I don't think I have them on my rip, but I will need to do a more thorough test to verify. Please open a separate issue for that, and I will look into it and respond.

dentnz commented 3 months ago

Thanks David,

I definitely think the sclr signal at reset is correct..

For the "no sound" is that used to mute/unmute audio by games? Or is it handled some other way with a proper command? There might be a side effect of clearing the buffer and a game "unmuting".

You mentioned use of the flush in a previous commit... perhaps adding it there alone would be more accurate? I'll have a look at your linked commits and try a build to see (hear) shortly.

In my limited testing, I didn't notice any further issues in the build, other than the popping. I only noticed this popping in Rayxamber 2 so far, but will expand the test cases and confirm on real hardware. Will raise an issue for the popping with this detail if required.

dentnz commented 3 months ago

Ah, seems the "no sound" is used in a "pause" scenario. I think clearing the fifo should be fine then.

How would you amend your logic in 146 to include the signal? By issuing the fifo flush from the scsi VHDL component? Might be better to leave it to the cd VHDL, since it is aware of the fifo, while the scsi is not IIRC.

Will get back to you about my testing shortly.

Thanks again!

dshadoff commented 3 months ago

To be clear, the original machine can have pops at various times (especially PSG, and the Hu6280 revision of the CPU), so I'll be checking both my own rips on MiSTer but also original hardware with the original disc.

For the "no sound" is that used to mute/unmute audio by games? Or is it handled some other way with a proper command? There might be a side effect of clearing the buffer and a game "unmuting".

Sorry, I don't follow. What "no sound" are you referring to here ?

I would be amending the logic in 146 to clear the FIFO (I'll need to look at it to determine the best spot). As there is no FIFO on real hardware (and the head can only do one thing at a time), a PAUSE, CD SEEK, or data READ command would cause audio playback to stop. Usually there is a FADE in place during these times, but apparently not always.

dentnz commented 3 months ago

Sorry, I don't follow. What "no sound" are you referring to here ?

CD_STOP_CD_SND

I would be amending the logic in 146 to clear the FIFO (I'll need to look at it to determine the best spot). As there is no FIFO on real hardware (and the head can only do one thing at a time), a PAUSE, CD SEEK, or data READ command would cause audio playback to stop. Usually there is a FADE in place during these times, but apparently not always.

As I said, if you plan to add this signal to the scsi.vhd module, it would mean adding a port to it. There's already the "STOP_CD_SND" port in play, and the cd.vhd logic instantiates the CDDA fifo. From an "abstraction" point of view, it's probably better that the scsi module isn't aware of the CDDA FIFO.

I think my fix should cover any use of the STOP_CD_SND signal, so perhaps just amend the PAUSE, CD_SEEK, READ commands to use that signal. Or perhaps, that's already done and no further change is required?

@sorgelig thanks for merging that in!

dshadoff commented 3 months ago

OK, I had a chance to review the code more closely. scsi.vhd was identifying when the sound should be stopped (due to pause), and cd.vhd was then taking action by muting the sound - your clearing of the FIFO at this point is the correct fix, and nothing more needs to be done about issue 146.

However, I did notice that your regeneration of the FIFO memory allocated a smaller size (this was not easy to see with the pull request web view): lpm_numwords changed from 8192 to 4096, and lpm_widthu changed from 13 to 12 This probably won't make a huge difference on most systems, but I wonder if some users will now find choppy noise during playback related to possible buffer underrun from slow media.

...Perhaps "wait and see" ?

dentnz commented 3 months ago

OK, I had a chance to review the code more closely. scsi.vhd was identifying when the sound should be stopped (due to pause), and cd.vhd was then taking action by muting the sound - your clearing of the FIFO at this point is the correct fix, and nothing more needs to be done about issue 146.

Great! Yeah, I think the change to cd.vhd will suffice.

However, I did notice that your regeneration of the FIFO memory allocated a smaller size (this was not easy to see with the pull request web view): lpm_numwords changed from 8192 to 4096, and lpm_widthu changed from 13 to 12 This probably won't make a huge difference on most systems, but I wonder if some users will now find choppy noise during playback related to possible buffer underrun from slow media.

...Perhaps "wait and see" ?

hmmm I was trying to figure out the correct sizing for the buffer from the megafunction initially. But then, I just opened the VHD directly with Quartus. I figured it would use what was there already there, and that me adding the new port was the only change added.

I guess 8kb is better than 16kb in terms of space on the FPGA, so let's "wait and see" as you say. We can always increase it back to 16kb if needed. 16kb seems a bit overkill really though.

I'll have a look at creating an issue for the popping sound as soon as I can.