dMZX Forums: Bug in validate20.cpp -> Archived MegaZeux Bugs -> Tracker

Line 307 of validate20.cpp is wrong. It says "if (flags & 0x40)" but it should be "if (flags & 0x20)".

(There are also a few bugs in player20.cpp having to do with playback of files containing MIDI instruments, although you may consider that to be unimportant for MegaZeux.)

Updating status to: Confirmed
Updating severity to: 2 - Fair

Good catch, I'll fix this soon. I can patch the MIDI issues too, though I doubt MZX will ever support MIDI instruments.

For reference, the corresponding line in the original source is 151. Did you report this to Reality too? If not, I can go ahead report it to them. I'll also report it to AdPlug if needed, as last I checked they have a pending PR using their own modification of the original code.

Lachesis, on 13 July 2019 - 11:49 AM, said:

Fixing it will at least allow files with MIDI instruments to load, and for the parts using FM to play back correctly, although the MIDI instruments will be silent. Although I mentioned the bugs elsewhere, I will also mention them here for reference:

• When loading a MIDI instrument in player20.cpp (in the Init function), it has "s += 6" even though it should be 5. (One byte is already skipped by "uint8_t alg = *s++;". In validate20.cpp, 6 is correct, since that byte is not already skipped.) (I don't know if this bug can potentially cause MegaZeux to crash.)
  
• In the PlayNote function, if it is a MIDI instrument, it will skip checking for a cmJumpToLine command. (This bug probably won't result in crashing, but will cause it to play rows that should be skipped.)

Quote

I have not reported it to anyone else.

This post has been edited by zzo38: 13 July 2019 - 09:02 PM

I've looked at all three issues. The tempo flag and MIDI instrument skip are pretty trivial fixes (and I think the latter is quite possibly a potential MZX crash), the MIDI instrument command check less so. I think the correct fix here might actually be to remove 709 through 713 and wrap 715 through 748 in an "if (inst->Algorithm < 7)" check. This would let the rest of the effects work too, though I think the only other effect that matters might be riffs (which I think can trigger other instruments).

I have an update on the MIDI loader bug. The player is actually correct and the documentation and validator are wrong; if you check the two example tracks provided by Reality or save a MIDI track with the official tracker, the MIDI instruments are 7 bytes long. There is an undocumented "volume" byte after the documented bytes.

The pull request for my fixes is here, but needs further verification for the MIDI effect issue. Both MIDI tracks provided with the official tracker also have custom BPM values so that issue is also verified fixed.

Updating status to: Fixed
Issue fixed in: GIT

Fixed in GIT 7ea4e8be and reported to Reality and Adplug (pending reply from either).

Moving to: Archived MegaZeux Bugs