search

sezero / mikmod

Mikmod Sound System (mirror of git repo at https://sf.net/projects/mikmod/)
http://mikmod.sourceforge.net/
72 stars 22 forks source link

Every pattern loaded from .FAR files is truncated by 2 rows #5

Closed AliceLR closed 3 years ago

AliceLR commented 4 years ago

Not sure if this is the correct place to report issues for MikMod, but I've been doing some testing with less common formats and found this bug. Every .FAR file (two attached) I load with MikMod has two rows truncated from the end of each pattern.

This line appears to be the culprit: https://github.com/sezero/mikmod/blob/master/libmikmod/loaders/load_far.c#L253 I haven't found decent documentation on this format yet, but as far as I can tell from looking at libmodplug and libxmp this is NOT the pattern row count. Modplug ignores this byte altogether and libxmp adds 1 to it and uses it as the row index for the final row of the pattern (i.e. it is 2 less than the effective row count). Both libraries use ([pattern size from the table] - 2)/64 to calculate the actual pattern row count.

budda1.far.zip rainruin.far.zip

sezero commented 4 years ago

Not sure if this is the correct place to report issues for MikMod, but

Usually at https://sourceforge.net/p/mikmod/bugs/ but this place is OK too.

but I've been doing some testing with less common formats and found this bug. Every .FAR file (two attached) I load with MikMod has two rows truncated from the end of each pattern.

This line appears to be the culprit: https://github.com/sezero/mikmod/blob/master/libmikmod/loaders/load_far.c#L253 I haven't found decent documentation on this format yet, but as far as I can tell from looking at libmodplug and libxmp this is NOT the pattern row count. Modplug ignores this byte altogether and libxmp adds 1 to it and uses it as the row index for the final row of the pattern (i.e. it is 2 less than the effective row count). Both libraries use ([pattern size from the table] - 2)/64 to calculate the actual pattern row count.

Will try looking at other libs and correct the issue soon.

AliceLR commented 4 years ago

Usually at https://sourceforge.net/p/mikmod/bugs/ but this place is OK too.

Good to know, thanks. I encountered several other (unrelated) bugs as well, so once I get a chance to give them a closer look expect some more reports. Are pull requests to this mirror okay too?

sezero commented 4 years ago

Are pull requests to this mirror okay too?

Yes, I will handle them and mirror the results.

AliceLR commented 3 years ago

Minor update because I somehow missed the obvious documentation in two places I should have known to look: there are two readily available docs on the .FAR format and they report conflicting/vague information about the byte in question. Both report that the number of rows in each pattern is (x - 2)/(4*16) where x is the value from the pattern length array.

Break location in the pattern (length in rows)

This is reported by the document available in libxmp's docs folder and on modland.com. It contradicts itself (if it's a break location, it isn't the length!) but is the closer to correct of the two as far as I can tell and explains why libxmp inserts a break. https://github.com/cmatsuoka/libxmp/blob/master/docs/formats/far100.doc https://modland.com/pub/documents/format_documentation/Farandole%20Composer%20(.far).txt

Length of pattern in rows ="LIR"

Is reported by the second doc available on modland.com. It proceeds to say that there are 4*"LIR" bytes of pattern data, which contradicts what it says earlier re: the pattern length table (and even if "LIR" was reliable, it should be 16*"LIR"...). https://modland.com/pub/documents/format_documentation/Farandole%20Composer%20(.far)%20%232.txt

AliceLR commented 3 years ago

Update on this: I wrote a utility to analyze the 36 .FAR files available on modland.com. Every break byte in these modules is 2 less than the calculated length. https://gist.github.com/AliceLR/0ae4db93779ab5a0e84cae3dadf9498c

To see how Farandole Composer handles the break byte, I hex edited a copy of the rain in the ruin.far linked above to have a break byte of 0x0E instead of 0x1E. Farandole Composer truncates the pattern using row (break byte + 1) as the final row. When the module is resaved, the data length of the pattern is resized to correspond to the truncated length. In other words, the length should never be more than 2 rows higher than the break byte and if it is, Farandole Composer will throw away anything past that.

rr_break rr_edit.far.zip

If the break byte is larger than the data row count Farandole Composer extends the pattern with empty data. I think it's a fairly safe bet nothing ever relied on this and the minimum of the calculated length and the break byte + 2 can be used.

PR incoming shortly.

sezero commented 3 years ago

OK, I intend to merge your patches tomorrow or the other day. If you give me your full name, I can give credit to you in the NEWS file properly. (Otherwise, I'll just reference you as AliceLR.)

AliceLR commented 3 years ago

"Alice Rowan" or "Lachesis" would be OK for the credits.

FWIW I also have two more patches pending: I intend to patch the other STM issue I reported, and I'm currently testing a fix for the .MED 9xx command (the OctaMED format is tricky so I'm trying to be even more careful than usual). Apologies for all of the spam, I encountered most of these issues by accident or while working on other patches!

I'm also interested in implementing a handful of misc. things that fall more into "non-trivial fix" or "feature request" territory (e.g. Farandole Composer's tone portamento effect) so I'll probably hold off on those for now.

sezero commented 3 years ago

You just keep sending patches (there is no new release date set to stone.)

If a patch is not yet ready for merge, mark it so (like do-not-apply, or something.)

sezero commented 3 years ago

"Alice Rowan" or "Lachesis" would be OK for the credits.

Patches from the seven PRs here are merged. Changelog updated: https://github.com/sezero/mikmod/commit/edc5551d1a197314ecad2535ec71f26251fc7a86