[vlc-devel] [PATCH v3 3/4] audioscrobbler: implement Now Playing feature
Hugo Beauzée-Luyssen
hugo at beauzee.fr
Thu Nov 19 11:08:09 CET 2015
Hi, sorry about the delay...
On 11/09/2015 04:39 PM, Jonas Lundqvist wrote:
> Submission of the currently playing song will show up on last.fm as
> "Now Playing".
> ---
> modules/misc/audioscrobbler.c | 150 ++++++++++++++++++++++++++++--------------
> 1 file changed, 102 insertions(+), 48 deletions(-)
>
> diff --git a/modules/misc/audioscrobbler.c b/modules/misc/audioscrobbler.c
> index 37228f3..fc08687 100644
> --- a/modules/misc/audioscrobbler.c
> +++ b/modules/misc/audioscrobbler.c
> @@ -25,8 +25,7 @@
> /* Last.fm Submissions protocol version: 1.2
> * http://www.last.fm/api/submissions
> *
> - * TODO: "Now Playing" feature (not mandatory)
> - * Update to new API? http://www.last.fm/api/scrobbling
> + * TODO: Update to new API? http://www.last.fm/api/scrobbling
> */
> /*****************************************************************************
> * Preamble
> @@ -87,6 +86,9 @@ struct intf_sys_t
> /* submission of played songs */
> vlc_url_t p_submit_url; /**< where to submit data */
>
> + /* submission of playing song */
> + vlc_url_t p_nowp_url; /**< where to submit data */
> +
> char psz_auth_token[33]; /**< Authentication token */
>
> /* data about song currently playing */
> @@ -95,7 +97,7 @@ struct intf_sys_t
> mtime_t time_pause; /**< time when vlc paused */
> mtime_t time_total_pauses; /**< total time in pause */
>
> - bool b_submit; /**< do we have to submit ? */
> + bool b_submit_nowp; /**< do we have to submit ? */
>
> bool b_meta_read; /**< if we read the song's
> * metadata already */
> @@ -191,7 +193,7 @@ static void ReadMetaData(intf_thread_t *p_this, input_thread_t *p_input)
> }
>
> /* Now we have read the mandatory meta data, so we can submit that info */
> - p_sys->b_submit = true;
> + p_sys->b_submit_nowp = true;
Nitpicking: I'd set the flag right before the cond_signal
>
> ALLOC_ITEM_META(p_sys->p_current_song.psz_b, Album);
> if (!p_sys->p_current_song.psz_b)
> @@ -210,6 +212,8 @@ static void ReadMetaData(intf_thread_t *p_this, input_thread_t *p_input)
>
> msg_Dbg(p_this, "Meta data registered");
>
> + vlc_cond_signal(&p_sys->wait);
> +
> end:
> vlc_mutex_unlock(&p_sys->lock);
> }
> @@ -223,7 +227,9 @@ static void AddToQueue (intf_thread_t *p_this)
> intf_sys_t *p_sys = p_this->p_sys;
>
> vlc_mutex_lock(&p_sys->lock);
> - if (!p_sys->b_submit)
> +
> + /* Check that we have the mandatory meta data */
> + if (!p_sys->p_current_song.psz_t || !p_sys->p_current_song.psz_a)
> goto end;
>
> /* wait for the user to listen enough before submitting */
> @@ -291,7 +297,6 @@ static void AddToQueue (intf_thread_t *p_this)
>
> end:
> DeleteSong(&p_sys->p_current_song);
> - p_sys->b_submit = false;
> vlc_mutex_unlock(&p_sys->lock);
> }
>
> @@ -354,7 +359,6 @@ static int ItemChange(vlc_object_t *p_this, const char *psz_var,
> VLC_UNUSED(oldval);
>
> p_sys->b_meta_read = false;
> - p_sys->b_submit = false;
>
> if (p_sys->p_input != NULL)
> {
> @@ -403,6 +407,7 @@ static int Open(vlc_object_t *p_this)
> return VLC_ENOMEM;
>
> p_intf->p_sys = p_sys;
> + p_sys->b_submit_nowp = false;
Since the struct is calloc'ed, this is not required
>
> vlc_mutex_init(&p_sys->lock);
> vlc_cond_init(&p_sys->wait);
> @@ -443,6 +448,7 @@ static void Close(vlc_object_t *p_this)
> for (i = 0; i < p_sys->i_songs; i++)
> DeleteSong(&p_sys->p_queue[i]);
> vlc_UrlClean(&p_sys->p_submit_url);
> + vlc_UrlClean(&p_sys->p_nowp_url);
> vlc_cond_destroy(&p_sys->wait);
> vlc_mutex_destroy(&p_sys->lock);
> free(p_sys);
> @@ -606,6 +612,13 @@ static int Handshake(intf_thread_t *p_this)
>
> /* We need to read the nowplaying url */
> p_buffer_pos += 7; /* we skip "http://" */
> + psz_url = strndup(p_buffer_pos, strcspn(p_buffer_pos, "\n"));
> + if (!psz_url)
> + goto oom;
> +
> + vlc_UrlParse(&p_sys->p_nowp_url, psz_url);
> + free(psz_url);
> +
> p_buffer_pos = strstr(p_buffer_pos, "http://");
> if (!p_buffer_pos || strlen(p_buffer_pos) == 7)
> goto proto;
> @@ -656,6 +669,7 @@ static void *Run(void *data)
> uint8_t p_buffer[1024];
> int canc = vlc_savecancel();
> bool b_handshaked = false;
> + bool b_nowp_submission_ongoing = false;
>
> /* data about audioscrobbler session */
> mtime_t next_exchange = 0; /**< when can we send data */
> @@ -672,7 +686,7 @@ static void *Run(void *data)
> vlc_mutex_lock(&p_sys->lock);
> mutex_cleanup_push(&p_sys->lock);
>
> - while (p_sys->i_songs == 0)
> + while (p_sys->i_songs == 0 && p_sys->b_submit_nowp == false)
> vlc_cond_wait(&p_sys->wait, &p_sys->lock);
>
> vlc_cleanup_pop();
> @@ -722,56 +736,87 @@ static void *Run(void *data)
>
> msg_Dbg(p_intf, "Going to submit some data...");
> char *psz_submit;
> + vlc_url_t *url;
> + char *psz_submit_song, *psz_submit_tmp;
> +
> if (asprintf(&psz_submit, "s=%s", p_sys->psz_auth_token) == -1)
> break;
>
> /* forge the HTTP POST request */
> vlc_mutex_lock(&p_sys->lock);
> - audioscrobbler_song_t *p_song;
> - for (int i_song = 0 ; i_song < p_sys->i_songs ; i_song++)
> +
> + if (p_sys->b_submit_nowp)
> {
> - char *psz_submit_song, *psz_submit_tmp;
> - p_song = &p_sys->p_queue[i_song];
> + b_nowp_submission_ongoing = true;
> + url = &p_sys->p_nowp_url;
> if (asprintf(&psz_submit_song,
> - "&a%%5B%d%%5D=%s"
> - "&t%%5B%d%%5D=%s"
> - "&i%%5B%d%%5D=%u"
> - "&o%%5B%d%%5D=P"
> - "&r%%5B%d%%5D="
> - "&l%%5B%d%%5D=%d"
> - "&b%%5B%d%%5D=%s"
> - "&n%%5B%d%%5D=%s"
> - "&m%%5B%d%%5D=%s",
> - i_song, p_song->psz_a,
> - i_song, p_song->psz_t,
> - i_song, (unsigned)p_song->date, /* HACK: %ju (uintmax_t) unsupported on Windows */
> - i_song,
> - i_song,
> - i_song, p_song->i_l,
> - i_song, p_song->psz_b,
> - i_song, p_song->psz_n,
> - i_song, p_song->psz_m
> - ) == -1)
> + "&a=%s"
> + "&t=%s"
> + "&b=%s"
> + "&l=%d"
> + "&n=%s"
> + "&m=%s",
> + p_sys->p_current_song.psz_a,
> + p_sys->p_current_song.psz_t,
> + p_sys->p_current_song.psz_b,
> + p_sys->p_current_song.i_l,
> + p_sys->p_current_song.psz_n,
> + p_sys->p_current_song.psz_m
> + ) == -1)
> { /* Out of memory */
> vlc_mutex_unlock(&p_sys->lock);
> goto out;
> }
> - psz_submit_tmp = psz_submit;
> - if (asprintf(&psz_submit, "%s%s",
> - psz_submit_tmp, psz_submit_song) == -1)
> - { /* Out of memory */
> - free(psz_submit_tmp);
> - free(psz_submit_song);
> - vlc_mutex_unlock(&p_sys->lock);
> - goto out;
> +
> + }
> + else
> + {
> + url = &p_sys->p_submit_url;
> + audioscrobbler_song_t *p_song;
> + for (int i_song = 0 ; i_song < p_sys->i_songs ; i_song++)
> + {
> + p_song = &p_sys->p_queue[i_song];
> + if (asprintf(&psz_submit_song,
> + "&a%%5B%d%%5D=%s"
> + "&t%%5B%d%%5D=%s"
> + "&i%%5B%d%%5D=%u"
> + "&o%%5B%d%%5D=P"
> + "&r%%5B%d%%5D="
> + "&l%%5B%d%%5D=%d"
> + "&b%%5B%d%%5D=%s"
> + "&n%%5B%d%%5D=%s"
> + "&m%%5B%d%%5D=%s",
> + i_song, p_song->psz_a,
> + i_song, p_song->psz_t,
> + i_song, (unsigned)p_song->date, /* HACK: %ju (uintmax_t) unsupported on Windows */
> + i_song,
> + i_song,
> + i_song, p_song->i_l,
> + i_song, p_song->psz_b,
> + i_song, p_song->psz_n,
> + i_song, p_song->psz_m
> + ) == -1)
> + { /* Out of memory */
> + vlc_mutex_unlock(&p_sys->lock);
> + goto out;
> + }
> }
> - free(psz_submit_song);
> - free(psz_submit_tmp);
> }
> +
> + psz_submit_tmp = psz_submit;
> + int print_ret = asprintf(&psz_submit, "%s%s",
> + psz_submit_tmp, psz_submit_song);
> + free(psz_submit_tmp);
> + free(psz_submit_song);
> vlc_mutex_unlock(&p_sys->lock);
>
> - int i_post_socket = net_ConnectTCP(p_intf, p_sys->p_submit_url.psz_host,
> - p_sys->p_submit_url.i_port);
> + if (print_ret == -1)
> + { /* Out of memory */
> + goto out;
> + }
> +
> + int i_post_socket = net_ConnectTCP(p_intf, url->psz_host,
> + url->i_port);
>
> if (i_post_socket == -1)
> {
> @@ -794,8 +839,8 @@ static void *Run(void *data)
> "\r\n"
> "%s\r\n"
> "\r\n",
> - p_sys->p_submit_url.psz_path, strlen(psz_submit),
> - p_sys->p_submit_url.psz_host, psz_submit
> + url->psz_path, strlen(psz_submit),
> + url->psz_host, psz_submit
> );
>
> free(psz_submit);
> @@ -841,9 +886,18 @@ static void *Run(void *data)
>
> if (strstr((char *) p_buffer, "OK"))
> {
> - for (int i = 0; i < p_sys->i_songs; i++)
> - DeleteSong(&p_sys->p_queue[i]);
> - p_sys->i_songs = 0;
> + if (b_nowp_submission_ongoing)
> + {
> + b_nowp_submission_ongoing = false;
> + p_sys->b_submit_nowp = false;
> + }
> + else
> + {
> + for (int i = 0; i < p_sys->i_songs; i++)
> + DeleteSong(&p_sys->p_queue[i]);
> + p_sys->i_songs = 0;
> + }
> +
This should probably be run with the lock held. Also, let's imagine that
the submission takes a while before failing, and that in the meantime,
we parsed another song, wouldn't the cleanup here destroy the new song info?
TL;DR: This looks racy to me.
> i_interval = 0;
> next_exchange = 0;
> msg_Dbg(p_intf, "Submission successful!");
>
During my tests, it seems that now playing doesn't get updated when
switching track while a previous one is playing, even though a request
is sent to last.fm, I didn't check the request itself, but I wouldn't be
surprised the old song info is being sent.
Regards,
More information about the vlc-devel
mailing list