Hello,<br><br><div class="gmail_quote">On 20 August 2012 21:31, Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net" target="_blank">remi@remlab.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Le lundi 20 août 2012 23:10:44 Mark Lee, vous avez écrit :<br>
<div><div class="h5">> diff --git a/lib/media_player.c b/lib/media_player.c<br>
> index a41b8c7..e14558e 100644<br>
> --- a/lib/media_player.c<br>
> +++ b/lib/media_player.c<br>
> @@ -34,11 +34,14 @@<br>
>  #include <vlc_input.h><br>
>  #include <vlc_vout.h><br>
>  #include <vlc_keys.h><br>
> +#include <vlc_aout_intf.h><br>
><br>
>  #include "libvlc_internal.h"<br>
>  #include "media_internal.h" // libvlc_media_set_state()<br>
>  #include "media_player_internal.h"<br>
><br>
> +#include "modules/audio_filter/equalizer_presets.h"<br>
> +<br>
>  /*<br>
>   * mapping of libvlc_navigate_mode_t to vlc_action_t<br>
>   */<br>
> @@ -482,6 +485,9 @@ libvlc_media_player_new( libvlc_instance_t *instance )<br>
>      var_Create (mp, "amem-rate", VLC_VAR_INTEGER | VLC_VAR_DOINHERIT);<br>
>      var_Create (mp, "amem-channels", VLC_VAR_INTEGER | VLC_VAR_DOINHERIT);<br>
><br>
> +    var_Create (mp, "equalizer-preamp", VLC_VAR_FLOAT);<br>
> +    var_Create (mp, "equalizer-bands", VLC_VAR_STRING);<br>
> +<br>
>      mp->p_md = NULL;<br>
>      mp->state = libvlc_NothingSpecial;<br>
>      mp->p_libvlc_instance = instance;<br>
> @@ -1409,3 +1415,56 @@ void libvlc_media_player_next_frame(<br>
> libvlc_media_player_t *p_mi ) vlc_object_release( p_input_thread );<br>
>      }<br>
>  }<br>
> +<br>
> +int libvlc_media_player_set_equalizer( libvlc_media_player_t *p_mi,<br>
> libvlc_equalizer_t *p_equalizer )<br>
> +{<br>
> +    lock( p_mi );<br>
> +<br>
> +    if ( !p_equalizer )<br>
> +    {<br>
> +        p_mi->p_equalizer = NULL;<br>
> +<br>
> +        unlock( p_mi );<br>
> +<br>
> +        aout_EnableFilter( p_mi, "equalizer", false );<br>
> +        return 0;<br>
> +    }<br>
> +<br>
> +    p_mi->p_equalizer = p_equalizer;<br>
> +<br>
> +    unlock( p_mi );<br>
> +<br>
> +    aout_EnableFilter( p_mi, "equalizer", true );<br>
<br>
</div></div>The three lines above could be factored in both cases.<br>
<br>
But see below...<br>
<div><div class="h5"><br>
> +<br>
> +    var_SetFloat( p_mi, "equalizer-preamp", p_equalizer->f_preamp );<br>
> +<br>
> +    char *psz_bands = NULL;<br>
> +    for ( int i = 0; i < EQZ_BANDS_MAX; i++ )<br>
> +    {<br>
> +        char *psz;<br>
> +        if ( asprintf( &psz, "%s %.07f", psz_bands ? psz_bands : "",<br>
> p_equalizer->f_amp[i] ) == -1 ) +        {<br>
> +            free( psz_bands );<br>
> +            return -1;<br>
> +        }<br>
> +        free( psz_bands );<br>
> +        psz_bands = psz;<br>
> +    }<br>
> +    var_SetString( p_mi, "equalizer-bands", psz_bands );<br>
> +    free( psz_bands );<br>
> +<br>
> +    return 0;<br>
> +}<br>
> +<br>
> +libvlc_equalizer_t *libvlc_media_player_get_equalizer(<br>
> libvlc_media_player_t *p_mi )<br>
> +{<br>
> +    libvlc_equalizer_t *p_equalizer;<br>
> +<br>
> +    lock( p_mi );<br>
> +<br>
> +    p_equalizer = p_mi->p_equalizer;<br>
> +<br>
> +    unlock( p_mi );<br>
> +<br>
> +    return p_equalizer;<br>
> +}<br>
<br>
</div></div>A lock that only reads a pointer. This is alarmingly suspicious as regards<br>
life cycle.<br></blockquote><div><br>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...<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I may be missing something, but I don't see why that pointer is needed inside<br>
the media player anyway.<br>
</blockquote></div><br>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. <br><br>It would simplify a bit if that method wasn't needed.<br>
<br>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?<br>
<br>Thanks again for your review comments. I'll have another go.<br><br>