[vlc-devel] [PATCH] extvlc option for xspf playlist and remembering what is playing when vlc is closed

[Rafaël Carré]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Le Tue, 29 Apr 2008 16:24:03 +0200,
Antoine Lejeune <phytos at via.ecp.fr> a écrit :

> From f0904393b1c209e0bec62706ab6c6c8b781ad023 Mon Sep 17 00:00:00 2001
> From: Antoine Lejeune <phytos at videolan.org>
> Date: Tue, 29 Apr 2008 16:15:28 +0200
> Subject: [PATCH] Ability to remember the item playing when VLC is
> closed so that when VLC is restarted the item can restart at its last
> position.


> +    /* Saving to the start-time option the playing time */
> +    INPUT_SAVE_TIME        /* res= */

res = what ? (res is result)


> +        case INPUT_SAVE_TIME:
> +        {
> +            char *psz_time;
> +            if( asprintf( &psz_time, I64Fd,
> p_input->i_time/1000000 ) == -1 )
> +                psz_time = NULL;
> +            input_Control( p_input, INPUT_ADD_OPTION, "start-time",
> psz_time ); +
> +            // If it's a dvd://, rename it if its title is > 0 (not
> the menu)
> +            const char *psz_access, *psz_demux;
> +            char *psz_path;
> +            char *psz_uri =
> strdup( p_input->p->input.p_item->psz_uri ); +
> +            MRLSplit( psz_uri, &psz_access, &psz_demux, &psz_path );
> +            if( strcmp( psz_access, "dvd" ) == 0 && 
> +                var_GetInteger( p_input, "title" ) > 0 &&
> +                asprintf( &p_input->p->input.p_item->psz_uri,
> +                          "dvdsimple://%s", psz_path ) == -1 )
> +                p_input->p->input.p_item->psz_uri = NULL;
> +
> +            free( psz_uri );
> +
> +            return VLC_SUCCESS;
> +        }

there is a memory leak

> diff --git a/src/playlist/loadsave.c b/src/playlist/loadsave.c
> index 1579db3..0dc9afc 100644
> --- a/src/playlist/loadsave.c
> +++ b/src/playlist/loadsave.c
> @@ -113,11 +113,7 @@ int playlist_MLLoad( playlist_t *p_playlist )
>      input_item_t *p_input;
>  
>      if( !config_GetInt( p_playlist, "media-library") ) return
> VLC_SUCCESS;
> -    if( !psz_datadir ) /* XXX: This should never happen */
> -    {
> -        msg_Err( p_playlist, "no data directory, cannot load media
> library") ;
> -        return VLC_EGENERIC;
> -    }
> +    assert( psz_datadir );

This is not the only place where we should use assert(), the same code
snippet is present in other files, and should be corrected in a
separate patch.

> +int playlist_RestorePlayingItem( playlist_t *p_playlist )
> +{

..

> +    if( asprintf( &psz_uri, "%s" DIR_SEP "last-played.xspf",
> psz_datadir ) == -1 )
> +    {
> +        psz_uri = NULL;

not needed

> +    struct stat p_stat;
> +    /* checks if playing item file is present */
> +    if( utf8_stat( psz_uri , &p_stat ) )
> +    {
> +        free( psz_uri );
> +        return VLC_EGENERIC;
> +    }
> +    free( psz_uri );

You should check p_stat

> +    if( asprintf( &psz_uri, "file/xspf-open://%s" DIR_SEP
> "last-played.xspf",
> +                  psz_datadir ) == -1 )
> +    {
> +        psz_uri = NULL;

not needed

> +    p_input = input_ItemNewExt( p_playlist, psz_uri, 
> +            "Playing when VLC was closed",

You should use gettext

> +            0, NULL, -1 );
> +
> +    vlc_event_attach( &p_input->event_manager,
> vlc_InputItemSubItemAdded,
> +                            input_item_subitem_added_to_PL,
> p_playlist );
> +    input_Read( p_playlist, p_input, true );
> +    vlc_event_detach( &p_input->event_manager,
> vlc_InputItemSubItemAdded,
> +                            input_item_subitem_added_to_PL,
> p_playlist ); +
> +    vlc_gc_decref( p_input );
> +    free( psz_uri );
> +    return VLC_SUCCESS;
> +
> +error:
> +    free( psz_uri );
> +    return VLC_ENOMEM;
> +}
> +
> +int playlist_SavePlayingItem( playlist_t *p_playlist )
> +{
> +    char *psz_datadir = p_playlist->p_libvlc->psz_datadir;
> +    if( !config_GetInt( p_playlist, "remember-playing-item" ) )
> +        return VLC_SUCCESS;
> +    if( !psz_datadir ) /* XXX: This should never happen */
> +    {

here you missed the assertion also.
I think there is a function you can use to get the datadir, to avoid
code duplication.

> +    char psz_dirname[ strlen( psz_datadir )
> +                      + sizeof( DIR_SEP "last-played.xspf")];

there is an off by one, my son.

> +    /* In order to save the playing item, it must be the only child
> of a node.
> +       The playlist is dead, we can't use playlist_NodeCreate so we
> create a
> +       new node from the playing item and we add the playing item to
> this new
> +       node. */
> +
> +    // We allocate the memory for the node
> +    DECMALLOC_ERR( p_item, playlist_item_t );
> +    playlist_item_t *p_current = ARRAY_VAL( p_playlist->current,
> +            p_playlist->i_current_index );
> +    // We set the properties of the node
> +    p_item->p_input = p_current->p_input;
> +    p_item->i_id = ++p_playlist->i_last_playlist_id;
> +    p_item->p_parent = NULL;
> +    p_item->i_children = 0;
> +    p_item->pp_children = NULL;
> +    p_item->i_flags = 0;
> +    p_item->p_playlist = p_playlist;
> +    p_item->i_children = 0;

instead of setting each member to 0 or NULL you can use memset() (you
can assume that 0 == NULL)

- -- 
Rafaël Carré
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFIH154YWCeGMCv8Q8RAoycAKDSgzHWXW0aUmiJH/LWduerLINHRQCdGN0F
SNFxjz1i8lNPabCB6wN5LFw=
=sH4c
-----END PGP SIGNATURE-----