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

Defer computing merged_track #565

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Conversation

akx
Copy link
Contributor

@akx akx commented Nov 27, 2023

Follows up on #470.

There's no point in computing merged_track (quite expensively) until it's needed.

However, the comment #470 (comment) should be addressed (separately): it's not clear that merged_track is a cached object, and changes to tracks or their contents will not be reflected automatically.

@rdoursenaud
Copy link
Member

Thanks, looks good on a surface level but…

Doesn't this defeat the purpose of pre-computing merged_track which was to avoid the delay before playback?

merged_track is still an utterly broken kludge anyway:

  • it doesn't update when a track is modified and becomes out of sync with the object
  • it seems to only be used for naive playback purposes where a better data structure or process would alleviate its need

I'm planning on getting rid of it for mido 2.x.

@akx
Copy link
Contributor Author

akx commented Nov 29, 2023

Doesn't this defeat the purpose of pre-computing merged_track which was to avoid the delay before playback?

That moved the delay to all instantiations of MidiFile, even if they didn't use merged_track (or iterate over the MidiFile), which is painful for downstream users (and energy bills thereof) of the library who don't need merged_track at all; see YatingMusic/miditoolkit#44 that speeds up that library's test suite by quite a lot. (check_msgdict is still rather slow, but that's a different can of worms.)

@rdoursenaud
Copy link
Member

Yeah, it wasn't a smart decision on my part to accept this PR as-is.
I'm just trying to be cautious and aware of the repercussions.

@rdoursenaud
Copy link
Member

I believe your approach is the better tradeoff for now.
I'll have to think and find a better approach that fits both use cases for the next release.

@rdoursenaud rdoursenaud merged commit 45b5151 into mido:main Nov 29, 2023
23 checks passed
@akx
Copy link
Contributor Author

akx commented Nov 29, 2023

@rdoursenaud I absolutely know the feeling of trying to be cautious (being the maintainer of babel) 😄

As for 2.0, I think __iter__ should probably be removed altogether (since #537 would necessitate a different API anyway); I think MidiFile.iter_events(all_tracks=True) sounds pretty good at first thought, etc... :)

@akx akx deleted the defer-merged-track branch November 29, 2023 10:51
@rdoursenaud
Copy link
Member

I'm new to the maintaining a popular library thing and still learning the ropes.
Thanks for your invaluable insights!
Feel free to stick around ;)

@by-nari
Copy link

by-nari commented Jan 29, 2024

This is a feature that I really need. I'm trying to remove a track from the event stream when playing but without success. I tried merging the remaining tracks and then playing them, but a small number of MIDI files would fail with this method. The best way is to know which track the event comes from, then remove them right there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants