Better thread safety by making sure that the raop globals are only accessed through the player thread

src/player.c

@@ -154,6 +154,7 @@ struct player_command
     void *noarg;
     struct spk_enum *spk_enum;
     struct raop_device *rd;
+    struct raop_metadata *rmd;
     struct player_status *status;
     struct player_source *ps;
     player_status_handler status_handler;
@@ -578,6 +579,9 @@ playback_abort(void);
 static int
 queue_clear(struct player_command *cmd);
 
+static void
+player_metadata_send(struct raop_metadata *rmd);
+
 static void
 player_laudio_status_cb(enum laudio_state status)
 {
@@ -626,7 +630,7 @@ player_laudio_status_cb(enum laudio_state status)
     }
 }
 
-/* Callbacks from the worker thread */
+/* Callback from the worker thread (async operation as it may block) */
 static void
 playcount_inc_cb(void *arg)
 {
@@ -635,14 +639,23 @@ playcount_inc_cb(void *arg)
   db_file_inc_playcount(*id);
 }
 
+/* Callback from the worker thread
+ * This prepares metadata in the worker thread, since especially the artwork
+ * retrieval may take some time. raop_metadata_prepare() is thread safe. The
+ * sending, however, must be done in the player thread.
+ */
 static void
-metadata_send_cb(void *arg)
+metadata_prepare_cb(void *arg)
 {
-  struct raop_metadata_arg *rma = arg;
+  struct raop_metadata *rmd;
 
-  raop_metadata_send(rma);
+  rmd = raop_metadata_prepare(arg);
+
+  if (rmd)
+    player_metadata_send(rmd);
 }
 
+/* Callback from the worker thread (async operation as it may block) */
 static void
 update_icy_cb(void *arg)
 {
@@ -667,7 +680,7 @@ metadata_purge(void)
 }
 
 static void
-metadata_send(struct player_source *ps, int startup)
+metadata_trigger(struct player_source *ps, int startup)
 {
   struct raop_metadata_arg rma;
 
@@ -695,11 +708,11 @@ metadata_send(struct player_source *ps, int startup)
   else
     {
       rma.rtptime = 0;
-      DPRINTF(E_LOG, L_PLAYER, "PTOH! Unhandled song boundary case in metadata_send()\n");
+      DPRINTF(E_LOG, L_PLAYER, "PTOH! Unhandled song boundary case in metadata_trigger()\n");
     }
 
-  /* Defer the actual work of sending the metadata to the worker thread */
+  /* Defer the actual work of preparing the metadata to the worker thread */
-  worker_execute(metadata_send_cb, &rma, sizeof(struct raop_metadata_arg), 0);
+  worker_execute(metadata_prepare_cb, &rma, sizeof(struct raop_metadata_arg), 0);
 }
 
 static void
@@ -732,7 +745,7 @@ metadata_icy_poll_cb(int fd, short what, void *arg)
   worker_execute(update_icy_cb, metadata, sizeof(struct http_icy_metadata), 0);
 
   status_update(player_state);
-  metadata_send(cur_streaming, 0);
+  metadata_trigger(cur_streaming, 0);
 
   /* Only free the struct, the content must be preserved for update_icy_cb */
   free(metadata);
@@ -1427,7 +1440,7 @@ source_open(struct player_source *ps, int no_md)
     }
 
   if (!no_md)
-    metadata_send(ps, (player_state == PLAY_PLAYING) ? 0 : 1);
+    metadata_trigger(ps, (player_state == PLAY_PLAYING) ? 0 : 1);
 
   ps->setup_done = 1;
 
@@ -1480,7 +1493,7 @@ source_next(int force)
 	     * so we have to handle metadata ourselves here
 	     */
 	    if (ret >= 0)
-	      metadata_send(cur_streaming, 0);
+	      metadata_trigger(cur_streaming, 0);
 	  }
 	else
 	  ret = source_open(cur_streaming, force);
@@ -2159,6 +2172,18 @@ device_remove_family(struct player_command *cmd)
   return 0;
 }
 
+static int
+metadata_send(struct player_command *cmd)
+{
+  struct raop_metadata *rmd;
+
+  rmd = cmd->arg.rmd;
+
+  raop_metadata_send(rmd);
+
+  return 0;
+}
+
 /* RAOP callbacks executed in the player thread */
 static void
 device_streaming_cb(struct raop_device *dev, struct raop_session *rs, enum raop_session_state status)
@@ -2862,7 +2887,7 @@ playback_start(struct player_command *cmd)
        * After a pause, the source is still open so source_open() doesn't get
        * called and we have to handle metadata ourselves.
        */
-      metadata_send(cur_streaming, 1);
+      metadata_trigger(cur_streaming, 1);
     }
 
   /* Start local audio if needed */
@@ -4869,6 +4894,39 @@ player_device_remove(struct raop_device *rd)
     }
 }
 
+/* Thread: worker */
+static void
+player_metadata_send(struct raop_metadata *rmd)
+{
+  struct player_command *cmd;
+  int ret;
+
+  cmd = (struct player_command *)malloc(sizeof(struct player_command));
+  if (!cmd)
+    {
+      DPRINTF(E_LOG, L_PLAYER, "Could not allocate player_command\n");
+
+      raop_metadata_free(rmd);
+      return;
+    }
+
+  memset(cmd, 0, sizeof(struct player_command));
+
+  cmd->nonblock = 1;
+
+  cmd->func = metadata_send;
+  cmd->arg.rmd = rmd;
+
+  ret = nonblock_command(cmd);
+  if (ret < 0)
+    {
+      free(cmd);
+      raop_metadata_free(rmd);
+
+      return;
+    }
+}
+
 
 /* RAOP devices discovery - mDNS callback */
 /* Thread: main (mdns) */

src/raop.c

@@ -154,6 +154,9 @@ struct raop_metadata
   uint64_t start;
   uint64_t end;
 
+  uint64_t offset;
+  int startup;
+
   struct raop_metadata *next;
 };
 
@@ -708,7 +711,7 @@ raop_crypt_encrypt_aes_key_base64(void)
 
 
 /* RAOP metadata */
-static void
+void
 raop_metadata_free(struct raop_metadata *rmd)
 {
   evbuffer_free(rmd->metadata);
@@ -751,8 +754,9 @@ raop_metadata_prune(uint64_t rtptime)
     }
 }
 
-static struct raop_metadata *
+/* Thread: worker */
-raop_metadata_prepare(int id, uint64_t rtptime)
+struct raop_metadata *
+raop_metadata_prepare(struct raop_metadata_arg *rma)
 {
   struct query_params qp;
   struct db_media_file_info dbmfi;
@@ -781,10 +785,10 @@ raop_metadata_prepare(int id, uint64_t rtptime)
       goto skip_artwork;
     }
 
-  ret = artwork_get_item(id, 600, 600, rmd->artwork);
+  ret = artwork_get_item(rma->id, 600, 600, rmd->artwork);
   if (ret < 0)
     {
-      DPRINTF(E_INFO, L_RAOP, "Failed to retrieve artwork for file id %d; no artwork will be sent\n", id);
+      DPRINTF(E_INFO, L_RAOP, "Failed to retrieve artwork for file id %d; no artwork will be sent\n", rma->id);
 
       evbuffer_free(rmd->artwork);
       rmd->artwork = NULL;
@@ -801,10 +805,10 @@ raop_metadata_prepare(int id, uint64_t rtptime)
   qp.sort = S_NONE;
   qp.filter = filter;
 
-  ret = snprintf(filter, sizeof(filter), "id = %d", id);
+  ret = snprintf(filter, sizeof(filter), "id = %d", rma->id);
   if ((ret < 0) || (ret >= sizeof(filter)))
     {
-      DPRINTF(E_LOG, L_RAOP, "Could not build filter for file id %d; metadata will not be sent\n", id);
+      DPRINTF(E_LOG, L_RAOP, "Could not build filter for file id %d; metadata will not be sent\n", rma->id);
 
       goto out_rmd;
     }
@@ -820,7 +824,7 @@ raop_metadata_prepare(int id, uint64_t rtptime)
   ret = db_query_fetch_file(&qp, &dbmfi);
   if (ret < 0)
     {
-      DPRINTF(E_LOG, L_RAOP, "Couldn't fetch file id %d; metadata will not be sent\n", id);
+      DPRINTF(E_LOG, L_RAOP, "Couldn't fetch file id %d; metadata will not be sent\n", rma->id);
 
       goto out_query;
     }
@@ -863,17 +867,11 @@ raop_metadata_prepare(int id, uint64_t rtptime)
 
   db_query_end(&qp);
 
-  rmd->start = rtptime;
+  rmd->start = rma->rtptime;
-  rmd->end = rtptime + (duration * 44100UL) / 1000UL;
+  rmd->end = rma->rtptime + (duration * 44100UL) / 1000UL;
 
-  /* Add rmd to metadata list */
+  rmd->startup = rma->startup;
-  if (metadata_tail)
+  rmd->offset = rma->offset;
-    metadata_tail->next = rmd;
-  else
-    {
-      metadata_head = rmd;
-      metadata_tail = rmd;
-    }
 
   return rmd;
 
@@ -2164,16 +2162,20 @@ raop_metadata_startup_send(struct raop_session *rs)
 }
 
 void
-raop_metadata_send(struct raop_metadata_arg *rma)
+raop_metadata_send(struct raop_metadata *rmd)
 {
   struct raop_session *rs;
-  struct raop_metadata *rmd;
   uint32_t delay;
   int ret;
 
-  rmd = raop_metadata_prepare(rma->id, rma->rtptime);
+  /* Add the rmd to the metadata list */
-  if (!rmd)
+  if (metadata_tail)
-    return;
+    metadata_tail->next = rmd;
+  else
+    {
+      metadata_head = rmd;
+      metadata_tail = rmd;
+    }
 
   for (rs = sessions; rs; rs = rs->next)
     {
@@ -2183,9 +2185,9 @@ raop_metadata_send(struct raop_metadata_arg *rma)
       if (!rs->wants_metadata)
 	continue;
 
-      delay = (rma->startup) ? RAOP_MD_DELAY_STARTUP : RAOP_MD_DELAY_SWITCH;
+      delay = (rmd->startup) ? RAOP_MD_DELAY_STARTUP : RAOP_MD_DELAY_SWITCH;
 
-      ret = raop_metadata_send_internal(rs, rmd, rma->offset, delay);
+      ret = raop_metadata_send_internal(rs, rmd, rmd->offset, delay);
       if (ret < 0)
 	{
 	  raop_session_failure(rs);

src/raop.h

@@ -26,6 +26,7 @@ enum raop_devtype {
 };
 
 struct raop_session;
+struct raop_metadata;
 
 struct raop_device
 {
@@ -98,6 +99,8 @@ enum raop_session_state
 
 typedef void (*raop_status_cb)(struct raop_device *dev, struct raop_session *rs, enum raop_session_state status);
 
+void
+raop_metadata_free(struct raop_metadata *rmd);
 
 void
 raop_metadata_purge(void);
@@ -105,6 +108,11 @@ raop_metadata_purge(void);
 void
 raop_metadata_prune(uint64_t rtptime);
 
+struct raop_metadata *
+raop_metadata_prepare(struct raop_metadata_arg *rma);
+
+void
+raop_metadata_send(struct raop_metadata *rmd);
 
 int
 raop_device_probe(struct raop_device *rd, raop_status_cb cb);
@@ -121,10 +129,6 @@ raop_playback_start(uint64_t next_pkt, struct timespec *ts);
 void
 raop_playback_stop(void);
 
-
-void
-raop_metadata_send(struct raop_metadata_arg *rma);
-
 int
 raop_set_volume_one(struct raop_session *rs, int volume, raop_status_cb cb);