Use automation track for MIDI program change events

[Veratil]

Fixes #6305

Creates an automation track similar to the Pitch/Panning/Volume update events. Tested with a few MIDIs I've saved for testing breaking LMMS MIDI import, along with both MIDIs provided by Rams in 6305.

Used an unordered map to dynamically create channel CC objects similar to the channels one. Reusing the smfMidiCC since it fits perfectly with the program change updates as well.

[Veratil]

Screenshot of MIDI imported from 6305 with Patch automation track

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.169%2Bg537ad1d38-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.169%2Bg537ad1d38-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.169%2Bg537ad1d38-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.169%2Bg537ad1d38-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://15704-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.169%2Bg537ad1d38-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15704?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://15707-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.169%2Bg537ad1d38-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15707?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://15706-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.169%2Bg537ad1d38-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15706?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/nktk25te9k7g5423/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42554065"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/tg5apcaaqnteqn2f/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42554065"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://15703-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.169%2Bg537ad1d38-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15703?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "2e82e9ea3ad964ee1d7baa89c001ce5d997cf7a2"}

[superpaik]

It crashes when playing, on Win10-64b
Test: import the linked midi (it's not mine, but downloaded from internet). Hit play. Crash.
It works ok on 1.2. The new version creates the automation patches for the tracks, which are not visible on v1.2

https://mega.nz/file/8UAGDJRZ#OR4Ies3IXoEk1lxrVtGRipSehZ_3P9oVUu4jpu0YlPs

[Veratil]

It crashes when playing, on Win10-64b Test: import the linked midi (it's not mine, but downloaded from internet). Hit play. Crash. It works ok on 1.2. The new version creates the automation patches for the tracks, which are not visible on v1.2

https://mega.nz/file/8UAGDJRZ#OR4Ies3IXoEk1lxrVtGRipSehZ_3P9oVUu4jpu0YlPs

This looks to be a playback bug, not an import bug.

EDIT: After playing around with deleting tracks, I've found that removing some will not crash playback. Sometimes it will crash after a longer period than with all tracks, but overall this is definitely a playback bug of some kind. Import still works. 👍

[PhysSong]

I could reproduce the crash, but current master also crashed. It happens with a note with pitch C-1(=0). stable-1.2 is fine because the note pitch range is restricted to C0-C8, unless you transpose down by an octave or more using master pitch and base note.

lmms/plugins/sf2_player/sf2_player.cpp

Lines 634 to 637 in f56fc68

	if( midiNote <= 0 || midiNote >= 128 )
	{
	return;
	}

So I conclude the crash is not related to this PR. I will open a separate issue and PR for the crash
@superpaik BTW, you can upload a .zip file directly while writing a comment.

[superpaik]

Oh, I see. I tried to attach the midi file directly, but I couldn't. I didn't think about make it a zip. Thanks!

Maybe the problem was introduced here #5349

[ScolderCreations]

Wait, what happens to the bank data?

[Veratil]

Wait, what happens to the bank data?

bank select is handled through control change event, patch select is a program change event.

bank select handler: https://github.com/LMMS/lmms/blob/master/plugins/MidiImport/MidiImport.cpp#L506

[SDwarfs]

if( midiNote <= 0 || midiNote >= 128 )
{
return;
}

The above code actually seems to cause the crash. I dived into the code and found #6406 to be fixed, when I change the if condition to:

if (midiNote < 0 || midiNote >= 128)

The actual crash happens at Sf2Player.cpp:658 when trying to set "pluginData->offset =", where pluginData is a cast version of the pointer n->pluginData, being unitialized. This usual initialization seems to happen after the above if statement, which does not happen for the case midiNote == 0 due to the "<= 0". As midiNote == 0 seems to be a valid value, this change seems reasonable to me.

Suggested new code (plugins/Sf2Player/Sf2Player.cpp:633):

if( midiNote < 0 || midiNote >= 128 )
{
return;
}

I've no idea how to send you a git "push" request or whatever needed for this change. I'd be glad If someone can send me info how to do it... or just adapt the code accordingly. Thanks!

[DomClark]

I dived into the code and found #6406 to be fixed, when I change the if condition to: if (midiNote < 0 || midiNote >= 128).

See my review comment on #6358, which makes this exact change. The if block is effectively

if (midiNote out of range)
{
    crash;
}

The solution to that is not to reduce the range of arguments that send control flow down the crashing path; it's to remove the crashing path altogether.

The range is still worth changing, since SF2 player should be able to play C-1 notes, but it is not a fix for the crash.

[Veratil]

So... Just to let y'all know that the midi note crash is off topic to this PR... :)