Import midi file from command line hangs

[zonkmachine]

When I try an import a midi file from the command line it will fail and open an empty project that will hang and if I press play will show time signatures as the top picture. When I force quit however the recovery file will be a correctly imported file ( bottom picture ). The same file will import just fine from the gui.

Example file: mrjoe_m.mid.zip
./lmms --import mrjoe_m.mid

[softrabbit]

@zonkmachine, is the file available anywhere?

[zonkmachine]

It failed the same way with all files I've got here.
mrjoe_m.mid.zip

[zonkmachine]

It actually saves three tracks and there should only have been two. The third track "Mr Joe" is the title and first in the meta data.

[zonkmachine]

I created a 4 note midi file without any meta data that opens correctly as one track on import from gui but fails the same way on import from the command line.
zonk1.nometa.mid.zip

[michaelgregorius]

I can reproduce this on master. You can import a MIDI file from the command line using the --import switch by the way, e.g.:

./lmms --import mrjoe_m.mid

It seems that this option is currently not documented in the help.

The cause of the problem seems to be that the frames per tick are still initialized to zero when the song is played. This leads to problems in Song::processNextBuffer() where the frames per tick are retrieved in line 274 using Engine::framesPerTick(). A few lines later in line 284 the following expression is then used to calculate ticks:

int ticks = m_playPos[m_playMode].getTicks() + 
                ( int )( currentFrame / framesPerTick );

This expression leads to a division by zero because framesPerTick is zero which in turn leads to a value of -2147483648 for ticks which might also explain the "exploded" time shown in the screenshot above.

The initialization of the frames per tick is done in the method LmmsCore::updateFramesPerTick() (aka Engine::updateFramesPerTick()) which in turn is called by Song::setTempo and Song::updateFramesPerTick. And in fact if you change the tempo of the song before playing it everything works fine.

Another thing that I have noticed is that the imported MIDI blocks are only rendered in the Song Editor after you have changed its size.

So all in all there are three things that need to be fixed:

1. Document the --import option.
2. Find a good place to call Engine::updateFramesPerTick along the import code path.
3. Fix the rendering of the Song Editor.

[midi-pascal]

@michaelgregorius The number 1 will be fixed via #2425 (some other options were missing in lmms --help). The PR is almost ready, I am testing command-line options right now.

[zonkmachine]

I've updated the original post with link to file and the command used.

Another thing that I have noticed is that the imported MIDI blocks are only rendered in the Song Editor after you have changed its size.

Thank you! That was crucial information and related to #2424
Fix for that specific issue is on the way.

[zonkmachine]

1. Document the --import option.

Fixed by midi-pascal

2. Find a good place to call Engine::updateFramesPerTick along the import code path.

@michaelgregorius Among the last things in MainWindow.cpp constructor after starting the timers. Tried it, works. Do you want me to fix that or are you working on this?

3. Fix the rendering of the Song Editor.

This should have been fixed by #2555

int ticks = m_playPos[m_playMode].getTicks() + 
                ( int )( currentFrame / framesPerTick );

This expression leads to a division by zero because framesPerTick is zero which in turn leads to a value of -2147483648 for ticks which might also explain the "exploded" time shown in the screenshot above.

Some epic trouble shooting there!
currentFrame / framesPerTick
So currentFrame should be checked for 0 to prevent any other mishaps?

[zonkmachine]

I went ahead and created a PR. I think that fixes it.

[michaelgregorius]

@zonkmachine I had no fix ready anyway, so going ahead was the best thing to do. :)

Regarding the check for 0, I think it would be better to just make sure that the variable framesPerTick is initialized and perhaps to assert that it's not 0 before the division.

[zonkmachine]

Thanks!

I bumped the PR with an assert for famesPerTick to be over 0. It's only turned on for debug builds. I think that should be enough for now. Now I need to check up on the work to switch to drumstick for MIDI import/export #3224 .

[zonkmachine]

which in turn is called by Song::setTempo and Song::updateFramesPerTick. And in fact if you change the tempo of the song before playing it everything works fine.

Actually the imported files set 140 bpm though there is tempo defined. The test project above is 160 bpm. I should call Song::setTempo instead of Engine::updateFramesPerTick directly.

[senseab]

I have tried with my branch. Import looks okay, but still hang

PS: I have not merged your code...

[zonkmachine]

I have tried with my branch. Import looks okay, but still hang

Trying it out now with libdrumstick0 libdrumstick-dev 0.5.0-3
It hangs in the exact same way on importing from the command line as on lmms master.
One observation here is that tempo isn't set until I press play when importing via the gui. If you can update that on import it may fix it overall.
Edit: The tempo isn't set until pressing play on the master branch either. It works the same.

PS: I have not merged your code...

Don't. It's not my fines hour in coding. Yet... ;)

[zonkmachine]

I closed #3287 for now as that fix isn't satisfying enough and there is good work done on the drumstick branch.

[zonkmachine]

I had another go at fixing this in #3409 . @tonychee7000 I haven't tested it with drumstick yet.

Currently the project tempo isn't updated until you press play. This is true both for projects imported via the command line and the gui. I haven't looked at programming for MIDI under an OS, only for embedded devices, but as I remember it the tempo is sent as actual ticks and not as tempo change messages. Maybe you can take the first of the tempo automation points and assign it to the project tempo?
https://github.com/LMMS/lmms/blob/master/plugins/MidiImport/MidiImport.cpp#L342:L348

[zonkmachine]

@tonychee7000 I haven't tested it with drumstick yet.

Right, no. The file I fixed up is deleted in the drumstick branch.

[senseab]

@zonkmachine Right, no. The file I fixed up is deleted in the drumstick branch.

I hadn't realized these things at that time, and just started my branch.