major fixes for MIDI patch change and note undo/redo. Patch change handling was completely broken because of the use of absolute floating point comparisons for time comparison, and serialization/deserialization of patch change property changes was borked because of int/char conversions by stringstream. Note undo/redo would fail for note removal if a note had been moved and/or had its note number changed as the next operation after it was added, because time-based lookup would fail. Similar small changes made for sysex messages, which just needed the musical_time comparisons and nothing else

This commit is contained in:
Paul Davis 2013-03-29 11:52:25 -04:00
parent f1ce235b6b
commit 86f1b8c71f
9 changed files with 259 additions and 81 deletions

View file

@ -114,6 +114,8 @@ public:
struct NoteChange {
NoteDiffCommand::Property property;
NotePtr note;
uint32_t note_id;
union {
uint8_t old_value;
TimeType old_time;
@ -161,6 +163,7 @@ public:
private:
struct Change {
boost::shared_ptr<Evoral::Event<TimeType> > sysex;
gint sysex_id;
SysExDiffCommand::Property property;
TimeType old_time;
TimeType new_time;
@ -204,6 +207,7 @@ public:
struct Change {
PatchChangePtr patch;
Property property;
gint patch_id;
union {
TimeType old_time;
uint8_t old_channel;

View file

@ -180,18 +180,29 @@ private:
void filter_channels (BufferSet& bufs, ChannelMode mode, uint32_t mask);
/* if mode is ForceChannel, force mask to the lowest set channel or 1 if no
* channels are set.
*/
#define force_mask(mode,mask) (((mode) == ForceChannel) ? (((mask) ? (1<<(ffs((mask))-1)) : 1)) : mask)
void _set_playback_channel_mode(ChannelMode mode, uint16_t mask) {
mask = force_mask (mode, mask);
g_atomic_int_set(&_playback_channel_mask, (uint32_t(mode) << 16) | uint32_t(mask));
}
void _set_playback_channel_mask (uint16_t mask) {
mask = force_mask (get_playback_channel_mode(), mask);
g_atomic_int_set(&_playback_channel_mask, (uint32_t(get_playback_channel_mode()) << 16) | uint32_t(mask));
}
void _set_capture_channel_mode(ChannelMode mode, uint16_t mask) {
mask = force_mask (mode, mask);
g_atomic_int_set(&_capture_channel_mask, (uint32_t(mode) << 16) | uint32_t(mask));
}
void _set_capture_channel_mask (uint16_t mask) {
mask = force_mask (get_capture_channel_mode(), mask);
g_atomic_int_set(&_capture_channel_mask, (uint32_t(get_capture_channel_mode()) << 16) | uint32_t(mask));
}
#undef force_mask
};
} /* namespace ARDOUR*/

View file

@ -289,6 +289,15 @@ MidiModel::NoteDiffCommand::operator() ()
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
Property prop = i->property;
if (!i->note) {
/* note found during deserialization, so try
again now that the model state is different.
*/
i->note = _model->find_note (i->note_id);
assert (i->note);
}
switch (prop) {
case NoteNumber:
if (temporary_removals.find (i->note) == temporary_removals.end()) {
@ -373,15 +382,25 @@ MidiModel::NoteDiffCommand::undo ()
/* notes we modify in a way that requires remove-then-add to maintain ordering */
set<NotePtr> temporary_removals;
/* lazily discover any affected notes that were not discovered when
* loading the history because of deletions, etc.
*/
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
if (!i->note) {
i->note = _model->find_note (i->note_id);
assert (i->note);
}
}
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
Property prop = i->property;
switch (prop) {
switch (prop) {
case NoteNumber:
if (
temporary_removals.find (i->note) == temporary_removals.end() &&
find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()
) {
if (temporary_removals.find (i->note) == temporary_removals.end() &&
find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()) {
/* We only need to mark this note for re-add if (a) we haven't
already marked it and (b) it isn't on the _removed_notes
@ -396,10 +415,8 @@ MidiModel::NoteDiffCommand::undo ()
break;
case StartTime:
if (
temporary_removals.find (i->note) == temporary_removals.end() &&
find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()
) {
if (temporary_removals.find (i->note) == temporary_removals.end() &&
find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()) {
/* See above ... */
@ -410,10 +427,8 @@ MidiModel::NoteDiffCommand::undo ()
break;
case Channel:
if (
temporary_removals.find (i->note) == temporary_removals.end() &&
find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()
) {
if (temporary_removals.find (i->note) == temporary_removals.end() &&
find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()) {
/* See above ... */
@ -651,15 +666,13 @@ MidiModel::NoteDiffCommand::unmarshal_change (XMLNode *xml_change)
}
/* we must point at the instance of the note that is actually in the model.
so go look for it ...
so go look for it ... it may not be there (it could have been
deleted in a later operation, so store the note id so that we can
look it up again later).
*/
change.note = _model->find_note (note_id);
if (!change.note) {
warning << "MIDI note #" << note_id << " not found in model - programmers should investigate this" << endmsg;
return change;
}
change.note_id = note_id;
return change;
}
@ -790,6 +803,15 @@ MidiModel::SysExDiffCommand::operator() ()
_model->remove_sysex_unlocked (*i);
}
/* find any sysex events that were missing when unmarshalling */
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
if (!i->sysex) {
i->sysex = _model->find_sysex (i->sysex_id);
assert (i->sysex);
}
}
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
switch (i->property) {
case Time:
@ -811,6 +833,15 @@ MidiModel::SysExDiffCommand::undo ()
_model->add_sysex_unlocked (*i);
}
/* find any sysex events that were missing when unmarshalling */
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
if (!i->sysex) {
i->sysex = _model->find_sysex (i->sysex_id);
assert (i->sysex);
}
}
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
switch (i->property) {
case Time:
@ -899,11 +930,7 @@ MidiModel::SysExDiffCommand::unmarshal_change (XMLNode *xml_change)
*/
change.sysex = _model->find_sysex (sysex_id);
if (!change.sysex) {
warning << "Sys-ex #" << sysex_id << " not found in model - programmers should investigate this" << endmsg;
return change;
}
change.sysex_id = sysex_id;
return change;
}
@ -1033,6 +1060,15 @@ MidiModel::PatchChangeDiffCommand::operator() ()
_model->remove_patch_change_unlocked (*i);
}
/* find any patch change events that were missing when unmarshalling */
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
if (!i->patch) {
i->patch = _model->find_patch_change (i->patch_id);
assert (i->patch);
}
}
set<PatchChangePtr> temporary_removals;
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
@ -1081,6 +1117,15 @@ MidiModel::PatchChangeDiffCommand::undo ()
_model->add_patch_change_unlocked (*i);
}
/* find any patch change events that were missing when unmarshalling */
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
if (!i->patch) {
i->patch = _model->find_patch_change (i->patch_id);
assert (i->patch);
}
}
set<PatchChangePtr> temporary_removals;
for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
@ -1207,8 +1252,8 @@ MidiModel::PatchChangeDiffCommand::unmarshal_patch_change (XMLNode* n)
XMLProperty* prop;
Evoral::event_id_t id;
Evoral::MusicalTime time = 0;
uint8_t channel = 0;
uint8_t program = 0;
int channel = 0;
int program = 0;
int bank = 0;
if ((prop = n->property ("id")) != 0) {
@ -1246,6 +1291,7 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
{
XMLProperty* prop;
Change c;
int an_int;
prop = n->property ("property");
assert (prop);
@ -1255,6 +1301,10 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
assert (prop);
Evoral::event_id_t const id = atoi (prop->value().c_str());
/* we need to load via an int intermediate for all properties that are
actually uint8_t (char/byte).
*/
prop = n->property ("old");
assert (prop);
{
@ -1262,11 +1312,14 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
if (c.property == Time) {
s >> c.old_time;
} else if (c.property == Channel) {
s >> c.old_channel;
s >> an_int;
c.old_channel = an_int;
} else if (c.property == Program) {
s >> c.old_program;
s >> an_int;
c.old_program = an_int;
} else if (c.property == Bank) {
s >> c.old_bank;
s >> an_int;
c.old_bank = an_int;
}
}
@ -1274,19 +1327,23 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
assert (prop);
{
istringstream s (prop->value ());
if (c.property == Time) {
s >> c.new_time;
} else if (c.property == Channel) {
s >> c.new_channel;
s >> an_int;
c.new_channel = an_int;
} else if (c.property == Program) {
s >> c.new_program;
s >> an_int;
c.new_program = an_int;
} else if (c.property == Bank) {
s >> c.new_bank;
s >> an_int;
c.new_bank = an_int;
}
}
c.patch = _model->find_patch_change (id);
assert (c.patch);
c.patch_id = id;
return c;
}
@ -1584,7 +1641,7 @@ MidiModel::write_lock()
assert (ms);
assert (!ms->mutex().trylock ());
return WriteLock(new WriteLockImpl(NULL, _lock, _control_lock));
return WriteLock(new WriteLockImpl(0, _lock, _control_lock));
}
int

View file

@ -169,7 +169,7 @@ MidiTrack::set_state (const XMLNode& node, int version)
playback_channel_mode = ChannelMode (string_2_enum(prop->value(), playback_channel_mode));
}
if ((prop = node.property ("capture-channel-mode")) != 0) {
playback_channel_mode = ChannelMode (string_2_enum(prop->value(), capture_channel_mode));
capture_channel_mode = ChannelMode (string_2_enum(prop->value(), capture_channel_mode));
}
if ((prop = node.property ("channel-mode")) != 0) {
/* 3.0 behaviour where capture and playback modes were not separated */

View file

@ -119,7 +119,7 @@ public:
uint8_t channel () const { return _program_change.buffer()[0] & 0xf; }
inline bool operator< (const PatchChange<Time>& o) const {
if (time() != o.time()) {
if (!musical_time_equal (time(), o.time())) {
return time() < o.time();
}
@ -131,7 +131,7 @@ public:
}
inline bool operator== (const PatchChange<Time>& o) const {
return (time() == o.time() && program() == o.program() && bank() == o.bank());
return (musical_time_equal (time(), o.time()) && program() == o.program() && bank() == o.bank());
}
/** The PatchChange is made up of messages() MIDI messages; this method returns them by index.
@ -165,4 +165,10 @@ private:
}
template<typename Time>
std::ostream& operator<< (std::ostream& o, const Evoral::PatchChange<Time>& p) {
o << "Patch Change " << p.id() << " @ " << p.time() << " bank " << (int) p.bank() << " program " << (int) p.program();
return o;
}
#endif

View file

@ -69,8 +69,7 @@ protected:
struct WriteLockImpl {
WriteLockImpl(Glib::Threads::RWLock& s, Glib::Threads::Mutex& c)
: sequence_lock(new Glib::Threads::RWLock::WriterLock(s))
, control_lock(new Glib::Threads::Mutex::Lock(c))
{ }
, control_lock(new Glib::Threads::Mutex::Lock(c)) { }
~WriteLockImpl() {
delete sequence_lock;
delete control_lock;
@ -126,7 +125,7 @@ public:
struct EarlierNoteComparator {
inline bool operator()(const boost::shared_ptr< const Note<Time> > a,
const boost::shared_ptr< const Note<Time> > b) const {
return a->time() < b->time();
return musical_time_less_than (a->time(), b->time());
}
};
@ -134,6 +133,7 @@ public:
typedef const Note<Time>* value_type;
inline bool operator()(const boost::shared_ptr< const Note<Time> > a,
const boost::shared_ptr< const Note<Time> > b) const {
return musical_time_greater_than (a->time(), b->time());
return a->time() > b->time();
}
};
@ -142,7 +142,7 @@ public:
typedef const Note<Time>* value_type;
inline bool operator()(const boost::shared_ptr< const Note<Time> > a,
const boost::shared_ptr< const Note<Time> > b) const {
return a->end_time() > b->end_time();
return musical_time_less_than (a->end_time(), b->end_time());
}
};
@ -186,7 +186,7 @@ public:
struct EarlierSysExComparator {
inline bool operator() (constSysExPtr a, constSysExPtr b) const {
return a->time() < b->time();
return musical_time_less_than (a->time(), b->time());
}
};
@ -199,7 +199,7 @@ public:
struct EarlierPatchChangeComparator {
inline bool operator() (constPatchChangePtr a, constPatchChangePtr b) const {
return a->time() < b->time();
return musical_time_less_than (a->time(), b->time());
}
};

View file

@ -43,6 +43,33 @@ static inline bool musical_time_equal (MusicalTime a, MusicalTime b) {
return fabs (a - b) <= (1.0/1920.0);
}
static inline bool musical_time_less_than (MusicalTime a, MusicalTime b) {
/* acceptable tolerance is 1 tick. Nice if there was no magic number here */
if (fabs (a - b) <= (1.0/1920.0)) {
return false; /* effectively identical */
} else {
return a < b;
}
}
static inline bool musical_time_greater_than (MusicalTime a, MusicalTime b) {
/* acceptable tolerance is 1 tick. Nice if there was no magic number here */
if (fabs (a - b) <= (1.0/1920.0)) {
return false; /* effectively identical */
} else {
return a > b;
}
}
static inline bool musical_time_greater_or_equal_to (MusicalTime a, MusicalTime b) {
/* acceptable tolerance is 1 tick. Nice if there was no magic number here */
if (fabs (a - b) <= (1.0/1920.0)) {
return true; /* effectively identical, note the "or_equal_to" */
} else {
return a >= b;
}
}
/** Type of an event (opaque, mapped by application) */
typedef uint32_t EventType;

View file

@ -720,25 +720,27 @@ void
Sequence<Time>::remove_note_unlocked(const constNotePtr note)
{
bool erased = false;
bool id_matched = false;
_edited = true;
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1 remove note #%2 %3 @ %4\n", this, note->id(), (int)note->note(), note->time()));
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1 remove note %2 @ %3\n", this, (int)note->note(), note->time()));
/* first try searching for the note using the time index, which is
* faster since the container is "indexed" by time. (technically, this
* means that lower_bound() can do a binary search rather than linear)
*
* this may not work, for reasons explained below.
*/
typename Sequence<Time>::Notes::iterator i = note_lower_bound(note->time());
while (i != _notes.end() && (*i)->time() == note->time()) {
typename Sequence<Time>::Notes::iterator i;
typename Sequence<Time>::Notes::iterator tmp = i;
++tmp;
for (i = note_lower_bound(note->time()); i != _notes.end() && musical_time_equal ((*i)->time(), note->time()); ++i) {
if (*i == note) {
NotePtr n = *i;
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing note %2 @ %3\n", this, (int)(*i)->note(), (*i)->time()));
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing note #%2 %3 @ %4\n", this, (*i)->id(), (int)(*i)->note(), (*i)->time()));
_notes.erase (i);
if (n->note() == _lowest_note || n->note() == _highest_note) {
if (note->note() == _lowest_note || note->note() == _highest_note) {
_lowest_note = 127;
_highest_note = 0;
@ -752,30 +754,100 @@ Sequence<Time>::remove_note_unlocked(const constNotePtr note)
}
erased = true;
break;
}
}
i = tmp;
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\ttime-based lookup did not find note #%2 %3 @ %4\n", this, note->id(), (int)note->note(), note->time()));
if (!erased) {
/* if the note's time property was changed in tandem with some
* other property as the next operation after it was added to
* the sequence, then at the point where we call this to undo
* the add, the note we are targetting currently has a
* different time property than the one we we passed via
* the argument.
*
* in this scenario, we have no choice other than to linear
* search the list of notes and find the note by ID.
*/
for (i = _notes.begin(); i != _notes.end(); ++i) {
if ((*i)->id() == note->id()) {
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\tID-based pass, erasing note #%2 %3 @ %4\n", this, (*i)->id(), (int)(*i)->note(), (*i)->time()));
_notes.erase (i);
if (note->note() == _lowest_note || note->note() == _highest_note) {
_lowest_note = 127;
_highest_note = 0;
for (typename Sequence<Time>::Notes::iterator ii = _notes.begin(); ii != _notes.end(); ++ii) {
if ((*ii)->note() < _lowest_note)
_lowest_note = (*ii)->note();
if ((*ii)->note() > _highest_note)
_highest_note = (*ii)->note();
}
}
erased = true;
id_matched = true;
break;
}
}
}
if (erased) {
Pitches& p (pitches (note->channel()));
typename Pitches::iterator j;
/* if we had to ID-match above, we can't expect to find it in
* pitches via note comparison either. so do another linear
* search to locate it. otherwise, we can use the note index
* to potentially speed things up.
*/
if (id_matched) {
for (j = p.begin(); j != p.end(); ++j) {
if ((*j)->id() == note->id()) {
p.erase (j);
break;
}
}
} else {
/* Now find the same note in the "pitches" list (which indexes
* notes by channel+time. We care only about its note number
* so the search_note has all other properties unset.
*/
NotePtr search_note (new Note<Time>(0, 0, 0, note->note(), 0));
typename Pitches::iterator j = p.lower_bound (search_note);
while (j != p.end() && (*j)->note() == note->note()) {
typename Pitches::iterator tmp = j;
++tmp;
for (j = p.lower_bound (search_note); j != p.end() && (*j)->note() == note->note(); ++j) {
if (*j == note) {
if ((*j) == note) {
DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing pitch %2 @ %3\n", this, (int)(*j)->note(), (*j)->time()));
p.erase (j);
break;
}
}
}
j = tmp;
if (j == p.end()) {
warning << string_compose ("erased note %1 not found in pitches for channel %2", *note, (int) note->channel()) << endmsg;
}
if (!erased) {
cerr << "Unable to find note to erase matching " << *note.get() << endl;
_edited = true;
} else {
cerr << "Unable to find note to erase matching " << *note.get() << endmsg;
}
}
@ -784,12 +856,13 @@ void
Sequence<Time>::remove_patch_change_unlocked (const constPatchChangePtr p)
{
typename Sequence<Time>::PatchChanges::iterator i = patch_change_lower_bound (p->time ());
while (i != _patch_changes.end() && (*i)->time() == p->time()) {
while (i != _patch_changes.end() && (musical_time_equal ((*i)->time(), p->time()))) {
typename Sequence<Time>::PatchChanges::iterator tmp = i;
++tmp;
if (*i == p) {
if (**i == *p) {
_patch_changes.erase (i);
}
@ -1148,7 +1221,7 @@ Sequence<Time>::patch_change_lower_bound (Time t) const
{
PatchChangePtr search (new PatchChange<Time> (t, 0, 0, 0));
typename Sequence<Time>::PatchChanges::const_iterator i = _patch_changes.lower_bound (search);
assert (i == _patch_changes.end() || (*i)->time() >= t);
assert (i == _patch_changes.end() || musical_time_greater_or_equal_to ((*i)->time(), t));
return i;
}