[vlc-devel] Fwd: [PATCH] Do not use album art if the file has a built-in cover.

[Samo Golež]

On 26. 01. 2021 11:53, Thomas Guillem wrote:
> Hello,
>
> Sorry for the very long delay.
>
> On Tue, Jan 12, 2021, at 19:17, Samo Golež wrote:
>> Currently, VLC uses the same image if the files are from the same
>> album, but with this PATCH, if the file has a built-in image, it will
>> be used instead of the album image.
>> ---
>> src/input/meta.c | 2 ++
>> src/preparser/art.c | 54 ++++++++++++++++++++++++++++++++++-------
>> src/preparser/art.h | 1 +
>> src/preparser/fetcher.c | 38 +++++++++++++++++++++--------
>> src/preparser/fetcher.h | 3 +++
>> 5 files changed, 79 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/input/meta.c b/src/input/meta.c
>> index 9ef43ca..ba7a97b 100644
>> --- a/src/input/meta.c
>> +++ b/src/input/meta.c
>> @@ -212,6 +212,8 @@ void input_ExtractAttachmentAndCacheArt(
>> input_thread_t *p_input,
>> msg_Warn( p_input, "art already fetched" );
>> if( likely(input_FindArtInCache( p_item ) == VLC_SUCCESS) )
>> return;
>> + if( likely(input_FindAlbumArtInCache( p_item ) == VLC_SUCCESS)
>> )
>> + return;
>> }
>> /* */
>> diff --git a/src/preparser/art.c b/src/preparser/art.c
>> index 9fa9e66..2256b04 100644
>> --- a/src/preparser/art.c
>> +++ b/src/preparser/art.c
>> @@ -56,12 +56,12 @@ static void ArtCacheCreateDir( char *psz_dir )
>> static char* ArtCacheGetDirPath( const char *psz_arturl, const char
>> *psz_artist,
>> const char *psz_album, const char
>> *psz_date,
>> - const char *psz_title )
>> + const char *psz_title, bool b_unique
>> )
>> {
>> char *psz_dir;
>> char *psz_cachedir = config_GetUserDir(VLC_CACHE_DIR);
>> - if( !EMPTY_STR(psz_artist) && !EMPTY_STR(psz_album) )
>> + if( !EMPTY_STR(psz_artist) && !EMPTY_STR(psz_album) && !b_unique )
>> {
>> char *psz_album_sanitized = strdup( psz_album );
>> if (!psz_album_sanitized)
>> @@ -108,6 +108,8 @@ static char* ArtCacheGetDirPath( const char
>> *psz_arturl, const char *psz_artist,
>> vlc_hash_md5_t md5;
>> vlc_hash_md5_Init( &md5 );
>> + if (!psz_arturl)
>> + return NULL;
>> vlc_hash_md5_Update( &md5, psz_arturl, strlen( psz_arturl ) );
>> if( !strncmp( psz_arturl, "attachment://", 13 ) )
>> vlc_hash_md5_Update( &md5, psz_title, strlen( psz_title )
>> );
>> @@ -120,7 +122,7 @@ static char* ArtCacheGetDirPath( const char
>> *psz_arturl, const char *psz_artist,
>> return psz_dir;
>> }
>> -static char *ArtCachePath( input_item_t *p_item )
>> +static char *ArtCachePath( input_item_t *p_item, bool b_unique )
>> {
>> char* psz_path = NULL;
>> const char *psz_artist;
>> @@ -147,16 +149,16 @@ static char *ArtCachePath( input_item_t *p_item )
>> if( (EMPTY_STR(psz_artist) || EMPTY_STR(psz_album) ) && !psz_arturl )
>> goto end;
>> - psz_path = ArtCacheGetDirPath( psz_arturl, psz_artist, psz_album,
>> psz_date, psz_title );
>> + psz_path = ArtCacheGetDirPath( psz_arturl, psz_artist, psz_album,
>> psz_date, psz_title, b_unique );
>> end:
>> vlc_mutex_unlock( &p_item->lock );
>> return psz_path;
>> }
>> -static char *ArtCacheName( input_item_t *p_item, const char *psz_type )
>> +static char *ArtCacheName( input_item_t *p_item, const char *psz_type,
>> bool b_unique )
>> {
>> - char *psz_path = ArtCachePath( p_item );
>> + char *psz_path = ArtCachePath( p_item, b_unique );
>> char *psz_ext = strdup( psz_type ? psz_type : "" );
>> char *psz_filename = NULL;
>> @@ -177,9 +179,9 @@ end:
>> }
>> /* */
>> -int input_FindArtInCache( input_item_t *p_item )
>> +static int input_FindInCache( input_item_t *p_item, bool b_unique )
>> {
>> - char *psz_path = ArtCachePath( p_item );
>> + char *psz_path = ArtCachePath( p_item, b_unique );
>> if( !psz_path )
>> return VLC_EGENERIC;
>> @@ -221,6 +223,18 @@ int input_FindArtInCache( input_item_t *p_item )
>> return b_found ? VLC_SUCCESS : VLC_EGENERIC;
>> }
>> +/* Find find file art */
>> +int input_FindArtInCache( input_item_t *p_item )
>> +{
>> + return input_FindInCache(p_item, true);
>> +}
>> +
>> +/* Find album art */
>> +int input_FindAlbumArtInCache( input_item_t *p_item )
>> +{
>> + return input_FindInCache(p_item, false);
>> +}
>> +
>> static char * GetDirByItemUIDs( char *psz_uid )
>> {
>> char *psz_cachedir = config_GetUserDir(VLC_CACHE_DIR);
>> @@ -286,7 +300,29 @@ int input_FindArtInCacheUsingItemUID( input_item_t
>> *p_item )
>> int input_SaveArt( vlc_object_t *obj, input_item_t *p_item,
>> const void *data, size_t length, const char
>> *psz_type )
>> {
>> - char *psz_filename = ArtCacheName( p_item, psz_type );
>> + // save album
>> + char *psz_album_filename = ArtCacheName( p_item, psz_type, false );
> So, will this result on saving embedded covers for every file 
> belonging to one album ?
> These covers will be likely the same, and we have already seen users 
> having HUGE embedded cover.


This will result saving embedded covers for every file that has own 
cover built-in + one cover for album. The reason for this is that we 
already save built-in cover if we do not detect album.

The other option I was thinking was to use built-in cover directly for 
display and then we could remove saving covers for files that has 
unknown album.

>
>> +
>> + struct stat s_album;
>> + if( psz_album_filename && vlc_stat( psz_album_filename, &s_album )
>> )
>> + {
>> + FILE *f = vlc_fopen( psz_album_filename, "wb" );
>> + if( f )
>> + {
>> + if( fwrite( data, 1, length, f ) != length )
>> + {
>> + msg_Err( obj, "%s: %s", psz_album_filename,
>> vlc_strerror_c(errno) );
>> + }
>> + else
>> + {
>> + msg_Dbg( obj, "album art saved to %s",
>> psz_album_filename );
>> + }
>> + fclose( f );
>> + }
> Could you refactor this part? Add a new function that check if the 
> file exist and write the data.
Will do.
>
>> + }
>> +
>> + // now save unique
>> + char *psz_filename = ArtCacheName( p_item, psz_type, true );
>> if( !psz_filename )
>> return VLC_EGENERIC;
>> diff --git a/src/preparser/art.h b/src/preparser/art.h
>> index e4cb82f..8bb0cc2 100644
>> --- a/src/preparser/art.h
>> +++ b/src/preparser/art.h
>> @@ -25,6 +25,7 @@
>> #define _INPUT_ART_H 1
>> int input_FindArtInCache( input_item_t * );
>> +int input_FindAlbumArtInCache( input_item_t * );
>> int input_FindArtInCacheUsingItemUID( input_item_t * );
>> int input_SaveArt( vlc_object_t *, input_item_t *,
>> diff --git a/src/preparser/fetcher.c b/src/preparser/fetcher.c
>> index 7616078..88a8911 100644
>> --- a/src/preparser/fetcher.c
>> +++ b/src/preparser/fetcher.c
>> @@ -144,7 +144,7 @@ Submit(input_fetcher_t *fetcher, vlc_executor_t
>> *executor, input_item_t *item,
>> return VLC_SUCCESS;
>> }
>> -static char* CreateCacheKey( input_item_t* item )
>> +static char* CreateCacheKey( input_item_t* item , bool b_album )
>> {
>> vlc_mutex_lock( &item->lock );
>> @@ -169,6 +169,19 @@ static char* CreateCacheKey( input_item_t* item )
>> {
>> key = NULL;
>> }
>> +
>> + if (key && !b_album)
>> + {
>> + char const *arturl = vlc_meta_Get( item->p_meta, vlc_meta_ArtworkURL );
>> + char const *title = vlc_meta_Get( item->p_meta, vlc_meta_Title );
> Artist + Title is not enough? Why are you using ArtworkURL as a cache key?
As said before, this is the same mechanism as for generating 
non-detectable album art key.
>
>> +
>> + if( !arturl || !title || asprintf( &key, "%s:%zu:%s:%zu",
>> + arturl, strlen( arturl ), title, strlen( title ) ) < 0 )
>> + {
>> + key = NULL;
>> + }
>> + }
>> +
>> vlc_mutex_unlock( &item->lock );
>> return key;
>> @@ -180,9 +193,9 @@ static void FreeCacheEntry( void* data, void* obj )
>> VLC_UNUSED( obj );
>> }
>> -static int ReadAlbumCache( input_fetcher_t* fetcher, input_item_t*
>> item )
>> +static int ReadCache( input_fetcher_t* fetcher, input_item_t* item,
>> bool b_album )
>> {
>> - char* key = CreateCacheKey( item );
>> + char* key = CreateCacheKey( item, b_album );
>> if( key == NULL )
>> return VLC_EGENERIC;
>> @@ -198,11 +211,11 @@ static int ReadAlbumCache( input_fetcher_t*
>> fetcher, input_item_t* item )
>> return art ? VLC_SUCCESS : VLC_EGENERIC;
>> }
>> -static void AddAlbumCache( input_fetcher_t* fetcher, input_item_t* item,
>> - bool overwrite )
>> +static void AddCache( input_fetcher_t* fetcher, input_item_t* item,
>> + bool b_album, bool overwrite )
>> {
>> char* art = input_item_GetArtURL( item );
>> - char* key = CreateCacheKey( item );
>> + char* key = CreateCacheKey( item, b_album );
>> if( key && art && strncasecmp( art, "attachment://", 13 ) )
>> {
>> @@ -278,12 +291,15 @@ static int SearchByScope(struct task *task, int 
>> scope)
>> }
>> if( ! CheckArt( item ) ||
>> - ! ReadAlbumCache( fetcher, item ) ||
>> + ! ReadCache( fetcher, item, ALBUM ) ||
>> + ! ReadCache( fetcher, item, SONG ) ||
>> ! input_FindArtInCacheUsingItemUID( item ) ||
>> ! input_FindArtInCache( item ) ||
>> + ! input_FindAlbumArtInCache( item ) ||
>> ! SearchArt( fetcher, item, scope ) )
>> {
>> - AddAlbumCache( fetcher, task->item, false );
>> + AddCache( fetcher, task->item, SONG, false );
>> + AddCache( fetcher, task->item, ALBUM, false );
>> int ret = Submit(fetcher, fetcher->executor_downloader, item,
>> task->options, task->cbs, task->userdata);
>> if (ret == VLC_SUCCESS)
>> @@ -306,7 +322,8 @@ static void RunDownloader(void *userdata)
>> vlc_interrupt_set(&task->interrupt);
>> - ReadAlbumCache( fetcher, task->item );
>> + if ( !!ReadCache( fetcher, task->item, SONG ) )
> Compare directly with VLC_SUCCESS or VLC_EGENERIC please.
Will do.
>
>> + ReadCache( fetcher, task->item, ALBUM );
>> char *psz_arturl = input_item_GetArtURL( task->item );
>> if( !psz_arturl )
>> @@ -351,7 +368,8 @@ static void RunDownloader(void *userdata)
>> output_stream.length, NULL );
>> free( output_stream.ptr );
>> - AddAlbumCache( fetcher, task->item, true );
>> + AddCache( fetcher, task->item, SONG, true );
>> + AddCache( fetcher, task->item, ALBUM, true );
>> out:
>> vlc_interrupt_set(NULL);
>> diff --git a/src/preparser/fetcher.h b/src/preparser/fetcher.h
>> index 680386a..609f921 100644
>> --- a/src/preparser/fetcher.h
>> +++ b/src/preparser/fetcher.h
>> @@ -26,6 +26,9 @@
>> #include <vlc_input_item.h>
>> +#define ALBUM true
>> +#define SONG false
> You should use an enum.
I was thinking the same. Will do.
>
>> +
>> /**
>> * Fetcher opaque structure.
>> *
>> -- 2.27.0
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel