Possible new feature, memory leak in midi output question

I have made a change in the 2.1 branch for my own use, but have a few questions:

1) Would it (described below) be of use (should I figure out how to make it optional and submit a PR)?

2) I may have run across a memory leak and would appreciate a pointer if not so I understand (and can submit an issue if it is a leak)?

3) Was there an easier approach to accomplish this?

Here's the scenario:  I have a midi player I wrote for a player piano, and also a music display on which I show scores either imaged, or in some cases PDF's from Musescore. For the latter, which can have measure numbers shown, it is helpful for the midi play to show the measure being played, and allow restart at a given measure (for a student learning, wanting to hear a passage played slowly to get the timing, etc.). Musescore is not used as a player as this runs on a wimpy Raspberry Pi and can't keep up with faster pieces (I've tried, even on a Pi 3B).

The measure numbers shown on the score do not allow for repeats.  Rather than trying to change that, I decided to try to include the measure numbers in the midi output itself. I chose a Meta type Marker, since it will be ignored generally by midi players, and I can include a textual indicator of the current printed measure (and in repeats it will show the same measure number again of course).  This seems most in keeping with "normal" measure number practice, rather than changing them to be unrolled. I also did not want to unroll the score for similar reasons - to have the score be more traditional for a student (or me for that matter).

Anyway... So far all this seems to work.  What I did was change exportmidi.cpp to add an event with any note-on in the measure, once for each measure in each track. To get the measure number I moved upstream to rendermidi.cpp and in the various collect sections passed the measure number into the routine, and stashed it in the NPlayEvent map to be visible later in exportmidi.cpp.

I do wonder if there was a better way (and perhaps more importantly if this approach is going to fail for some specific types of scores I had not anticipated)?  I am attaching a file if anyone cares to look.

While looking at this, I did run across something I do not understand.  My C++ skills are old and rusty, so maybe this is obvious but... using this type of Meta event means using the setEData function, which sets a pointer _edata to an arbitrary character string created on the heap (at least that's how other existing code does it, so I followed suit).

I see no where this is freed; there's no destructor for MidiEvent, nor does the destructor for the track these are loaded into do anything.  Is this a memory leak, albeit a relatively small one?

Is that why the code at the top of exportmidi.cpp, to include title, copyright, etc. was never finished? (It's disabled with an if #0 even though it looks simple and almost complete).

Or if it is freed in some way, take pity on someone trying to learn C++ again and can you show me where?

Anyway... I'm good where I am, the code works, I can include it in future releases, not asking someone to create a new feature for me.  My main question is whether there might be any interest in it, and if so I'll try to figure out how to add it as an option (maybe on the measure number section of general) and do a PR for the master branch?  

Linwood

diff.txt

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer

lasconic wrote

>
> 1) Would it (described below) be of use (should I figure out how to make it
> optional and submit a PR)?
>
I don't think your use case is large enough to add this in MuseScore
itself. Adding an option doesn't help.

Probably not, thanks.

lasconic wrote

> 2) I may have run across a memory leak and would appreciate a pointer if
> not
> so I understand (and can submit an issue if it is a leak)?

You are probably right MIDIEvent misses a destructor.

I'll put an issue in.  I thought about fixing it but it may be a bit more complicated as the _edata pointer, at least in theory, could point to storage something else is managing. What may work is a "new" allocation in setEData so it makes a copy, but then callers would need to dispose of their own storage which just moves the problem back to the caller.

lasconic wrote

> 3) Was there an easier approach to accomplish this?
>

I believe your implementation will output an event for each track, right?
Do you really need this?
I'm not sure if it would work but a NPlayEvent does have a reference to a
note and so to its measure, no() etc...
So maybe you don't need to store these in the Event and can do the whole
logic in ExportMidi::write ?

YES!!!  Thank you.  It was the findMeasure buried in element which note inherited that I missed when I first tried to do it that way.  This prunes this down to just two tiny sections.  Thank you.

lasconic wrote

I believe your implementation will output an event for each track, right?
Do you really need this?

I didn't answer this, sorry.  That's a firm "maybe".  The problem is full measure rests do not generate any midi events at all, so as an example, if only base is played for the first 5 measures, and I output only on the first track, I miss those five entirely.  I could probably create something to track and see if I've output measure X before and suppress it, however I'd need to also match it to some kind of tick since in repeats I DO want to output that measure again.

But in reality, disk space is cheap, midi output is tiny overall, and even if this adds 30% to the size of a file it's a non-event for me. I could see if this were done in orchestra music with a huge number of tracks it might be different.  I've got two (that was another aspect of this -- by working in Export Midi I fixed it to only output the grand staff when there's a vocal staff above it, and since it's just for me it's hard coded to "2" -- I can remove that when 3.0 has the midi channel controls).

Thanks again for the pointer.  By staying in just ExportMidi I paid more attention to it, and doing the track/channel manipulation was pretty easy as well.

MIDI has no concept of rests whatsoever, not just whole measure rests.  
Rests trigger nothing in the world of MIDI devices, so they simply don't
exist.  You must infer rests via note end and the next note begin for a
track, if you really need them.

On 4/5/2017 8:48 AM, Linwood wrote:


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer

sideways wrote

MIDI has no concept of rests whatsoever, not just whole measure rests.  

I understand, that was my point.   I'm triggering the meta output which tags a measure by a midi event.   In a whole measure rest there is no midi event in the measure, so that measure is not tagged.  It vanishes, in the sense of labels. If there's a (say) quarter rest then a note in the measure, the measure still gets tagged on the first played note in that measure, so it is tagged a bit late, but is tagged.

The reason for mentioning it at all was with respect to writing the measure label in multiple tracks. The issue is that one track may have notes, and the other rests (no notes), so unless I somehow look across tracks for the same instance (relative to repeats) of the same measure, I can't know if I have already written the label.  So I am err'ing on the side of writing more than needed, rather than missing some.

And really moot since not intending to offer this as a PR.