[vlc-devel] [PATCH] Add new API to libvlc for persistent equalizer settings.

[Rémi Denis-Courmont]

Le lundi 20 août 2012 23:10:44 Mark Lee, vous avez écrit :
> diff --git a/lib/media_player.c b/lib/media_player.c
> index a41b8c7..e14558e 100644
> --- a/lib/media_player.c
> +++ b/lib/media_player.c
> @@ -34,11 +34,14 @@
>  #include <vlc_input.h>
>  #include <vlc_vout.h>
>  #include <vlc_keys.h>
> +#include <vlc_aout_intf.h>
> 
>  #include "libvlc_internal.h"
>  #include "media_internal.h" // libvlc_media_set_state()
>  #include "media_player_internal.h"
> 
> +#include "modules/audio_filter/equalizer_presets.h"
> +
>  /*
>   * mapping of libvlc_navigate_mode_t to vlc_action_t
>   */
> @@ -482,6 +485,9 @@ libvlc_media_player_new( libvlc_instance_t *instance )
>      var_Create (mp, "amem-rate", VLC_VAR_INTEGER | VLC_VAR_DOINHERIT);
>      var_Create (mp, "amem-channels", VLC_VAR_INTEGER | VLC_VAR_DOINHERIT);
> 
> +    var_Create (mp, "equalizer-preamp", VLC_VAR_FLOAT);
> +    var_Create (mp, "equalizer-bands", VLC_VAR_STRING);
> +
>      mp->p_md = NULL;
>      mp->state = libvlc_NothingSpecial;
>      mp->p_libvlc_instance = instance;
> @@ -1409,3 +1415,56 @@ void libvlc_media_player_next_frame(
> libvlc_media_player_t *p_mi ) vlc_object_release( p_input_thread );
>      }
>  }
> +
> +int libvlc_media_player_set_equalizer( libvlc_media_player_t *p_mi,
> libvlc_equalizer_t *p_equalizer )
> +{
> +    lock( p_mi );
> +
> +    if ( !p_equalizer )
> +    {
> +        p_mi->p_equalizer = NULL;
> +
> +        unlock( p_mi );
> +
> +        aout_EnableFilter( p_mi, "equalizer", false );
> +        return 0;
> +    }
> +
> +    p_mi->p_equalizer = p_equalizer;
> +
> +    unlock( p_mi );
> +
> +    aout_EnableFilter( p_mi, "equalizer", true );

The three lines above could be factored in both cases.

But see below...

> +
> +    var_SetFloat( p_mi, "equalizer-preamp", p_equalizer->f_preamp );
> +
> +    char *psz_bands = NULL;
> +    for ( int i = 0; i < EQZ_BANDS_MAX; i++ )
> +    {
> +        char *psz;
> +        if ( asprintf( &psz, "%s %.07f", psz_bands ? psz_bands : "",
> p_equalizer->f_amp[i] ) == -1 ) +        {
> +            free( psz_bands );
> +            return -1;
> +        }
> +        free( psz_bands );
> +        psz_bands = psz;
> +    }
> +    var_SetString( p_mi, "equalizer-bands", psz_bands );
> +    free( psz_bands );
> +
> +    return 0;
> +}
> +
> +libvlc_equalizer_t *libvlc_media_player_get_equalizer(
> libvlc_media_player_t *p_mi )
> +{
> +    libvlc_equalizer_t *p_equalizer;
> +
> +    lock( p_mi );
> +
> +    p_equalizer = p_mi->p_equalizer;
> +
> +    unlock( p_mi );
> +
> +    return p_equalizer;
> +}

A lock that only reads a pointer. This is alarmingly suspicious as regards 
life cycle.

I may be missing something, but I don't see why that pointer is needed inside 
the media player anyway.

-- 
Rémi Denis-Courmont
C/C++ software engineer looking for a job
http://www.linkedin.com/in/remidenis