[vlc-devel] [PATCH] Media library module for VLC, GSoC 09
Rémi Duraffort
ivoire at videolan.org
Tue Aug 25 12:15:31 CEST 2009
Hello,
> diff --git a/include/vlc_media_library.h b/include/vlc_media_library.h
> new file mode 100644
> index 0000000..a913323
> --- /dev/null
> +++ b/include/vlc_media_library.h
> @@ -0,0 +1,1210 @@
> +/*****************************************************************************
> + * vlc_media_library.h: SQL-based media library
> + *****************************************************************************
> + * Copyright (C) 2008 the VideoLAN Team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
encoding ? 'é' and not '??'.
> + * @brief Copy all members of a ml_media_t to another.
> + * @param b Destination media, already allocated
> + * @param a Source media, cannot be NULL, const
> + * @note This does not check memory allocation (for strdup). It is threadsafe
> + * @todo Free b content, before inserting a?
Maybe yes !
> + */
> +static inline int ml_CopyMedia( ml_media_t *b, ml_media_t *a )
> +{
> + if( !a || !b ) return VLC_EGENERIC;
> + assert( a != b );
> + ml_LockMedia( a );
> + ml_LockMedia( b );
> + b->b_sparse = a->b_sparse;
> + b->i_id = a->i_id;
> + b->i_type = a->i_type;
> + b->i_album_id = a->i_album_id;
> + b->i_disc_number = a->i_disc_number;
> + b->i_track_number = a->i_track_number;
> + b->i_year = a->i_year;
> + b->i_vote = a->i_vote;
> + b->i_score = a->i_score;
> + b->i_filesize = a->i_filesize;
> + b->i_duration = a->i_duration;
> + b->i_played_count = a->i_played_count;
> + b->i_last_played = a->i_last_played;
> + b->i_skipped_count = a->i_skipped_count;
> + b->i_last_skipped = a->i_last_skipped;
> + b->i_first_played = a->i_first_played;
> + b->i_import_time = a->i_import_time;
> + b->i_bitrate = a->i_bitrate;
> + b->i_samplerate = a->i_samplerate;
> + b->i_bpm = a->i_bpm;
> + if( a->psz_uri ) b->psz_uri = strdup( a->psz_uri );
> + if( a->psz_title ) b->psz_title = strdup( a->psz_title );
> + if( a->psz_orig_title ) b->psz_orig_title = strdup( a->psz_orig_title );
> + if( a->psz_album ) b->psz_album = strdup( a->psz_album );
> + if( a->psz_cover ) b->psz_cover = strdup( a->psz_cover );
> + if( a->psz_genre ) b->psz_genre = strdup( a->psz_genre );
> + if( a->psz_comment ) b->psz_comment = strdup( a->psz_comment );
> + if( a->psz_extra ) b->psz_extra = strdup( a->psz_extra );
> + if( a->psz_preview ) b->psz_preview = strdup( a->psz_preview );
> + if( a->psz_language ) b->psz_language = strdup( a->psz_language );
Two cases for the if( a->psz_bla ) b->psz_bla = strdup( a->psz_bla )
* b->psz_bla already exist => leak
* b->psz_bla and a->psz_bla does not exist => b->psz_bla is undefined !!
It depends on how you use this function: is b contening some data before
the call to ml_CopyMedia ? If yes, you must free before doing the
strdup. If not you must set to NULL b->psz_bla if a->psz_bla is NULL.
> + b->p_people = NULL;
> + if( a->p_people ) ml_CopyPersons( &( b->p_people ), a->p_people );
> + ml_UnlockMedia( a );
> + ml_UnlockMedia( b );
Unlocking in the reverse order is better to avoid deaddlocks.
> +/**
> + * @brief Free a find tree
> + * @param Find tree to free
> + * @param true to free any associated strings, false to not free them
> + */
> +static inline void ml_GenericFreeFindTree( ml_ftree_t* tree, bool freestrings )
> +{
> + if( tree == NULL )
> + return;
> + if( tree->left )
> + ml_GenericFreeFindTree( tree->left, freestrings );
> + free( tree->left );
Can be inside the if( tree->left ).
> + if( tree->right )
> + ml_GenericFreeFindTree( tree->right, freestrings );
> + free( tree->right );
same
> +/**
> + * @brief Checks if a given find tree has leaf nodes
> + * @param Find tree
> + * @return Number of leaf nodes
> + */
> +static inline int ml_FtreeHasOp( ml_ftree_t* tree )
> +{
> + if( tree == NULL )
> + return 0;
> + if( tree->criteria > 0 && tree->op == ML_OP_NONE )
> + return 1;
> + else
> + return ml_FtreeHasOp( tree->left ) + ml_FtreeHasOp( tree->right );
> +}
Personnaly I don't like the name as you get the number of leaf nodes and
not if the given tree had leaf node or not.
But that's not realy important.
> +/**
> + * @brief Find a value in the ML database, fill p_result with it.
> + * @param p_media_library Media library object
> + * @param p_result Object to put result into
> + * @param Args [ SelectType [ PersonType ] Value ] ... ML_END
> + * @note Do not use this function directly.
> + */
> +static inline int __ml_GetValue( media_library_t *p_media_library,
> + ml_result_t *p_result,
> + va_list args )
> +{
> + vlc_array_t *p_result_array = vlc_array_new();
> + int i_ret = p_media_library->functions.pf_Find( p_media_library,
> + p_result_array,
> + args );
> + if( i_ret != VLC_SUCCESS )
> + goto exit;
> + if( vlc_array_count( p_result_array ) > 0 )
> + memcpy( p_result,
> + ( ml_result_t* ) vlc_array_item_at_index( p_result_array, 0 ),
> + sizeof( ml_result_t) );
Dummy question: Are you leaing the results > 0 ?
What happend is pf_Find return more thant one result ? (if that's
possible)
> diff --git a/modules/media_library/item_list.c b/modules/media_library/item_list.c
> new file mode 100644
> index 0000000..eb0f8bc
> --- /dev/null
> +++ b/modules/media_library/item_list.c
> @@ -0,0 +1,290 @@
> +/*****************************************************************************
> + * item_list.c: An input_item_t+media_id couple list for the media library
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN Team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
é instead of ??
> +/**
> + * @brief Add an item to the head of the list
> + * @param p_ml
> + * @param p_media media object. ID must be non zero and valid
> + * @param p_item input item to add, MUST NOT be NULL
> + * @param locked flag set if the list is locked. do not use
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +int __item_list_add( watch_thread_t *p_wt, ml_media_t* p_media, input_item_t *p_item,
> + bool locked )
> +{
> + if( !locked )
> + vlc_mutex_lock( &p_wt->list_mutex );
> + ml_LockMedia( p_media );
> + assert( p_media->i_id );
> + /* Ensure duplication does not occur */
> + il_foreachlist( p_wt->p_hlist[ item_list_hash( p_item->i_id ) ], p_elt )
> + {
> + if( p_elt->p_item->i_id == p_item->i_id )
> + {
> + if( !locked )
> + vlc_mutex_unlock( &p_wt->list_mutex );
> + ml_UnlockMedia( p_media );
Unlock in the reverse order.
> + return VLC_EGENERIC;
> + }
> + }
> +
> + item_list_t *p_new = ( item_list_t* ) calloc( 1, sizeof( item_list_t ) );
> + if( !p_new )
> + {
> + if( !locked )
> + vlc_mutex_unlock( &p_wt->list_mutex );
> + ml_UnlockMedia( p_media );
same.
> + return VLC_ENOMEM;
> + }
> + p_new->p_next = p_wt->p_hlist[ item_list_hash( p_item->i_id ) ];
> + p_new->i_refs = 0;
> + p_new->i_media_id = p_media->i_id;
> + p_new->p_media = p_media;
> + p_new->p_item = p_item;
> + p_wt->p_hlist[ item_list_hash( p_item->i_id ) ] = p_new;
> + ml_UnlockMedia( p_media );
> + if( !locked )
> + vlc_mutex_unlock( &p_wt->list_mutex );
> + return VLC_SUCCESS;
> +}
> +/**
> + * @brief Flag an item as updated
> + * @param p_ml this media library
> + * @param p_item item to find and flag
> + * @param b_played raise play count or not, update last play
> + * @return media_id found, or VLC_EGENERIC
> + */
> +int item_list_updateInput( watch_thread_t *p_wt, input_item_t *p_item,
> + bool b_played )
> +{
> + vlc_mutex_lock( &p_wt->list_mutex );
> + il_foreachlist( p_wt->p_hlist[ item_list_hash( p_item->i_id ) ], p_elt )
> + {
> + if( p_elt->p_item == p_item )
> + {
> + /* Item found, flag and return */
> + p_elt->i_age = 0;
> + p_elt->i_update |= b_played ? 3 : 1;
3 : 1 ? What does this mean ?
> + vlc_mutex_unlock( &p_wt->list_mutex );
> + return p_elt->i_media_id;
> + }
> + }
> + vlc_mutex_unlock( &p_wt->list_mutex );
> + return VLC_EGENERIC;
> +}
> diff --git a/modules/media_library/media_pool.c b/modules/media_library/media_pool.c
> +/**
> + * @brief Insert a media into the media pool
> + * @param p_ml ML object
> + * @param p_media Media object to insert
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +int pool_InsertMedia( media_library_t* p_ml, ml_media_t* p_media )
> +{
> + ml_LockMedia( p_media );
> + assert( p_media );
> + assert( p_media->i_id > 0 );
> + if( p_media->b_pool )
> + {
> + msg_Dbg( p_ml, "Already in pool! %s %d", p_media->psz_uri, p_media->i_id );
> + ml_UnlockMedia( p_media );
> + return VLC_EGENERIC;
> + }
> + p_media->b_pool = true;
> + int i_ret = VLC_SUCCESS;
> + vlc_mutex_lock( &p_ml->p_sys->pool_mutex );
> + mp_foreachlist( p_ml->p_sys->p_mediapool[ (mediapool_hash(p_media->i_id)) ], p_item )
> + {
> + if( p_media == p_item->p_media )
> + {
> + i_ret = VLC_EGENERIC;
> + break;
> + }
> + else if( p_media->i_id == p_item->p_media->i_id )
> + {
> + i_ret = VLC_EGENERIC;
> + msg_Warn( p_ml, "A media of the same id was found, but in different objects!" );
> + break;
> + }
> + }
> + if( i_ret == VLC_SUCCESS )
> + {
> + ml_poolobject_t* p_new = ( ml_poolobject_t * ) calloc( 1, sizeof( ml_poolobject_t* ) );
> + if( !p_new )
> + i_ret = VLC_EGENERIC;
> + else
> + {
> + ml_gc_incref( p_media );
> + p_new->p_media = p_media;
> + p_new->p_next = p_ml->p_sys->p_mediapool[ ( mediapool_hash( p_media->i_id ) ) ];
> + p_ml->p_sys->p_mediapool[ ( mediapool_hash( p_media->i_id ) ) ] = p_new;
> + }
> + }
> + ml_UnlockMedia( p_media );
> + vlc_mutex_unlock( &p_ml->p_sys->pool_mutex );
Unlock in reverse order.
> + return i_ret;
> +}
> diff --git a/modules/media_library/ml_watch.c b/modules/media_library/ml_watch.c
> new file mode 100644
> index 0000000..ccef6fe
> --- /dev/null
> +++ b/modules/media_library/ml_watch.c
> @@ -0,0 +1,547 @@
> +/*****************************************************************************
> + * ml_watch.c: SQL-based media library: Medias watching system
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +/**
> + * @brief Init watching system
> + * @return Error if the object or the thread could not be created
> + */
> +int watch_Init( media_library_t *p_ml )
> +{
> + /* init and launch watching thread */
Cosmetics:
watch_thread_t p_watch = p_ml->p_sys->p_watch;
and then using it instead of p_ml->p_sys->p_watch.
> + p_ml->p_sys->p_watch = vlc_object_create( p_ml, sizeof(watch_thread_t) );
> + if( !p_ml->p_sys->p_watch )
> + return VLC_ENOMEM;
> +
> + vlc_mutex_init( &p_ml->p_sys->p_watch->list_mutex );
> + p_ml->p_sys->p_watch->p_ml = p_ml;
> +
> + vlc_object_attach( p_ml->p_sys->p_watch, p_ml );
> +
> + vlc_cond_init( &p_ml->p_sys->p_watch->cond );
> + vlc_mutex_init( &p_ml->p_sys->p_watch->lock );
> +/**
> + * @brief Close the watching system
> + * @param p_ml The Media Library Object
> + */
> +void watch_Close( media_library_t *p_ml )
> +{
> + playlist_t *p_pl = pl_Hold( p_ml );
> + var_DelCallback( p_pl, "item-current", watch_PlaylistItemCurrent, p_ml );
> + var_DelCallback( p_pl, "playlist-item-append", watch_PlaylistItemAppend, p_ml );
> + var_DelCallback( p_pl, "playlist-item-deleted", watch_PlaylistItemDeleted, p_ml );
> + pl_Release( p_ml );
> +
> + /* Stop the watch thread and join in */
> + vlc_object_kill( p_ml->p_sys->p_watch );
> + vlc_thread_join( p_ml->p_sys->p_watch );
> +
> + /* Flush item list */
> + vlc_mutex_lock( &p_ml->p_sys->p_watch->list_mutex );
Dummy question: is it needed to lock the p_watch as the object is dead
now ?
> +/**
> + * @brief Callback when item is added to playlist
> + */
> +static int watch_PlaylistItemAppend( vlc_object_t *p_this, char const *psz_var,
> + vlc_value_t oldval, vlc_value_t newval,
> + void *data )
> +{
This is really long to be inside a playlist callback. Is it possible to
send a signal to the watch thread to do the callback in the watch
thread ?
> +}
> +/**
> + * @brief Update informations in the DB for an input item
> + *
> + * @param p_ml this media library instance
> + * @param i_media_id may be 0 (but not recommended)
> + * @param p_item input item that was updated
> + * @param b_raise_count increment the played count
> + * @return result of UpdateMedia()
> + */
> +static int watch_update_Item( media_library_t *p_ml,
> + int i_media_id, input_item_t *p_item,
> + bool b_raise_count, bool locked )
> +{
> +#ifndef NDEBUG
> + msg_Dbg( p_ml, "automatically updating media %d", i_media_id );
> +#endif
> + ml_media_t* p_media = item_list_mediaOfItem( p_ml->p_sys->p_watch, p_item, locked );
> + CopyInputItemToMedia( p_media, p_item );
> + ml_LockMedia( p_media );
> + p_media->i_played_count += b_raise_count ? 1 : 0;
> + ml_UnlockMedia( p_media );
> + int i_ret = UpdateMedia( p_ml, p_media );
> +
> + /* Add the poster to the album */
> + ml_LockMedia( p_media );
I don't like the sequence unlock/lock. Is it needed ?
> + if( p_media->i_album_id && p_media->psz_cover )
> + {
> + SetAlbumCover( p_ml, p_media->i_album_id, p_media->psz_cover );
> + }
> + ml_UnlockMedia( p_media );
> +
> + return i_ret;
> +}
> diff --git a/modules/media_library/sql_add.c b/modules/media_library/sql_add.c
> +/*****************************************************************************
> + * sql_media_library.c: SQL-based media library
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN Team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
> + * Srikanth Raju <srikiraju at gmail dot com>
'é'
> diff --git a/modules/media_library/sql_delete.c b/modules/media_library/sql_delete.c
> +/*****************************************************************************
> + * sql_delete.c: SQL-based media library: all database delete functions
> + *****************************************************************************
> + * Copyright (C) 2008-2009 VLC Team
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +/**
> + * @brief Generic DELETE function for many medias
> + * Delete a media and all its referencies which don't point
> + * an anything else.
> + *
> + * @param p_ml This media_library_t object
> + * @param p_array list of ids to delete
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + * TODO: Expand to delete media/artist/album given any params
> + */
> +int Delete( media_library_t *p_ml, vlc_array_t *p_array )
> +{
> + char *psz_idlist = NULL, *psz_tmp = NULL;
> + int i_return = VLC_ENOMEM;
> +
> + int i_rows = 0, i_cols = 0;
> + char **pp_results = NULL;
> +
> + if( vlc_array_count( p_array ) <= 0 )
> + {
> + i_return = VLC_SUCCESS;
> + goto quit_delete_final;
> + }
> + for( int i = 0; i < vlc_array_count( p_array ); i++ )
> + {
> + FREENULL( psz_tmp );
> + ml_element_t* find = ( ml_element_t * )
> + vlc_array_item_at_index( p_array, i );
> + assert( find->criteria == ML_ID );
> + if( !psz_idlist )
> + {
> + if( asprintf( &psz_tmp, "( %d", find->value.i ) == -1)
> + {
> + goto quit_delete_final;
> + }
> + }
> + else
> + {
> + if( asprintf( &psz_tmp, "%s, %d", psz_idlist,
> + find->value.i ) == -1)
> + {
> + goto quit_delete_final;
> + }
> + }
> + free( psz_idlist );
> + psz_idlist = strdup( psz_tmp );
just copying and setting psz_tmp to NUL ?
> + }
> + free( psz_tmp );
> + if( asprintf( &psz_tmp, "%s )", psz_idlist ? psz_idlist : "(" ) == -1 )
> + {
> + goto quit_delete_final;
> + }
> + psz_idlist = strdup( psz_tmp );
why using strdup here ?
> diff --git a/modules/media_library/sql_media_library.c b/modules/media_library/sql_media_library.c
> +/*****************************************************************************
> + * sql_media_library.c: SQL-based media library
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN Team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +/**
> + * @brief Load module
> + * @param obj Parent object
> + */
> +static int load( vlc_object_t *obj )
> +{
> + msg_Dbg( obj, "loading media library" );
> +
> + media_library_t *p_ml = ( media_library_t * ) obj;
> + p_ml->p_sys = ( media_library_sys_t* )
> + malloc( sizeof( media_library_sys_t ) );
> + if( !p_ml->p_sys )
> + return VLC_ENOMEM;
> + memset( p_ml->p_sys, 0, sizeof( media_library_sys_t ) );
calloc instead of malloc+memset.
> + p_ml->functions.pf_Find = FindVa;
> + p_ml->functions.pf_FindAdv = FindAdv;
> + p_ml->functions.pf_Control = Control;
> + p_ml->functions.pf_InputItemFromMedia = GetInputItemFromMedia;
> + p_ml->functions.pf_Update = Update;
> + p_ml->functions.pf_Delete = Delete;
> + p_ml->functions.pf_GetMedia = GetMedia;
> +
> + vlc_mutex_init( &p_ml->p_sys->lock );
> +
> + p_ml->p_sys->p_sql = sql_Create( p_ml, NULL );
> + if( !p_ml->p_sys->p_sql )
> + {
you are leaking the mutex
> + free( p_ml->p_sys );
> + return VLC_EGENERIC;
> + }
> +
> + InitDatabase( p_ml );
> +
> + /* Initialise the media pool */
> + ARRAY_INIT( p_ml->p_sys->mediapool );
> + vlc_mutex_init( &p_ml->p_sys->pool_mutex );
> +
> + /* Create variables system */
> + var_Create( p_ml, "media-added", VLC_VAR_INTEGER );
> + var_Create( p_ml, "media-deleted", VLC_VAR_INTEGER );
> + var_Create( p_ml, "media-meta-change", VLC_VAR_INTEGER );
Non inherit variables ?
> +
> + /* Launching the directory monitoring thread */
> + monitoring_thread_t *p_mon =
> + vlc_object_create( p_ml, sizeof( monitoring_thread_t ) );
> + if( !p_mon )
> + {
> + msg_Err( p_ml, "out of memory" );
you are leaking memory here (p_sys, mutex, ...)
> + return VLC_EGENERIC;
> + }
> + p_ml->p_sys->p_mon = p_mon;
> +
> + p_mon->p_ml = p_ml;
> +
> + if( vlc_thread_create( p_mon, "Media Library Monitoring",
> + RunMonitoringThread, VLC_THREAD_PRIORITY_LOW ) )
> + {
> + msg_Err( p_ml, "cannot spawn the media library monitoring thread" );
> + vlc_object_release( p_mon );
> + vlc_object_release( p_ml );
Also leaking.
> + return VLC_EGENERIC;
> + }
> +
> + /* Starting the watching system (starts a thread) */
> + watch_Init( p_ml );
> +
> + msg_Dbg( p_ml, "module loaded" );
> +
> + return VLC_SUCCESS;
> +}
> +/**
> + * @brief Unload module
> + *
> + * @param obj the media library object
> + * @return Nothing
> + */
> +static void unload( vlc_object_t *obj )
> +{
> + media_library_t *p_ml = ( media_library_t* ) obj;
> +
> + if( p_ml->p_sys )
> + {
AFAIK p_sys can't be NULL.
> + /* Stopping the watching system */
> + watch_Close( p_ml );
> +
> + /* Free memory */
> + free( p_ml->p_sys->psz_host );
> + free( p_ml->p_sys->psz_user );
> + free( p_ml->p_sys->psz_password );
> + p_ml->p_sys->i_port = 0;
Is it needed (I guess this isn't used anymore) ?
> +
> + /* Stop the monitoring thread */
> + vlc_object_kill( p_ml->p_sys->p_mon );
> + vlc_thread_join( p_ml->p_sys->p_mon );
> + vlc_object_release( p_ml->p_sys->p_mon );
Dummy question: Is it safe to kill the monitor after freeing some memory from
p_sys ?
> +
> + /* Destroy the variable */
> + var_Destroy( p_ml, "media-meta-change" );
> + var_Destroy( p_ml, "media-deleted" );
> + var_Destroy( p_ml, "media-added" );
> +
> + /* Empty the media pool */
> + vlc_mutex_lock( &p_ml->p_sys->pool_mutex );
Is it really needed as all threads are dead ?
> + ml_media_t* item;
> + FOREACH_ARRAY( item, p_ml->p_sys->mediapool )
> + vlc_gc_decref( item );
> + FOREACH_END()
> + vlc_mutex_unlock( &p_ml->p_sys->pool_mutex );
> + vlc_mutex_destroy( &p_ml->p_sys->pool_mutex );
> +
> + vlc_object_release( p_ml->p_sys->p_sql );
> +
> + vlc_mutex_destroy( &p_ml->p_sys->lock );
> +
> + free( p_ml->p_sys );
> + }
> + else
> + msg_Warn( p_ml, "something strange happened" );
can't happen normally.
> +/**
> + * @brief fills a vlc_array_t with results of an SQL query
> + * medias in ml_result_t
> + *
> + * @param p_ml the media library object
> + * @param p_array array to fill with ml_media_t elements (might be initialized)
> + * @param pp_results result of sql query
> + * @param i_rows row number
> + * @param i_cols column number
> + * @return VLC_SUCCESS or a VLC error code
> + * Warning: this returns VLC_EGENERIC if i_rows == 0 (empty result)
> + **/
> +int SQLToMediaArray( media_library_t *p_ml, vlc_array_t *p_result_array,
> + char **pp_results, int i_rows, int i_cols )
> +{
> + int i_ret = VLC_SUCCESS;
> [...]
> +#undef ELSE_CMP
> +#undef IF_CMP
> +
> + /* Read rows 1 to i_rows */
> + ml_media_t *p_media = NULL;
> + ml_result_t *p_result = NULL;
> +
> + for( int row = 1; ( row <= i_rows ) && ( i_ret == VLC_SUCCESS ); row++ )
> + {
> + p_media = GetMedia( p_ml, 0, ML_MEDIA, false );
> + if( !p_media )
> + {
> + free( indexes );
> + return VLC_ENOMEM;
> + }
> + p_result = ( ml_result_t * ) calloc( 1, sizeof( ml_result_t ) );
> + if( !p_result )
> + {
> + free( p_result );
p_result == NULL here :)
> + ml_gc_decref( p_media );
> + free( indexes );
> + return VLC_ENOMEM;
> + }
> + int i_appendrow;
> + ml_result_t* p_append = NULL;
> + for( i_appendrow = 0; i_appendrow < vlc_array_count( p_intermediate_array ); i_appendrow++ )
> + {
> + p_append = ( ml_result_t* )
> + vlc_array_item_at_index( p_intermediate_array, i_appendrow );
> + if( p_append->id == p_media->i_id )
> + break;
> + }
> + ml_UnlockMedia( p_media );
> + if( i_appendrow == vlc_array_count( p_intermediate_array ) )
> + {
> + ml_LockMedia( p_media );
unloc/lock ? needed ? safe ?
> + p_result->id = p_media->i_id;
> + p_result->type = ML_TYPE_MEDIA;
> + p_result->value.p_media = p_media;
> + if( psz_append_pname && i_append_pid && psz_append_prole )
> + ml_CreateAppendPersonAdv( &(p_result->value.p_media->p_people),
> + psz_append_prole, psz_append_pname, i_append_pid );
> + ml_UnlockMedia( p_media );
> + vlc_array_append( p_intermediate_array, p_result );
> + }
> + else /* This is a repeat row and the people need to be put together */
> + {
> + ml_LockMedia( p_append->value.p_media );
same.
> + if( psz_append_pname && i_append_pid && psz_append_prole )
> + ml_CreateAppendPersonAdv( &(p_append->value.p_media->p_people),
> + psz_append_prole, psz_append_pname, i_append_pid );
> + ml_UnlockMedia( p_append->value.p_media );
> + ml_gc_decref( p_media );
> + }
> + FREENULL( psz_append_prole );
> + FREENULL( psz_append_pname );
> + i_append_pid = 0;
> + }
> + p_media = NULL;
> + free( indexes );
> +/**
> + * @brief Set current database
> + *
> + * @param p_ml the media library object
> + * @param __psz_host DB host or filename if local file (SQLite...)
> + * @param port DB port
> + * @param psz_user DB user
> + * @param psz_pass DB password
> + * @return VLC_SUCCESS or vlc error code
> + */
> +int SetDatabase( media_library_t *p_ml, const char *__psz_host,
> + int port, const char *psz_user, const char *psz_pass )
> +{
I don't like __psz_host.
> + assert( p_ml );
> +
> + if( p_ml->p_sys->psz_host )
> + {
> + free( p_ml->p_sys->psz_host );
> + free( p_ml->p_sys->psz_user );
> + free( p_ml->p_sys->psz_password );
> + p_ml->p_sys->i_port = 0;
> + }
> +
> + /* Select database name */
> + char *psz_host = NULL;
> + if( !__psz_host || !*__psz_host )
> + {
> + psz_host = config_GetPsz( p_ml, "ml-filename" );
> +
> + /* Let's consider that a filename with a DIR_SEP is a full URL */
> + if( strchr( psz_host, DIR_SEP_CHAR ) == NULL )
> + {
> + char *psz_datadir = config_GetUserDataDir();
Removed by [8210d67fc5e51e0951dbad9b833b89a351bdb1f6]
> + char *psz_tmp = psz_host;
> + if( asprintf( &psz_host, "%s" DIR_SEP "%s",
> + psz_datadir, psz_tmp ) == -1 )
> + {
> + free( psz_datadir );
> + free( psz_tmp );
> + msg_Err( p_ml, "asprintf failed (%m)" );
if asprintf failed that's not needed to print anything.
> + return VLC_ENOMEM;
> + }
> + free( psz_datadir );
> + free( psz_tmp );
> + }
> +/**
> + * @brief Create a new (empty) database. The database might be initialized
> + *
> + * @param p_ml This ML
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + * @note This function is transactional
> + */
> +int CreateEmptyDatabase( media_library_t *p_ml )
> +{
> + /* Emulating foreign keys with triggers */
> + /* Warning: Lots of SQL */
> +#ifdef SQLITE_MODULE
hum !! You can't be sure that the sql backend is the sqlite one with an
ifdef.
Maybe adding a function inside the sql backend and implementing it in
the sqlite module ?
> +/**
> + * @brief Initiates database (create the database and the tables if needed)
> + *
> + * @param p_ml This ML
> + * @return VLC_SUCCESS or an error code
> + */
> +int InitDatabase( media_library_t *p_ml )
> +{
> +#if ML_DBVERSION != 1
> +#error "ML versioning code needs to be updated. Is this done correctly?"
> +#endif
??
> +/**
> + * @brief Copy an input_item_t to a ml_media_t
> + * @param p_media Destination
> + * @param p_item Source
> + * @note Media ID will not be set! This function is threadsafe. Leaves
> + * unsyncable items alone
> + */
> +void CopyInputItemToMedia( ml_media_t *p_media, input_item_t *p_item )
> +{
> + ml_LockMedia( p_media );
> +#if 0
> + // unused meta :
> + input_item_GetCopyright( item )
> + input_item_GetRating( item ) /* TODO */
> + input_item_GetGetting( item )
> + input_item_GetNowPlaying( item )
> + input_item_GetTrackID( item )
> + input_item_GetSetting( item )
> +#endif
> + p_media->psz_title = input_item_GetTitle ( p_item );
> + p_media->psz_uri = input_item_GetURL ( p_item );
> + if( !p_media->psz_uri )
> + p_media->psz_uri = strdup( p_item->psz_uri );
> + p_media->psz_album = input_item_GetAlbum ( p_item );
> + p_media->psz_cover = input_item_GetArtURL ( p_item );
> + p_media->psz_genre = input_item_GetGenre ( p_item );
> + p_media->psz_language = input_item_GetLanguage ( p_item );
> + p_media->psz_comment = input_item_GetDescription ( p_item );
> + char *psz_track = input_item_GetTrackNum ( p_item );
> + p_media->i_track_number = psz_track ? atoi( psz_track ) : 0;
> + free( psz_track );
> + char *psz_date = input_item_GetDate( p_item );
> + p_media->i_year = psz_date ? atoi( psz_date ) : 0;
> + free( psz_date );
> + p_media->i_duration = p_item->i_duration;
> +
> + /* People */
> + char *psz_tmp = input_item_GetArtist( p_item );
> + if( psz_tmp )
> + ml_CreateAppendPersonAdv( &p_media->p_people, ML_PERSON_ARTIST,
> + psz_tmp, 0 );
> + FREENULL( psz_tmp );
FREENULL isn't needed, free is enough.
> + psz_tmp = input_item_GetPublisher( p_item );
> + if( psz_tmp )
> + ml_CreateAppendPersonAdv( &p_media->p_people, ML_PERSON_PUBLISHER,
> + psz_tmp, 0 );
> + FREENULL( psz_tmp );
same
> + psz_tmp = input_item_GetEncodedBy( p_item );
> + if( psz_tmp )
> + ml_CreateAppendPersonAdv( &p_media->p_people, ML_PERSON_ENCODER,
> + psz_tmp, 0 );
> + FREENULL( psz_tmp );
same.
> +/**
> + * @brief Copy a ml_media_t to an input_item_t
> + * @param p_item Destination
> + * @param p_media Source
> + */
> +void CopyMediaToInputItem( input_item_t *p_item, ml_media_t *p_media )
> +{
> + vlc_gc_incref( p_item );
Dummy question: needed ?
> + vlc_gc_decref( p_item );
> +}
> diff --git a/modules/media_library/sql_media_library.h b/modules/media_library/sql_media_library.h
> +/*****************************************************************************
> + * sql_media_library.h : Media Library Interface
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +/*****************************************************************************
> + * Free result of ml_Query
> + *****************************************************************************/
> +static inline void FreeSQLResult( media_library_t *p_ml, char **ppsz_result )
> +{
> + if( ppsz_result )
> + {
> + vlc_mutex_lock( &p_ml->p_sys->lock );
> + sql_Free( p_ml->p_sys->p_sql, ppsz_result );
> + vlc_mutex_unlock( &p_ml->p_sys->lock );
do you need the lock to free the result of an sql query ?
> + }
> +}
> +
> diff --git a/modules/media_library/sql_monitor.c b/modules/media_library/sql_monitor.c
> +/*****************************************************************************
> + * sql_monitor.c: SQL-based media library: directory scanning and monitoring
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +/**
> + * @brief Directory Monitoring thread loop
> + */
> +void *RunMonitoringThread( vlc_object_t *p_this )
> +{
> + monitoring_thread_t *p_mon = (monitoring_thread_t*) p_this;
> + vlc_cond_init( &p_mon->wait );
> + vlc_mutex_init( &p_mon->lock );
Potential race condition here ? A caller using the lock before the call
to vlc_mutex_init ?
It depends on how you launch the thread.
> +
> + while( vlc_object_alive( p_mon ) )
> + {
> + vlc_mutex_lock( &p_mon->lock );
> +
> + /* Update */
> + UpdateLibrary( p_mon );
> +
> + if( !vlc_object_alive( p_mon ) )
you leave the while loop with the lock taken => crash when destroying
it.
> + break;
> +
> + /* We wait MONITORING_DELAY seconds or wait that the media library
> + signals us to do something */
> + vlc_cond_timedwait( &p_mon->wait, &p_mon->lock,
> + mdate() + 1000000*MONITORING_DELAY );
> +
> + vlc_mutex_unlock( &p_mon->lock );
> + }
> + vlc_cond_destroy( &p_mon->wait );
> + vlc_mutex_destroy( &p_mon->lock );
> + return NULL;
> +}
> +/**
> + * @brief Update library if new files found or updated
> + */
> +static void UpdateLibrary( monitoring_thread_t *p_mon )
> +{
> + int i_rows, i_cols, i;
> + char **pp_results;
> + media_library_t *p_ml = p_mon->p_ml;
> +
> + struct stat s_stat;
> +
> + bool b_recursive = var_CreateGetBool( p_mon, "ml-recursive-scan" );
If possible, create the variable only on time and then call var_GetBool.
(Here you can create the varaible in RunMonitoringThread for example).
> diff --git a/modules/media_library/sql_search.c b/modules/media_library/sql_search.c
> +/*****************************************************************************
> + * sql_search.c: SQL-based media library: all find/get functions
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN team
> + * $Id$
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +/**
> + * @brief Append a string and format it using SQL vmprintf
> + **/
> +static int AppendStringFmtVa( media_library_t *p_ml,
> + char **ppsz_dst, const char *psz_fmt,
> + va_list args )
> +{
> + char *psz_tmp = NULL, *psz_fullexp = NULL;
> +
> + assert( ppsz_dst != NULL );
> +
> + if( !( *ppsz_dst ) )
> + {
> + /* New expression */
> + *ppsz_dst = sql_VPrintf( p_ml->p_sys->p_sql, psz_fmt, args );
> + if( !( *ppsz_dst ) )
> + return VLC_ENOMEM;
> + }
> + else
> + {
> + /* Create new expression B */
> + psz_tmp = sql_VPrintf( p_ml->p_sys->p_sql, psz_fmt, args );
> + if( !psz_tmp )
> + return VLC_ENOMEM;
> +
> + /* Allocate new string buffer */
> + int i_full_length = strlen( psz_tmp ) + 1;
> + i_full_length += strlen( *ppsz_dst );
> +
> + psz_fullexp = malloc( i_full_length );
> + if( !psz_fullexp )
> + return VLC_ENOMEM;
> +
> + /* Concatene strings: "AB" */
> + strcpy( psz_fullexp, *ppsz_dst );
> + strcat( psz_fullexp, psz_tmp );
asprintf maybe ?
You might miss an undef CASE_* before.
> +#undef CASE_INT
> +#define CASE_INT( casestr, fmt, table ) \
> diff --git a/modules/media_library/sql_update.c b/modules/media_library/sql_update.c
> +/*****************************************************************************
> + * sql_update.c: SQL-based media library: all database update functions
> + *****************************************************************************
> + * Copyright (C) 2008-2009 the VideoLAN team
> + *Id: d122346ecab5359e81067cb4cedef34be8f6977f
> + *
> + * Authors: Antoine Lejeune <phytos at videolan.org>
> + * Jean-Philippe Andr?? <jpeg at videolan.org>
> + * R??mi Duraffort <ivoire at videolan.org>
> + * Adrien Maglo <magsoft at videolan.org>
'é'
> +#define SET_STR( a ) \
> +if( !psz_set[i_type] ) \
> +{ \
> + psz_set[i_type] = sql_Printf( p_ml->p_sys->p_sql, a, find->value.str ); \
> + if( !psz_set[i_type] ) \
> + goto quit_buildupdate; \
> +} \
> +break;
AFAIK the #undef SET_STR is missing (when you no longer need it).
> +/**
> + * @brief Generic UPDATE query builder
> + *
> + * @param p_ml This media_library_t object
> + * @param ppsz_query *ppsz_query will contain query for update
> + * @param ppsz_id_query will contain query to get the ids of updated files
> + * @param selected_type the type of the element we're selecting
> + * @param where parse tree of where condition
> + * @param changes list of changes to make in the entries
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + */
> +int BuildUpdate( media_library_t *p_ml,
> + char **ppsz_query, char **ppsz_id_query,
> + const char *psz_lvalue,
> + ml_select_e selected_type,
> + ml_ftree_t *where, vlc_array_t *changes )
> +{
this function is really too long. If you can split it in some logical
sub functions ?
> + for( unsigned i = 0; i <= ML_DIRECTORY; i++ )
> + {
> + if( psz_set[i] )
> + {
> + if( i == ML_EXTRA || i == ML_LANGUAGE )
> + {
> + FREENULL( psz_tmp );
free is enough.
> + if( asprintf( &psz_tmp, "%s%s%s", psz_extra ? psz_extra : "",
> + psz_extra ? ", ": "", psz_set[i] ) == -1 )
> + goto quit_buildupdate;
> + free( psz_extra );
> + psz_extra = strdup( psz_tmp );
> + }
> + else
> + {
> + FREENULL( psz_tmp );
same
> + if( asprintf( &psz_tmp, "%s%s%s", psz_fullset ? psz_fullset : "",
> + psz_fullset ? ", ": "", psz_set[i] ) == -1 )
> + goto quit_buildupdate;
> + free( psz_fullset );
> + psz_fullset = strdup( psz_tmp );
> + }
> + }
> + }
> + i_ret = VLC_SUCCESS;
> +}
Last comment: could you split this patch in at least two because that's
really difficult to review such a bug patch.
Thanks for your work.
--
Rémi Duraffort | ivoire
More information about the vlc-devel
mailing list