[vlc-devel] [GCI] last.fm access module

Rémi Denis-Courmont remi at remlab.net
Thu Jan 13 23:02:30 CET 2011


   Hello,

+#define LASTFM_API_KEY "ea18e6eaef5304bfcbb11be37ebbb5c7"

I wouldn't publish my personal key on vlc-devel.

+#define LASTFM_SECRET "e796a0cd80aa9627b791ac77325c5026"

Hmm, is secret the correct term? Are the LastFM ToS acceptable? I wouldn't 
want VideoLAN and Linux distributions violating them by accident.

+#define LASTFM_ROOT_URL "http://ws.audioscrobbler.com/2.0/"

(...)

+static int Open( vlc_object_t *p_this )
+{
+    access_t *p_access = ( access_t * )p_this;
+    access_sys_t *p_sys = calloc( 1, sizeof( access_sys_t ) );
+    if( !p_sys )
+        return VLC_ENOMEM;
+
+    curl_global_init( CURL_GLOBAL_ALL );
+
+    access_InitFields( p_access );
+    ACCESS_SET_CALLBACKS( Read, NULL, Control, NULL );
+    p_access->p_sys = p_sys;
+
+    vlc_mutex_init( &p_sys->lock );
+    vlc_cond_init( &p_sys->wait );
+
+    if( vlc_clone( &p_sys->thread, Run, p_access, VLC_THREAD_PRIORITY_LOW ) )
+    {
+        vlc_cond_destroy( &p_sys->wait );
+        vlc_mutex_destroy( &p_sys->lock );

p_sys is leaked.

+        return VLC_EGENERIC;
+    }
+
+    free( p_access->psz_demux );
+    p_access->psz_demux = strdup( "mp3" );

You should not override the demux if it was explicitly set. This would break 
the dump demux for instance. You should only hint this if the demux is not 
specified.

+    return VLC_SUCCESS;
+}
+
+static void Close( vlc_object_t *p_this )
+{
+    access_t *p_access = ( access_t * )p_this;
+    access_sys_t *p_sys = p_access->p_sys;
+
+    vlc_cancel( p_sys->thread );
+    vlc_join( p_sys->thread, NULL );
+
+    vlc_cond_destroy( &p_sys->wait );
+    vlc_mutex_destroy( &p_sys->lock );
+
+    free( p_sys->psz_session_key );
+    if( p_sys->p_station )
+    {
+        free( p_sys->p_station->psz_name );
+        free( p_sys->p_station->psz_url );
+    }
+    free( p_sys->p_station );
+
+    struct lastfm_track_t *p_track = p_sys->p_current_track;
+    struct lastfm_track_t *p_next_track = NULL;

Assigned value is never used.
The variable could be moved inside the while loop.

+    while( p_track )
+    {
+        p_next_track = p_track->p_next;
+        free( p_track->psz_url );
+        free( p_track->psz_title );
+        free( p_track->psz_artist );
+        free( p_track->psz_album );
+        free( p_track );
+        p_track = p_next_track;
+    }
+
+    if( p_sys->p_stream )
+        stream_Delete( p_sys->p_stream );
+
+    free( p_sys );
+
+    curl_global_cleanup();
+}
+
+static ssize_t Read( access_t *p_access, uint8_t *p_buffer, size_t i_len )
+{
+    access_sys_t *p_sys = p_access->p_sys;
+    int i_read = 0;
+
+    vlc_mutex_lock( &p_sys->lock );
+    if( p_sys->b_error )
+    {
+        p_access->info.b_eof = true;
+        vlc_mutex_unlock( &p_sys->lock );
+        return 0;
+    }
+    vlc_mutex_unlock( &p_sys->lock );
+
+    if( !p_sys->p_stream )
+    {
+        vlc_mutex_lock( &p_sys->lock );

I see no point in unlocking and locking again the same mutex immediately.

+        bool tuned = p_sys->psz_session_key && p_sys->p_station && p_sys-
>p_current_track;
+        if( !tuned )
+        {
+            vlc_mutex_unlock( &p_sys->lock );
+            return -1;
+        }
+
+        p_sys->p_stream = stream_UrlNew( p_access, p_sys->p_current_track-
>psz_url );
+        if( !p_sys->p_stream )
+            msg_Err( p_access, "connection problem, will skip to the next 
track" );
+
+        vlc_mutex_unlock( &p_sys->lock );
+    }
+
+    if( p_sys->p_stream )
+        i_read = stream_Read( p_sys->p_stream, p_buffer, i_len );
+
+    while( i_read == 0 )
+    {
+        msg_Dbg( p_access, "switching to the next track" );
+
+        if( p_sys->p_stream )
+        {
+            stream_Delete( p_sys->p_stream );
+            p_sys->p_stream = NULL;
+        }
+
+        vlc_mutex_lock( &p_sys->lock );
+
+        struct lastfm_track_t *p_current_track = p_sys->p_current_track;
+        struct lastfm_track_t *p_next_track = p_current_track->p_next;
+        free( p_current_track->psz_url );
+        free( p_current_track->psz_title );
+        free( p_current_track->psz_artist );
+        free( p_current_track->psz_album );
+        free( p_current_track );
+
+        p_sys->p_current_track = p_next_track;
+        if( !p_sys->p_current_track )
+        {
+            msg_Err( p_access, "no more tracks" );
+            vlc_mutex_unlock( &p_sys->lock );
+            return -1;
+        }
+
+        p_sys->p_stream = stream_UrlNew( p_access, p_sys->p_current_track-
>psz_url );
+        if( !p_sys->p_stream )
+        {
+            msg_Err( p_access, "connection problem, try the next track" );
+            vlc_mutex_unlock( &p_sys->lock );
+            continue;
+        }
+
+        if( !p_sys->p_current_track->p_next )
+        {
+            msg_Dbg( p_access, "waking up the updating thread" );
+            p_sys->b_pull_new_tracks = true;
+            vlc_cond_signal( &p_sys->wait );
+        }
+
+        vlc_mutex_unlock( &p_sys->lock );
+
+        i_read = stream_Read( p_sys->p_stream, p_buffer, i_len );
+    }
+
+    return i_read;
+}
+
+static int Control( access_t *p_access, int i_query, va_list args )
+{
+    bool *pb_bool;
+    int64_t *pi_64;
+
+    switch( i_query )
+    {
+        case ACCESS_CAN_SEEK:
+        case ACCESS_CAN_FASTSEEK:
+        case ACCESS_CAN_PAUSE:
+            pb_bool = va_arg( args, bool* );
+            *pb_bool = false;
+            break;
+        case ACCESS_CAN_CONTROL_PACE:
+            pb_bool = va_arg( args, bool* );
+            *pb_bool = true;
+            break;
+        case ACCESS_GET_PTS_DELAY:
+            pi_64 = va_arg( args, int64_t * );
+            *pi_64 = var_InheritInteger( p_access, "http-caching" ) * 1000;
+            break;
+        case ACCESS_GET_TITLE_INFO:
+        case ACCESS_GET_META:
+        case ACCESS_GET_CONTENT_TYPE:
+        case ACCESS_GET_SIGNAL:
+        case ACCESS_SET_PAUSE_STATE:
+        case ACCESS_SET_TITLE:
+        case ACCESS_SET_SEEKPOINT:
+        case ACCESS_SET_PRIVATE_ID_STATE:
+        case ACCESS_SET_PRIVATE_ID_CA:
+        case ACCESS_GET_PRIVATE_ID_STATE:
+            return VLC_EGENERIC;
+        default:
+            msg_Warn( p_access, "unimplemented query in control" );
+            return VLC_EGENERIC;
+    }
+    return VLC_SUCCESS;
+}
+
+/*****************************************************************************
+ * Run: main thread
+ 
*****************************************************************************/
+static void *Run( void *data )
+{
+    access_t *p_access = data;
+    access_sys_t *p_sys  = p_access->p_sys;
+    int canc;
+
+    canc = vlc_savecancel();
+    if( Authenticate( p_access ) != VLC_SUCCESS )
+        goto error;
+
+    if( TuneInToStation( p_access ) != VLC_SUCCESS )
+        goto error;
+
+    vlc_mutex_lock( &p_sys->lock );
+    p_sys->b_pull_new_tracks = true;
+    vlc_mutex_unlock( &p_sys->lock );

Again, there is no point in releasing the mutex if you take it back 
immediately.

+    vlc_restorecancel( canc );

vlc_restorecancel() could be moved before vlc_mutex_lock() (it makes no real 
difference).

+    for( ;; )
+    {
+        mutex_cleanup_push( &p_sys->lock );
+        vlc_mutex_lock( &p_sys->lock );
+        while( !p_sys->b_pull_new_tracks )
+            vlc_cond_wait( &p_sys->wait, &p_sys->lock );
+
+        p_sys->b_pull_new_tracks = false;
+        vlc_cleanup_pop();
+        vlc_mutex_unlock( &p_sys->lock );
+
+        msg_Dbg( p_access, "waking up..." );
+
+        canc = vlc_savecancel();
+        if( PullNewTracks( p_access ) != VLC_SUCCESS )
+            goto error;
+        vlc_restorecancel( canc );
+    }
+
+error:
+    vlc_restorecancel( canc );
+    vlc_mutex_lock( &p_sys->lock );
+    p_sys->b_error = true;
+    vlc_mutex_unlock( &p_sys->lock );
+    return NULL;
+}
+
+/*****************************************************************************
+ * Authenticates: performs the authentication of the user
+ 
*****************************************************************************/
+// FIXME: We should be doing this only once by storing the session key
+// persistently and then reusing next time it's needed.
+static int Authenticate( access_t *p_access )
+{
+    msg_Dbg( p_access, "Authenticating..." );
+
+    access_sys_t *p_sys  = p_access->p_sys;
+    int i_retcode = VLC_SUCCESS;
+    char *psz_lastfm_username = var_InheritString( p_access, "lastfm-
username" );
+    char *psz_lastfm_password = var_InheritString( p_access, "lastfm-
password" );
+    char *psz_auth_token = NULL;
+
+    stream_t *p_stream = NULL;
+    xml_reader_t *p_xml_reader = NULL;
+    int i_xml_reader_node_type = 0;
+    char *psz_xml_reader_element_name = NULL;
+
+    char *psz_status = NULL;
+    int i_errorcode = 0;
+    char *psz_error = NULL;
+
+    char *psz_session_key = NULL;
+
+    if( !psz_lastfm_username || !psz_lastfm_password ||
+        *psz_lastfm_username == '\0' || *psz_lastfm_password == '\0' )
+    {
+        msg_Err( p_access, "last.fm credentials missing" );
+        i_retcode = VLC_ENOVAR;
+        goto error;
+    }
+
+    psz_auth_token = AuthToken( psz_lastfm_username, psz_lastfm_password );
+    if( !psz_auth_token )
+    {
+        msg_Err( p_access, "out of memory" );
+        i_retcode = VLC_ENOMEM;
+        goto error;
+    }
+
+    msg_Dbg( p_access, "username: %s; password: %s; auth token: %s",
+             psz_lastfm_username, psz_lastfm_password, psz_auth_token );
+
+    struct url_argument_t arguments[] =
+    {
+        { "api_key", LASTFM_API_KEY },
+        { "authToken", psz_auth_token },
+        { "method", "auth.getMobileSession" },
+        { "username", psz_lastfm_username },
+        { 0, 0 },
+        { 0, 0 }
+    };
+
+    p_stream = Request( p_access, arguments, REST_GET );
+    if( !p_stream )
+    {
+        msg_Err( p_access, "network error" );
+        i_retcode = VLC_EGENERIC;
+        goto error;
+    }
+
+    p_xml_reader = xml_ReaderCreate( p_access, p_stream );
+    if( !p_xml_reader )
+    {
+        msg_Err( p_access, "out of memory" );
+        i_retcode = VLC_ENOMEM;
+        goto error;
+    }
+
+    while( xml_ReaderRead( p_xml_reader ) == 1 )

This XML API always blows my mind... I don't know how many memory leaks or 
invalid pointer reference we have had due to unexpected XML content :-( I hope 
you've reviewed all the failure cases...

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis



More information about the vlc-devel mailing list