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

[Mark Lee]

Hello,

On 20 August 2012 21:31, Rémi Denis-Courmont <remi at remlab.net> wrote:

> 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 did that because other operations on the media player instance also
acquired the lock (e.g. reading the state) so I thought it was generally
required. I admit my knowledge is lacking here. When in doubt I try to
follow existing code, but having said that it may be moot...


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

It was only so it could be easily retrieved by
libvlc_media_player_get_equalizer(), but I guess it's questionable how
useful that is.

It would simplify a bit if that method wasn't needed.

I suppose *if* it were needed a var_Address could be used instead? Or we
could just say the equalizer pointer is not stored on the media player at
all and it's up to the application to manage the situation. Do you have any
opinion on that?

Thanks again for your review comments. I'll have another go.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20120820/1c305d44/attachment.html>