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

Fix SMPTE time divisions not being read/written correctly #275

Merged
merged 1 commit into from Oct 8, 2023

Conversation

TheNathannator
Copy link
Contributor

Discovered this while testing a purpose-built MIDI parser using DryWetMidi as a storage model/serializer. Probably went a little overboard with that testing lol, but I found a bug as a result and thought I'd fix it.

Take an SMPTE format of 30 FPS and a resolution of 80 (the example provided in the MIDI file standard):

new MidiFile() { TimeDivision = new SmpteTimeDivision(SmpteFormat.Thirty, 80) };

The expected written value is E2 50, but the actual value is E1 B0. This happens because DryWetMidi was negating the whole short value instead of just the upper byte, which mangles the resolution and causes the format to be 1 lower than it should be (since the two's-compliment +1 step is being done to the lower byte instead of the upper). Only the SMPTE format value should be written negated, the resolution value is positive.


One concern I have with this change is compatibility with files written by prior versions of DryWetMidi, but I consider that concern mostly irrelevant when these files most likely don't parse correctly in actual use (unless the target application is also using DryWetMidi).

There could be a read/write option for this, but I'm not familiar enough with the architecture to feel confident in implementing that myself currently. Do let me know if that's something you'd want added.

@melanchall
Copy link
Owner

Hi @TheNathannator,

First of all, thank you very much for the PR!

Honestly, I have no extensive testing of SMPTE time division since I found no such files in wild nature. Also there were no issues from users related to this type of the time division since the project born. Also this is the reason why this

One concern I have with this change is compatibility with files written by prior versions of DryWetMidi

won't be an issue :-)

One thing that should be added to the PR is unit tests. But I'll add them by myself after PR merged.

Thank you again,
Max

@melanchall melanchall merged commit 5f585a7 into melanchall:develop Oct 8, 2023
62 checks passed
@melanchall melanchall added the bug Bug in the library label Oct 8, 2023
@melanchall melanchall added this to To do in DryWetMIDI via automation Oct 8, 2023
@melanchall melanchall moved this from To do to Done in DryWetMIDI Oct 8, 2023
@melanchall melanchall added this to the 7.0.2 milestone Oct 8, 2023
@TheNathannator
Copy link
Contributor Author

I thought about unit tests, but I took a look at the existing ones and figured I'd rather have one be written properly after-the-fact than try to cobble one together with code I'm not familiar with lol

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in the library
Projects
DryWetMIDI
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants