[player] Use input progress metadata to update clients

Before, we were ignoring progress metadata, and we were also updating clients
and db too early with input metadata (right when read from the input, instead of
waiting until playback (speakers) were at that position.

This change adds a complicated async chain of events from when the update is
received.

src/input.c

@@ -177,6 +177,9 @@ map_data_kind(int data_kind)
 static void
 metadata_free(struct input_metadata *metadata, int content_only)
 {
+  if (!metadata)
+    return;
+
   free(metadata->artist);
   free(metadata->title);
   free(metadata->album);
@@ -193,7 +196,6 @@ static struct input_metadata *
 metadata_get(struct input_source *source)
 {
   struct input_metadata *metadata;
-  struct db_queue_item *queue_item;
   int ret;
 
   if (!inputs[source->type]->metadata_get)
@@ -205,37 +207,7 @@ metadata_get(struct input_source *source)
   if (ret < 0)
     goto out_free_metadata;
 
-  queue_item = db_queue_fetch_byitemid(source->item_id);
+  metadata->item_id = source->item_id;
-  if (!queue_item)
-    {
-      DPRINTF(E_LOG, L_PLAYER, "Bug! Input source item_id does not match anything in queue\n");
-      goto out_free_metadata;
-    }
-
-  // Update queue item if metadata changed
-  if (metadata->artist || metadata->title || metadata->album || metadata->genre || metadata->artwork_url || metadata->len_ms)
-    {
-      // Since we won't be using the metadata struct values for anything else
-      // than this we just swap pointers
-      if (metadata->artist)
-	swap_pointers(&queue_item->artist, &metadata->artist);
-      if (metadata->title)
-	swap_pointers(&queue_item->title, &metadata->title);
-      if (metadata->album)
-	swap_pointers(&queue_item->album, &metadata->album);
-      if (metadata->genre)
-	swap_pointers(&queue_item->genre, &metadata->genre);
-      if (metadata->artwork_url)
-	swap_pointers(&queue_item->artwork_url, &metadata->artwork_url);
-      if (metadata->len_ms)
-	queue_item->song_length = metadata->len_ms;
-
-      ret = db_queue_update_item(queue_item);
-      if (ret < 0)
-	DPRINTF(E_LOG, L_PLAYER, "Database error while updating queue with new metadata\n");
-    }
-
-  free_queue_item(queue_item, 0);
 
   return metadata;
 
@@ -867,7 +839,6 @@ input_flush(short *flags)
   flush(flags);
 }
 
-// Not currently used, perhaps remove?
 void
 input_metadata_free(struct input_metadata *metadata, int content_only)
 {

src/input.h

@@ -74,9 +74,12 @@ struct input_metadata
   // queue_item id
   uint32_t item_id;
 
-  // Input can override the default player progress by setting this
+  // Input can override the default player progress by setting this. For the
-  // FIXME only implemented for Airplay speakers currently
+  // other fields the receiver can check whether an update happened by checking
-  uint32_t pos_ms;
+  // if it is non-zero/null, but not for pos_ms since 0 and even negative values
+  // are valid.
+  bool pos_is_updated;
+  int32_t pos_ms;
 
   // Sets new song length (input will also update queue_item)
   uint32_t len_ms;
@@ -87,9 +90,6 @@ struct input_metadata
   char *album;
   char *genre;
   char *artwork_url;
-
-  // Indicates whether we are starting playback. Just passed on to output.
-  int startup;
 };
 
 struct input_definition
@@ -219,7 +219,7 @@ void
 input_flush(short *flags);
 
 /*
- * Free the entire struct
+ * Free input_metadata
  */
 void
 input_metadata_free(struct input_metadata *metadata, int content_only);

src/player.c

@@ -175,9 +175,14 @@ struct player_source
   // Item-Id of the file/item in the queue
   uint32_t item_id;
 
-  // Length of the file/item in milliseconds
+  // Length of the file/item in milliseconds, 0 for endless (unless the input
+  // has given us a track length)
   uint32_t len_ms;
 
+  // Set when opening the item based on initial track length (so is not changed
+  // by later input track length metadata)
+  bool is_seekable;
+
   // Quality of the source (sample rate etc.)
   struct media_quality quality;
 
@@ -199,8 +204,14 @@ struct player_source
   uint64_t play_start;
   uint64_t play_end;
 
+  // When we receive a metadata update from the input it shouldn't be pushed to
+  // clients until the speakers have reached the position that matches the
+  // position the player was reading at when it got INPUT_FLAG_METADATA.
+  uint64_t metadata_update;
+
   // The number of milliseconds into the media that we started
   uint32_t seek_ms;
+
   // This should at any time match the millisecond position of the media that is
   // coming out of your device. Will be 0 during initial buffering.
   uint32_t pos_ms;
@@ -308,6 +319,14 @@ static uint32_t cur_plversion;
 // Play history
 static struct player_history *history;
 
+// When we receive track metadata from the input we have to wait until playback
+// has reached the position before using it. We use this to record the update.
+struct metadata_pending_register
+{
+  uint64_t pos;
+  struct input_metadata *metadata;
+} metadata_pending[16];
+
 
 /* -------------------------------- Forwards -------------------------------- */
 
@@ -348,31 +367,6 @@ scrobble_cb(void *arg)
 }
 #endif
 
-static int
-metadata_finalize_cb(struct output_metadata *metadata)
-{
-  if (!pb_session.playing_now)
-    {
-      DPRINTF(E_WARN, L_PLAYER, "Aborting metadata_send(), playback stopped during metadata preparation\n");
-      return -1;
-    }
-  else if (metadata->item_id != pb_session.playing_now->item_id)
-    {
-      DPRINTF(E_WARN, L_PLAYER, "Aborting metadata_send(), item_id changed during metadata preparation (%" PRIu32 " -> %" PRIu32 ")\n",
-	metadata->item_id, pb_session.playing_now->item_id);
-      return -1;
-    }
-
-  if (!metadata->pos_ms)
-    metadata->pos_ms = pb_session.playing_now->pos_ms;
-  if (!metadata->len_ms)
-    metadata->len_ms = pb_session.playing_now->len_ms;
-  if (!metadata->pts.tv_sec)
-    metadata->pts = pb_session.pts;
-
-  return 0;
-}
-
 /*
  * Add the song with the given id to the list of previously played songs
  */
@@ -470,6 +464,132 @@ status_update(enum play_status status)
   listener_notify(LISTENER_PLAYER);
 }
 
+/* ------ All this is for dealing with metadata received from the input ----- */
+
+static int
+metadata_pending_add(struct input_metadata *metadata, uint64_t pos)
+{
+  int i;
+
+  if (pos == 0)
+    return -1; // Invalid position
+
+  for (i = 0; i < ARRAY_SIZE(metadata_pending); i++)
+    {
+      if (metadata_pending[i].metadata == NULL)
+	break;
+    }
+
+  if (i == ARRAY_SIZE(metadata_pending))
+    {
+      DPRINTF(E_LOG, L_PLAYER, "Error, too many pending metadata updates\n");
+      return -1;
+    }
+
+  metadata_pending[i].pos = pos;
+  metadata_pending[i].metadata = metadata;
+  return 0;
+}
+
+static uint64_t
+metadata_pending_next_pos(void)
+{
+  uint64_t next_pos;
+  int i;
+
+  for (i = 0, next_pos = 0; i < ARRAY_SIZE(metadata_pending); i++)
+    {
+      if (metadata_pending[i].metadata && (metadata_pending[i].pos < next_pos || !next_pos))
+	next_pos = metadata_pending[i].pos;
+    }
+
+  return next_pos;
+}
+
+static int
+metadata_finalize_cb(struct output_metadata *metadata)
+{
+  if (!pb_session.playing_now)
+    {
+      DPRINTF(E_WARN, L_PLAYER, "Aborting metadata_send(), playback stopped during metadata preparation\n");
+      return -1;
+    }
+  else if (metadata->item_id != pb_session.playing_now->item_id)
+    {
+      DPRINTF(E_WARN, L_PLAYER, "Aborting metadata_send(), item_id changed during metadata preparation (%" PRIu32 " -> %" PRIu32 ")\n",
+	metadata->item_id, pb_session.playing_now->item_id);
+      return -1;
+    }
+
+  if (!metadata->pos_ms)
+    metadata->pos_ms = pb_session.playing_now->pos_ms;
+  if (!metadata->len_ms)
+    metadata->len_ms = pb_session.playing_now->len_ms;
+  if (!metadata->pts.tv_sec)
+    metadata->pts = pb_session.pts;
+
+  return 0;
+}
+
+static enum command_state
+metadata_finalize(void *arg, int *retval)
+{
+  if (!pb_session.playing_now)
+    return COMMAND_END; // Playback ended while we doing the metadata update
+
+  outputs_metadata_send(pb_session.playing_now->item_id, false, metadata_finalize_cb);
+
+  status_update(player_state);
+
+  return COMMAND_END;
+}
+
+// Done in worker thread because we avoid blocking db updates in the player
+static void
+metadata_update_queue_cb(void *arg)
+{
+  struct input_metadata *metadata = *(struct input_metadata **)arg;
+  struct db_queue_item *queue_item;
+  int ret;
+
+  queue_item = db_queue_fetch_byitemid(metadata->item_id);
+  if (!queue_item)
+    {
+      DPRINTF(E_LOG, L_PLAYER, "Bug! Could not update queue metadata, the item_id is unknown (%u)\n", metadata->item_id);
+      input_metadata_free(metadata, 0);
+      return;
+    }
+
+  // Update queue item if metadata changed
+  if (metadata->artist || metadata->title || metadata->album || metadata->genre || metadata->artwork_url || metadata->len_ms)
+    {
+      // Since we won't be using the metadata struct values for anything else
+      // than this we just swap pointers
+      if (metadata->artist)
+	swap_pointers(&queue_item->artist, &metadata->artist);
+      if (metadata->title)
+	swap_pointers(&queue_item->title, &metadata->title);
+      if (metadata->album)
+	swap_pointers(&queue_item->album, &metadata->album);
+      if (metadata->genre)
+	swap_pointers(&queue_item->genre, &metadata->genre);
+      if (metadata->artwork_url)
+	swap_pointers(&queue_item->artwork_url, &metadata->artwork_url);
+      if (metadata->len_ms)
+	queue_item->song_length = metadata->len_ms;
+
+      ret = db_queue_update_item(queue_item);
+      if (ret < 0)
+	DPRINTF(E_LOG, L_PLAYER, "Database error while updating queue with new metadata\n");
+    }
+
+  free_queue_item(queue_item, 0);
+  input_metadata_free(metadata, 0);
+
+  // Now return to the player thread and run metadata_finalize
+  commands_exec_async(cmdbase, metadata_finalize, NULL);
+}
+
 
 /* ----------- Audio source handling (interfaces with input module) --------- */
 
@@ -500,6 +620,7 @@ source_create(struct db_queue_item *queue_item, uint32_t seek_ms)
   ps->data_kind = queue_item->data_kind;
   ps->media_kind = queue_item->media_kind;
   ps->len_ms = queue_item->song_length;
+  ps->is_seekable = (queue_item->song_length > 0);
   ps->path = strdup(queue_item->path);
   ps->seek_ms = seek_ms;
 
@@ -595,8 +716,9 @@ source_print(char *line, size_t linesize, struct player_source *ps, const char *
       pos += snprintf(line + pos, linesize - pos, "%s.play_start=%" PRIu64 "; ", name, ps->play_start);
       pos += snprintf(line + pos, linesize - pos, "%s.read_end=%" PRIu64 "; ", name, ps->read_end);
       pos += snprintf(line + pos, linesize - pos, "%s.play_end=%" PRIu64 "; ", name, ps->play_end);
-      pos += snprintf(line + pos, linesize - pos, "%s.pos_ms=%d; ", name, ps->pos_ms);
+      pos += snprintf(line + pos, linesize - pos, "%s.metadata_update=%" PRIu64 "; ", name, ps->metadata_update);
-      pos += snprintf(line + pos, linesize - pos, "%s.seek_ms=%d; ", name, ps->seek_ms);
+      pos += snprintf(line + pos, linesize - pos, "%s.pos_ms=%u; ", name, ps->pos_ms);
+      pos += snprintf(line + pos, linesize - pos, "%s.seek_ms=%u; ", name, ps->seek_ms);
     }
   else
     pos += snprintf(line + pos, linesize - pos, "%s=(null); ", name);
@@ -712,6 +834,8 @@ session_update_read_start(uint32_t seek_ms)
 static inline void
 session_update_read(int nsamples)
 {
+  uint32_t step_ms;
+
   // Did we just complete our first read? Then set the start timestamp
   if (pb_session.start_ts.tv_sec == 0)
     {
@@ -722,9 +846,15 @@ session_update_read(int nsamples)
   // Advance position
   pb_session.pos += nsamples;
 
+  // Need to know sample rate to calculate pos_ms step
+  if (!pb_session.playing_now->quality.sample_rate)
+    return;
+
+  step_ms = (1000 * nsamples) / pb_session.playing_now->quality.sample_rate;
+
   // After we have started playing we also must calculate new pos_ms
-  if (pb_session.playing_now->quality.sample_rate && pb_session.pos > pb_session.playing_now->play_start)
+  if (pb_session.pos > pb_session.playing_now->play_start)
-    pb_session.playing_now->pos_ms = pb_session.playing_now->seek_ms + 1000UL * (pb_session.pos - pb_session.playing_now->play_start) / pb_session.playing_now->quality.sample_rate;
+    pb_session.playing_now->pos_ms += step_ms;
 }
 
 static void
@@ -764,6 +894,25 @@ session_update_read_quality(struct media_quality *quality)
   free(quality);
 }
 
+static void
+session_update_read_metadata(void)
+{
+  if (!pb_session.reading_now)
+    return;
+
+  // Sets when to trigger the next event_play_metadata()
+  pb_session.reading_now->metadata_update = metadata_pending_next_pos();
+}
+
+static void
+session_update_play_metadata(struct input_metadata *metadata)
+{
+  if (metadata->pos_is_updated)
+    pb_session.playing_now->pos_ms = metadata->pos_ms;
+  if (metadata->len_ms)
+    pb_session.playing_now->len_ms = metadata->len_ms;
+}
+
 static void
 session_restart(void)
 {
@@ -850,16 +999,31 @@ event_read_start_next()
 static void
 event_read_metadata(struct input_metadata *metadata)
 {
+  uint64_t delay;
+  int ret;
+
   DPRINTF(E_DBG, L_PLAYER, "event_read_metadata()\n");
 
-  // FIXME Right now, metadata->pos_ms is not honoured. If it is >0 it is sent
+  // Add the metadata to the register of pending events with a trigger position
-  // to the outputs, but the player's pos_ms is not adjusted. That means we
+  // that corresponds to OUTPUTS_BUFFER_DURATION into the future. If we have
-  // don't always show correct progress for http streams, pipes and files with
+  // received a negative position we assume the metadata needs to be delayed
-  // chapters.
+  // until the position is 0.
+  if (metadata->pos_is_updated && metadata->pos_ms < 0)
+    {
+      delay = pb_session.reading_now->output_buffer_samples + (-metadata->pos_ms) * (uint64_t)pb_session.reading_now->quality.sample_rate / 1000;
+      metadata->pos_ms = 0;
+    }
+  else
+    delay = pb_session.reading_now->output_buffer_samples;
 
-  outputs_metadata_send(pb_session.playing_now->item_id, false, metadata_finalize_cb);
+  ret = metadata_pending_add(metadata, pb_session.pos + delay);
+  if (ret < 0)
+    {
+      input_metadata_free(metadata, 0);
+      return;
+    }
 
-  status_update(player_state);
+  session_update_read_metadata();
 }
 
 static void
@@ -912,6 +1076,38 @@ event_play_start()
   status_update(PLAY_PLAYING);
 }
 
+static void
+event_play_metadata()
+{
+  int i;
+
+  DPRINTF(E_DBG, L_PLAYER, "event_play_metadata()\n");
+
+  for (i = 0; i < ARRAY_SIZE(metadata_pending); i++)
+    {
+      // Proces all events with position from metadata_update (included) to
+      // current read position (excluded)
+      if (!(metadata_pending[i].pos >= pb_session.playing_now->metadata_update && metadata_pending[i].pos < pb_session.pos))
+	continue;
+
+      // Just in case
+      if (!metadata_pending[i].metadata)
+	continue;
+
+      session_update_play_metadata(metadata_pending[i].metadata);
+
+      // Triggers an async chain of metadata update, first worker will do an
+      // update of the db, then the player will update outputs, where the worker
+      // may be called by the output, and then player sends status_update
+      worker_execute(metadata_update_queue_cb, &(metadata_pending[i].metadata), sizeof(metadata_pending[i].metadata), 0);
+
+      memset(&metadata_pending[i], 0, sizeof(struct metadata_pending_register));
+    }
+
+  // Set trigger (playing_now->metadata_update) to next pending metadata
+  session_update_read_metadata();
+}
+
 // Checks if the new playback position requires change of play status, plus
 // calls session_update_read that updates playback position
 static inline void
@@ -935,8 +1131,16 @@ event_read(int nsamples)
   session_update_read(nsamples);
 
   // Check if the playback position passed the play_start position
-  if (pb_session.pos - nsamples < pb_session.playing_now->play_start && pb_session.pos >= pb_session.playing_now->play_start)
+  if (pb_session.pos > pb_session.playing_now->play_start && pb_session.pos <= pb_session.playing_now->play_start + nsamples)
     event_play_start();
+
+  if (pb_session.playing_now->metadata_update == 0)
+    return;
+
+  // Check if the playback position passed an input metadata update. The event
+  // must process all metadata updates in the read interval.
+  if (pb_session.pos > pb_session.playing_now->metadata_update && pb_session.pos <= pb_session.playing_now->metadata_update + nsamples)
+    event_play_metadata();
 }
 
 
@@ -2178,7 +2382,7 @@ static enum command_state
 playback_seek(void *arg, int *retval)
 {
   // Only check if the current playing track is seekable, other checks will be done in playback_pause()
-  if (pb_session.playing_now && pb_session.playing_now->len_ms <= 0)
+  if (pb_session.playing_now && !pb_session.playing_now->is_seekable)
     {
       DPRINTF(E_WARN, L_PLAYER, "Failed to seek, track is not seekable\n");