dirkwhoffmann / vAmiga

vAmiga is a user-friendly Amiga 500, 1000, 2000 emulator for macOS
https://dirkwhoffmann.github.io/vAmiga
Other
293 stars 24 forks source link

(very minor) Crash with certain extended ADF file #788

Closed mras0 closed 1 year ago

mras0 commented 1 year ago

Testing some stuff where I create the EADF myself, and noticed that it can in certain situations (like this: beemoved_altmfm.zip) crash vAmiga (at least ports). The attached extended ADF works in latest WinUAE (4.9.10b2).

EDIT: Sample it's supposed to be playing is this freely available one: http://helpguide.sony.net/high-res/sample1/v1/data/Sample_BeeMoved_96kHz24bit.flac.zip

dirkwhoffmann commented 1 year ago

I have trouble reproducing the crash. In the Mac version, it seems to play correctly:

Bildschirm­foto 2023-03-26 um 09 52 50

vAmigaWeb (@mithrendal's port) with Aros shows this (which is probably wrong):

Bildschirm­foto 2023-03-26 um 09 53 03

For some reason I haven't been able to install Kick 1.3. in vAmigaWeb.

vAmiga.net does not accept the disk, which is the expected behavior at the moment.

UPDATE: With Kick 1.3. it plays the music in vAmigaWeb, too:

Bildschirm­foto 2023-03-26 um 10 23 58
mras0 commented 1 year ago

Sorry, maybe false positive then, seems to work in vAmigaWeb (I'd forgotten it supported extended ADFs with just the normal extension). It's probably correct behavior in Aros (bootblock is lazy and does that if it can't AllocAbs specific chip memory areas).

Had a closer look, and for me it crashes in EXTFile::encodeStandardTrack for track 161 because adf in EXTFile::encodeDisk only holds the standard 160 tracks. The file can store 164 tracks (even though not that many are used) and unused ones are just left as plain amiga dos ones.

dirkwhoffmann commented 1 year ago

for track 161 because adf in EXTFile::encodeDisk only holds the standard 160 tracks

Thanks for pointing this out! Definitely a bug.

dirkwhoffmann commented 1 year ago

I think I've fixed it. The culprit was this line in EXTFile::encodeDisk:

   // Create an empty ADF
    ADFFile adf(getDiameter(), getDensity()); 

As @mras0 has already pointed out, this statement always creates an ADF with 160 tracks.

The new code looks like this:

    // Create an empty ADF
    ADFFile adf(*this);

It calles a new constructor that creates an ADF with the layout of another FloppyFile:

ADFFile(const class FloppyFile &file) throws { init(file); }

Hmm, while writing this post, I realize that this might not be a good solution, because most people will probably expect that this constructor also copies the data from file. A better solution will be to add a constructor that takes some layout descriptor... I need to think about it again...

Also, I should probably change the name EXTFile to EADFFile. I like the name EADF.

dirkwhoffmann commented 1 year ago

FloppyDiskDescriptor has been added. The new code looks like this:

   // Create an empty ADF
    ADFFile adf(getDescriptor());

This ensures that the created ADF uses the same layout parameters as the EADF.

mras0 commented 1 year ago

Thanks, that fixed it.

The sample seems play slightly different (worse, in my subjective opinion) in vAmiga(Web) compared to WinUAE. That would be a different "issue", and maybe vAmgia is more accurate, but if you want I can open another ticket with info.

dirkwhoffmann commented 1 year ago

The sample seems play slightly different (worse, in my subjective opinion) in vAmiga(Web) compared to WinUAE.

I've compared FSUAE with vAmiga Mac. To my (untrained) ears, they sound very much alike. However, they use different filters internally, so somebody with a better trained ear might still hear a difference.

vAmigaWeb sounds a bit different to me and is much louder. Maybe @mithrendal uses a too large scaling factor with the result that some amplitudes get chopped off.

mras0 commented 1 year ago

I've compared FSUAE with vAmiga Mac. To my (untrained) ears, they sound very much alike. However, they use different filters internally, so somebody with a better trained ear might still hear a difference.

vAmigaWeb sounds a bit different to me and is much louder. Maybe @mithrendal uses a too large scaling factor with the result that some amplitudes get chopped off.

I usually can't make out subtle difference like this, but in this case I think there's an audible difference in the beginning (first couple of seconds) where it shouldn't be close to clipping, though my mind may be playing tricks on me.

Now that you mention filtering, one thing I noticed is there's no filtering applied when the LED is off in vAmiga (which is the case for this test). According to https://eab.abime.net/showthread.php?t=112931 (and UAE source code) there are always low- and high-pass filter applied on A500. WinUAE doesn't seem to implement the high pass filter, but pt2-clone does (and has more information).

This is just info for you consideration, not something I expect you to do anything about :)

For reference the first couple of seconds form WinUAE & vAmigaSDL. For the latter I set the samplerate to 48000 and saved the audio output to a raw file containing (32-bit float stereo). In both cases I've normalized the output with audacity to make sure volume is the same. BeeMoved_samples.zip

dirkwhoffmann commented 1 year ago

This is just info for you consideration, not something I expect you to do anything about :)

Sounds like a nice project to me and I am eager to do it. I think vAmiga would really benefit from having realistic audio filters.

The relevant parts of the pt2-clone source code are crystal clear, so it should be easy to adapt the code. I'll open up another issue for the filter project...

mithrendal commented 1 year ago

Hi @mras0 and @dirkwhoffmann

I have found another ADF which fails to insert ...

https://www.pouet.net/prod.php?which=94129

it works in SAE but not in latest vAmiga Mac v2.4.b1 and also not in v2.3

is it maybe also because of the extended format?

mras0 commented 1 year ago

It's a normal ADF, but it only contains 79 cylinders worth of data (compared to the standard 80). I bet if you pad it with 11264 zero bytes it'll work.

dirkwhoffmann commented 1 year ago
truncate -s +11264 demo.adf
Bildschirmfoto 2023-04-12 um 21 44 48
dirkwhoffmann commented 1 year ago

Part of v2.4