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

[Jakub Wieczorek]

Hello,

Thanks. Comments inline.

2011/1/13 Rémi Denis-Courmont <remi at remlab.net>:
>   Hello,
>
> +#define LASTFM_API_KEY "ea18e6eaef5304bfcbb11be37ebbb5c7"
>
> I wouldn't publish my personal key on vlc-devel.

Maybe. Too late, though.

> +#define LASTFM_SECRET "e796a0cd80aa9627b791ac77325c5026"
>
> Hmm, is secret the correct term?

That's what it's called here and there.

> Are the LastFM ToS acceptable? I wouldn't want VideoLAN and Linux distributions violating them by accident.

Thanks for taking this up. I kind of forgot to make sure I conform to
them..., my mistake. Turns out I probably am not.

>From http://www.last.fm/api/tos:
- I'm violating 2.7. I hope I can find a technical solution to it.
- I thought 3.1 could clash with GPL but then I came to a conclusion
that last.fm streaming service is just content like a DVD movie and
therefore these are subject to their own terms of use and don't have
anything to do with the license of the application.
- 2.5 and 2.6 are also intriguing - nothing stops the user from
broadcasting a last.fm stream to the network or dumping it to a file
but I guess I'm reading into it too much and that making it
technically possible for the user to violate the terms doesn't mean we
are.

Anyway, IANAL and I'd be grateful if you could review the ToS as well
when you have time.

> +    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.

Fixed.

> +        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.

Fixed.

> +    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.

Fixed.

> +    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.

Fixed.

> +
> +    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.

Fixed.

> +    vlc_restorecancel( canc );
>
> vlc_restorecancel() could be moved before vlc_mutex_lock() (it makes no real
> difference).

Fixed.

> +    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...

Well, as the comment in the code says, it assumes the correctness of
the data it receives from the last.fm service, to the extent described
in the documentation. But, it's also written in such a way that it
shouldn't leak or crash in cases the output changes, it'll simply stop
working.

Best regards,
Jakub Wieczorek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-an-access-module-that-works-with-last.fm-radio-s.patch
Type: text/x-patch
Size: 42775 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110115/94396fda/attachment.bin>