Fix MIDI loop recording.

This changes how things work a bit, but I am committing it for 3.0 since the
previous revision often crashed (and never worked), this one seems to work
fine, and the code is quite a bit more cogent.

I have tested the following use cases with live input and audible output:

 * Non-loop recording, armed before roll
 * Non-loop recording, arm while rolling
 * Loop recording, armed before roll
 * Loop recording, arm during roll

In the last case, the source/region is created starting at the loop start
rather than the current transport frame as usual so time makes sense when it
wraps around.

See the documentation added to the code for details, but the basic idea here is
to simply push MIDI events to the source with increasing monotonic time,
ignoring looping altogether.  Essentially we pretend the loop does not exist,
but the source knows all the details so it can implement whatever behaviour is
appropriate.

Currently, this is simply recording a complete non-destructive copy of the
input, which is a good thing.  Perhaps not what the user expects of loop
recording, but at least it works and is one sensible option.  We will need to
add more later.

Display while recording is a little bit wacky, but whatever.


git-svn-id: svn://localhost/ardour2/branches/3.0@13940 d708f5d6-7413-0410-9779-e7cbd77b26cf

5 files changed, 139 insertions, 98 deletions

diff --git a/libs/ardour/ardour/midi_diskstream.h b/libs/ardour/ardour/midi_diskstream.h
index 45ea975ea9..be0e0fe48c 100644
--- a/libs/ardour/ardour/midi_diskstream.h
+++ b/libs/ardour/ardour/midi_diskstream.h
@@ -195,6 +195,7 @@ class MidiDiskstream : public Diskstream
 	volatile gint                _frames_written_to_ringbuffer;
 	volatile gint                _frames_read_from_ringbuffer;
 	volatile gint                _frames_pending_write;
+	volatile gint                _num_captured_loops;
 
 	/** A buffer that we use to put newly-arrived MIDI data in for
 	    the GUI to read (so that it can update itself).
diff --git a/libs/ardour/ardour/midi_source.h b/libs/ardour/ardour/midi_source.h
index ed2950e9e5..a5aac8859b 100644
--- a/libs/ardour/ardour/midi_source.h
+++ b/libs/ardour/ardour/midi_source.h
@@ -88,7 +88,20 @@ class MidiSource : virtual public Source, public boost::enable_shared_from_this<
 	virtual void mark_streaming_midi_write_started (NoteMode mode);
 	virtual void mark_streaming_write_started ();
 	virtual void mark_streaming_write_completed ();
-	void mark_write_starting_now ();
+
+	/** Mark write starting with the given time parameters.
+	 *
+	 * This is called by MidiDiskStream::process before writing to the capture
+	 * buffer which will be later read by midi_read().
+	 *
+	 * @param position The timeline position the source now starts at.
+	 * @param capture_length The current length of the capture, which may not
+	 * be zero if record is armed while rolling.
+	 * @param loop_length The loop length if looping, otherwise zero.
+	 */
+	void mark_write_starting_now (framecnt_t position,
+	                              framecnt_t capture_length,
+	                              framecnt_t loop_length);
 
 	/* like ::mark_streaming_write_completed() but with more arguments to
 	 * allow control over MIDI-specific behaviour. Expected to be used only
@@ -104,9 +117,6 @@ class MidiSource : virtual public Source, public boost::enable_shared_from_this<
 	std::string captured_for() const               { return _captured_for; }
 	void        set_captured_for (std::string str) { _captured_for = str; }
 
-	framepos_t last_write_end() const { return _last_write_end; }
-	void set_last_write_end (framepos_t pos) { _last_write_end = pos; }
-
 	static PBD::Signal1<void,MidiSource*> MidiSourceCreated;
 
 	XMLNode& get_state ();
@@ -157,11 +167,16 @@ class MidiSource : virtual public Source, public boost::enable_shared_from_this<
 	                                  framecnt_t                     cnt,
 	                                  MidiStateTracker*              tracker) const = 0;
 
-	virtual framecnt_t write_unlocked (MidiRingBuffer<framepos_t>& dst,
+	/** Write data to this source from a MidiRingBuffer.
+	 *  @param source Buffer to read from.
+	 *  @param position This source's start position in session frames.
+	 *  @param cnt The duration of this block to write for.
+	 */
+	virtual framecnt_t write_unlocked (MidiRingBuffer<framepos_t>& source,
 	                                   framepos_t                  position,
 	                                   framecnt_t                  cnt) = 0;
 
-	std::string      _captured_for;
+	std::string _captured_for;
 
 	boost::shared_ptr<MidiModel> _model;
 	bool                         _writing;
@@ -171,7 +186,12 @@ class MidiSource : virtual public Source, public boost::enable_shared_from_this<
 
 	mutable double     _length_beats;
 	mutable framepos_t _last_read_end;
-	framepos_t         _last_write_end;
+
+	/** The total duration of the current capture. */
+	framepos_t _capture_length;
+
+	/** Length of transport loop during current capture, or zero. */
+	framepos_t _capture_loop_length;
 
 	/** Map of interpolation styles to use for Parameters; if they are not in this map,
 	 *  the correct interpolation style can be obtained from EventTypeMap::interpolation_of ()
diff --git a/libs/ardour/midi_diskstream.cc b/libs/ardour/midi_diskstream.cc
index 48fd3fb77f..d8429fdcd1 100644
--- a/libs/ardour/midi_diskstream.cc
+++ b/libs/ardour/midi_diskstream.cc
@@ -74,6 +74,7 @@ MidiDiskstream::MidiDiskstream (Session &sess, const string &name, Diskstream::F
 	, _frames_written_to_ringbuffer(0)
 	, _frames_read_from_ringbuffer(0)
 	, _frames_pending_write(0)
+	, _num_captured_loops(0)
 	, _gui_feed_buffer(AudioEngine::instance()->raw_buffer_size (DataType::MIDI))
 {
 	in_set_state = true;
@@ -97,6 +98,7 @@ MidiDiskstream::MidiDiskstream (Session& sess, const XMLNode& node)
 	, _frames_written_to_ringbuffer(0)
 	, _frames_read_from_ringbuffer(0)
 	, _frames_pending_write(0)
+	, _num_captured_loops(0)
 	, _gui_feed_buffer(AudioEngine::instance()->raw_buffer_size (DataType::MIDI))
 {
 	in_set_state = true;
@@ -200,9 +202,7 @@ MidiDiskstream::non_realtime_input_change ()
 	}
 
 	g_atomic_int_set(&_frames_pending_write, 0);
-	if (_write_source) {
-		_write_source->set_last_write_end (_session.transport_frame());
-	}
+	g_atomic_int_set(&_num_captured_loops, 0);
 }
 
 int
@@ -299,6 +299,26 @@ MidiDiskstream::set_note_mode (NoteMode m)
 		_write_source->model()->set_note_mode(m);
 }
 
+/** Get the start, end, and length of a location "atomically".
+ *
+ * Note: Locations don't get deleted, so all we care about when I say "atomic"
+ * is that we are always pointing to the same one and using start/length values
+ * obtained just once.  Use this function to achieve this since location being
+ * a parameter achieves this.
+ */
+static void
+get_location_times(const Location* location,
+                   framepos_t*     start,
+                   framepos_t*     end,
+                   framepos_t*     length)
+{
+	if (location) {
+		*start  = location->start();
+		*end    = location->end();
+		*length = *end - *start;
+	}
+}
+
 int
 MidiDiskstream::process (framepos_t transport_frame, pframes_t nframes, framecnt_t& playback_distance)
 {
@@ -330,6 +350,12 @@ MidiDiskstream::process (framepos_t transport_frame, pframes_t nframes, framecnt
 		return 1;
 	}
 
+	const Location* const loop_loc    = loop_location;
+	framepos_t            loop_start  = 0;
+	framepos_t            loop_end    = 0;
+	framepos_t            loop_length = 0;
+	get_location_times(loop_loc, &loop_start, &loop_end, &loop_length);
+
 	adjust_capture_position = 0;
 
 	if (nominally_recording || (re && was_recording && _session.get_record_enabled() && _session.config.get_punch_in())) {
@@ -338,9 +364,19 @@ MidiDiskstream::process (framepos_t transport_frame, pframes_t nframes, framecnt
 		calculate_record_range(ot, transport_frame, nframes, rec_nframes, rec_offset);
 
 		if (rec_nframes && !was_recording) {
-			_write_source->mark_write_starting_now ();
+			if (loop_loc) {
+				/* Loop recording, so pretend the capture started at the loop
+				   start rgardless of what time it is now, so the source starts
+				   at the loop start and can handle time wrapping around.
+				   Otherwise, start the source right now as usual.
+				*/
+				capture_captured    = transport_frame - loop_start;
+				capture_start_frame = loop_start;
+			}
+			_write_source->mark_write_starting_now(
+				capture_start_frame, capture_captured, loop_length);
 			g_atomic_int_set(&_frames_pending_write, 0);
-			capture_captured = 0;
+			g_atomic_int_set(&_num_captured_loops, 0);
 			was_recording = true;
 		}
 	}
@@ -370,7 +406,18 @@ MidiDiskstream::process (framepos_t transport_frame, pframes_t nframes, framecnt
 				DEBUG_TRACE (DEBUG::MidiIO, DEBUG_STR(a).str());
 			}
 #endif
-			_capture_buf->write(ev.time() + transport_frame, ev.type(), ev.size(), ev.buffer());
+			/* Write events to the capture buffer in frames from session start,
+			   but ignoring looping so event time progresses monotonically.
+			   The source knows the loop length so it knows exactly where the
+			   event occurs in the series of recorded loops and can implement
+			   any desirable behaviour.  We don't want to send event with
+			   transport time here since that way the source can not
+			   reconstruct their actual time; future clever MIDI looping should
+			   probabl be implemented in the source instead of here.
+			*/
+			const framecnt_t loop_offset = _num_captured_loops * loop_length;
+			_capture_buf->write(transport_frame + loop_offset + ev.time(),
+			                    ev.type(), ev.size(), ev.buffer());
 		}
 		g_atomic_int_add(&_frames_pending_write, nframes);
 
@@ -551,29 +598,17 @@ MidiDiskstream::internal_playback_seek (framecnt_t distance)
 int
 MidiDiskstream::read (framepos_t& start, framecnt_t dur, bool reversed)
 {
-	framecnt_t this_read = 0;
-	bool reloop = false;
-	framepos_t loop_end = 0;
-	framepos_t loop_start = 0;
-	Location *loc = 0;
+	framecnt_t this_read   = 0;
+	bool       reloop      = false;
+	framepos_t loop_end    = 0;
+	framepos_t loop_start  = 0;
+	framecnt_t loop_length = 0;
+	Location*  loc         = 0;
 
 	if (!reversed) {
 
-		framecnt_t loop_length = 0;
-
-		/* Make the use of a Location atomic for this read operation.
-
-		   Note: Locations don't get deleted, so all we care about
-		   when I say "atomic" is that we are always pointing to
-		   the same one and using a start/length values obtained
-		   just once.
-		*/
-
-		if ((loc = loop_location) != 0) {
-			loop_start = loc->start();
-			loop_end = loc->end();
-			loop_length = loop_end - loop_start;
-		}
+		loc = loop_location;
+		get_location_times(loc, &loop_start, &loop_end, &loop_length);
 
 		/* if we are looping, ensure that the first frame we read is at the correct
 		   position within the loop.
@@ -948,33 +983,15 @@ MidiDiskstream::transport_stopped_wallclock (struct tm& /*when*/, time_t /*twhen
 void
 MidiDiskstream::transport_looped (framepos_t transport_frame)
 {
+	/* Here we only keep track of the number of captured loops so monotonic
+	   event times can be delivered to the write source in process().  Trying
+	   to be clever here is a world of trouble, it is better to simply record
+	   the input in a straightforward non-destructive way.  In the future when
+	   we want to implement more clever MIDI looping modes it should be done in
+	   the Source and/or entirely after the capture is finished.
+	*/
 	if (was_recording) {
-
-		// adjust the capture length knowing that the data will be recorded to disk
-		// only necessary after the first loop where we're recording
-		if (capture_info.size() == 0) {
-			capture_captured += _capture_offset;
-
-			if (_alignment_style == ExistingMaterial) {
-				capture_captured += _session.worst_output_latency();
-			} else {
-				capture_captured += _roll_delay;
-			}
-		}
-
-		finish_capture ();
-
-		// the next region will start recording via the normal mechanism
-		// we'll set the start position to the current transport pos
-		// no latency adjustment or capture offset needs to be made, as that already happened the first time
-		capture_start_frame = transport_frame;
-		first_recordable_frame = transport_frame; // mild lie
-		last_recordable_frame = max_framepos;
-		was_recording = true;
-	}
-
-	if (!Config->get_seamless_loop()) {
-		reset_tracker ();
+		g_atomic_int_add(&_num_captured_loops, 1);
 	}
 }
 
diff --git a/libs/ardour/midi_source.cc b/libs/ardour/midi_source.cc
index 7ec82ed4e6..30dc23f998 100644
--- a/libs/ardour/midi_source.cc
+++ b/libs/ardour/midi_source.cc
@@ -59,7 +59,8 @@ MidiSource::MidiSource (Session& s, string name, Source::Flag flags)
 	, _model_iter_valid(false)
 	, _length_beats(0.0)
 	, _last_read_end(0)
-	, _last_write_end(0)
+	, _capture_length(0)
+	, _capture_loop_length(0)
 {
 }
 
@@ -69,7 +70,8 @@ MidiSource::MidiSource (Session& s, const XMLNode& node)
 	, _model_iter_valid(false)
 	, _length_beats(0.0)
 	, _last_read_end(0)
-	, _last_write_end(0)
+	, _capture_length(0)
+	, _capture_loop_length(0)
 {
 	if (set_state (node, Stateful::loading_state_version)) {
 		throw failed_constructor();
@@ -266,7 +268,7 @@ MidiSource::midi_write (MidiRingBuffer<framepos_t>& source,
 	if (cnt == max_framecnt) {
 		_last_read_end = 0;
 	} else {
-		_last_write_end += cnt;
+		_capture_length += cnt;
 	}
 
 	return ret;
@@ -284,10 +286,12 @@ MidiSource::mark_streaming_midi_write_started (NoteMode mode)
 }
 
 void
-MidiSource::mark_write_starting_now ()
+MidiSource::mark_write_starting_now (framecnt_t position,
+                                     framecnt_t capture_length,
+                                     framecnt_t loop_length)
 {
 	/* I'm not sure if this is the best way to approach this, but
-	   _last_write_end needs to be set up with the transport frame
+	   _capture_length needs to be set up with the transport frame
 	   when a record actually starts, as it is used by
 	   SMFSource::write_unlocked to decide whether incoming notes
 	   are within the correct time range.
@@ -297,8 +301,12 @@ MidiSource::mark_write_starting_now ()
 	   because it is not RT-safe.
 	*/
 
-	set_timeline_position (_session.transport_frame ());
-	_last_write_end = _session.transport_frame ();
+	set_timeline_position(position);
+	_capture_length      = capture_length;
+	_capture_loop_length = loop_length;
+
+	BeatsFramesConverter converter(_session.tempo_map(), position);
+	_length_beats = converter.from(capture_length);
 }
 
 void
diff --git a/libs/ardour/smf_source.cc b/libs/ardour/smf_source.cc
index 2b4c7ef7c9..5b42852923 100644
--- a/libs/ardour/smf_source.cc
+++ b/libs/ardour/smf_source.cc
@@ -211,14 +211,10 @@ SMFSource::read_unlocked (Evoral::EventSink<framepos_t>& destination,
 	return duration;
 }
 
-/** Write data to this source from a MidiRingBuffer.
- *  @param source Buffer to read from.
- *  @param position This source's start position in session frames.
- */
 framecnt_t
 SMFSource::write_unlocked (MidiRingBuffer<framepos_t>& source,
                            framepos_t                  position,
-                           framecnt_t                  duration)
+                           framecnt_t                  cnt)
 {
 	if (!_writing) {
 		mark_streaming_write_started ();
@@ -231,55 +227,57 @@ SMFSource::write_unlocked (MidiRingBuffer<framepos_t>& source,
 	size_t   buf_capacity = 4;
 	uint8_t* buf          = (uint8_t*)malloc(buf_capacity);
 
-	if (_model && ! _model->writing()) {
+	if (_model && !_model->writing()) {
 		_model->start_write();
 	}
 
 	Evoral::MIDIEvent<framepos_t> ev;
-
-	// cerr << "SMFSource::write unlocked, begins writing from src buffer with _last_write_end = " << _last_write_end << " dur = " << duration << endl;
-
 	while (true) {
+		/* Get the event time, in frames since session start but ignoring looping. */
 		bool ret;
-
 		if (!(ret = source.peek ((uint8_t*)&time, sizeof (time)))) {
-			/* no more events to consider */
+			/* Ring is empty, no more events. */
 			break;
 		}
 
-		if ((duration != max_framecnt) && (time > (_last_write_end + duration))) {
-			DEBUG_TRACE (DEBUG::MidiIO, string_compose ("SMFSource::write_unlocked: dropping event @ %1 because it is later than %2 + %3\n",
-								    time, _last_write_end, duration));
+		if ((cnt != max_framecnt) &&
+		    (time > position + _capture_length + cnt)) {
+			/* The diskstream doesn't want us to write everything, and this
+			   event is past the end of this block, so we're done for now. */
 			break;
 		}
 
+		/* Read the time, type, and size of the event. */
 		if (!(ret = source.read_prefix (&time, &type, &size))) {
-			error << _("Unable to read event prefix, corrupt MIDI ring buffer") << endmsg;
+			error << _("Unable to read event prefix, corrupt MIDI ring") << endmsg;
 			break;
 		}
 
+		/* Enlarge body buffer if necessary now that we know the size. */
 		if (size > buf_capacity) {
 			buf_capacity = size;
 			buf = (uint8_t*)realloc(buf, size);
 		}
 
+		/* Read the event body into buffer. */
 		ret = source.read_contents(size, buf);
 		if (!ret) {
-			error << _("Read time/size but not buffer, corrupt MIDI ring buffer") << endmsg;
+			error << _("Event has time and size but no body, corrupt MIDI ring") << endmsg;
 			break;
 		}
 
-		/* convert from session time to time relative to the source start */
-		assert(time >= position);
+		/* Convert event time from absolute to source relative. */
+		if (time < position) {
+			error << _("Event time is before MIDI source position") << endmsg;
+			break;
+		}
 		time -= position;
-
+			
 		ev.set(buf, size, time);
 		ev.set_event_type(EventTypeMap::instance().midi_event_type(ev.buffer()[0]));
-		ev.set_id (Evoral::next_event_id());
+		ev.set_id(Evoral::next_event_id());
 
 		if (!(ev.is_channel_event() || ev.is_smf_meta_event() || ev.is_sysex())) {
-			/*cerr << "SMFSource: WARNING: caller tried to write non SMF-Event of type "
-					<< std::hex << int(ev.buffer()[0]) << endl;*/
 			continue;
 		}
 
@@ -289,16 +287,14 @@ SMFSource::write_unlocked (MidiRingBuffer<framepos_t>& source,
 	Evoral::SMF::flush ();
 	free (buf);
 
-	return duration;
+	return cnt;
 }
 
-
 /** Append an event with a timestamp in beats (double) */
 void
 SMFSource::append_event_unlocked_beats (const Evoral::Event<double>& ev)
 {
-	assert(_writing);
-	if (ev.size() == 0)  {
+	if (!_writing || ev.size() == 0)  {
 		return;
 	}
 
@@ -306,9 +302,9 @@ SMFSource::append_event_unlocked_beats (const Evoral::Event<double>& ev)
                name().c_str(), ev.id(), ev.time(), ev.size());
 	       for (size_t i = 0; i < ev.size(); ++i) printf("%X ", ev.buffer()[i]); printf("\n");*/
 
-	assert(ev.time() >= 0);
 	if (ev.time() < _last_ev_time_beats) {
-		cerr << "SMFSource: Warning: Skipping event with non-monotonic time" << endl;
+		warning << string_compose(_("Skipping event with unordered time %1"), ev.time())
+		        << endmsg;
 		return;
 	}
 
@@ -337,9 +333,7 @@ SMFSource::append_event_unlocked_beats (const Evoral::Event<double>& ev)
 void
 SMFSource::append_event_unlocked_frames (const Evoral::Event<framepos_t>& ev, framepos_t position)
 {
-	assert(_writing);
-	if (ev.size() == 0)  {
-		cerr << "SMFSource: asked to append zero-size event\n";
+	if (!_writing || ev.size() == 0)  {
 		return;
 	}
 
@@ -348,7 +342,8 @@ SMFSource::append_event_unlocked_frames (const Evoral::Event<framepos_t>& ev, fr
 	// for (size_t i=0; i < ev.size(); ++i) printf("%X ", ev.buffer()[i]); printf("\n");
 
 	if (ev.time() < _last_ev_time_frames) {
-		cerr << "SMFSource: Warning: Skipping event with non-monotonic time" << endl;
+		warning << string_compose(_("Skipping event with unordered time %1"), ev.time())
+		        << endmsg;
 		return;
 	}