[vlc-devel] [RFC v2] audioscrobbler: submit the currently playing track

[Jonas Lundqvist]

Hi Hugo,

On Wed, Nov 04, 2015 at 01:55:37PM +0100, Hugo Beauzée-Luyssen wrote:
> >@@ -22,11 +22,10 @@
> >   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> >   *****************************************************************************/
> >
> >-/* audioscrobbler protocol version: 1.2
> >- * http://www.audioscrobbler.net/development/protocol/
> >+/* 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.lastfm.fr/api
> >+ * TODO:    Update to new API? http://www.last.fm/api/scrobbling
> >   */
> 
> This should be in a separate patch

Sure, I'll split the changes into smaller patches.

> >@@ -88,11 +87,8 @@ struct intf_sys_t
> >      vlc_url_t               p_submit_url;       /**< where to submit data   */
> >
> >      /* submission of playing song */
> >-#if 0 //NOT USED
> >-    char                    *psz_nowp_host;     /**< where to submit data   */
> >-    int                     i_nowp_port;        /**< port to which submit   */
> >-    char                    *psz_nowp_file;     /**< file to which submit   */
> >-#endif
> 
> Removal of dead code should be in another patch as well.


Ok, I'll fix it.

> >@@ -216,8 +212,11 @@ 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);
> >+
> 
> Extra whitespace change

Ok, I'll fix it.

> >@@ -389,6 +388,7 @@ static int ItemChange(vlc_object_t *p_this, const char *psz_var,
> >      p_sys->p_input = vlc_object_hold(p_input);
> >      var_AddCallback(p_input, "intf-event", PlayingChange, p_intf);
> >
> >+
> 
> extra whitespace

Ok, I'll fix it.

> >@@ -449,10 +450,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);
> >-#if 0 //NOT USED
> >-    free(p_sys->psz_nowp_host);
> >-    free(p_sys->psz_nowp_file);
> >-#endif
> 
> ditto

Ok, I'll fix it.

> >+        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);
> >+        }
> >+
> >+        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;
> >          }
> 
> You could free those unconditionally, and then check for asprintf error

How do you mean?

int ret = asprintf(&psz_submit, "%s%s", ...)
free(psz_submit_tmp);
free(psz_submit_song);
vlc_mutex_unlock(&p_sys->lock);
if (ret == -1) {
	goto out;
}

Or?

> 
> >+        free(psz_submit_song);
> >+        free(psz_submit_tmp);
> >+
> >          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);
> >+        int i_post_socket = net_ConnectTCP(p_intf, url->psz_host,
> >+                                        url->i_port);
> >
> 
> You didn't check for vlc_UrlParse success in Handshake, so host/url could be
> NULL here.

Basically I just looked at the current code for the p_submit_url and there is no check for vlc_UrlParse errors there either.

So I assume that you want the check for that (old) piece of code also?

Best regards
Jonas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20151104/d0a0f1aa/attachment.sig>