#1692 Bank Editor: Allow bank sharing for percussion

[Ted Felix]

The same Bank Select might mean something completely different on a percussion channel vs. a non-percussion channel. Our bank editor currently does not support this. Only one program list can be associated with one bank select. We need the ability to specify a program list for a bank select on a percussion channel and a different program list for the same bank select on a non-percussion channel.

The JV-1080 provides an example. From the Roland JV-1080 Owner's Manual pp. 79, 124, and 126:

• MSB:LSB:PC 80:0:0 selects user patch #1 (Symphonique) on a non-percussion channel.
• MSB:LSB:PC 80:0:0 selects user drum set #1 (HouseDrumSet) on a percussion channel (10).

In addition to supporting the above, I propose a pop-up dialog for atomically setting/editing the MSB:LSB:Percussion key for each program list. This will allow for simpler duplicate bank detection and prevention as well.

Code review of MidiProgramsEditor is currently underway to support fixing this limitation.

[Ted Felix]

Just pushed some MidiProgramsEditor cleanup as [56f02a].

[Ted Felix]

Rg needs to support "Bank Select + Percussion" as a unique key identifying a bank. Then the bank editor needs to be upgraded to handle this as well.

The workaround now is to have multiple devices with different rgd files associated with the same MIDI port. Rg used to have problems preserving this across save/load, but that was fixed recently [663099] and will be officially in 24.12. For now, use the latest git.

While investigating this I discovered that the bank editor is filling the undo history with micro-garbage. That needs to be addressed as well. See comments in MidiProgramsEditor::slotNameChanged().

I did some cleanup in MidiProgramsEditor, MidiBank, and MidiProgram. My analysis was limited to the bank editor ("Bank and Program details" window). More work needs to be done on MidiDevice and the main window.

I'm switching to something else if anyone wants to take over.

[Philip Leishman]

I never used this dialog much - I was always happy with good old GM.
But things are certainly broken here.
At first I thought undo redo was not working properly - open the device manager - click on "New" and then press control-Z. The new device is still there - but not really - close and reopen the dialog and it is gone. So an update of the dialog is missing! And yes that undo for every character typed is not good.
Another problem: Create a new device and add 2 banks. Set the MSB for the first bank to some number. Now just switch back and forth between the two banks and watch that number count down !!!

So things to do:
1. Refresh on undo/redo - it seems like we need DeviceObserverclass - the DeviceMamager would be an observer and refresh when thing change
2. Micro commands - As Ted suggests maybe just issue the command when the word is complete.
3. The MSB/LSB issue - In the codes are some comments about removing the uniqueness code. This seems a good idea. This would address the initial issue.

[Philip Leishman]

I have made a start here.
At this stage I have just improved the undo/redo behaviour in the DeviceManagerDialog
Next step - the bank editor
See merge request
Comments welcome

[Ted Felix]

Did some testing on your branch. I'm seeing a use after free. Procedure:

1. Create a new device.
2. Click Banks... to edit banks.
3. Click Add Bank twice.

Highlights from the ASAN report:

==78999==ERROR: AddressSanitizer: heap-use-after-free on address 0x60700083046c at pc 0x77f92e6553eb bp 0x7ffce5bb68b0 sp 0x7ffce5bb68a0
READ of size 4 at 0x60700083046c thread T0
    #0 0x77f92e6553ea in Rosegarden::MidiDeviceTreeWidgetItem::getDeviceId() const /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/MidiDeviceTreeWidgetItem.h:50
    #1 0x77f92e6b0247 in Rosegarden::BankEditorDialog::populateDeviceEditors(QTreeWidgetItem*) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:857
    #2 0x77f92e6af776 in Rosegarden::BankEditorDialog::slotPopulateDeviceEditors(QTreeWidgetItem*, QTreeWidgetItem*) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:776
    #3 0x77f92e6cbfa7 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1>, QtPrivate::List<QTreeWidgetItem*, QTreeWidgetItem*>, void, void (Rosegarden::BankEditorDialog::*)(QTreeWidgetItem*, QTreeWidgetItem*)>::call(void (Rosegarden::BankEditorDialog::*)(QTreeWidgetItem*, QTreeWidgetItem*), Rosegarden::BankEditorDialog*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:152
    #4 0x77f92e6c920d in void QtPrivate::FunctionPointer<void (Rosegarden::BankEditorDialog::*)(QTreeWidgetItem*, QTreeWidgetItem*)>::call<QtPrivate::List<QTreeWidgetItem*, QTreeWidgetItem*>, void>(void (Rosegarden::BankEditorDialog::*)(QTreeWidgetItem*, QTreeWidgetItem*), Rosegarden::BankEditorDialog*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:185
    #5 0x77f92e6c5e21 in QtPrivate::QSlotObject<void (Rosegarden::BankEditorDialog::*)(QTreeWidgetItem*, QTreeWidgetItem*), QtPrivate::List<QTreeWidgetItem*, QTreeWidgetItem*>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:418
    #6 0x77f92aef1792  (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2f1792)
    #7 0x77f92be66111 in QTreeWidget::currentItemChanged(QTreeWidgetItem*, QTreeWidgetItem*) (/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x466111)
...

0x60700083046c is located 60 bytes inside of 72-byte region [0x607000830430,0x607000830478)
freed by thread T0 here:
    #0 0x77f9310b724f in operator delete(void*, unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:172
    #1 0x77f92e6d0516 in Rosegarden::MidiBankTreeWidgetItem::~MidiBankTreeWidgetItem() /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/MidiBankTreeWidgetItem.h:36
    #2 0x77f92e6af11c in Rosegarden::BankEditorDialog::updateDeviceItem(Rosegarden::MidiDeviceTreeWidgetItem*) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:726
    #3 0x77f92e6acebd in Rosegarden::BankEditorDialog::updateDialog() /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:573
    #4 0x77f92e6be573 in Rosegarden::BankEditorDialog::deviceModified(Rosegarden::Device*) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:1953
    #5 0x77f92e9528f9 in Rosegarden::Device::notifyDeviceModified() /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/base/Device.cpp:132
    #6 0x77f92da1e636 in Rosegarden::MidiDevice::setVariationType(Rosegarden::MidiDevice::VariationType) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/base/MidiDevice.h:146
    #7 0x77f92ec7d477 in Rosegarden::ModifyDeviceCommand::execute() /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/commands/studio/ModifyDeviceCommand.cpp:133
    #8 0x77f92dbd73cb in Rosegarden::CommandHistory::addCommand(Rosegarden::Command*, long) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/document/CommandHistory.cpp:127
    #9 0x77f92e6b7e2c in Rosegarden::BankEditorDialog::addCommandToHistory(Rosegarden::Command*) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:1560
    #10 0x77f92e6b15a7 in Rosegarden::BankEditorDialog::slotApply() /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:967
    #11 0x77f92e64de16 in Rosegarden::MidiProgramsEditor::slotNewLSB(int) /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/MidiProgramsEditor.cpp:372
...

previously allocated by thread T0 here:
    #0 0x77f9310b61e7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x77f92e6b28c6 in Rosegarden::BankEditorDialog::slotAddBank() /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/BankEditorDialog.cpp:1099
...

SUMMARY: AddressSanitizer: heap-use-after-free /home/ted/tmp/devel/rosegarden/rosegarden-workspace/rosegarden-git/src/gui/studio/MidiDeviceTreeWidgetItem.h:50 in Rosegarden::MidiDeviceTreeWidgetItem::getDeviceId() const

Let me know if you need to full dump.

[Philip Leishman]

That is quite right. The BankEditor still needs a lot of work.
This merge request is not ready for merging.
What should work better now is the DeviceManager - undo redo should work OK. Don't look at the bank editor yet !!

[Ted Felix]

Ok, that makes sense. I think the Bank Editor may have been abandoned to a certain extent since working with xml isn't very difficult. Also there are tools for creating .rgd files, so the bank editor isn't really necessary.

It will be nice to have it working properly for a change. For quick edits it's a lot easier than digging into the xml.

[Philip Leishman]

OK - this was more difficult than I thought.
The original concept was to make changes in the gui widgets and use these to update the devices.
I have changed to making direct changes to the device and using a studio/device observer mechanism to update the widgets. That means undo redo works automatically.
Also removed a lot of unnecessary state from the editor classes - the state is in the device.
So all together a lot of changes - still may be some bugs around !
I believe I have addressed the original issues (percussion + micro undo/redo).
I noticed the undo/redo is limited to 50 actions - not a lot if inputting a bank. I tried to look at memory usage of undo/redo and I believe it is not a lot. Suggestion - increase maximum undo/redo to 300 actions ?
Please test and merge.

[Ted Felix]

Re: undo limit, I think the thing to bear in mind is that no one wants to press Ctrl+Z 50 times, let alone 300. It would be best to focus on reducing the number of commands that the bank editor adds to the command history.

[Ted Felix]

Did some testing against [a18c86].

Looks like bank sharing is working fine now. Saves fine. Plays back fine. Looks correct in the MIPP. The main issue appears to be fixed.

Found a bug in the bank editor. Deleting a bank clears all the program names in all the other banks on that device. Procedure:

1. Create a new playback device.
2. Click Banks... to open the Manage MIDI Banks and Programs dialog.
3. On the New Device, click Add Bank.
4. Go into that new bank and name two of the programs.
5. Click Add Bank to add another bank.
6. Select the new bank and click Delete then Yes to delete the new bank.
7. Select the bank with the two program names.
8. Confirm: Names are still there.

When I run the above, deleting a bank clears all the program names for all banks on that device.

[Philip Leishman]

Thanks for finding that. The fix was quite easy.
See branch

[9d6be2]

[Ted Felix]

Everything is working great.

Before I merge, I want to give BankEditorDialog and MidiProgramsEditor a serious review. This will mean massive changes as usual to bring these two up to standards. So, avoid making any changes to those two while I'm working on this. This will probably take a week or two.

[Philip Leishman]

OK

[Ted Felix]

Just finished BankEditorDialog review. MidiProgramsEditor is next. Maybe one more week and then I'll merge all this to master.

Pushing work in progress to the bug1692-ted branch:

https://sourceforge.net/p/rosegarden/git/ci/bug1692-ted/tree/

[Ted Felix]

Merged as [ef3898]. Please test latest git.

[Ted Felix]

Lots of crashes reported on the user list. Need to investigate. Hopefully easy to reproduce. I'll have a look on Monday.

[chuck elliot]

Steps to cause crash: open RG, open MDM (no devices connected), create new device, select new device and banks, import Korg M1, accept default settings, close dialog.

ClientNotify fails name = rosegarden notification = 3 val1 = 0 val2 = 0
Cannot write socket fd = 19 err = Broken pipe
CheckRes error
Could not write notification
ClientNotify fails name = rosegarden notification = 3 val1 = 0 val2 = 0
Cannot write socket fd = 19 err = Broken pipe
CheckRes error
Could not write notification
ClientNotify fails name = rosegarden notification = 3 val1 = 0 val2 = 0
Cannot write socket fd = 19 err = Broken pipe
CheckRes error
Could not write notification
ClientNotify fails name = rosegarden notification = 3 val1 = 0 val2 = 0
Segmentation fault (core dumped)
JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
chuck@robin:~/rosegarden-git/build$ JackEngine::XRun: client = rosegarden was not finished, state = Triggered
JackAudioDriver::ProcessGraphAsyncMaster: Process error
Unknown error...
terminate called after throwing an instance of 'Jack::JackTemporaryException'
  what():  

[Ted Felix]

Ok, I was able to reproduce. Key is you need to do a release build. Everything works fine in a debug build.

[Ted Felix]

Just pushed a fix for the crash, [39a720]. Please test latest git.

[Philip Leishman]

I have been away from my computer for quite a while - I missed all the excitement.
Thanks to Ted for sorting all this out !

[chuck elliot]

I retested MDM in fixed [39a720] git version and was unable to reproduce the crash using similar steps to before. Further fiddling around therein did not uncover any problems. I will have a go at editing the Korg-M1 rgd using MDM when I have time. It is currently missing from the library but needs updating anyway for percussion maps and controls. Thanks for all the effort.