View Issue Details

IDCategoryLast Update
0006523bugs2017-08-02 23:02
ReporterEjisAssigned Tox42 
Reproducibilityalways 
Status resolvedResolutionfixed 
Product Version5.3 
Fixed in Version 
Summary0006523: (4.2) Serious problems with playlist names
DescriptionHello,

First, I had multiple crashes, and when I reopened my session, I found out that regions were erased, and started panicking. Then I looked at the playlists for these regions, and I saw 3 to 10 playlists with the same name. One of them had the regions I was looking for. As I don’t want to mess with the .ardour file too much, I decided, for each track, to give them a unique name to help me recognize them if that happens again. So each track has a main playlist named "lol" (because originality). That’s when it becomes really funny. When I reloaded the session, all the tracks had the same regions! Ardour doesn’t seem to care about IDs, so it thought all the tracks shared the same playlist!

So, the problems are:
- Ardour can randomly create empty playlists on some tracks, with all the same names, and randomly picks one, so you have to look for the good playlist that actually contains something.
- If each track has a playlist with the same name than another track’s playlist, Ardour will think they share the same one, and you’ll get the same regions.

Maybe Ardour should prevent the user from giving the same names to different playlists, and, more importantly, looking for IDs rather than names!
TagsNo tags attached.

Relationships

duplicate of 0005681 resolvedx42 POTENTIAL DATA LOSS due to duplicate playlist reference after renaming tracks 
related to 0007438 feedbackx42 Session gots corrupted after some manipulations 

Activities

elgoun

2016-01-12 03:56

reporter  

fix_6523.patch (4,962 bytes)
diff --git a/libs/ardour/ardour/audio_diskstream.h b/libs/ardour/ardour/audio_diskstream.h
index 21a1789..61d0e96 100644
--- a/libs/ardour/ardour/audio_diskstream.h
+++ b/libs/ardour/ardour/audio_diskstream.h
@@ -227,7 +227,8 @@ class LIBARDOUR_API AudioDiskstream : public Diskstream
 
 	int use_new_write_source (uint32_t n=0);
 
-	int find_and_use_playlist (const std::string &);
+	int find_and_use_playlist_by_name (const std::string &);
+	int find_and_use_playlist_by_id (const PBD::ID &);
 
 	void allocate_temporary_buffers ();
 
diff --git a/libs/ardour/ardour/diskstream.h b/libs/ardour/ardour/diskstream.h
index fb9be65..441c312 100644
--- a/libs/ardour/ardour/diskstream.h
+++ b/libs/ardour/ardour/diskstream.h
@@ -261,7 +261,8 @@ class LIBARDOUR_API Diskstream : public SessionObject, public PublicDiskstream
 
 	virtual int use_new_write_source (uint32_t n=0) = 0;
 
-	virtual int find_and_use_playlist (const std::string&) = 0;
+	virtual int find_and_use_playlist_by_name (const std::string&) = 0;
+	virtual int find_and_use_playlist_by_id (const PBD::ID&) = 0;
 
 	virtual void allocate_temporary_buffers () = 0;
 
diff --git a/libs/ardour/ardour/midi_diskstream.h b/libs/ardour/ardour/midi_diskstream.h
index 45f0176..974aab3 100644
--- a/libs/ardour/ardour/midi_diskstream.h
+++ b/libs/ardour/ardour/midi_diskstream.h
@@ -153,7 +153,8 @@ class LIBARDOUR_API MidiDiskstream : public Diskstream
 
 	int use_new_write_source (uint32_t n=0);
 
-	int find_and_use_playlist (const std::string&);
+	int find_and_use_playlist_by_name (const std::string&);
+	int find_and_use_playlist_by_id (const PBD::ID&);
 
 	void allocate_temporary_buffers ();
 
diff --git a/libs/ardour/audio_diskstream.cc b/libs/ardour/audio_diskstream.cc
index 6f7c2d6..173c754 100644
--- a/libs/ardour/audio_diskstream.cc
+++ b/libs/ardour/audio_diskstream.cc
@@ -245,7 +245,7 @@ AudioDiskstream::get_input_sources ()
 }
 
 int
-AudioDiskstream::find_and_use_playlist (const string& name)
+AudioDiskstream::find_and_use_playlist_by_name (const string& name)
 {
 	boost::shared_ptr<AudioPlaylist> playlist;
 
@@ -262,6 +262,21 @@ AudioDiskstream::find_and_use_playlist (const string& name)
 }
 
 int
+AudioDiskstream::find_and_use_playlist_by_id (const PBD::ID& p_id)
+{
+	boost::shared_ptr<AudioPlaylist> playlist;
+
+	playlist = boost::dynamic_pointer_cast<AudioPlaylist> (_session.playlists->by_id (p_id));
+
+	if (!playlist) {
+		error << string_compose(_("AudioDiskstream: Playlist \"%1\" isn't an audio playlist"), p_id.to_s()) << endmsg;
+		return -1;
+	}
+
+	return use_playlist (playlist);
+}
+
+int
 AudioDiskstream::use_playlist (boost::shared_ptr<Playlist> playlist)
 {
 	assert(boost::dynamic_pointer_cast<AudioPlaylist>(playlist));
diff --git a/libs/ardour/diskstream.cc b/libs/ardour/diskstream.cc
index 7f42140..919dc8f 100644
--- a/libs/ardour/diskstream.cc
+++ b/libs/ardour/diskstream.cc
@@ -465,6 +465,7 @@ Diskstream::get_state ()
 
 	node->add_property ("flags", enum_2_string (_flags));
 	node->add_property ("playlist", _playlist->name());
+	node->add_property("playlist-id", _playlist->id().to_s());
 	node->add_property("name", _name);
 	id().print (buf, sizeof (buf));
 	node->add_property("id", buf);
@@ -510,12 +511,23 @@ Diskstream::set_state (const XMLNode& node, int /*version*/)
                 set_align_choice (Automatic, true);
         }
 
-	if ((prop = node.property ("playlist")) == 0) {
-		return -1;
-	}
+	if ((prop = node.property ("playlist-id")) != 0) {
+		if (find_and_use_playlist_by_id (prop->value())) {
+			return -1;
+		}
 
-	if (find_and_use_playlist (prop->value())) {
-		return -1;
+	} else {
+		/*
+		 * "Old" Behavior
+		 * Permits "old" sessions to load
+		 */
+		if ((prop = node.property ("playlist")) == 0) {
+			return -1;
+		}
+
+		if( find_and_use_playlist_by_name (prop->value())) {
+			return -1;
+		}
 	}
 
 	if ((prop = node.property ("speed")) != 0) {
diff --git a/libs/ardour/midi_diskstream.cc b/libs/ardour/midi_diskstream.cc
index 485967b..207cc07 100644
--- a/libs/ardour/midi_diskstream.cc
+++ b/libs/ardour/midi_diskstream.cc
@@ -212,7 +212,7 @@ MidiDiskstream::non_realtime_input_change ()
 }
 
 int
-MidiDiskstream::find_and_use_playlist (const string& name)
+MidiDiskstream::find_and_use_playlist_by_name (const string& name)
 {
 	boost::shared_ptr<MidiPlaylist> playlist;
 
@@ -229,6 +229,21 @@ MidiDiskstream::find_and_use_playlist (const string& name)
 }
 
 int
+MidiDiskstream::find_and_use_playlist_by_id (const PBD::ID& p_id)
+{
+	boost::shared_ptr<MidiPlaylist> playlist;
+
+	playlist = boost::dynamic_pointer_cast<MidiPlaylist> (_session.playlists->by_id (p_id));
+
+	if (!playlist) {
+		error << string_compose(_("MidiDiskstream: Playlist \"%1\" isn't a midi playlist"), p_id.to_s()) << endmsg;
+		return -1;
+	}
+
+	return use_playlist (playlist);
+}
+
+int
 MidiDiskstream::use_playlist (boost::shared_ptr<Playlist> playlist)
 {
 	if (boost::dynamic_pointer_cast<MidiPlaylist>(playlist)) {
fix_6523.patch (4,962 bytes)

elgoun

2016-01-12 04:06

reporter   ~0017774

Last edited: 2016-01-12 04:44

View 2 revisions

I can reproduce this issue.

Here is a patch that makes ardour links diskstream to playlist by their ID instead of their name in session file.

It is now possible to have playlists with the same name in different tracks.

Ejis

2016-01-12 10:55

reporter   ~0017775

Great! Thank you! Hope this will end in the official build soon.

timbyr

2016-01-15 12:18

developer   ~0017791

The patch looks good and it seems to fix the issue after minimal testing.

x42

2016-01-15 13:45

administrator   ~0017793

Actually I think this patch is more of a stopgap than solution.

We have the same issue with all sources e.g embed two external audio-files with the same name from different folders.

mc888

2016-01-18 03:44

reporter   ~0017808

This is probably related - recently I did a save as template, and started a new session from template. Tracks in the new session had playlist information from the original session. Not good when you want to record a different song!

timbyr

2016-02-02 03:39

developer   ~0017864

x42: Why do you consider the patch a stopgap solution?

There may be a similar issue with external audio files but I don't think it is directly related to this issue.

timbyr

2016-09-19 09:59

developer   ~0018687

This is still an issue in 5.3. The patch still applies and should fix the issue.

Referring to playlists by ID rather than by name still seems like the correct approach to me and not a stopgap.

x42

2016-10-13 21:27

administrator   ~0018811

Indeed the patch looks fine.
I got confused by just reading the description. lol.

x42

2016-10-13 21:28

administrator   ~0018812

PS. still: Allowing duplicate names for playlists seems wrong. The name should be unique, and hence lookup by name would work.

timbyr

2016-10-14 01:08

developer   ~0018813

I'm not sure I agree that allowing duplicate playlist names is wrong, but if Ardour is going to enforce no duplicate track names then I guess it follows that should also not allow duplicate playlist names.

But either way this patch fixes a current issue and I think playlists should be looked up by ID rather than by name as the ID is constant where as the name can change (which might be a source of problems?)

x42

2016-10-14 01:24

administrator   ~0018814

Since the track-name is used as port-name and jack-ports must be unique, there can't be duplicate track-names.

Anyway, the issue is more practical: Playlists are displayed in the drop-down menu by name only. It's really confusing if there are 2 or more listed with the same name.

x42

2016-11-28 11:19

administrator   ~0019080

Since Ardour 5.4-470-ge9eea8d it is no longer possible to have two playlists with the same name.

Issue History

Date Modified Username Field Change
2015-08-19 18:08 Ejis New Issue
2016-01-12 03:56 elgoun File Added: fix_6523.patch
2016-01-12 04:06 elgoun Note Added: 0017774
2016-01-12 04:06 elgoun Status new => confirmed
2016-01-12 04:44 elgoun Note Edited: 0017774 View Revisions
2016-01-12 10:55 Ejis Note Added: 0017775
2016-01-15 12:18 timbyr Note Added: 0017791
2016-01-15 13:45 x42 Note Added: 0017793
2016-01-18 03:44 mc888 Note Added: 0017808
2016-02-02 03:39 timbyr Note Added: 0017864
2016-09-19 09:59 timbyr Note Added: 0018687
2016-09-19 09:59 timbyr Product Version => 5.3
2016-10-13 21:27 x42 Note Added: 0018811
2016-10-13 21:28 x42 Note Added: 0018812
2016-10-14 01:08 timbyr Note Added: 0018813
2016-10-14 01:24 x42 Note Added: 0018814
2016-11-28 11:17 x42 Relationship added duplicate of 0005681
2016-11-28 11:19 x42 Note Added: 0019080
2016-11-28 11:19 x42 Status confirmed => resolved
2016-11-28 11:19 x42 Resolution open => fixed
2016-11-28 11:19 x42 Assigned To => x42
2017-08-02 23:02 x42 Relationship added related to 0007438