From 80e54c6ea05d35465f50f84897f113e0a48b7afd Mon Sep 17 00:00:00 2001
From: David Robillard <d@drobilla.net>
Date: Sat, 14 Feb 2009 20:52:15 +0000
Subject: [PATCH] MIDI robustness.

- Separate SMF::open and SMF::create, more powerful interface for both.
- Correctly handle note ons with velocity 0 as note offs in sequence.
- Use SMF (i.e. libsmf) for MIDI import


git-svn-id: svn://localhost/ardour2/branches/3.0@4558 d708f5d6-7413-0410-9779-e7cbd77b26cf
---
 libs/ardour/filter.cc              |   2 +-
 libs/ardour/import.cc              |  45 +++++----
 libs/ardour/smf_source.cc          |   2 +-
 libs/evoral/evoral/Event.hpp       |   2 +-
 libs/evoral/evoral/MIDIEvent.hpp   |  10 +-
 libs/evoral/evoral/SMF.hpp         |  20 ++--
 libs/evoral/src/Event.cpp          |  10 +-
 libs/evoral/src/SMF.cpp            | 152 +++++++++++++++++++++--------
 libs/evoral/src/Sequence.cpp       |  26 +++--
 libs/evoral/src/libsmf/smf_tempo.c |   4 +
 libs/evoral/test/SMFTest.cpp       |   4 +-
 11 files changed, 185 insertions(+), 92 deletions(-)

diff --git a/libs/ardour/filter.cc b/libs/ardour/filter.cc
index be382f72da..0075555773 100644
--- a/libs/ardour/filter.cc
+++ b/libs/ardour/filter.cc
@@ -100,7 +100,7 @@ Filter::finish (boost::shared_ptr<Region> region, SourceList& nsrcs, string regi
 		boost::shared_ptr<SMFSource> smfs = boost::dynamic_pointer_cast<SMFSource>(*si);
 		if (smfs) {
 			smfs->set_timeline_position (region->position());
-			smfs->flush_footer ();
+			smfs->flush ();
 		}
 		
 		/* now that there is data there, requeue the file for analysis */
diff --git a/libs/ardour/import.cc b/libs/ardour/import.cc
index 76975d8237..97cebf1456 100644
--- a/libs/ardour/import.cc
+++ b/libs/ardour/import.cc
@@ -37,7 +37,7 @@
 #include <pbd/basename.h>
 #include <pbd/convert.h>
 
-#include <evoral/SMFReader.hpp>
+#include <evoral/SMF.hpp>
 
 #include <ardour/ardour.h>
 #include <ardour/session.h>
@@ -309,10 +309,11 @@ write_audio_data_to_new_files (ImportableSource* source, Session::import_status&
 }
 
 static void
-write_midi_data_to_new_files (Evoral::SMFReader* source, Session::import_status& status,
+write_midi_data_to_new_files (Evoral::SMF<double>* source, Session::import_status& status,
 			       vector<boost::shared_ptr<Source> >& newfiles)
 {
-	Evoral::Event<double> ev(0, 0.0, 4, NULL, true);
+	uint32_t buf_size = 4;
+	uint8_t* buf      = (uint8_t*)malloc(buf_size);
 
 	status.progress = 0.0f;
 
@@ -329,15 +330,28 @@ write_midi_data_to_new_files (Evoral::SMFReader* source, Session::import_status&
 		uint32_t size    = 0;
 		
 		while (!status.cancel) {
+			size = buf_size;
 
-			if (source->read_event(4, ev.buffer(), &size, &delta_t) < 0)
+			int ret = source->read_event(&delta_t, &size, &buf);
+			if (size > buf_size)
+				buf_size = size;
+
+			if (ret < 0) { // EOT
 				break;
-
+			}
+			
 			t += delta_t;
-			ev.time() = (double)t / (double)source->ppqn();
-			ev.size() = size;
 
-			smfs->append_event_unlocked(Beats, ev);
+			if (ret == 0) { // Meta
+				continue;
+			}
+
+			smfs->append_event_unlocked(Beats, Evoral::Event<double>(
+						0,
+						(double)t / (double)source->ppqn(),
+						size,
+						buf));
+
 			if (status.progress < 0.99)
 				status.progress += 0.01;
 		}
@@ -358,7 +372,7 @@ write_midi_data_to_new_files (Evoral::SMFReader* source, Session::import_status&
 	}
 
 	} catch (...) {
-		error << "Corrupt MIDI file " << source->filename() << endl;
+		error << "Corrupt MIDI file " << source->path() << endl;
 	}
 }
 
@@ -388,8 +402,8 @@ Session::import_audiofiles (import_status& status)
 			p != status.paths.end() && !status.cancel;
 			++p, ++cnt)
 	{
-		boost::shared_ptr<ImportableSource> source;
-		std::auto_ptr<Evoral::SMFReader>    smf_reader;
+		boost::shared_ptr<ImportableSource>  source;
+		std::auto_ptr< Evoral::SMF<double> > smf_reader;
 		const DataType type = ((*p).rfind(".mid") != string::npos) ? 
 			DataType::MIDI : DataType::AUDIO;
 		
@@ -405,14 +419,11 @@ Session::import_audiofiles (import_status& status)
 
 		} else {
 			try {
-				smf_reader = std::auto_ptr<Evoral::SMFReader>(new Evoral::SMFReader(*p));
+				smf_reader = std::auto_ptr< Evoral::SMF<double> >(new Evoral::SMF<double>());
+				smf_reader->open(*p);
 				channels = smf_reader->num_tracks();
-			} catch (const Evoral::SMFReader::UnsupportedTime& err) {
-				error << _("Import: unsupported MIDI time stamp format") << endmsg;
-				status.done = status.cancel = true;
-				return;
 			} catch (...) {
-				error << _("Import: error reading MIDI file") << endmsg;
+				error << _("Import: error opening MIDI file") << endmsg;
 				status.done = status.cancel = true;
 				return;
 			}
diff --git a/libs/ardour/smf_source.cc b/libs/ardour/smf_source.cc
index 6d80857e7b..e160d7e277 100644
--- a/libs/ardour/smf_source.cc
+++ b/libs/ardour/smf_source.cc
@@ -65,7 +65,7 @@ SMFSource::SMFSource (Session& s, std::string path, Flag flags)
 		throw failed_constructor ();
 	}
 	
-	if (open(path)) {
+	if (create(path)) {
 		throw failed_constructor ();
 	}
 
diff --git a/libs/evoral/evoral/Event.hpp b/libs/evoral/evoral/Event.hpp
index 86a2f146bb..7703963b95 100644
--- a/libs/evoral/evoral/Event.hpp
+++ b/libs/evoral/evoral/Event.hpp
@@ -43,7 +43,7 @@ namespace Evoral {
 template<typename Time>
 struct Event {
 #ifdef EVORAL_EVENT_ALLOC
-	Event(EventType type=0, Time timestamp=0, uint32_t size=0, uint8_t* buffer=NULL, bool alloc=false);
+	Event(EventType type=0, Time time=0, uint32_t size=0, uint8_t* buf=NULL, bool alloc=false);
 	
 	/** Copy \a copy.
 	 * 
diff --git a/libs/evoral/evoral/MIDIEvent.hpp b/libs/evoral/evoral/MIDIEvent.hpp
index d6751db0a8..c9541632d1 100644
--- a/libs/evoral/evoral/MIDIEvent.hpp
+++ b/libs/evoral/evoral/MIDIEvent.hpp
@@ -36,8 +36,8 @@ namespace Evoral {
  */
 template<typename Time>
 struct MIDIEvent : public Event<Time> {
-	MIDIEvent(EventType type=0, Time timestamp=0, uint32_t size=0, uint8_t* buffer=NULL, bool alloc=false)
-		: Event<Time>(type, timestamp, size, buffer, alloc)
+	MIDIEvent(EventType type=0, Time time=0, uint32_t size=0, uint8_t* buf=NULL, bool alloc=false)
+		: Event<Time>(type, time, size, buf, alloc)
 	{}
 	
 	MIDIEvent(const Event<Time>& copy, bool alloc)
@@ -45,12 +45,10 @@ struct MIDIEvent : public Event<Time> {
 	{}
 
 #ifdef EVORAL_MIDI_XML
-	/** Event from XML ala http://www.midi.org/dtds/MIDIEvents10.dtd
-	 */
+	/** Event from XML ala http://www.midi.org/dtds/MIDIEvents10.dtd */
 	MIDIEvent(const XMLNode& event);
 	
-	/** Event to XML ala http://www.midi.org/dtds/MIDIEvents10.dtd
-	 */
+	/** Event to XML ala http://www.midi.org/dtds/MIDIEvents10.dtd */
 	boost::shared_ptr<XMLNode> to_xml() const;
 #endif
 
diff --git a/libs/evoral/evoral/SMF.hpp b/libs/evoral/evoral/SMF.hpp
index c2760d9df5..5f91f12bb6 100644
--- a/libs/evoral/evoral/SMF.hpp
+++ b/libs/evoral/evoral/SMF.hpp
@@ -46,14 +46,21 @@ public:
 	SMF() : _last_ev_time(0), _smf(0), _smf_track(0), _empty(true) {};
 	virtual ~SMF();
 	
-	int  open(const std::string& path, bool create=true, int track=1) THROW_FILE_ERROR;
+	int  open(const std::string& path, int track=1) THROW_FILE_ERROR;
+	int  create(const std::string& path, int track=1, int ppqn=19200) THROW_FILE_ERROR;
 	void close() THROW_FILE_ERROR;
+	
+	const std::string& path() const { return _path; };
 
 	void seek_to_start() const;
+	int  seek_to_track(int track);
 	
-	uint16_t ppqn()     const { return _ppqn; }
-	bool     is_empty() const { return _empty; }
-	bool     eof()      const { assert(false); return true; }
+	int read_event(uint32_t* delta_t, uint32_t* size, uint8_t** buf) const;
+	
+	uint16_t num_tracks() const;
+	uint16_t ppqn()       const;
+	bool     is_empty()   const { return _empty; }
+	bool     eof()        const { assert(false); return true; }
 	
 	Time last_event_time() const { return _last_ev_time; }
 	
@@ -62,11 +69,6 @@ public:
 	void end_write() THROW_FILE_ERROR;
 	
 	void flush() {};
-	int  flush_header() { return 0; }
-	int  flush_footer() { return 0; }
-
-protected:
-	int read_event(uint32_t* delta_t, uint32_t* size, uint8_t** buf) const;
 
 private:
 	static const uint16_t _ppqn = 19200;
diff --git a/libs/evoral/src/Event.cpp b/libs/evoral/src/Event.cpp
index 532b8b498a..d309d8a0f7 100644
--- a/libs/evoral/src/Event.cpp
+++ b/libs/evoral/src/Event.cpp
@@ -23,17 +23,17 @@ namespace Evoral {
 #ifdef EVORAL_EVENT_ALLOC
 
 template<typename Timestamp>
-Event<Timestamp>::Event(EventType type, Timestamp timestamp, uint32_t size, uint8_t* buffer, bool alloc)
+Event<Timestamp>::Event(EventType type, Timestamp time, uint32_t size, uint8_t* buf, bool alloc)
 	: _type(type)
-	, _time(timestamp)
+	, _time(time)
 	, _size(size)
-	, _buf(buffer)
+	, _buf(buf)
 	, _owns_buf(alloc)
 {
 	if (alloc) {
 		_buf = (uint8_t*)malloc(_size);
-		if (buffer) {
-			memcpy(_buf, buffer, _size);
+		if (buf) {
+			memcpy(_buf, buf, _size);
 		} else {
 			memset(_buf, 0, _size);
 		}
diff --git a/libs/evoral/src/SMF.cpp b/libs/evoral/src/SMF.cpp
index eda2da233c..e190423c45 100644
--- a/libs/evoral/src/SMF.cpp
+++ b/libs/evoral/src/SMF.cpp
@@ -17,8 +17,10 @@
  * 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#define __STDC_LIMIT_MACROS 1
 #include <cassert>
 #include <iostream>
+#include <stdint.h>
 #include "evoral/Event.hpp"
 #include "evoral/SMF.hpp"
 #include "libsmf/smf.h"
@@ -37,53 +39,113 @@ SMF<Time>::~SMF()
 	} 
 }
 
-/** Attempt to open the SMF file for reading and writing.
- *
- * Currently SMF is always read/write.
- *
- * \return  0 on success
- *         -1 if the file can not be opened or created
- *         -2 if the file exists but specified track does not
+template<typename Time>
+uint16_t
+SMF<Time>::num_tracks() const
+{
+	return _smf->number_of_tracks;
+}
+
+template<typename Time>
+uint16_t
+SMF<Time>::ppqn() const
+{
+	assert(_smf->ppqn >= 0 && _smf->ppqn <= UINT16_MAX);
+	return (uint16_t)_smf->ppqn;
+}
+
+/** Seek to the specified track (1-based indexing)
+ * \return 0 on success
  */
 template<typename Time>
 int
-SMF<Time>::open(const std::string& path, bool create, int track) THROW_FILE_ERROR
+SMF<Time>::seek_to_track(int track)
 {
+	_smf_track = smf_get_track_by_number(_smf, track);
+	if (_smf_track != NULL) {
+		_smf_track->next_event_number = (_smf_track->number_of_events == 0) ? -1 : 1;
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+/** Attempt to open the SMF file for reading and/or writing.
+ *
+ * \return  0 on success
+ *         -1 if the file can not be opened or created
+ *         -2 if the file exists but specified track does not exist
+ */
+template<typename Time>
+int
+SMF<Time>::open(const std::string& path, int track) THROW_FILE_ERROR
+{
+	assert(track >= 1);
 	if (_smf) { 
 		smf_delete(_smf);
 	}
 	
 	_path = path;
-	
 	_smf = smf_load(_path.c_str());
-	if (!_smf) {
-		if (!create) {
-			return -1;
-		}
-
-		_smf = smf_new();
-		if (smf_set_ppqn(_smf, _ppqn) != 0) {
-			throw FileError();
-		}
-		
-		if (_smf == NULL) {
-			return -1;
-		}
-		
-		for (int i = 0; i < track; ++i) {
-			_smf_track = smf_track_new();
-			assert(_smf_track);
-			smf_add_track(_smf, _smf_track);
-		}
+	if (_smf == NULL) {
+		return -1;
 	}
-		
+
 	_smf_track = smf_get_track_by_number(_smf, track);
 	if (!_smf_track)
 		return -2;
 
 	cerr << "Track " << track << " # events: " << _smf_track->number_of_events << endl;
+	if (_smf_track->number_of_events == 0) {
+		_smf_track->next_event_number = -1;
+		_empty = true;
+	} else {
+		_smf_track->next_event_number = 1;
+		_empty = false;
+	}
 	
-	_empty = !(_smf_track->number_of_events > 0);
+	return 0;
+}
+
+
+/** Attempt to create a new SMF file for reading and/or writing.
+ *
+ * \return  0 on success
+ *         -1 if the file can not be created
+ *         -2 if the track can not be created
+ */
+template<typename Time>
+int
+SMF<Time>::create(const std::string& path, int track, int ppqn) THROW_FILE_ERROR
+{
+	assert(track >= 1);
+	if (_smf) { 
+		smf_delete(_smf);
+	}
+	
+	_path = path;
+
+	_smf = smf_new();
+	if (smf_set_ppqn(_smf, ppqn) != 0) {
+		throw FileError();
+	}
+	
+	if (_smf == NULL) {
+		return -1;
+	}
+	
+	for (int i = 0; i < track; ++i) {
+		_smf_track = smf_track_new();
+		assert(_smf_track);
+		smf_add_track(_smf, _smf_track);
+	}
+
+	_smf_track = smf_get_track_by_number(_smf, track);
+	if (!_smf_track)
+		return -2;
+
+	_smf_track->next_event_number = -1;
+	_empty = true;
 	
 	return 0;
 }
@@ -106,7 +168,7 @@ template<typename Time>
 void
 SMF<Time>::seek_to_start() const
 {
-	smf_rewind(_smf);
+	_smf_track->next_event_number = 1;
 }
 
 /** Read an event from the current position in file.
@@ -116,24 +178,24 @@ SMF<Time>::seek_to_start() const
  * will have it's time field set to it's delta time, in SMF tempo-based ticks, using the
  * rate given by ppqn() (it is the caller's responsibility to calculate a real time).
  *
- * \a size should be the capacity of \a buf.  If it is not large enough, \a buf will
- * be freed and a new buffer allocated in its place, the size of which will be placed
- * in size.
+ * \a buf must be a pointer to a buffer allocated with malloc, or a pointer to NULL.
+ * \a size must be the capacity of \a buf.  If it is not large enough, \a buf will
+ * be reallocated and *size will be set to the new size of buf.
  *
- * Returns event length (including status byte) on success, 0 if event was
- * skipped (eg a meta event), or -1 on EOF (or end of track).
+ * \return event length (including status byte) on success, 0 if event was
+ * skipped (e.g. a meta event), or -1 on EOF (or end of track).
  */
 template<typename Time>
 int
 SMF<Time>::read_event(uint32_t* delta_t, uint32_t* size, uint8_t** buf) const
 {
-	smf_event_t *event;
+	smf_event_t* event;
 	
 	assert(delta_t);
 	assert(size);
 	assert(buf);
 	
-    if ((event = smf_get_next_event(_smf)) != NULL) {
+    if ((event = smf_track_get_next_event(_smf_track)) != NULL) {
     	if (smf_event_is_metadata(event)) {
     		return 0;
     	}
@@ -148,6 +210,11 @@ SMF<Time>::read_event(uint32_t* delta_t, uint32_t* size, uint8_t** buf) const
     	}
     	memcpy(*buf, event->midi_buffer, size_t(event_size));
     	*size = event_size;
+	
+		/*printf("SMF::read_event:\n");
+		for (size_t i=0; i < *size; ++i) {
+			printf("%X ", (*buf)[i]);
+		} printf("\n");*/
     	
     	return event_size;
     } else {
@@ -161,7 +228,12 @@ SMF<Time>::append_event_delta(uint32_t delta_t, const Event<Time>& ev)
 {
 	assert(ev.size() > 0);
 	
-	smf_event_t *event;
+	/*printf("SMF::append_event_delta:\n");
+	for (size_t i=0; i < ev.size(); ++i) {
+		printf("%X ", ev.buffer()[i]);
+	} printf("\n");*/
+
+	smf_event_t* event;
 
 	event = smf_event_new_from_pointer((void *) ev.buffer(), int(ev.size()));
 	assert(event != NULL);
@@ -169,7 +241,7 @@ SMF<Time>::append_event_delta(uint32_t delta_t, const Event<Time>& ev)
 	memcpy(event->midi_buffer, ev.buffer(), ev.size());
 	
 	assert(_smf_track);
-	smf_track_add_event_delta_pulses (_smf_track, event, int(delta_t));
+	smf_track_add_event_delta_pulses(_smf_track, event, int(delta_t));
 	_last_ev_time = ev.time();
 	
 	if (ev.size() > 0) {
diff --git a/libs/evoral/src/Sequence.cpp b/libs/evoral/src/Sequence.cpp
index 0e96b5019e..78a9adda80 100644
--- a/libs/evoral/src/Sequence.cpp
+++ b/libs/evoral/src/Sequence.cpp
@@ -17,18 +17,17 @@
  */
 
 #define __STDC_LIMIT_MACROS 1
-
-#include <iostream>
-#include <cmath>
 #include <algorithm>
+#include <cmath>
+#include <iostream>
 #include <stdexcept>
 #include <stdint.h>
-#include "evoral/Sequence.hpp"
-#include "evoral/ControlList.hpp"
 #include "evoral/Control.hpp"
+#include "evoral/ControlList.hpp"
 #include "evoral/ControlSet.hpp"
 #include "evoral/EventSink.hpp"
 #include "evoral/MIDIParameters.hpp"
+#include "evoral/Sequence.hpp"
 #include "evoral/TypeMap.hpp"
 
 using namespace std;
@@ -676,8 +675,7 @@ Sequence<Time>::append(const Event<Time>& event)
 	assert(_writing);
 
 	if (ev.is_note_on()) {
-		append_note_on_unlocked(ev.channel(), ev.time(), ev.note(),
-				ev.velocity());
+		append_note_on_unlocked(ev.channel(), ev.time(), ev.note(), ev.velocity());
 	} else if (ev.is_note_off()) {
 		append_note_off_unlocked(ev.channel(), ev.time(), ev.note());
 	} else if (ev.is_sysex()) {
@@ -717,13 +715,19 @@ void
 Sequence<Time>::append_note_on_unlocked(uint8_t chan, Time time, uint8_t note_num, uint8_t velocity)
 {
 	#ifdef DEBUG_SEQUENCE
-	debugout << this << " c" << (int)chan << " note " << (int)note_num << " off @ " << time << endl;
+	debugout << this << " c=" << (int)chan << " note " << (int)note_num
+		<< " on @ " << time << " v=" << (int)velocity << endl;
 	#endif
 	assert(note_num <= 127);
 	assert(chan < 16);
 	assert(_writing);
 	_edited = true;
 
+	if (velocity == 0) {
+		append_note_off_unlocked(chan, time, note_num);
+		return;
+	}
+
 	if (note_num < _lowest_note)
 		_lowest_note = note_num;
 	if (note_num > _highest_note)
@@ -733,7 +737,8 @@ Sequence<Time>::append_note_on_unlocked(uint8_t chan, Time time, uint8_t note_nu
 	_notes.push_back(new_note);
 	if (!_percussive) {
 		#ifdef DEBUG_SEQUENCE
-		debugout << "Sustained: Appending active note on " << (unsigned)(uint8_t)note_num << endl;
+		debugout << "Sustained: Appending active note on " << (unsigned)(uint8_t)note_num
+			<< " channel " << chan << endl;
 		#endif
 		_write_notes[chan].push_back(_notes.size() - 1);
 	} else {
@@ -748,7 +753,8 @@ void
 Sequence<Time>::append_note_off_unlocked(uint8_t chan, Time time, uint8_t note_num)
 {
 	#ifdef DEBUG_SEQUENCE
-	debugout << this << " c" << (int)chan << " note " << (int)note_num << " off @ " << time << endl;
+	debugout << this << " c=" << (int)chan << " note " << (int)note_num
+		<< " off @ " << time << endl;
 	#endif
 	assert(note_num <= 127);
 	assert(chan < 16);
diff --git a/libs/evoral/src/libsmf/smf_tempo.c b/libs/evoral/src/libsmf/smf_tempo.c
index a5592f115c..d328b3a2b7 100644
--- a/libs/evoral/src/libsmf/smf_tempo.c
+++ b/libs/evoral/src/libsmf/smf_tempo.c
@@ -407,6 +407,10 @@ smf_track_add_event_delta_pulses(smf_track_t *track, smf_event_t *event, int del
 	assert(event->time_seconds == -1.0);
 	assert(track->smf != NULL);
 
+	if (!smf_event_is_valid(event)) {
+		g_critical("Added event is invalid");
+	}
+
 	smf_track_add_event_pulses(track, event, last_event_pulses(track) + delta);
 }
 
diff --git a/libs/evoral/test/SMFTest.cpp b/libs/evoral/test/SMFTest.cpp
index f401ea1abd..5a199a3bbd 100644
--- a/libs/evoral/test/SMFTest.cpp
+++ b/libs/evoral/test/SMFTest.cpp
@@ -8,9 +8,9 @@ void
 SMFTest::createNewFileTest ()
 {
 	TestSMF<Time> smf;
-	smf.open("NewFile.mid");
+	smf.create("NewFile.mid");
 	smf.close();
-	CPPUNIT_ASSERT(access(smf.path().c_str(), R_OK) == 0);
+	CPPUNIT_ASSERT(access("NewFile.mid", R_OK) == 0);
 	unlink(smf.path().c_str());
 }