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

[Hugo Beauzée-Luyssen]

On 11/04/2015 06:59 PM, Jonas Lundqvist wrote:
> Hi Hugo,
>

Hello Jonas,

> 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?
>

Exactly what you wrote :)

> 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?
>

Indeed that would be better IMHO, however, I just realized that the 
vlc_UrlParse documentation states that "errors cannot be detected". I 
guess the best course of action would then be to check that the basic 
fields you require are != NULL ?

> Best regards
> Jonas
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>