[vlc-devel] [PATCH 1/9] playlist: assign unique id to playlist items
Romain Vimont
rom1v at videolabs.io
Mon Mar 4 14:26:40 CET 2019
On Mon, Mar 04, 2019 at 01:23:38PM +0200, Rémi Denis-Courmont wrote:
> Hi,
>
> While RPC or IPC interfaces do need a weak identifier for playlist items, I don't think that reproducing the old playlist item ID is a good idea.
>
> The past has shown that, if the playlist exposes those ID's, they will be used wrongly. That is to say, they will be used by local interfaces that really should not use them on the one hand.
> And they will be used with improper locking and/or reference counting.
The id can only be used to retrieve the actual index of the item, via
vlc_playlist_IndexOfId().
The implementation of this function starts with:
vlc_playlist_AssertLocked(playlist)
> That and the callback deadlocks are The Two Design Bugs in the legacy playlist. I don't want to see them again.
>
> As such I'd argue that the mapping should be done privately and most suitably by those interfaces that require it, rather than the playlist core.
TBH, I was not very happy adding an id to playlist items, but it is very
convenient to have "weak identifiers" attached to the item, to avoid
implementing a mapping for every RPC/IPC.
I think the API using the id is limited enough (only used to get the
actual item) to be used wrongly.
> Le 4 mars 2019 12:53:21 GMT+02:00, Romain Vimont <rom1v at videolabs.io> a écrit :
> >Playlist items are intended to be handled by index (with lock held) or
> >by instance (vlc_playlist_item_t *).
> >
> >Some modules, like http, require to reference playlist items
> >"remotely".
> >To simplify their implementation, assign an id, unique within a
> >playlist
> >instance, to every items.
> >---
> > include/vlc_playlist.h | 16 ++++++++++++++++
> > src/libvlccore.sym | 2 ++
> > src/playlist/content.c | 27 +++++++++++++++++++++------
> > src/playlist/item.c | 9 ++++++++-
> > src/playlist/item.h | 3 ++-
> > src/playlist/playlist.c | 1 +
> > src/playlist/playlist.h | 1 +
> > src/playlist/preparse.c | 9 ++++++---
> > src/playlist/test.c | 4 ++--
> > 9 files changed, 59 insertions(+), 13 deletions(-)
> >
> >diff --git a/include/vlc_playlist.h b/include/vlc_playlist.h
> >index 52edae9957..c282e32859 100644
> >--- a/include/vlc_playlist.h
> >+++ b/include/vlc_playlist.h
> >@@ -317,6 +317,12 @@ vlc_playlist_item_Release(vlc_playlist_item_t *);
> > VLC_API input_item_t *
> > vlc_playlist_item_GetMedia(vlc_playlist_item_t *);
> >
> >+/**
> >+ * Return a unique id for the playlist item instance.
> >+ */
> >+VLC_API uint64_t
> >+vlc_playlist_item_GetId(vlc_playlist_item_t *);
> >+
> > /* Playlist */
> >
> > /**
> >@@ -656,6 +662,16 @@ vlc_playlist_IndexOf(vlc_playlist_t *playlist,
> >const vlc_playlist_item_t *item);
> > VLC_API ssize_t
> >vlc_playlist_IndexOfMedia(vlc_playlist_t *playlist, const input_item_t
> >*media);
> >
> >+/**
> >+ * Return the index of a given item id.
> >+ *
> >+ * \param playlist the playlist, locked
> >+ * \param id the id to locate
> >+ * \return the index of the playlist item having the id (-1 if not
> >found)
> >+ */
> >+VLC_API ssize_t
> >+vlc_playlist_IndexOfId(vlc_playlist_t *playlist, uint64_t id);
> >+
> > /**
> > * Return the playback "repeat" mode.
> > *
> >diff --git a/src/libvlccore.sym b/src/libvlccore.sym
> >index ef9e2d5561..79c1aa1c72 100644
> >--- a/src/libvlccore.sym
> >+++ b/src/libvlccore.sym
> >@@ -928,6 +928,7 @@ vlc_player_vout_Snapshot
> > vlc_playlist_item_Hold
> > vlc_playlist_item_Release
> > vlc_playlist_item_GetMedia
> >+vlc_playlist_item_GetId
> > vlc_playlist_New
> > vlc_playlist_Delete
> > vlc_playlist_Lock
> >@@ -947,6 +948,7 @@ vlc_playlist_Shuffle
> > vlc_playlist_Sort
> > vlc_playlist_IndexOf
> > vlc_playlist_IndexOfMedia
> >+vlc_playlist_IndexOfId
> > vlc_playlist_GetPlaybackRepeat
> > vlc_playlist_GetPlaybackOrder
> > vlc_playlist_SetPlaybackRepeat
> >diff --git a/src/playlist/content.c b/src/playlist/content.c
> >index 43b3800d9f..5f307a9a79 100644
> >--- a/src/playlist/content.c
> >+++ b/src/playlist/content.c
> >@@ -205,6 +205,18 @@ vlc_playlist_IndexOfMedia(vlc_playlist_t
> >*playlist, const input_item_t *media)
> > return -1;
> > }
> >
> >+ssize_t
> >+vlc_playlist_IndexOfId(vlc_playlist_t *playlist, uint64_t id)
> >+{
> >+ vlc_playlist_AssertLocked(playlist);
> >+
> >+ playlist_item_vector_t *items = &playlist->items;
> >+ for (size_t i = 0; i < items->size; ++i)
> >+ if (items->data[i]->id == id)
> >+ return i;
> >+ return -1;
> >+}
> >+
> > void
> > vlc_playlist_Clear(vlc_playlist_t *playlist)
> > {
> >@@ -218,13 +230,15 @@ vlc_playlist_Clear(vlc_playlist_t *playlist)
> > }
> >
> > static int
> >-vlc_playlist_MediaToItems(input_item_t *const media[], size_t count,
> >- vlc_playlist_item_t *items[])
> >+vlc_playlist_MediaToItems(vlc_playlist_t *playlist, input_item_t
> >*const media[],
> >+ size_t count, vlc_playlist_item_t *items[])
> > {
> >+ vlc_playlist_AssertLocked(playlist);
> > size_t i;
> > for (i = 0; i < count; ++i)
> > {
> >- items[i] = vlc_playlist_item_New(media[i]);
> >+ uint64_t id = playlist->idgen++;
> >+ items[i] = vlc_playlist_item_New(media[i], id);
> > if (unlikely(!items[i]))
> > break;
> > }
> >@@ -250,7 +264,7 @@ vlc_playlist_Insert(vlc_playlist_t *playlist,
> >size_t index,
> > return VLC_ENOMEM;
> >
> > /* create playlist items in place */
> >- int ret = vlc_playlist_MediaToItems(media, count,
> >+ int ret = vlc_playlist_MediaToItems(playlist, media, count,
> > &playlist->items.data[index]);
> > if (ret != VLC_SUCCESS)
> > {
> >@@ -307,7 +321,8 @@ vlc_playlist_Replace(vlc_playlist_t *playlist,
> >size_t index,
> > vlc_playlist_AssertLocked(playlist);
> > assert(index < playlist->items.size);
> >
> >- vlc_playlist_item_t *item = vlc_playlist_item_New(media);
> >+ uint64_t id = playlist->idgen++;
> >+ vlc_playlist_item_t *item = vlc_playlist_item_New(media, id);
> > if (!item)
> > return VLC_ENOMEM;
> >
> >@@ -347,7 +362,7 @@ vlc_playlist_Expand(vlc_playlist_t *playlist,
> >size_t index,
> > return VLC_ENOMEM;
> >
> > /* create playlist items in place */
> >- ret = vlc_playlist_MediaToItems(&media[1], count - 1,
> >+ ret = vlc_playlist_MediaToItems(playlist, &media[1], count
> >- 1,
> > &playlist->items.data[index + 1]);
> > if (ret != VLC_SUCCESS)
> > {
> >diff --git a/src/playlist/item.c b/src/playlist/item.c
> >index 3cf06f9b3a..e4a1060e93 100644
> >--- a/src/playlist/item.c
> >+++ b/src/playlist/item.c
> >@@ -28,13 +28,14 @@
> > #include <vlc_input_item.h>
> >
> > vlc_playlist_item_t *
> >-vlc_playlist_item_New(input_item_t *media)
> >+vlc_playlist_item_New(input_item_t *media, uint64_t id)
> > {
> > vlc_playlist_item_t *item = malloc(sizeof(*item));
> > if (unlikely(!item))
> > return NULL;
> >
> > vlc_atomic_rc_init(&item->rc);
> >+ item->id = id;
> > item->media = media;
> > input_item_Hold(media);
> > return item;
> >@@ -61,3 +62,9 @@ vlc_playlist_item_GetMedia(vlc_playlist_item_t *item)
> > {
> > return item->media;
> > }
> >+
> >+uint64_t
> >+vlc_playlist_item_GetId(vlc_playlist_item_t *item)
> >+{
> >+ return item->id;
> >+}
> >diff --git a/src/playlist/item.h b/src/playlist/item.h
> >index f55cc83e84..1c4f03add7 100644
> >--- a/src/playlist/item.h
> >+++ b/src/playlist/item.h
> >@@ -29,11 +29,12 @@ typedef struct input_item_t input_item_t;
> > struct vlc_playlist_item
> > {
> > input_item_t *media;
> >+ uint64_t id;
> > vlc_atomic_rc_t rc;
> > };
> >
> >/* _New() is private, it is called when inserting new media in the
> >playlist */
> > vlc_playlist_item_t *
> >-vlc_playlist_item_New(input_item_t *media);
> >+vlc_playlist_item_New(input_item_t *media, uint64_t id);
> >
> > #endif
> >diff --git a/src/playlist/playlist.c b/src/playlist/playlist.c
> >index 618433c9e3..94e33c0e52 100644
> >--- a/src/playlist/playlist.c
> >+++ b/src/playlist/playlist.c
> >@@ -52,6 +52,7 @@ vlc_playlist_New(vlc_object_t *parent)
> > vlc_list_init(&playlist->listeners);
> > playlist->repeat = VLC_PLAYLIST_PLAYBACK_REPEAT_NONE;
> > playlist->order = VLC_PLAYLIST_PLAYBACK_ORDER_NORMAL;
> >+ playlist->idgen = 0;
> >
> > return playlist;
> > }
> >diff --git a/src/playlist/playlist.h b/src/playlist/playlist.h
> >index 1090d2146b..da1e082fae 100644
> >--- a/src/playlist/playlist.h
> >+++ b/src/playlist/playlist.h
> >@@ -57,6 +57,7 @@ struct vlc_playlist
> >struct vlc_list listeners; /**< list of vlc_playlist_listener_id.node
> >*/
> > enum vlc_playlist_playback_repeat repeat;
> > enum vlc_playlist_playback_order order;
> >+ uint64_t idgen;
> > };
> >
> > static inline void
> >diff --git a/src/playlist/preparse.c b/src/playlist/preparse.c
> >index c636757ff5..c82ce2b675 100644
> >--- a/src/playlist/preparse.c
> >+++ b/src/playlist/preparse.c
> >@@ -32,14 +32,17 @@
> > typedef struct VLC_VECTOR(input_item_t *) media_vector_t;
> >
> > static void
> >-vlc_playlist_CollectChildren(media_vector_t *dest, input_item_node_t
> >*node)
> >+vlc_playlist_CollectChildren(vlc_playlist_t *playlist,
> >+ media_vector_t *dest,
> >+ input_item_node_t *node)
> > {
> >+ vlc_playlist_AssertLocked(playlist);
> > for (int i = 0; i < node->i_children; ++i)
> > {
> > input_item_node_t *child = node->pp_children[i];
> > input_item_t *media = child->p_item;
> > vlc_vector_push(dest, media);
> >- vlc_playlist_CollectChildren(dest, child);
> >+ vlc_playlist_CollectChildren(playlist, dest, child);
> > }
> > }
> >
> >@@ -50,7 +53,7 @@ vlc_playlist_ExpandItem(vlc_playlist_t *playlist,
> >size_t index,
> > vlc_playlist_AssertLocked(playlist);
> >
> > media_vector_t flatten = VLC_VECTOR_INITIALIZER;
> >- vlc_playlist_CollectChildren(&flatten, node);
> >+ vlc_playlist_CollectChildren(playlist, &flatten, node);
> >
> >int ret = vlc_playlist_Expand(playlist, index, flatten.data,
> >flatten.size);
> > vlc_vector_destroy(&flatten);
> >diff --git a/src/playlist/test.c b/src/playlist/test.c
> >index df0eb6fe0e..dfdaeaf6ed 100644
> >--- a/src/playlist/test.c
> >+++ b/src/playlist/test.c
> >@@ -1502,7 +1502,7 @@ test_request_remove_adapt(void)
> > vlc_playlist_AddListener(playlist, &cbs, &ctx, false);
> > assert(listener);
> >
> >- vlc_playlist_item_t *dummy = vlc_playlist_item_New(media[10]);
> >+ vlc_playlist_item_t *dummy = vlc_playlist_item_New(media[10], 0);
> > assert(dummy);
> >
> >/* remove items in a wrong order at wrong position, as if the playlist
> >had
> >@@ -1700,7 +1700,7 @@ test_request_move_adapt(void)
> > vlc_playlist_AddListener(playlist, &cbs, &ctx, false);
> > assert(listener);
> >
> >- vlc_playlist_item_t *dummy = vlc_playlist_item_New(media[15]);
> >+ vlc_playlist_item_t *dummy = vlc_playlist_item_New(media[15], 0);
> > assert(dummy);
> >
> >/* move items in a wrong order at wrong position, as if the playlist
> >had
> >--
> >2.20.1
> >
> >_______________________________________________
> >vlc-devel mailing list
> >To unsubscribe or modify your subscription options:
> >https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list