dramatic change in logic and naming for operations related to adding a MIDI region...
authorPaul Davis <paul@linuxaudiosystems.com>
Sun, 13 Apr 2014 14:29:07 +0000 (10:29 -0400)
committerPaul Davis <paul@linuxaudiosystems.com>
Mon, 14 Apr 2014 06:17:30 +0000 (02:17 -0400)
Existing code would cause data loss due to creation of two Source objects referring the same path, one with removable flags and one without. Careful code review suggested a variety of thinkos, function naming problems and other confusion that caused this. I have tried ot extensively comment what is going on with these operations, because it is one key area in which MIDI differs from audio: with audio, capture is the only way to add a new audio region, but for MIDI there are GUI input events that can add a new region.

12 files changed:
gtk2_ardour/editor_ops.cc
gtk2_ardour/midi_time_axis.cc
libs/ardour/ardour/audiofilesource.h
libs/ardour/ardour/midi_region.h
libs/ardour/ardour/midi_source.h
libs/ardour/ardour/session.h
libs/ardour/ardour/smf_source.h
libs/ardour/file_source.cc
libs/ardour/midi_diskstream.cc
libs/ardour/midi_region.cc
libs/ardour/midi_source.cc
libs/ardour/session.cc

index f764af3671c9c2b110db6b7545fd90413bf6063f..f7dc46659a11de94952b07b213a81f8af348b71c 100644 (file)
@@ -4818,12 +4818,17 @@ Editor::fork_region ()
                MidiRegionView* const mrv = dynamic_cast<MidiRegionView*>(*r);
 
                if (mrv) {
-                       boost::shared_ptr<Playlist> playlist = mrv->region()->playlist();
-                       boost::shared_ptr<MidiRegion> newregion = mrv->midi_region()->clone ();
-
-                       playlist->clear_changes ();
-                       playlist->replace_region (mrv->region(), newregion, mrv->region()->position());
-                       _session->add_command(new StatefulDiffCommand (playlist));
+                       try {
+                               boost::shared_ptr<Playlist> playlist = mrv->region()->playlist();
+                               boost::shared_ptr<MidiSource> new_source = _session->create_midi_source_by_stealing_name (mrv->midi_view()->track());
+                               boost::shared_ptr<MidiRegion> newregion = mrv->midi_region()->clone (new_source);
+                               
+                               playlist->clear_changes ();
+                               playlist->replace_region (mrv->region(), newregion, mrv->region()->position());
+                               _session->add_command(new StatefulDiffCommand (playlist));
+                       } catch (...) {
+                               error << string_compose (_("Could not unlink %1"), mrv->region()->name()) << endmsg;
+                       }
                }
 
                r = tmp;
index 59c30c0c021cb1b15d42dc82e6d66ecf8ede453f..8ced280661ec9f6806acfe53294d38b5abc784db 100644 (file)
@@ -1521,8 +1521,7 @@ MidiTimeAxisView::add_region (framepos_t pos, framecnt_t length, bool commit)
 
        real_editor->snap_to (pos, 0);
 
-       boost::shared_ptr<Source> src = _session->create_midi_source_for_session (
-               view()->trackview().track().get(), view()->trackview().track()->name());
+       boost::shared_ptr<Source> src = _session->create_midi_source_by_stealing_name (view()->trackview().track());
        PropertyList plist;
 
        plist.add (ARDOUR::Properties::start, 0);
index 53819c1c9e0fe43162dc3cf80ce1d38ff6056c86..e0a199fd72a46b920ff77a5fba68ea62196f5896 100644 (file)
@@ -39,10 +39,6 @@ class LIBARDOUR_API AudioFileSource : public AudioSource, public FileSource {
 public:
        virtual ~AudioFileSource ();
 
-       bool set_name (const std::string& newname) {
-               return (set_source_name(newname, destructive()) == 0);
-       }
-
        std::string peak_path (std::string audio_path);
        std::string find_broken_peakfile (std::string missing_peak_path,
                                          std::string audio_path);
index b326bb30d87a0f8df2794240535de47b3ee45497..38229b998beae62516c956095142813cfc42e717 100644 (file)
@@ -64,6 +64,7 @@ class LIBARDOUR_API MidiRegion : public Region
        ~MidiRegion();
 
        boost::shared_ptr<MidiRegion> clone (std::string path = std::string()) const;
+       boost::shared_ptr<MidiRegion> clone (boost::shared_ptr<MidiSource>) const;
 
        boost::shared_ptr<MidiSource> midi_source (uint32_t n=0) const;
 
index ba50102ec915b7ed89ee8c2aeda1be73db05317b..07a32c5bfcdd5d0c7c17f86ce615d9102b4076a5 100644 (file)
@@ -49,9 +49,21 @@ class LIBARDOUR_API MidiSource : virtual public Source, public boost::enable_sha
        MidiSource (Session& session, const XMLNode&);
        virtual ~MidiSource ();
 
-       boost::shared_ptr<MidiSource> clone (const std::string& path,
-                                            Evoral::MusicalTime begin = Evoral::MinMusicalTime,
-                                            Evoral::MusicalTime end = Evoral::MaxMusicalTime);
+       /** Write the data in the given time range to another MidiSource
+        * \param newsrc MidiSource to which data will be written. Should be a
+        *        new, empty source. If it already has contents, the results are
+        *        undefined. Source must be writable.
+        *
+        * \param begin time of earliest event that can be written.
+        * \param end time of latest event that can be written.
+        *
+        * Returns zero on success, non-zero if the write failed for any
+        * reason.
+        *
+        */
+       int write_to (boost::shared_ptr<MidiSource> newsrc,
+                     Evoral::MusicalTime begin = Evoral::MinMusicalTime,
+                     Evoral::MusicalTime end = Evoral::MaxMusicalTime);
 
        /** Read the data in a given time range from the MIDI source.
         * All time stamps in parameters are in audio frames (even if the source has tempo time).
index d183494f8759ff4a59084e63b535c2ffd3e977e0..ecb47a102a049320b0a4d254c5cb0e950310ebcb 100644 (file)
@@ -194,7 +194,7 @@ class LIBARDOUR_API Session : public PBD::StatefulDestructible, public PBD::Scop
 
        std::string peak_path (std::string) const;
 
-       std::string change_source_path_by_name (std::string oldpath, std::string oldname, std::string newname, bool destructive);
+       std::string generate_new_source_path_from_name (std::string oldpath, std::string oldname, std::string newname, bool destructive);
 
        std::string peak_path_from_audio_path (std::string) const;
        std::string new_audio_source_name (const std::string&, uint32_t nchans, uint32_t chan, bool destructive);
@@ -582,11 +582,12 @@ class LIBARDOUR_API Session : public PBD::StatefulDestructible, public PBD::Scop
        boost::shared_ptr<AudioFileSource> create_audio_source_for_session (
                size_t, std::string const &, uint32_t, bool destructive);
 
-       boost::shared_ptr<MidiSource> create_midi_source_for_session (
-               Track*, std::string const &);
+       boost::shared_ptr<MidiSource> create_midi_source_for_session (std::string const &);
+       boost::shared_ptr<MidiSource> create_midi_source_by_stealing_name (boost::shared_ptr<Track>);
 
        boost::shared_ptr<Source> source_by_id (const PBD::ID&);
-       boost::shared_ptr<Source> source_by_path_and_channel (const std::string&, uint16_t);
+       boost::shared_ptr<AudioFileSource> source_by_path_and_channel (const std::string&, uint16_t) const;
+       boost::shared_ptr<MidiSource> source_by_path (const std::string&) const;
        uint32_t count_sources_by_origin (const std::string&);
 
        void add_playlist (boost::shared_ptr<Playlist>, bool unused = false);
index 6e69b7b2f33f83056949c723eeeb4aeabc4e96e2..193330ef3688686c3e3c60e358d5a988233a6f7e 100644 (file)
@@ -49,8 +49,6 @@ public:
                return safe_midi_file_extension(path);
        }
 
-       bool set_name (const std::string& newname) { return (set_source_name(newname, false) == 0); }
-
        void append_event_unlocked_beats (const Evoral::Event<Evoral::MusicalTime>& ev);
        void append_event_unlocked_frames (const Evoral::Event<framepos_t>& ev, framepos_t source_start);
 
index 2b8a65ab9221887e0a1dc3d0a6af39528e4cdc81..6dc6ad9500f73e3bfdfc000298b80a1814915bbc 100644 (file)
@@ -520,7 +520,7 @@ FileSource::set_source_name (const string& newname, bool destructive)
 {
        Glib::Threads::Mutex::Lock lm (_lock);
        string oldpath = _path;
-       string newpath = _session.change_source_path_by_name (oldpath, _name, newname, destructive);
+       string newpath = _session.generate_new_source_path_from_name (oldpath, _name, newname, destructive);
 
        if (newpath.empty()) {
                error << string_compose (_("programming error: %1"), "cannot generate a changed file path") << endmsg;
index 6fd40be57bbed22abf0f2a4a9f8819a2cbf94adc..fdf155d78b76cdc9795d7928161d13cc07a7d222 100644 (file)
@@ -1198,7 +1198,7 @@ MidiDiskstream::use_new_write_source (uint32_t n)
 
        try {
                _write_source = boost::dynamic_pointer_cast<SMFSource>(
-                       _session.create_midi_source_for_session (0, name ()));
+                       _session.create_midi_source_for_session (name ()));
 
                if (!_write_source) {
                        throw failed_constructor();
@@ -1223,13 +1223,19 @@ MidiDiskstream::use_new_write_source (uint32_t n)
 std::string
 MidiDiskstream::steal_write_source_name ()
 {
-       std::string our_new_name = _session.new_midi_source_name (_write_source->name());
-       std::string our_old_name = _write_source->name();
-       
-       if (_write_source->set_source_name (our_new_name, false)) {
+       string our_old_name = _write_source->name();
+
+       /* this will bump the name of the current write source to the next one
+        * (e.g. "MIDI 1-1" gets renamed to "MIDI 1-2"), thus leaving the
+        * current write source name (e.g. "MIDI 1-1" available). See the
+        * comments in Session::create_midi_source_for_track() about why we do
+        * this.
+        */
+
+       if (_write_source->set_source_name (name(), false)) {
                return string();
        }
-
+       
        return our_old_name;
 }
 
index 8509e55f97017a06597897bbdcc92e5615edd4d9..e7298e752623423c4a7c8e472001a44a9077c5ef 100644 (file)
@@ -25,6 +25,8 @@
 #include <set>
 
 #include <glibmm/threads.h>
+#include <glibmm/fileutils.h>
+#include <glibmm/miscutils.h>
 
 #include "pbd/xml++.h"
 #include "pbd/basename.h"
@@ -36,6 +38,7 @@
 #include "ardour/midi_source.h"
 #include "ardour/region_factory.h"
 #include "ardour/session.h"
+#include "ardour/source_factory.h"
 #include "ardour/tempo.h"
 #include "ardour/types.h"
 
@@ -126,16 +129,31 @@ MidiRegion::~MidiRegion ()
  */
 boost::shared_ptr<MidiRegion>
 MidiRegion::clone (string path) const
+{
+       boost::shared_ptr<MidiSource> newsrc;
+
+       /* caller must check for pre-existing file */
+       assert (!Glib::file_test (path, Glib::FILE_TEST_EXISTS));
+       newsrc = boost::dynamic_pointer_cast<MidiSource>(
+               SourceFactory::createWritable(DataType::MIDI, _session,
+                                             path, false, _session.frame_rate()));
+       return clone (newsrc);
+}
+
+boost::shared_ptr<MidiRegion>
+MidiRegion::clone (boost::shared_ptr<MidiSource> newsrc) const
 {
        BeatsFramesConverter bfc (_session.tempo_map(), _position);
        Evoral::MusicalTime const bbegin = bfc.from (_start);
        Evoral::MusicalTime const bend = bfc.from (_start + _length);
 
-       boost::shared_ptr<MidiSource> ms = midi_source(0)->clone (path, bbegin, bend);
+       if (midi_source(0)->write_to (newsrc, bbegin, bend)) {
+               return boost::shared_ptr<MidiRegion> ();
+       }
 
        PropertyList plist;
 
-       plist.add (Properties::name, PBD::basename_nosuffix (ms->name()));
+       plist.add (Properties::name, PBD::basename_nosuffix (newsrc->name()));
        plist.add (Properties::whole_file, true);
        plist.add (Properties::start, _start);
        plist.add (Properties::start_beats, _start_beats);
@@ -143,7 +161,7 @@ MidiRegion::clone (string path) const
        plist.add (Properties::length_beats, _length_beats);
        plist.add (Properties::layer, 0);
 
-       return boost::dynamic_pointer_cast<MidiRegion> (RegionFactory::create (ms, plist, true));
+       return boost::dynamic_pointer_cast<MidiRegion> (RegionFactory::create (newsrc, plist, true));
 }
 
 void
index d4452490fbac5d02baad1d213fc2a24376c5b990..655222413aa0a5f5d836c6121c5511a99d8569c0 100644 (file)
@@ -332,29 +332,9 @@ MidiSource::mark_streaming_write_completed ()
        mark_midi_streaming_write_completed (Evoral::Sequence<Evoral::MusicalTime>::DeleteStuckNotes);
 }
 
-boost::shared_ptr<MidiSource>
-MidiSource::clone (const string& path, Evoral::MusicalTime begin, Evoral::MusicalTime end)
+int
+MidiSource::write_to (boost::shared_ptr<MidiSource> newsrc, Evoral::MusicalTime begin, Evoral::MusicalTime end)
 {
-       string newpath;
-
-       /* get a new name for the MIDI file we're going to write to
-        */
-       
-       if (path.empty()) {
-               string newname = PBD::basename_nosuffix(_name.val());
-               newname = bump_name_once (newname, '-');
-               newname += ".mid";
-               newpath = _session.new_source_path_from_name (DataType::MIDI, newname);
-       } else {
-               /* caller must check for pre-existing file */
-               assert (!Glib::file_test (path, Glib::FILE_TEST_EXISTS));
-               newpath = path;
-       }
-
-       boost::shared_ptr<MidiSource> newsrc = boost::dynamic_pointer_cast<MidiSource>(
-               SourceFactory::createWritable(DataType::MIDI, _session,
-                                             newpath, false, _session.frame_rate()));
-
        newsrc->set_timeline_position(_timeline_position);
        newsrc->copy_interpolation_from (this);
        newsrc->copy_automation_state_from (this);
@@ -367,7 +347,7 @@ MidiSource::clone (const string& path, Evoral::MusicalTime begin, Evoral::Musica
                }
        } else {
                error << string_compose (_("programming error: %1"), X_("no model for MidiSource during ::clone()"));
-               return boost::shared_ptr<MidiSource>();
+               return -1;
        }
 
        newsrc->flush_midi();
@@ -384,7 +364,7 @@ MidiSource::clone (const string& path, Evoral::MusicalTime begin, Evoral::Musica
 
        boost::dynamic_pointer_cast<FileSource> (newsrc)->prevent_deletion ();
 
-       return newsrc;
+       return 0;
 }
 
 void
index 93bce28fcd3c2cfd27067e73e2a26324d067546d..68ea4c3faf8f880cd591ecaf1edbc82f26faa2ec 100644 (file)
@@ -3304,12 +3304,16 @@ Session::source_by_id (const PBD::ID& id)
        return source;
 }
 
-boost::shared_ptr<Source>
-Session::source_by_path_and_channel (const string& path, uint16_t chn)
+boost::shared_ptr<AudioFileSource>
+Session::source_by_path_and_channel (const string& path, uint16_t chn) const
 {
+       /* Restricted to audio files because only audio sources have channel
+          as a property.
+       */
+
        Glib::Threads::Mutex::Lock lm (source_lock);
 
-       for (SourceMap::iterator i = sources.begin(); i != sources.end(); ++i) {
+       for (SourceMap::const_iterator i = sources.begin(); i != sources.end(); ++i) {
                boost::shared_ptr<AudioFileSource> afs
                        = boost::dynamic_pointer_cast<AudioFileSource>(i->second);
 
@@ -3317,7 +3321,31 @@ Session::source_by_path_and_channel (const string& path, uint16_t chn)
                        return afs;
                }
        }
-       return boost::shared_ptr<Source>();
+
+       return boost::shared_ptr<AudioFileSource>();
+}
+
+boost::shared_ptr<MidiSource>
+Session::source_by_path (const std::string& path) const
+{
+       /* Restricted to MIDI files because audio sources require a channel
+          for unique identification, in addition to a path.
+       */
+
+       Glib::Threads::Mutex::Lock lm (source_lock);
+
+       for (SourceMap::const_iterator s = sources.begin(); s != sources.end(); ++s) {
+               boost::shared_ptr<MidiSource> ms
+                       = boost::dynamic_pointer_cast<MidiSource>(s->second);
+               boost::shared_ptr<FileSource> fs
+                       = boost::dynamic_pointer_cast<FileSource>(s->second);
+               
+               if (ms && fs && fs->path() == path) {
+                       return ms;
+               }
+       }
+
+       return boost::shared_ptr<MidiSource>();
 }
 
 uint32_t
@@ -3338,9 +3366,8 @@ Session::count_sources_by_origin (const string& path)
        return cnt;
 }
 
-
 string
-Session::change_source_path_by_name (string path, string oldname, string newname, bool destructive)
+Session::generate_new_source_path_from_name (string path, string oldname, string newname, bool destructive)
 {
        string look_for;
        string old_basename = PBD::basename_nosuffix (oldname);
@@ -3427,7 +3454,9 @@ Session::change_source_path_by_name (string path, string oldname, string newname
 
                        if (!matching_unsuffixed_filename_exists_in (dir, buf)) {
                                path = Glib::build_filename (dir, buf);
-                               break;
+                               if (!source_by_path (path)) {
+                                       break;
+                               }
                        }
 
                        path = "";
@@ -3575,17 +3604,18 @@ Session::create_audio_source_for_session (size_t n_chans, string const & n, uint
                SourceFactory::createWritable (DataType::AUDIO, *this, path, destructive, frame_rate()));
 }
 
-/** Return a unique name based on \a base for a new internal MIDI source */
+/** Return a unique name based on \a owner_name for a new internal MIDI source */
 string
-Session::new_midi_source_name (const string& base)
+Session::new_midi_source_name (const string& owner_name)
 {
        uint32_t cnt;
        char buf[PATH_MAX+1];
        const uint32_t limit = 10000;
        string legalized;
+       string possible_name;
 
        buf[0] = '\0';
-       legalized = legalize_for_path (base);
+       legalized = legalize_for_path (owner_name);
 
        // Find a "version" of the file name that doesn't exist in any of the possible directories.
 
@@ -3597,12 +3627,17 @@ Session::new_midi_source_name (const string& base)
                for (i = session_dirs.begin(); i != session_dirs.end(); ++i) {
 
                        SessionDirectory sdir((*i).path);
+                       
+                       snprintf (buf, sizeof(buf), "%s-%u.mid", legalized.c_str(), cnt);
+                       possible_name = buf;
 
-                       std::string p = Glib::build_filename (sdir.midi_path(), legalized);
-
-                       snprintf (buf, sizeof(buf), "%s-%u.mid", p.c_str(), cnt);
+                       std::string possible_path = Glib::build_filename (sdir.midi_path(), possible_name);
+                       
+                       if (Glib::file_test (possible_path, Glib::FILE_TEST_EXISTS)) {
+                               existing++;
+                       }
 
-                       if (Glib::file_test (buf, Glib::FILE_TEST_EXISTS)) {
+                       if (source_by_path (possible_path)) {
                                existing++;
                        }
                }
@@ -3614,49 +3649,62 @@ Session::new_midi_source_name (const string& base)
                if (cnt > limit) {
                        error << string_compose(
                                        _("There are already %1 recordings for %2, which I consider too many."),
-                                       limit, base) << endmsg;
+                                       limit, owner_name) << endmsg;
                        destroy ();
                        throw failed_constructor();
                }
        }
 
-       return Glib::path_get_basename(buf);
+       return possible_name;
 }
 
 
 /** Create a new within-session MIDI source */
 boost::shared_ptr<MidiSource>
-Session::create_midi_source_for_session (Track* track, string const & n)
+Session::create_midi_source_for_session (string const & basic_name)
 {
        std::string name;
 
-       if (track) {
-               /* the caller passes in the track the source will be used in,
-                  so that we can keep the numbering sane. 
-                  
-                  Rationale: a track with the name "Foo" that has had N
-                  captures carried out so far will already have a write source
-                  named "Foo-N+1.mid" waiting to be used for the next capture.
-
-                  If we call new_midi_source_name() we will get "Foo-N+2". But
-                  there is no region corresponding to "Foo-N+1", so when
-                  "Foo-N+2" appears in the track, the gap presents the user
-                  with odd behaviour - why did it skip past Foo-N+1?
+       if (name.empty()) {
+               name = new_midi_source_name (basic_name);
+       }
 
-                  We could explain this to the user in some odd way, but
-                  instead we rename "Foo-N+1.mid" as "Foo-N+2.mid", and then
-                  use "Foo-N+1" here.
+       const string path = new_source_path_from_name (DataType::MIDI, name);
 
-                  If that attempted rename fails, we get "Foo-N+2.mid" anyway.
-               */
+       return boost::dynamic_pointer_cast<SMFSource> (
+               SourceFactory::createWritable (
+                       DataType::MIDI, *this, path, false, frame_rate()));
+}
 
-               MidiTrack* mt = dynamic_cast<MidiTrack*> (track);
-               assert (mt);
-               name = track->steal_write_source_name ();
-       }
+/** Create a new within-session MIDI source */
+boost::shared_ptr<MidiSource>
+Session::create_midi_source_by_stealing_name (boost::shared_ptr<Track> track)
+{
+       /* the caller passes in the track the source will be used in,
+          so that we can keep the numbering sane. 
+          
+          Rationale: a track with the name "Foo" that has had N
+          captures carried out so far will ALREADY have a write source
+          named "Foo-N+1.mid" waiting to be used for the next capture.
+          
+          If we call new_midi_source_name() we will get "Foo-N+2". But
+          there is no region corresponding to "Foo-N+1", so when
+          "Foo-N+2" appears in the track, the gap presents the user
+          with odd behaviour - why did it skip past Foo-N+1?
+          
+          We could explain this to the user in some odd way, but
+          instead we rename "Foo-N+1.mid" as "Foo-N+2.mid", and then
+          use "Foo-N+1" here.
+          
+          If that attempted rename fails, we get "Foo-N+2.mid" anyway.
+       */
+       
+       boost::shared_ptr<MidiTrack> mt = boost::dynamic_pointer_cast<MidiTrack> (track);
+       assert (mt);
+       std::string name = track->steal_write_source_name ();
 
        if (name.empty()) {
-               name = new_midi_source_name (n);
+               return boost::shared_ptr<MidiSource>();
        }
 
        const string path = new_source_path_from_name (DataType::MIDI, name);