search

mido / mido

MIDI Objects for Python
https://mido.readthedocs.io
MIT License
1.41k stars 213 forks source link

Modifying output of `fix_end_of_track()` patially inconsistently modifies its input object #530

Open page200 opened 1 year ago

page200 commented 1 year ago

fix_end_of_track() yields msg or msg.copy() depending on whether the previous message was a premature 'end_of_track' message.

If I understand the difference between msg and msg.copy() correctly, the user of fix_end_of_track(), when trying to modify messages (for example changing the note from A to A♯) yielded by fix_end_of_track(), will also modify some of the notes of the original input object of fix_end_of_track() but not other notes. So the changes of that original object will be inconsistent, they will depend on whether the previous message was 'end_of_track'.

We can either replace yield msg by yield msg.copy() or remove if accum: and else: yield msg.

rdoursenaud commented 1 year ago

I don't understand your description very well but I think this is coming from Message objects being mutable. IMHO Message objects coming from either Ports (MIDI Inputs) or Tracks (MIDI Files) should be immutable. Only objects the user creates should be mutable.

page200 commented 1 year ago

Yes, this is about Message objects being mutable. Even if we make some of them immutable, the bug will still affect mutable ones. Even if mido didn't create any mutable ones, users' custom code or future mido versions/forks might experiment with mutable ones for some reason and the bug would resurface (and be difficult to detect) if not fixed.

rdoursenaud commented 1 year ago

Would freezing messages when associating them to a MIDIFile fix this?

page200 commented 1 year ago

Even if we make it impossible for users to pass mutable messages as inputs to fix_end_of_track() (which we shouldn't), users might still modify the code when experimenting with some algorithms or improving mido, and then this bug would affect them.

Consistently using msg.copy() instead of mixing msg with msg.copy() uses more memory though (unless we use "copy on write"). So here's a new idea:

Let's have an additional input argument to fix_end_of_track() that decides whether the safe (default for users) or memory-friendly variant should be used. Then merge_tracks() and MidiFile.save() can use the memory-friendly one, and users can use the safe one by default or whatever they choose. Would you welcome this as a pull request?

rdoursenaud commented 1 year ago

Since this is part of the public-ish API (not prepended by _ but not documented either), I'd accept such a PR, even though it clearly hasn't been designed for direct user interaction, rather as a facility to ensure that we don't save non-compliant files.

page200 commented 1 year ago

(Note to myself: merge_tracks() has to use the safe mode because its output can be saved.)