milkytracker / MilkyTracker

An FT2 compatible music tracker
http://milkytracker.github.io/
Other
1.67k stars 160 forks source link

When saving sample to WAV the loop end is off by one #338

Closed jiffygist closed 3 months ago

jiffygist commented 6 months ago

I think the loop end when exporting looped sample to WAV the loop end should be one byte less than it is in MilkyTracker like it is done in OpenMPT and ft2-clone.

For example I looped the first 3 bytes and I exported the sample to a wav file for which sndfile-info reports the loop end is 3. When I open this file in OpenMPT the first 4 bytes are looped and when I correct the loop back to first 3 bytes and export to wav, the loop end in the new wav file is 2.

Also when the loop end is at the end of the sample (equal to sample length), the loop will be ignored in ft2-clone which expects it to be at max sample length - 1.

Wave format descriptions 1 2 say that this value should be "byte offset into the waveform data of the last sample to be played in the loop".

coderofsalvation commented 3 months ago

thanks for reporting this. so when exporting/importing the .xm across milky/mpt this inconsistency does not happen?

jiffygist commented 3 months ago

No, for samples in the xm files everything works correctly.

It is only a problem with wav export. ft2-clone and OpenMPT subtract one when exporting and milky does not https://github.com/milkytracker/MilkyTracker/blob/master/src/milkyplay/SampleLoaderWAV.cpp#L669 https://github.com/8bitbubsy/ft2-clone/blob/master/src/ft2_sample_saver.c#L405 https://github.com/OpenMPT/openmpt/blob/master/soundlib/WAVTools.cpp#L324

coderofsalvation commented 3 months ago

so I guess the correct fix would be changing:

sampleLoop.dwEnd = smp->loopstart + smp->looplen;

into

sampleLoop.dwEnd = smp->loopstart + smp->looplen-1;

As looplen already contains loopstart. WDYT?

jiffygist commented 3 months ago

Yes, and in loadSample also

mp_dword end = sampleLoop.dwEnd + 1 /* / (hdr.numBits*hdr.numChannels / 8)*/;
coderofsalvation commented 3 months ago

OK this is fixed in https://github.com/milkytracker/MilkyTracker/commit/05df5bdf8a2d544027d8c4e7ec39c4dcdeb960ee

I did some testing with importing/exporting and it works.

coderofsalvation commented 3 months ago

btw. do you happen to be able to compile https://github.com/milkytracker/MilkyTracker/tree/rc/1.05 at all?

jiffygist commented 3 months ago

Works, thanks

coderofsalvation commented 3 months ago

np, its the release candidate (rc)

On Tue, Mar 26, 2024, 17:51 Herman @.***> wrote:

Works, thanks

— Reply to this email directly, view it on GitHub https://github.com/milkytracker/MilkyTracker/issues/338#issuecomment-2020972852, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABL6ZAUUPS35JPRWFCJNCDY2GRRRAVCNFSM6AAAAABA4WM572VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRQHE3TEOBVGI . You are receiving this because you modified the open/close state.Message ID: @.***>