davidluzgouveia / midi-parser

Simple single-file C# MIDI parser
MIT License
67 stars 4 forks source link

Tempo overflows for any value greater than 256 #5

Open Phibonacci opened 5 months ago

Phibonacci commented 5 months ago

The type used to store the meta event tempo is too small, even when converted as BPM. Since data1 is a byte and cast does not check for overflows, any value greater than 256 will result in an silent overflow.

        private static bool ParseMetaEvent(
            byte[] data,
            ref int position,
            byte metaEventType,
            ref byte data1,
            ref byte data2)
        {
            switch (metaEventType)
            {
                case (byte)MetaEventType.Tempo:
                    var mspqn = (data[position + 1] << 16) | (data[position + 2] << 8) | data[position + 3];
                    data1 = (byte)(60000000.0 / mspqn);
                    position += 4;
                    return true;

I would advise changing data1 and data2 types to int as I did.

On another note converting the tempo into BPM is confusing for a library that aims to offer a minimalist representation of MIDI events. I would suggest keeping the original MIDI value and maybe use a static function to convert it. Also the loss in precision might be unwanted for some specific use cases.

Thank you for this library, it helped me a lot.

davidluzgouveia commented 5 months ago

Hello @Phibonacci, I appreciate your feedback!

Looking back, I realize this was a significant oversight on my part. I probably didn't encounter songs with such high tempos before, which led me to overlook this aspect.

Initially, when I wrote this, it wasn't intended to be universally applicable; rather, it was tailored to a specific project I was working on at that time, which had its own set of constraints, such as rounding BPM tempos to whole numbers being acceptable.

In hindsight, opting to use bytes on the model to conserve memory seems unnecessary, especially considering the small size of these files.

I'll make some time to review and update the code to fix these issues!

cauto84 commented 4 months ago

How to get the BPM of the track? I'm a bit confused, each note is returning a different value for the tempo or BeatsMinute

Phibonacci commented 4 months ago

You should have a look at the MIDI protocol first to understand how it works.

Only the meta event packets of type MetaEventType.Tempo are setting the tempo. In this library it is directly converted into BPM from PPQ (Pulses per quarter note) following this formula: 60000 / (BPM * PPQ)

But there is a bug where a BPM > 256 will result in an overflow.

If you want to get the original MIDI tempo expressed in PPQ instead you should have a look at my modification.

Whatever you decide you should at least change the types of data1 and data2 from bytes to int as I did to avoid an overflow for high tempos.