diff options
author | Paul Davis <paul@linuxaudiosystems.com> | 2014-04-13 10:29:07 -0400 |
---|---|---|
committer | Paul Davis <paul@linuxaudiosystems.com> | 2014-04-14 02:17:30 -0400 |
commit | 343b06d8d3522d6b017d887ca754c91aec2430fd (patch) | |
tree | 81aa51fa0fea6826e62480314c1ac78b112b3a9a | |
parent | d2a31ab6eed8b3e8f34dbc1caf2285bb64777f55 (diff) |
dramatic change in logic and naming for operations related to adding a MIDI region on demand and cloning/unlinking
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.
-rw-r--r-- | gtk2_ardour/editor_ops.cc | 17 | ||||
-rw-r--r-- | gtk2_ardour/midi_time_axis.cc | 3 | ||||
-rw-r--r-- | libs/ardour/ardour/audiofilesource.h | 4 | ||||
-rw-r--r-- | libs/ardour/ardour/midi_region.h | 1 | ||||
-rw-r--r-- | libs/ardour/ardour/midi_source.h | 18 | ||||
-rw-r--r-- | libs/ardour/ardour/session.h | 9 | ||||
-rw-r--r-- | libs/ardour/ardour/smf_source.h | 2 | ||||
-rw-r--r-- | libs/ardour/file_source.cc | 2 | ||||
-rw-r--r-- | libs/ardour/midi_diskstream.cc | 18 | ||||
-rw-r--r-- | libs/ardour/midi_region.cc | 24 | ||||
-rw-r--r-- | libs/ardour/midi_source.cc | 28 | ||||
-rw-r--r-- | libs/ardour/session.cc | 126 |
12 files changed, 158 insertions, 94 deletions
diff --git a/gtk2_ardour/editor_ops.cc b/gtk2_ardour/editor_ops.cc index f764af3671..f7dc46659a 100644 --- a/gtk2_ardour/editor_ops.cc +++ b/gtk2_ardour/editor_ops.cc @@ -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; diff --git a/gtk2_ardour/midi_time_axis.cc b/gtk2_ardour/midi_time_axis.cc index 59c30c0c02..8ced280661 100644 --- a/gtk2_ardour/midi_time_axis.cc +++ b/gtk2_ardour/midi_time_axis.cc @@ -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); diff --git a/libs/ardour/ardour/audiofilesource.h b/libs/ardour/ardour/audiofilesource.h index 53819c1c9e..e0a199fd72 100644 --- a/libs/ardour/ardour/audiofilesource.h +++ b/libs/ardour/ardour/audiofilesource.h @@ -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); diff --git a/libs/ardour/ardour/midi_region.h b/libs/ardour/ardour/midi_region.h index b326bb30d8..38229b998b 100644 --- a/libs/ardour/ardour/midi_region.h +++ b/libs/ardour/ardour/midi_region.h @@ -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; diff --git a/libs/ardour/ardour/midi_source.h b/libs/ardour/ardour/midi_source.h index ba50102ec9..07a32c5bfc 100644 --- a/libs/ardour/ardour/midi_source.h +++ b/libs/ardour/ardour/midi_source.h @@ -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). diff --git a/libs/ardour/ardour/session.h b/libs/ardour/ardour/session.h index d183494f87..ecb47a102a 100644 --- a/libs/ardour/ardour/session.h +++ b/libs/ardour/ardour/session.h @@ -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); diff --git a/libs/ardour/ardour/smf_source.h b/libs/ardour/ardour/smf_source.h index 6e69b7b2f3..193330ef36 100644 --- a/libs/ardour/ardour/smf_source.h +++ b/libs/ardour/ardour/smf_source.h @@ -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); diff --git a/libs/ardour/file_source.cc b/libs/ardour/file_source.cc index 2b8a65ab92..6dc6ad9500 100644 --- a/libs/ardour/file_source.cc +++ b/libs/ardour/file_source.cc @@ -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; diff --git a/libs/ardour/midi_diskstream.cc b/libs/ardour/midi_diskstream.cc index 6fd40be57b..fdf155d78b 100644 --- a/libs/ardour/midi_diskstream.cc +++ b/libs/ardour/midi_diskstream.cc @@ -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; } diff --git a/libs/ardour/midi_region.cc b/libs/ardour/midi_region.cc index 8509e55f97..e7298e7526 100644 --- a/libs/ardour/midi_region.cc +++ b/libs/ardour/midi_region.cc @@ -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" @@ -127,15 +130,30 @@ 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 diff --git a/libs/ardour/midi_source.cc b/libs/ardour/midi_source.cc index d4452490fb..655222413a 100644 --- a/libs/ardour/midi_source.cc +++ b/libs/ardour/midi_source.cc @@ -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 diff --git a/libs/ardour/session.cc b/libs/ardour/session.cc index 93bce28fcd..68ea4c3faf 100644 --- a/libs/ardour/session.cc +++ b/libs/ardour/session.cc @@ -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); |