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.

Sign up for GitHub

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

Jump to bottom

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

Open
page200 opened this issue Aug 4, 2023 · 6 comments · May be fixed by #532
Open

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

page200 opened this issue Aug 4, 2023 · 6 comments · May be fixed by #532
Assignees
@page200
Labels
bug feature:midifile Standard MIDI File (SMF) implementation investigate

Comments

@page200
Copy link

page200 commented Aug 4, 2023

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.
@page200 page200 added the bug label Aug 4, 2023
@page200 page200 changed the title "Pass by value" vs. "pass by reference" inconsistent in fix_end_of_track() Modifying output of fix_end_of_track() patially inconsistently modifies its input object Aug 4, 2023
@rdoursenaud
Copy link
Member

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

page200 commented Aug 4, 2023 •
edited
Loading

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

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

@rdoursenaud rdoursenaud added the feature:midifile Standard MIDI File (SMF) implementation label Aug 4, 2023
@page200
Copy link
Author

page200 commented Aug 4, 2023 •
edited
Loading

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

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

page200 commented Aug 4, 2023

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

page200 added a commit to page200/mido that referenced this issue Aug 4, 2023
@page200
Update tracks.py to fix issue mido#530. Gotta update midifiles.py acc…
ffcf556 
…ordingly.

I'm explicitly using `safe=True` in `merge_tracks()` to make it clear that that's a deliberate choice and `safe=False` is out of the question there (because the outputs of `merge_tracks()` might get modified by the user later).
page200 added a commit to page200/mido that referenced this issue Aug 4, 2023
@page200
Update midifiles.py: this should conclude the fix of issue mido#530
0e742ed 
Now `write_track()` explicitly promises to not further modify the outputs of `fix_end_of_track()`, so that the memory-friendly variant is used deliberately in this particular case, rather than used unintentionally always (with dangers) like four commits ago.
@page200 page200 linked a pull request Aug 4, 2023 that will close this issue
Open
@rdoursenaud rdoursenaud linked a pull request Aug 7, 2023 that will close this issue
Fix issue #530 #532
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@page200 page200

Labels
bug feature:midifile Standard MIDI File (SMF) implementation investigate
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix issue #530 page200/mido
2 participants
@rdoursenaud @page200