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.
This commit is contained in:
parent
d2a31ab6ee
commit
343b06d8d3
|
@ -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;
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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;
|
||||
|
||||
|
|
|
@ -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).
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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);
|
||||
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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);
|
||||
std::string possible_path = Glib::build_filename (sdir.midi_path(), possible_name);
|
||||
|
||||
if (Glib::file_test (possible_path, Glib::FILE_TEST_EXISTS)) {
|
||||
existing++;
|
||||
}
|
||||
|
||||
snprintf (buf, sizeof(buf), "%s-%u.mid", p.c_str(), cnt);
|
||||
|
||||
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?
|
||||
|
||||
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.
|
||||
*/
|
||||
|
||||
MidiTrack* mt = dynamic_cast<MidiTrack*> (track);
|
||||
assert (mt);
|
||||
name = track->steal_write_source_name ();
|
||||
if (name.empty()) {
|
||||
name = new_midi_source_name (basic_name);
|
||||
}
|
||||
|
||||
const string path = new_source_path_from_name (DataType::MIDI, name);
|
||||
|
||||
return boost::dynamic_pointer_cast<SMFSource> (
|
||||
SourceFactory::createWritable (
|
||||
DataType::MIDI, *this, path, false, frame_rate()));
|
||||
}
|
||||
|
||||
/** 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);
|
||||
|
|
Loading…
Reference in New Issue