Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tempo overflows for any value greater than 256 #5

Open
Phibonacci opened this issue Apr 1, 2024 · 3 comments
Open

Tempo overflows for any value greater than 256 #5

Phibonacci opened this issue Apr 1, 2024 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@Phibonacci
Copy link

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
Copy link
Owner

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!

@davidluzgouveia davidluzgouveia added the bug Something isn't working label Apr 6, 2024
@davidluzgouveia davidluzgouveia self-assigned this Apr 6, 2024
@cauto84
Copy link

cauto84 commented May 5, 2024

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
Copy link
Author

Phibonacci commented May 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants