[vlc-devel] [PATCH 1/2] libvlc: add equalizer API

[Mark Lee]

Hello,

On 10 July 2013 20:05, Rémi Denis-Courmont <remi at remlab.net> wrote:

> Le mercredi 10 juillet 2013 21:54:18, Mark Lee a écrit :
> >
> +/*************************************************************************
> > **** + * 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, unsigned u_band ) +{
> > +    if ( !p_equalizer )
> > +        return -1;
>
> Whenever you accept NULL, please document it. Alternatively, do not accept
> NULL.
>

OK, so I can add something in the header file method documentation
explicitly
stating that passing in NULL will return an error.


>
> > +
> > +    if ( u_band >= EQZ_BANDS_MAX )
> > +        return -1;
> > +
> > +    if ( f_amp < -20.0f )
> > +        f_amp = -20.0f;
> > +    else if ( f_amp > 20.0f )
> > +        f_amp = 20.0f;
> > +
> > +    p_equalizer->f_amp[ u_band ] = f_amp;
> > +    return 0;
> > +}
> > +
> > diff --git a/lib/media_player.c b/lib/media_player.c
> > index 376159f..bb43a82 100644
> > --- a/lib/media_player.c
> > +++ b/lib/media_player.c
> > @@ -33,6 +33,7 @@
> >  #include <vlc_demux.h>
> >  #include <vlc_input.h>
> >  #include <vlc_vout.h>
> > +#include <vlc_aout.h>
> >  #include <vlc_keys.h>
> >
> >  #include "libvlc_internal.h"
> > @@ -462,6 +463,10 @@ libvlc_media_player_new( libvlc_instance_t
> *instance )
> >      var_Create (mp, "video-title-position", VLC_VAR_INTEGER);
> >      var_Create (mp, "video-title-timeout", VLC_VAR_INTEGER);
> >
> > +    /* Equalizer */
> > +    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;
> > @@ -1404,3 +1409,46 @@ void libvlc_media_player_set_video_title_display(
> > libvlc_media_player_t *p_mi, l var_SetBool( p_mi, "video-title-show",
> > false );
> >      }
> >  }
> > +
> > +int libvlc_media_player_set_equalizer( libvlc_media_player_t *p_mi,
> > libvlc_equalizer_t *p_equalizer ) +{
> > +    float f_preamp;
> > +    char *psz_bands;
> > +
> > +    if ( p_equalizer )
> > +    {
> > +        f_preamp = p_equalizer->f_preamp;
> > +        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 ? p_equalizer->f_amp[i] : 0.f ) == -1 )
> > +            {
> > +                free( psz_bands );
> > +                return -1;
> > +            }
>
> How large can this get? Pre-allocating the buffer would be faster and
> simpler,
> especially on the stack.
>
>
It's usually 100 bytes or so.

I thought I copied/adapted that from some code elsewhere in vlc, I can
rework it.


> > +            free( psz_bands );
> > +            psz_bands = psz;
> > +        }
> > +    }
> > +    else
> > +    {
> > +        f_preamp = 0.f;
> > +        psz_bands = NULL;
> > +    }
> > +
> > +    var_SetFloat( p_mi, "equalizer-preamp", f_preamp );
> > +    var_SetString( p_mi, "equalizer-bands", psz_bands );
> > +
> > +    audio_output_t *p_aout = input_resource_HoldAout(
> > p_mi->input.p_resource ); +    if ( p_aout )
> > +    {
> > +        var_SetFloat( p_aout, "equalizer-preamp", f_preamp );
> > +        var_SetString( p_aout, "equalizer-bands", psz_bands );
> > +
> > +        vlc_object_release( p_aout );
> > +    }
> > +
> > +    free( psz_bands );
> > +    return 0;
> > +}
> > diff --git a/src/audio_output/filters.c b/src/audio_output/filters.c
> > index 41b23b4..ea451a3 100644
> > --- a/src/audio_output/filters.c
> > +++ b/src/audio_output/filters.c
> > @@ -314,8 +314,7 @@ static int EqualizerCallback (vlc_object_t *obj,
> const
> > char *var, void *data)
> >  {
> >      const char *val = newval.psz_string;
> > -
> > -    if (*val)
> > +    if (!strcmp("equalizer", var) && *val)
>
> Tautology.
>

Sorry, I don't understand this.

I am reusing this existing EqualizerCallback to newly track the
"equalizer-preamp"
variable (currently it is used only for the "equalizer" variable, which is
triggered by
some other independent mechanism to create and set an "equalizer-preset"
variable).

I am reusing the callback because it already has the code to enable/disable
the
equalizer filter, but it also has another code path I do not want to
execute (originally
I had it as a separate callback).

So, the value of "var" can now be "equalizer" or "equalizer-bands", and the
value
of "*val" legitimately can either be NULL or not-NULL.

Apologies if I've misunderstood, would you please elaborate?


> >      {
> >          var_Create (obj, "equalizer-preset", VLC_VAR_STRING);
> >          var_SetString (obj, "equalizer-preset", val);
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
>


<http://apricasoftware.co.uk>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130710/37ae5393/attachment.html>