[vlc-devel] [PATCH 1/4] Base Media Library Module files
Jean-Baptiste Kempf
jb at videolan.org
Mon Oct 18 08:48:32 CEST 2010
On Mon, Oct 18, 2010 at 01:09:21AM +0530, Srikanth Raju wrote :
> +#define EXTENSIONS_AUDIO_CSV "a52", "aac", "ac3", "ape", "dts", "flac", "it", \
> + "m4a", "m4p", "mka", "mlp", "mod", "mp1", "mp2", "mp3",\
> + "oga", "ogg", "oma", "s3m", "spx" \
> + "wav", "wma", "wv", "xm"
> +
> +#define EXTENSIONS_VIDEO_CSV "asf", "avi", "divx", "dv", "flv", "gxf", "iso", \
> + "m1v", "m2v", "m2t", "m2ts", "m4v", "mkv", "mov",\
> + "mp2", "mp4", "mpeg", "mpeg1", \
> + "mpeg2", "mpeg4", "mpg", "mts", "mxf", "nuv", \
> + "ogg", "ogm", "ogv", "ogx", "ps", \
> + "rec", "rm", "rmvb", "ts", "vob", "wmv"
Why again a list of extensions?
Why in #define too?
> + set_description( _("Media Library based on a SQL database") )
SQL or SQLite?
> + add_string( "ml-filename", "media-library.db", NULL,
> + MEDIA_LIBRARY_PATH_TEXT, MEDIA_LIBRARY_PATH_LONGTEXT, false )
Bad name. What about VLC-media-library? Why .db?
> + add_string( "ml-username", "", NULL,
> + "Username for the database", "Username for the database", false )
> + add_string( "ml-password", "", NULL,
> + "Password for the database", "Password for the database", false )
> + add_integer( "ml-port", 0, NULL,
> + "Port for the database", "Port for the database", false )
Are those strings marked as translatable?
> + /* TODO: The above is all fine as long as the DB is sqlite. */
> + add_bool( "ml-recursive-scan", true, NULL, RECURSIVE_TEXT,
> + RECURSIVE_LONGTEXT, false )
Why do we need that one?
> + add_bool( "ml-auto-add", true, NULL, "Auto add new medias",
> + "Automatically add new medias to ML", false )
idem as above (strings)
> + /* Launching the directory monitoring thread */
> + monitoring_thread_t *p_mon =
> + vlc_object_create( p_ml, sizeof( monitoring_thread_t ) );
> + if( !p_mon )
> + {
> + vlc_mutex_destroy( &p_ml->p_sys->lock );
> + sql_Destroy( p_ml->p_sys->p_sql );
> + free( p_ml->p_sys );
> + msg_Err( p_ml, "out of memory" );
> + return VLC_EGENERIC;
If you are out of memory, msg_Err will fail and you should return
VLC_ENOMEM
> + 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_mutex_destroy( &p_ml->p_sys->lock );
> + sql_Destroy( p_ml->p_sys->p_sql );
> + free( p_ml->p_sys );
> + vlc_object_release( p_mon );
> + return VLC_EGENERIC;
> + }
Common error path with above, maybe?
> + msg_Dbg( p_ml, "module loaded" );
Please make it more specific.
> + /* Media */
> + IF_CMP( "directory_id", ML_DIRECTORY )
> + ELSE_CMP( "cover", ML_COVER )
> + ELSE_CMP( "comment", ML_COMMENT )
> + ELSE_CMP( "preview", ML_PREVIEW )
> + ELSE_CMP( "duration", ML_DURATION )
> + ELSE_CMP( "genre", ML_GENRE )
> + ELSE_CMP( "id", ML_ID )
> + ELSE_CMP( "first_played", ML_FIRST_PLAYED )
> + ELSE_CMP( "last_played", ML_LAST_PLAYED )
> + ELSE_CMP( "played_count", ML_PLAYED_COUNT )
> + ELSE_CMP( "skipped_count", ML_SKIPPED_COUNT )
> + ELSE_CMP( "last_skipped", ML_LAST_SKIPPED )
> + ELSE_CMP( "import_time", ML_IMPORT_TIME )
> + ELSE_CMP( "score", ML_SCORE )
> + ELSE_CMP( "title", ML_TITLE )
> + ELSE_CMP( "original_title", ML_ORIGINAL_TITLE )
> + ELSE_CMP( "track", ML_TRACK_NUMBER )
> + ELSE_CMP( "disc", ML_DISC_NUMBER )
> + ELSE_CMP( "type", ML_TYPE )
> + ELSE_CMP( "uri", ML_URI )
> + ELSE_CMP( "vote", ML_VOTE )
> + ELSE_CMP( "year", ML_YEAR )
> + ELSE_CMP( "filesize", ML_FILESIZE )
> + ELSE_CMP( "directory_id", ML_DIRECTORY )
> + /* Album */
> + ELSE_CMP( "album_title", ML_ALBUM )
> + ELSE_CMP( "album_id", ML_ALBUM_ID )
> + ELSE_CMP( "album_cover", ML_ALBUM_COVER )
> + /* People */
> + ELSE_CMP( "people_name", ML_PEOPLE )
> + ELSE_CMP( "people_id", ML_PEOPLE_ID )
> + ELSE_CMP( "people_role", ML_PEOPLE_ROLE )
> + /* Extras */
> + ELSE_CMP( "language", ML_LANGUAGE )
> + ELSE_CMP( "extra", ML_EXTRA )
> + else
> + {
> + msg_Warn( p_ml, "unknown column: %s", res( 0, col ) );
> + }
Missing episode, season
> + switch( indexes[ col ] )
> + {
> + case ML_ALBUM_ID:
> + p_media->i_album_id = atoinull( res( row, col ) );
> + break;
> + case ML_ALBUM:
> + p_media->psz_album = strdupnull( res( row, col ) );
> + break;
> + case ML_ALBUM_COVER:
> + /* See ML_COVER */
> + // Discard attachment://
> + if( !p_media->psz_cover || !*p_media->psz_cover
> + || !strncmp( p_media->psz_cover, "attachment://", 13 ) )
> + {
> + free( p_media->psz_cover );
> + p_media->psz_cover = strdupnull( res( row, col ) );
> + }
> + break;
> + case ML_PEOPLE:
> + psz_append_pname = strdupnull( res( row, col ) );
> + break;
> + case ML_PEOPLE_ID:
> + i_append_pid = atoinull( res( row, col ) );
> + break;
> + case ML_PEOPLE_ROLE:
> + psz_append_prole = strdupnull( res( row, col ) );
> + break;
> + case ML_COMMENT:
> + p_media->psz_comment = strdupnull( res( row, col ) );
> + break;
> + case ML_COVER:
> + /* See ML_ALBUM_COVER */
> + if( !p_media->psz_cover || !*p_media->psz_cover
> + || !strncmp( p_media->psz_cover, "attachment://", 13 ) )
> + {
> + free( p_media->psz_cover );
> + p_media->psz_cover = strdupnull( res( row, col ) );
> + }
> + break;
> + case ML_DISC_NUMBER:
> + p_media->i_disc_number = atoinull( res( row, col ) );
> + break;
> + case ML_DURATION:
> + p_media->i_duration = atoinull( res( row, col ) );
> + break;
> + case ML_EXTRA:
> + p_media->psz_extra = strdupnull( res( row, col ) );
> + break;
> + case ML_FILESIZE:
> + p_media->i_filesize = atoinull( res( row, col ) );
> + break;
> + case ML_FIRST_PLAYED:
> + p_media->i_first_played = atoinull( res( row, col ) );
> + break;
> + case ML_GENRE:
> + p_media->psz_genre = strdupnull( res( row, col ) );
> + break;
> + case ML_ID:
> + p_media->i_id = atoinull( res( row, col ) );
> + if( p_media->i_id <= 0 )
> + {
> + msg_Warn( p_ml, "entry with id null or inferior to zero" );
> + }
> + break;
> + case ML_IMPORT_TIME:
> + p_media->i_import_time = atoinull( res( row, col ) );
> + break;
> + case ML_LANGUAGE:
> + p_media->psz_language = strdupnull( res( row, col ) );
> + break;
> + case ML_LAST_PLAYED:
> + p_media->i_last_played = atoinull( res( row, col ) );
> + break;
> + case ML_LAST_SKIPPED:
> + p_media->i_last_skipped = atoinull( res( row, col ) );
> + break;
> + case ML_ORIGINAL_TITLE:
> + p_media->psz_orig_title = strdupnull( res( row, col ) );
> + break;
> + case ML_PLAYED_COUNT:
> + p_media->i_played_count = atoinull( res( row, col ) );
> + break;
> + case ML_PREVIEW:
> + p_media->psz_preview = strdupnull( res( row, col ) );
> + break;
> + case ML_SCORE:
> + p_media->i_score = atoinull( res( row, col ) );
> + break;
> + case ML_SKIPPED_COUNT:
> + p_media->i_skipped_count = atoinull( res ( row, col ) );
> + break;
> + case ML_TITLE:
> + p_media->psz_title = strdupnull( res( row, col ) );
> + break;
> + case ML_TRACK_NUMBER:
> + p_media->i_track_number = atoinull( res( row, col ) );
> + break;
> + case ML_TYPE:
> + p_media->i_type = atoinull( res( row, col ) );
> + break;
> + case ML_URI:
> + p_media->psz_uri = strdupnull( res( row, col ) );
> + if( !p_media->psz_uri )
> + {
> + msg_Warn( p_ml, "entry without uri" );
> + }
> + break;
> + case ML_VOTE:
> + p_media->i_vote = atoinull( res( row, col ) );
> + break;
> + case ML_YEAR:
> + p_media->i_year = atoinull( res( row, col ) );
> + break;
> + case ML_DIRECTORY:
> + break; // The column directory_id is'nt part of the media model
> + default:
> + msg_Warn( p_ml, "unknown element, row %d column %d (of %d) - %s - %s",
> + row, col, i_cols, res( 0 , col ), res( row, col ) );
> + break;
> + }
Can't this ^^ be factorized?
> +int CreateEmptyDatabase( media_library_t *p_ml )
I would argue that this needs to be in a separate file.
> +int Begin( media_library_t* p_ml )
inline?
> +void Commit( media_library_t* p_ml )
inline?
> +void Rollback( media_library_t* p_ml )
inline?
> +struct media_library_sys_t
please reorder to pack it better (hint: bool at the end)
> +int GetIdOfInputItem( media_library_t *p_ml,
> + input_item_t *p_item );
> +int GetIdOfURI( media_library_t *p_ml,
> + const char *psz_uri );
Too vague. What is this id?
> +int SetAlbumCover( media_library_t *p_ml,
> + int i_album_id,
> + const char *psz_cover );
SetArtCover, there are other things than albums out there...
I would argue that we are missing a few infos about video, compared to
audio here... Which is a bit ironic, compared to what VLC is. :)
But, this could be done in a second time.
Best Regards,
--
Jean-Baptiste Kempf
http://www.jbkempf.com/
+33 672 704 734
More information about the vlc-devel
mailing list