From: Chris C. <ca...@al...> - 2006-01-30 11:34:28
|
[cc'ing rg-devel -- feel free to follow up to only that list] On Sunday 29 Jan 2006 22:44, Pedro Lopez-Cabanillas wrote: > Recording sysex events from vkeybd have wrong behavior, it keeps the buffer > from the former sysex message, and appends the new message bytes at the > end. It inserts the messages at the right time on the segment, though. Well I'm not familiar with the code for recording sysex at all, but naively it looks as though the code that sets up the data-block file for a sysex (AlsaDriver.cpp:2463 and around, MappedEvent.cpp:499 and thereabouts) never sets a data-block ID on the event at all, and so all incoming sysex events are appended to the same data-block file, namely $TMPDIR/datablock_0. I've committed some debug output so you can see what I mean. Notice that prepareToWrite always wants to write to data-block 0, and registerDataBlock (which allocates a new data-block ID) is never called. The trouble is that this would suggest it can never have worked properly at all, or at least not in the last several months, since this code hasn't changed in ages. Any more thoughts? Chris |
From: Pedro Lopez-C. <ped...@gm...> - 2006-01-30 21:03:21
|
On Monday 30 January 2006 12:34, Chris Cannam wrote: > > Recording sysex events from vkeybd have wrong behavior, it keeps the > > buffer from the former sysex message, and appends the new message bytes > > at the end. It inserts the messages at the right time on the segment, > > though. > > Well I'm not familiar with the code for recording sysex at all, but naively > it looks as though the code that sets up the data-block file for a sysex > (AlsaDriver.cpp:2463 and around, MappedEvent.cpp:499 and thereabouts) never > sets a data-block ID on the event at all, and so all incoming sysex events > are appended to the same data-block file, namely $TMPDIR/datablock_0. > > I've committed some debug output so you can see what I mean. Notice that > prepareToWrite always wants to write to data-block 0, and registerDataBlock > (which allocates a new data-block ID) is never called. In MappedEvent.cpp:115 there is a call to registerDataBlockForEvent, but this constructor is not used for recorded sysex events. AlsaDriver.cpp:2463 uses the default constructor, so I guess that there should be an explicit call to registerDataBlockForEvent somewere before setDataBlockForEvent() in AlsaDriver.cpp:2470, right? Anyway,this is the sequencer output (recording): Received non-controller event (dest=130:0, controller is 130:2) NON-REALTIME SYSEX B 0 = fffffff0 B 1 = 41 B 2 = 10 B 3 = 42 B 4 = 12 B 5 = 40 B 6 = 01 B 7 = 30 B 8 = 00 B 9 = 00 B 10 = fffffff7 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToWrite Writing 9 chars to file for datablock 0 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToRead Adding NOTE OFF at 6.599999999R Adding NOTE OFF at 7.099999999R Adding NOTE OFF at 7.599999999R Adding NOTE OFF at 8.099999999R Received non-controller event (dest=130:0, controller is 130:2) NON-REALTIME SYSEX B 0 = fffffff0 B 1 = 41 B 2 = 10 B 3 = 42 B 4 = 12 B 5 = 40 B 6 = 01 B 7 = 30 B 8 = 01 B 9 = 00 B 10 = fffffff7 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToWrite Writing 9 chars to file for datablock 0 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToRead Adding NOTE OFF at 8.599999999R Adding NOTE OFF at 9.099999999R Adding NOTE OFF at 9.599999999R Received non-controller event (dest=130:0, controller is 130:2) NON-REALTIME SYSEX B 0 = fffffff0 B 1 = 41 B 2 = 10 B 3 = 42 B 4 = 12 B 5 = 40 B 6 = 01 B 7 = 30 B 8 = 02 B 9 = 00 B 10 = fffffff7 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToWrite Writing 9 chars to file for datablock 0 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToRead Adding NOTE OFF at 10.099999999R Adding NOTE OFF at 10.599999999R Adding NOTE OFF at 11.099999999R Adding NOTE OFF at 11.599999999R Received non-controller event (dest=130:0, controller is 130:2) NON-REALTIME SYSEX B 0 = fffffff0 B 1 = 41 B 2 = 10 B 3 = 42 B 4 = 12 B 5 = 40 B 6 = 01 B 7 = 30 B 8 = 03 B 9 = 00 B 10 = fffffff7 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToWrite Writing 9 chars to file for datablock 0 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0]: prepareToRead Adding NOTE OFF at 12.099999999R Adding NOTE OFF at 12.599999999R Adding NOTE OFF at 13.099999999R Adding NOTE OFF at 13.599999999R Adding NOTE OFF at 14.099999999R Adding NOTE OFF at 14.599999999R rosegarden (sequencer): RosegardenSequencerApp::stop() - stopping And this is the playback debugging output: AlsaDriver - initialisePlayback rosegarden (sequencer): [calling jumpToTime on start] rosegarden (sequencer): jumpToTime( 0.000000000R) AlsaDriver::startClocks AlsaDriver::startClocks: started clocks RosegardenSequencerApp::incrementTransportToken: incrementing to 10 rosegarden (sequencer): Sequencer status changed from 0 to 1 DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_5]: prepareToRead DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_6]: prepareToRead AudioPlayQueue::~AudioPlayQueue() DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_7]: prepareToRead DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_8]: prepareToRead rosegarden (sequencer): RosegardenSequencerApp::stop() - stopping > The trouble is that this would suggest it can never have worked properly at > all, or at least not in the last several months, since this code hasn't > changed in ages. Yes. I think it never worked properly since sysex event data was m-mapped, and I've never tested it seriously. Sorry. Another issue is the "empty payload problem", reported by b.o...@fr... that originated this thread. I've verified the last RG beta, published under the name "rosegarden-1.1beta1.tar.bz2" [1] on December 28th, and it has a lot worse sysex problems. First, that version doesn't record sysex data (empty sysex events events after recording), and second: if you load a file containing existing valid sysex events, or if you create sysex events from scratch using the event list, they are sent to MIDI output ports as empty events, as the original bug report states. This behavior has been corrected at some later time, and current CVS HEAD has only the problem on recording. I'm not going to investigate when and how the other problems have been solved, because I don't have enough time and lack of motivation. [1] See: http://www.mail-archive.com/ros...@li.../msg07980.html http://www.telegraph-road.org/rosegarden/ Regards, Pedro |
From: Pedro Lopez-C. <ped...@gm...> - 2006-01-30 21:40:26
|
On Monday 30 January 2006 21:56, Pedro Lopez-Cabanillas wrote: > I guess that there should be an explicit > call to registerDataBlockForEvent somewere before setDataBlockForEvent() in > AlsaDriver.cpp:2470, right? No. First, registerDataBlockForEvent() is protected, and second: it sets the data block itself. I've tried to workaround the issues to only get more problems. Sorry, but I'm not in a position to look deeper into this issue now. Regards, Pedro |
From: Guillaume L. <gla...@te...> - 2006-01-30 23:26:29
|
On Monday 30 January 2006 22:40, Pedro Lopez-Cabanillas wrote: > On Monday 30 January 2006 21:56, Pedro Lopez-Cabanillas wrote: > > I guess that there should be an explicit > > call to registerDataBlockForEvent somewere before setDataBlockForEvent() > > in AlsaDriver.cpp:2470, right? > > No. First, registerDataBlockForEvent() is protected, and second: it sets > the data block itself. registerDataBlockForEvent() is called at the construction of a MappedEvent if the event's type is a sysex (see MappedEvent.cpp:115). If I recall correctly, the idea was to store the sysex events' payload into those DataBlocks, because the mmap-based code needs fixed-size events. -- Guillaume. http://www.telegraph-road.org |
From: Pedro Lopez-C. <ped...@gm...> - 2006-01-31 07:01:57
|
On Monday 30 January 2006 23:54, Guillaume Laurent wrote: > registerDataBlockForEvent() is called at the construction of a MappedEvent > if the event's type is a sysex (see MappedEvent.cpp:115). Yes. Quoting myself: "In MappedEvent.cpp:115 there is a call to registerDataBlockForEvent, but this constructor is not used for recorded sysex events. AlsaDriver.cpp:2463 uses the default constructor". So registerDataBlockForEvent() is never called, and recorded sysex events are using the same DataBlockFile[/home/pedro/tmp/kde-pedro//datablock_0] every time, without clearing it. The sysex event's payload is appended to the previous recorded one. Regards, Pedro |
From: Chris C. <ca...@al...> - 2006-01-31 12:03:46
|
On Monday 30 Jan 2006 21:40, Pedro Lopez-Cabanillas wrote: > On Monday 30 January 2006 21:56, Pedro Lopez-Cabanillas wrote: > > I guess that there should be an explicit > > call to registerDataBlockForEvent somewere before setDataBlockForEvent() > > in AlsaDriver.cpp:2470, right? > > No. First, registerDataBlockForEvent() is protected, and second: it sets > the data block itself. I've tried to workaround the issues to only get more > problems. Sorry, but I'm not in a position to look deeper into this issue > now. OK, I've committed what I think is a fix. The superficial problem (no data block ID set for mapped events created at the sequencer side) turned out to also mask a deeper problem (separate data block ID counts at GUI and sequencer meant that outgoing events from the GUI overwrote the incoming event data blocks -- this may not have caused any actual problems because the incoming events were probably no longer used by then, but it's hard to be certain). Anyway, I've adjusted setDataBlockForEvent so as to call registerDataBlockForEvent if there is no ID set (ID 0 is officially not used) and I've made it use random IDs instead of incrementing ones, so as to hopefully avoid using the same range in both processes. We also check whether a datablock file already exists and avoid overwriting it (not enough on its own because of potential race conditions). Together those should make things pretty safe. So, please test! One last problem: the datablock files created on the sequencer side are not deleted when Rosegarden exits. Chris |
From: Pedro Lopez-C. <ped...@gm...> - 2006-01-31 21:22:56
Attachments:
rosegarden-20060131.patch
|
On Tuesday 31 January 2006 13:04, Chris Cannam wrote: > OK, I've committed what I think is a fix. The superficial problem (no data > block ID set for mapped events created at the sequencer side) turned out to > also mask a deeper problem (separate data block ID counts at GUI and > sequencer meant that outgoing events from the GUI overwrote the incoming > event data blocks -- this may not have caused any actual problems because > the incoming events were probably no longer used by then, but it's hard to > be certain). > > Anyway, I've adjusted setDataBlockForEvent so as to call > registerDataBlockForEvent if there is no ID set (ID 0 is officially not > used) and I've made it use random IDs instead of incrementing ones, so as > to hopefully avoid using the same range in both processes. We also check > whether a datablock file already exists and avoid overwriting it (not > enough on its own because of potential race conditions). Together those > should make things pretty safe. > > So, please test! OK. Tested. It works. I don't see any other problem now with sysex events. > One last problem: the datablock files created on the sequencer side are not > deleted when Rosegarden exits. What about calling to DataBlockRepository::clear() at ~AlsaDriver() destructor? I've tested the attached patch and it deletes the datablock files generated by the sequencer. I can commit that, if you agree. Regards, Pedro |
From: Chris C. <ca...@al...> - 2006-01-31 21:36:31
|
On Tuesday 31 Jan 2006 21:22, Pedro Lopez-Cabanillas wrote: > What about calling to DataBlockRepository::clear() at ~AlsaDriver() > destructor? Hm, I added a call to that from AlsaDriver::shutdown() (called from dtor) but it looks like I forgot to commit it. I tested it and I'm pretty sure it didn't work, so either I screwed up or something's different between our setups. Chris |