frno7 / psgplay

PSG play is a music player and emulator for the Atari ST Programmable Sound Generator (PSG) YM2149.
24 stars 4 forks source link

Fix replay of SNDH files by !Cube #13

Open frno7 opened 2 years ago

frno7 commented 2 years ago

Discussed in https://github.com/frno7/psgplay/discussions/12

Originally posted by **rnjacobs** May 7, 2022 I was hoping to listen to sndh files composed by !cube - http://cube.gfxile.net/t/index.php?page=journal&journal=YM2149F - but none of them work in psgplay. Am I doing something wrong? Is he doing something unusual? Should they work? Thank you!
frno7 commented 2 years ago

@rnjacobs, I briefly tested the first file Threshold and can confirm that there is some problem replaying it properly. I haven’t yet investigated the cause.

frno7 commented 2 years ago

@rnjacobs, I think it’d be a good idea to make a list of problematic SNDH files. I’ve noticed that Mad_Max/Games/Lethal_Xcess_(STe).sndh, for instance, in the SNDH archive isn’t playing properly, and some others. Next one could try to group these files by their common problem causes, and from there work out fixes for them.

chris-y commented 2 years ago

A couple I've noticed recently:

Lotek_Style/Artefakt.sndh goes beeeep after about 50 seconds. Dubmood/Zabutoms_Bice4.sndh appears to be silent.

frno7 commented 2 years ago

Thanks, @chris-y! I can confirm that both of them are problematic.

frno7 commented 2 years ago

Dubmood/Zabutoms_Bice4.sndh attempts to trap into GEMDOS for Malloc (72) and Mfree (73), and XBIOS Unlocksnd (129). The command psgplay --disassemble --trace=cpu sndh/Dubmood/Zabutoms_Bice4.sndh | grep -C2 trap reveals these traps:

    move.l  d1,-(sp)            ; init
    move.w  #72,-(sp)           ; init
    trap    #1              ; init
    addq.l  #6,sp               ; init
    movem.l (sp)+,d1-d2/a0-a2       ; init
--
    move.l  (a1),-(sp)          ; exit
    move.w  #73,-(sp)           ; exit
    trap    #1              ; exit
    addq.l  #6,sp               ; exit
    movem.l (sp)+,d0-d2/a0-a2       ; exit
--
    movem.l d0-d2/a0-a2,-(sp)       ; exit
    move.w  #129,-(sp)          ; exit
    trap    #14             ; exit
    addq.l  #2,sp               ; exit
    movem.l (sp)+,d0-d2/a0-a2       ; exit
--
    move.l  _992(pc),-(sp)          ; exit
    move.w  #73,-(sp)           ; exit
    trap    #1              ; exit
    addq.l  #6,sp               ; exit
    movem.l (sp)+,d0-d2/a0-a2       ; exit
--
    move.l  _99a(pc),-(sp)          ; exit
    move.w  #73,-(sp)           ; exit
    trap    #1              ; exit
    addq.l  #6,sp               ; exit
    movem.l (sp)+,d0-d2/a0-a2       ; exit
--
    move.l  _a4a(pc),-(sp)          ; exit
    move.w  #73,-(sp)           ; exit
    trap    #1              ; exit
    addq.l  #6,sp               ; exit
    movem.l (sp)+,d0-d2/a0-a2       ; exit
--
    move.l  _afa(pc),-(sp)          ; exit
    move.w  #73,-(sp)           ; exit
    trap    #1              ; exit
    addq.l  #6,sp               ; exit
    movem.l (sp)+,d0-d2/a0-a2       ; exit

PSG play doesn’t have TOS for GEMDOS and XBIOS built-in, which is why Dubmood/Zabutoms_Bice4.sndh misbehaves. I think there are three alternatives to fix this:

  1. Replace/remove these GEMDOS and XBIOS traps from the SNDH file.
  2. Emulate the (hopefully small) subset of GEMDOS and XBIOS traps used by (most if not all) SNDH files.
  3. Boot EmuTOS for the complete package.
frno7 commented 2 years ago

Apparently only four kinds of traps are used in the entire SNDH archive of 4925 SNDH files. The command find sndh -iname '*'.sndh | sort | parallel psgplay --disassemble --trace=cpu 2>/dev/null | awk '$1 == "trap" { print $1, $2, t } { t = $1 " " $2 }' | sort | uniq -c yields

    287 trap #14 move.w #129,-(sp)
      4 trap #14 move.w #31,-(sp)
    179 trap #1  move.w #72,-(sp)
   1266 trap #1  move.w #73,-(sp)

In addition to GEMDOS Malloc (72), Mfree (73), and XBIOS Unlocksnd (129) mentioned, there’s also XBIOS Xbtimer (31). I’d say that’s a small and reasonable subset that’s certainly easy to emulate without having a complete TOS. I’ve registered issue #14 for this.

frno7 commented 2 years ago

I’ve noticed with the -fsanitize=undefined option to GCC there’s an error lib/m68k/m68kops.c:7814:13: runtime error: left shift of negative value -11 at about 48 seconds into Lotek_Style/Artefakt.sndh. That’s a bug in the Musashi Motorola 68000 processor emulation, but it remains to figure out whether or not this is also the cause of the beep.

chris-y commented 2 years ago

I’ve noticed with the -fsanitize=undefined option to GCC there’s an error lib/m68k/m68kops.c:7814:13: runtime error: left shift of negative value -11 at about 48 seconds into Lotek_Style/Artefakt.sndh. That’s a bug in the Musashi Motorola 68000 processor emulation, but it remains to figure out whether or not this is also the cause of the beep.

I think this is the one somebody commented to me that it looks like the 68k emulation has crashed and the YM has carried on... that would fit with what you're saying.

frno7 commented 2 years ago

I think this is the one somebody commented to me that it looks like the 68k emulation has crashed and the YM has carried on... that would fit with what you're saying.

It’s puzzling, because with the GEMDOS emulation today Lotek_Style/Artefakt.sndh plays without beeping until 01:23, which is more than 30 seconds longer than before, and it doesn’t even attempt to trap into the GEMDOS. I’m currently unable to explain why it ‘improved’ too.

The --trace=cpu option seems to indicate that the m68k processor happily runs about when the beep occurs, so it doesn’t halt. More like it’s running amok, perhaps as a result of a computational malfunction, such as an incorrect result due to the left shift of negative value -11 for a division instruction. Another alternative is an interrupt malfunction of some kind, which would be a problem with the MFP emulation in that case. Hmm...

frno7 commented 2 years ago

@chris-y, some songs apparently consumes major amounts of stack space, and 2 KiB is not quite enough to satisfy their demands. With the GEMDOS Malloc issue #14 code already committed, and an absolutely massive 96 KiB of stack space, Dubmood/Zabutoms_Bice4.sndh plays nicely. Commits are forthcoming. Not only that, @rnjacobs’ songs by !Cube are yielding to treatment too. :-)

frno7 commented 2 years ago

I’ve added a stack bounds check with the --trace option in commit 745aec7b5dd16b1aa6ba9432216142d9b090f570. With the previous 2 KiB stack it printed an error such as Dubmood/Zabutoms_Bice4.sndh: stack bounds error usp 4096 isp 98. Commit 3b0ff933c522a308e9b2dc067a422bb8ec519606 increases stack space to 96 KiB, separately for the user (USP) and interrupt (ISP) stack pointers, to avoid running out of stack.

With this, Dubmood/Zabutoms_Bice4.sndh should play perfectly. Lotek_Style/Artefakt.sndh plays for 30 seconds longer, and !Cube’s Threshold.sndh emits, well, something similar to the original, while not exactly crashing, strangely.

frno7 commented 2 years ago

Commit cc6bb26f74e16cf2a709e0cc17bafc0477267787 improves Mad_Max/Games/Lethal_Xcess_(STe).sndh mentioned, and others.

rnjacobs commented 2 years ago

One problem: right now, the script/tos script depends on xxd to compile the "tos" library, but nothing checks that xxd exists or that the resulting tos.h is plausible. So things appeared to compile and work fine, but songs that depended on those traps were still silent.

Installing xxd fixed everything. And now I can enjoy more sndh music in a TUI!

frno7 commented 2 years ago

@rnjacobs, I’ve fixed the command failure in commit 9ebedf0d998530d00078a983fa0318a839d805d4. I’ve also made xxd configurable in commit 521b4e63e9a95166525355ffbe6d127f89856909, and more reliable with a temporary file in commit 73e71d6891a2a90fc68ff42103ade34044af1292.

Didn’t the precompiled lib/tos/tos file that comes with the repo handle the traps?

rnjacobs commented 2 years ago

Yeah, I should have said "depends on xxd to make the 'tos' library embeddable via tos.h"

chris-y commented 1 year ago

Not sure if this is a problem file or not: Daglish_Ben/Footballer_of_the_Year_2.sndh It plays a couple of seconds then the rest is silent.

frno7 commented 1 year ago

Not sure if this is a problem file or not: Daglish_Ben/Footballer_of_the_Year_2.sndh It plays a couple of seconds then the rest is silent.

No, I suppose track 1 actually is a short sound effect. Track 2 is similar, definitely sounding like a game over effect. Track 11 is some kind of tune, of about 2 minutes, though. :-)

chris-y commented 1 year ago

OK, I see what it is up to in interactive mode. I've raised a new issue as it isn't relevant to this one.

chris-y commented 1 year ago

Is this the same issue or a different one? Starts whistling after a few seconds. 505/DMA/Saboteaser.sndh

frno7 commented 1 year ago

Is this the same issue or a different one? Starts whistling after a few seconds. 505/DMA/Saboteaser.sndh

It plays nicely for 10 seconds, and then silence, when I try it. According to the timedb.inc.h table, its length is 1 frame, but I’m not quite sure what that actually means:

TIMEDB_ENTRY( 1d103200, 1, 1, STE), /* 04646 - 505 - Saboteaser */

@benjihan?

chris-y commented 1 year ago

Another one here which I think is just playing the first few seconds then stopping. It has a TIME tag on this, maybe that's being misinterpreted? 505/Psycho.sndh

frno7 commented 1 year ago

Another one here which I think is just playing the first few seconds then stopping. It has a TIME tag on this, maybe that's being misinterpreted? 505/Psycho.sndh

Hmm, it plays perfectly for me. My psgplay --version says psgplay version 0.5. What about yours?

chris-y commented 1 year ago

psgplay version 0.4-76-gf44740e I was setting the track to play (that file only has one) using -t 1. I'm not able to test this properly at the moment to see if that was causing it.

frno7 commented 1 year ago

psgplay -t 1 505/Psycho.sndh plays nicely with psgplay version 0.5, it appears.

chris-y commented 1 year ago

I see what I've done... Nothing to see here! 🫣

chris-y commented 1 year ago

This one goes a bit weird around a minute in: 505/Relix/BRN_2010.sndh

frno7 commented 1 year ago

This one goes a bit weird around a minute in: 505/Relix/BRN_2010.sndh

Yeah. As a reference, I played it with SND Player and Hatari. Still somewhat weird, but less so. I assume there’s some problem with PSG play there... :-)

chris-y commented 1 year ago

This glitches after 15 seconds or so: Tao/Steps/Just_Feel_It.sndh

Should sound like: http://sndhrecord.atari.org/mp3/Tao/Steps/Just_Feel_It.mp3

frno7 commented 1 year ago

This glitches after 15 seconds or so: Tao/Steps/Just_Feel_It.sndh

Confirmed! Needs some investigation.

frno7 commented 1 year ago

This glitches after 15 seconds or so: Tao/Steps/Just_Feel_It.sndh

@chris-y, please replay after removing line 190, having timer->timeout.c = 0;, in the file lib/atari/mfp.c, here:

https://github.com/frno7/psgplay/blob/51063ade6b9fc29364e3252772b5baaea28b193b/lib/atari/mfp.c#L188-L190

Does that sound better? I believe it’s a fix, by code removal. :-) There’s a FIXME on line 188, however, so something else may need to be done, eventually.

I’m testing it with some unrelated improvements I’m preparing.

frno7 commented 1 year ago

This one goes a bit weird around a minute in: 505/Relix/BRN_2010.sndh

Incidentally, that one also plays lovely with the fix suggested in https://github.com/frno7/psgplay/issues/13#issuecomment-1556709690, no?

chris-y commented 1 year ago

Yes, that looks like it fixes it!

frno7 commented 1 year ago

Thanks, @chris-y! Fix in commit aabcdc3318e07544d37ea559ac721ee71a7ae083.

frno7 commented 1 year ago

For the protocol: I’ve noticed that

sound quite bad, and so they remain to be investigated and corrected.

frno7 commented 1 year ago

@chris-y, commit 863f1113bf3f1c968ca6f6205f67276009e66240 significantly improves audio quality during start and stop. The issue is that the YM2149 PSG hardware fundamentally outputs a unipolar signal that has to be releveled for bipolar stereo samples. Anyhow, PSG play now has 10 ms logistic sigmoid fade in/out during this transformation, to avoid a sharp and loud noise every time a track starts or stops. Listening carefully, one can still hear a muffled thump, somewhat reminiscent of a pickup needle hitting a vinyl plate. :-)

There are also - and + keys to control volume with commit cad18e74e6bfe661afe9414fed8a2ce341d91b59. I’m considering adding bass and treble control keys as well, since tone shaping eventually will be done for the Atari STE LMC1992 hardware.

frno7 commented 1 year ago

For the protocol: I’ve noticed that Tao/Steps/Ride_The_Sky.sndh, and Tao/Steps/Rise.sndh sound quite bad, and so they remain to be investigated and corrected.

Fixed their envelope shapes in commit cbe1d2c4d1cae8fbb962a74db4e35bbcdbbb16fa! In fact, I believe this fix is a major improvement for a lot of SNDH files.

By the way, here’s a vector of SNDH files that put a PSG player through its paces:

:-)

benjihan commented 1 year ago

Sorry I didn't see the notification. Probably a mistake. sc68 STE/DMA/LMC simulation is very limited. Specially DMA sound only usually do not work. In that case the detection program got silence in input. I'm guessing that's what it is. I should probably remove those from the database (or add them manually).

chris-y commented 8 months ago

This file is just playing a tone. I updated to the latest as I was a bit of of date, but no change: Tao/TSD_STe/Intensity.sndh

frno7 commented 8 months ago

Thanks, @chris-y! Indeed, Tao/TSD_STe/Intensity.sndh doesn’t sound good. Several, if not all, files in the Tao/TSD_STe directory are apparently problematic. Interesting. I will need to debug them all. :-)

chris-y commented 5 months ago

Another one for the collection: Count_Zero/Kohl_Ohr_Schock.sndh

This one seems to just play a tone.

frno7 commented 5 months ago

Oh, thanks @chris-y. Indeed, Count_Zero/Kohl_Ohr_Schock.sndh is problematic, but silent for me. I’ll debug it.

I’m thinking about automating tests for all SNDH files.