Author Topic: Minor conversion error  (Read 11369 times)

saga

  • Posts: 2754
Minor conversion error
« on: 6 Nov '15 - 14:13 »
While playing around with writing my own UNMO3 implementation I have come across the following errors in UNMO3 and MO3:

1. The UNMO3 decoder does not seem to guard against out-of-range parameter values, in particular for the XM volume command. Let's say the original XM file has a volume command in the effect column with a parameter value larger than 40. When decoding the MO3 file back to XM, this value is written to the volume column, but its parameter is not validated, so as a consequence an entirely different effect is written to the volume column.

2. Also in XM, MO3 stores the panbrello command (Y) correctly, but UNMO3 converts it back to P (panning slide) instead.

3. Similarly, command Z in the S3M format is stored correctly in the MO3 file but UNMO3 converts it back to command S instead.

4. Volume-column panning in the S3M format is not converted correctly by MO3. It stores it as volume command (F 40 in the MO3 file).

5. Also, technically, UNMO3 should really write out a "beats per track" value in the MTM header of 64, not 0.

6. There also seems to be an issue with the MIDI channel setting, at least in IT instruments (not tested with XM yet): In IT, no MIDI channel = 0, first = 1, last = 16, mapped = 17. However, It seems that in the MO3 file both "no channel" and the first channel are encoded as 0, the second channel is 1, etc...
« Last Edit: 17 Nov '15 - 15:02 by saga »

Ian @ un4seen

  • Administrator
  • Posts: 26108
Re: Minor conversion error
« Reply #1 on: 6 Nov '15 - 16:58 »
1. The UNMO3 decoder does not seem to guard against out-of-range parameter values, in particular for the XM volume command. Let's say the original XM file has a volume command in the effect column with a parameter value larger than 40. When decoding the MO3 file back to XM, this value is written to the volume column, but its parameter is not validated, so as a consequence an entirely different effect is written to the volume column.

2. Also in XM, MO3 stores the panbrello command (Y) correctly, but UNMO3 converts it back to P (panning slide) instead.

Oops! I'll fix that.

3. Similarly, command Z in the S3M format is stored correctly in the MO3 file but UNMO3 converts it back to command S instead.

It seems to be converted back to Zxx here, but perhaps something else is missing? I can see them in OpenMPT, but they don't seem to have any effect anymore.

4. Volume-column panning in the S3M format is not converted correctly by MO3. It stores it as volume command (F 40 in the MO3 file).

Yep, MO3 (and BASS/XMPlay) currently limits the S3M volume column to 64 (max volume), as that's how ST3 and IT treat it. Perhaps there's a way to detect an S3M file that was created with OpenMPT, so that the volume column can be handled differently then?

Also, technically, UNMO3 should really write out a "beats per track" value in the MTM header of 64, not 0.

I'll fix that too.

There also seems to be an issue with the MIDI channel setting, at least in IT instruments (not tested with XM yet): In IT, no MIDI channel = 0, first = 1, last = 16, mapped = 17. However, It seems that in the MO3 file both "no channel" and the first channel are encoded as 0, the second channel is 1, etc...

There is indeed an issue with a MIDI channel 1 setting being changed to "off" in IT files (channel 2 and above should be retained fine). The problem goes back to the fact that the IT setting was mapped to the existing XM setting (XM support came first), and XM has 0 = channel 1. XM has a separate MIDI enabling switch, and MO3 is setting an instrument flag (0x01) for that, but it's not doing the same for IT. I don't remember if there was a particular reason for that, but I can't think of any reason now, so I'll enable the flag for IT files too in future.

saga

  • Posts: 2754
Re: Minor conversion error
« Reply #2 on: 6 Nov '15 - 17:08 »
Quote
Yep, MO3 (and BASS/XMPlay) currently limits the S3M volume column to 64 (max volume), as that's how ST3 and IT treat it. Perhaps there's a way to detect an S3M file that was created with OpenMPT, so that the volume column can be handled differently then?
Wouldn't it make more sense to just interpret values 65...129 always as a panning command in S3M files? I think OpenMPT was actually not the first tracker to support panning in the volume column, so that would be a more elegant solution - especially since you cannot actually enter invalid (>= 65) volume commands in ScreamTracker. So if there are panning commands in an S3M file, they should be interpreted as such, or the file is corrupted. The latter case can be ignored.

Ian @ un4seen

  • Administrator
  • Posts: 26108
Re: Minor conversion error
« Reply #3 on: 6 Nov '15 - 17:53 »
Mightn't that result in something similar to issue #1 above, ie. invalid parameter resulting in wrong effect?

saga

  • Posts: 2754
Re: Minor conversion error
« Reply #4 on: 6 Nov '15 - 18:13 »
Quote
It seems to be converted back to Zxx here, but perhaps something else is missing? I can see them in OpenMPT, but they don't seem to have any effect anymore.
Nevermind, I found the cause for the issue - It's a hack in OpenMPT's S3M loader to detect certain S3M files which use Z00-Z0F commands as panning commands, and I tried the conversion with such low values - so they were automatically converted to panning commands.
The Zxx commands do nothing because UNMO3 identifies itself as ScreamTracker 3.01. OpenMPT clears the MIDI macro contents for S3M files that are not made with Impulse Tracker 2.14, Schism Tracker or OpenMPT.

Quote
Mightn't that result in something similar to issue #1 above, ie. invalid parameter resulting in wrong effect?
Well, trying to reason about broken files is always difficult. But I cannot recall ever coming across a real S3M file which contained volume commands in the 65-129 range that were not supposed to be panning commands. So I'd say that always treating these commands as panning should be a safe thing to do.
« Last Edit: 6 Nov '15 - 18:22 by saga »