[vlc-devel] [PATCH 1/2] Add new API to libvlc for persistent equalizer settings.
Mark Lee
mark.lee at capricasoftware.co.uk
Sun Aug 12 11:07:57 CEST 2012
Hello,
Thanks for your comments...
On 12 August 2012 09:47, Rémi Denis-Courmont <remi at remlab.net> wrote:
> Le dimanche 12 août 2012 11:30:45 Mark Lee, vous avez écrit :
> > /** @} media_player */
> > diff --git a/lib/audio.c b/lib/audio.c
> > index 27d7cd4..8e7b959 100644
> > --- a/lib/audio.c
> > +++ b/lib/audio.c
> > @@ -42,6 +42,8 @@
> > #include "libvlc_internal.h"
> > #include "media_player_internal.h"
> >
> > +#include "modules/audio_filter/equalizer_presets.h"
> > +
> > /*
> > * Remember to release the returned audio_output_t since it is locked at
> > * the end of this function.
> > @@ -507,3 +509,153 @@ int libvlc_audio_set_delay( libvlc_media_player_t
> > *p_mi, int64_t i_delay ) }
> > return ret;
> > }
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_get_preset_count : Get the number of
> > equalizer presets +
> >
> **************************************************************************
> > ***/ +unsigned libvlc_audio_equalizer_get_preset_count( void )
> > +{
> > + return NB_PRESETS;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_get_preset_name : Get the name for a
> preset
> > +
> >
> **************************************************************************
> > ***/ +const char *libvlc_audio_equalizer_get_preset_name( int i_index )
> +{
> > + if ( i_index < 0 || i_index >= NB_PRESETS )
> > + return NULL;
>
> You should use unsigned for consistency with ..._count and to simplify.
>
> OK.
> > +
> > + return preset_list_text[ i_index ];
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_get_band_count : Get the number of
> > equalizer frequency bands +
> >
> **************************************************************************
> > ***/ +unsigned libvlc_audio_equalizer_get_band_count( void )
> > +{
> > + return EQZ_BANDS_MAX;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_get_band_frequency : Get the frequency
> for
> > a band +
> >
> **************************************************************************
> > ***/ +float libvlc_audio_equalizer_get_band_frequency( int i_index )
> > +{
> > + if ( i_index < 0 || i_index >= EQZ_BANDS_MAX )
> > + return -1;
> > +
> > + return f_vlc_frequency_table_10b[ i_index ];
> > +}
>
> Same here and later.
>
> OK.
> +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_new : Create a new audio equalizer with
> > zeroed values +
> >
> **************************************************************************
> > ***/ +libvlc_equalizer_t *libvlc_audio_equalizer_new( void )
> > +{
> > + libvlc_equalizer_t *p_equalizer;
> > +
> > + p_equalizer = malloc( sizeof( *p_equalizer ) );
> > + if ( p_equalizer == NULL )
> > + return NULL;
> > +
> > + p_equalizer->f_preamp = 0.0;
> > +
> > + for ( int i = 0; i < EQZ_BANDS_MAX; i++ )
> > + p_equalizer->f_amp[ i ] = 0.0;
> > +
> > + return p_equalizer;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_new_from_preset : Create a new audio
> > equalizer based on a preset +
> >
> **************************************************************************
> > ***/ +libvlc_equalizer_t *libvlc_audio_equalizer_new_from_preset( int
> > i_index ) +{
> > + libvlc_equalizer_t *p_equalizer;
> > +
> > + if ( i_index < 0 || i_index >= NB_PRESETS )
> > + return NULL;
> > +
> > + p_equalizer = libvlc_audio_equalizer_new();
> > + if ( !p_equalizer )
> > + return NULL;
> > +
> > + p_equalizer->f_preamp = eqz_preset_10b[ i_index ].f_preamp;
> > +
> > + for ( int i = 0; i < EQZ_BANDS_MAX; i++ )
> > + p_equalizer->f_amp[ i ] = eqz_preset_10b[ i_index ].f_amp[ i ];
> > +
> > + return p_equalizer;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_release : Release a previously created
> > equalizer +
> >
> **************************************************************************
> > ***/ +void libvlc_audio_equalizer_release( libvlc_equalizer_t
> *p_equalizer
> > ) +{
> > + if ( p_equalizer )
> > + free( p_equalizer );
>
> if() is not needed. Refer to ISO C.
>
> OK.
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_set_preamp : Set the preamp value for an
> > equalizer +
> >
> **************************************************************************
> > ***/ +int libvlc_audio_equalizer_set_preamp( libvlc_equalizer_t
> > *p_equalizer, float f_preamp ) +{
> > + if ( !p_equalizer )
> > + return -1;
> > +
> > + if ( f_preamp < -20.0 )
>
> Use 'f' postfix to avoid conversions to/from double.
>
> OK.
> > + f_preamp = -20.0;
> > + else if ( f_preamp > 20.0 )
> > + f_preamp = 20.0;
> > +
> > + p_equalizer->f_preamp = f_preamp;
> > + return 0;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_get_preamp : Get the preamp value for an
> > equalizer +
> >
> **************************************************************************
> > ***/ +float libvlc_audio_equalizer_get_preamp( libvlc_equalizer_t
> > *p_equalizer ) +{
> > + if ( !p_equalizer )
> > + return 0;
>
> 0.f would be more consistent.
>
> OK.
> > +
> > + return p_equalizer->f_preamp;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_set_amp_at_index : Set the amplification
> > value for an equalizer band +
> >
> **************************************************************************
> > ***/ +int libvlc_audio_equalizer_set_amp_at_index( libvlc_equalizer_t
> > *p_equalizer, float f_amp, int i_band ) +{
> > + if ( !p_equalizer )
> > + return -1;
> > +
> > + if ( i_band < 0 || i_band >= EQZ_BANDS_MAX )
> > + return -1;
> > +
> > + if ( f_amp < -20.0 )
> > + f_amp = -20.0;
> > + else if ( f_amp > 20.0 )
> > + f_amp = 20.0;
> > +
> > + p_equalizer->f_amp[ i_band ] = f_amp;
> > + return 0;
> > +}
> > +
> >
> +/*************************************************************************
> > **** + * libvlc_audio_equalizer_get_amp_at_index : Get the amplification
> > value for an equalizer band +
> >
> **************************************************************************
> > ***/ +float libvlc_audio_equalizer_get_amp_at_index( libvlc_equalizer_t
> > *p_equalizer, int i_band ) +{
> > + if ( !p_equalizer )
> > + return 0;
> > +
> > + if ( i_band < 0 || i_band >= EQZ_BANDS_MAX )
> > + return 0;
> > +
> > + return p_equalizer->f_amp[ i_band ];
> > +}
>
> > diff --git a/lib/media_player.c b/lib/media_player.c
> > index a41b8c7..f2cb46d 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,49 @@ 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 )
> > +{
> > + if ( !p_mi )
> > + return -1;
>
> Is this useful to check in practice?
>
> I suppose not. It's not done everywhere else so I should remove it.
> > +
> > + p_mi->p_equalizer = p_equalizer;
> > +
> > + if ( p_equalizer )
> > + {
> > + aout_EnableFilter( p_mi, "equalizer", true );
> > +
> > + var_SetFloat( p_mi, "equalizer-preamp", p_equalizer->f_preamp );
> > +
> > + // This implementation slightly adapted from
> > modules/audio_filter/equalizer.c
> > + char *psz_bands = NULL;
> > + for ( int i = 0; i < EQZ_BANDS_MAX; i++ )
> > + {
> > + lldiv_t d;
> > + char *psz;
> > +
> > + d = lldiv( p_equalizer->f_amp[i] * 10000000, 10000000 );
> > + if ( asprintf( &psz, "%s %lld.%07llu", psz_bands ?
> psz_bands :
> > "", d.quot, d.rem ) == -1 )
>
> Does plain %f not work?
>
> I originally did it with a different implementation that used a simple %f.
But then I looked at the source code already in equalizer.c and I thought I
should be consistent with that. I will check this and compare the results.
> > + {
> > + free( psz_bands );
> > + return -1;
> > + }
> > + free( psz_bands );
> > + psz_bands = psz;
> > + }
> > + var_SetString( p_mi, "equalizer-bands", psz_bands );
> > + free( psz_bands );
> > + }
> > + else
> > + aout_EnableFilter( p_mi, "equalizer", false );
> > +
> > + return 0;
> > +}
> > +
> > +libvlc_equalizer_t *libvlc_media_player_get_equalizer(
> > libvlc_media_player_t *p_mi ) +{
> > + if ( !p_mi )
> > + return NULL;
> > +
> > + return p_mi->p_equalizer;
> > +}
>
> > diff --git a/src/audio_output/common.c b/src/audio_output/common.c
> > index c50883b..25f6ad0 100644
> > --- a/src/audio_output/common.c
> > +++ b/src/audio_output/common.c
> > @@ -88,6 +88,9 @@ audio_output_t *aout_New( vlc_object_t * p_parent )
> > var_Create (aout, "mute", VLC_VAR_BOOL | VLC_VAR_DOINHERIT);
> > var_AddCallback (aout, "mute", var_Copy, p_parent);
> >
> > + var_AddCallback (p_parent, "equalizer-preamp", var_Copy, aout );
> > + var_AddCallback (p_parent, "equalizer-bands", var_Copy, aout );
> > +
>
> You need to ensure the aout variables exist, and initialized, to do that.
>
> OK, this is the only comment I'm not sure about. I have seen in
equalizer.c that it creates those variables on the aout, but of course the
equalizer may not be enabled yet. Am I OK to simply create and initialise
those variables in the same place I register the callbacks? Or will that
interfere with what equalizer.c is doing? It does seem to work OK, but I'll
look at this again.
> > /* Visualizations */
> > var_Create (aout, "visual", VLC_VAR_STRING | VLC_VAR_HASCHOICE);
> > text.psz_string = _("Visualizations");
> > @@ -197,6 +200,10 @@ void aout_Destroy (audio_output_t *aout)
> > var_DelCallback (aout, "mute", var_Copy, aout->p_parent);
> > var_SetFloat (aout, "volume", -1.f);
> > var_DelCallback (aout, "volume", var_Copy, aout->p_parent);
> > +
> > + var_DelCallback (aout->p_parent, "equalizer-bands", var_Copy, aout);
> > + var_DelCallback (aout->p_parent, "equalizer-preamp", var_Copy,
> aout);
> > +
> > vlc_object_release (aout);
> > }
>
>
Thanks again for the review.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20120812/6008125d/attachment.html>
More information about the vlc-devel
mailing list