[vlc-devel] [PATCH] Media library module for VLC, GSoC 09

[Srikanth Raju]

Hello,

>> + *          Jean-Philippe Andr?? <jpeg at videolan.org>
>> + *          R??mi Duraffort <ivoire at videolan.org>
>> + *          Adrien Maglo <magsoft at videolan.org>
> encoding ? 'é' and not '??'.
>
Ok, I'm not sure what I'm doing wrong here at all. It seems like the encoding is
correct on my system.

>> +/**
>> + * @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)

They would be ignored. This is just a "middle" function. ml_GetInt and
ml_GetString
do the work. And they use LIMIT 1

>
>> +/**
>> + * @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 ?

It specifies whether the play count needs to be updated or not.
Documented in item_list.h

>
>> +/**
>> + * @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 ?
>

Yes. Because the watch system is closed first and it is not the only
thread that can access the
list. Besides, it's good practice!

>
>> +/**
>> + * @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 ?

Hmm, yes. But the function is also called rarely. Only when the
playlist changes.
At any rate, I shall move it.

>> +}
>
>> +/**
>> + * @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 ?

UpdateMedia is threadsafe and does its own locking. Perhaps I will
need to implement
a __UpdateMedia( p_ml, p_media, islocked )?
But it's perfectly OK if media changes between the locks.

>> +    /* 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 ?
>

These variables act as an event system for the Media library. And aren't for
configuration. Inherit variables inherit the values from the parent
object which we
don't care about.


>> +/**
>> + * @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 ?
>

I don't see why that is a problem! The memory that is freed has nothing to do
with the monitor object.

>> +
>> +        /* 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 ?
>

Not really. Good practice, though?
>
>
>> +/**
>> + * @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 ?
>

If I'm checking module_get_name is "SQLite", is that good enough?

>
>> +/**
>> + * @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
> ??
>

Everytime DB changes, ML_DBVERSION changes too. Then these defines
reminds the coder to update the versioning for the database. That is
what to do if the user had DB version 1, then he updates, get version 2.
VLC must do the changes on the schema from version 1 to 2. :D

>
>> +/**
>> + * @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.

I don't understand. Which caller is using the lock?

>> +
>> +    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;
>> +
Oops. Removed these two lines.

>> +        /* 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;
>> +}
>
>
> Last comment: could you split this patch in at least two because that's
> really difficult to review such a bug patch.
>
Most of the other changes have been incorporated.

>
> Thanks for your work.
>
> --
> Rémi Duraffort | ivoire

-- 
Regards,
Srikanth Raju