Hello,<br><br>Thanks for your comments...<br><br><div class="gmail_quote">On 12 August 2012 09:47, 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 dimanche 12 aoűt 2012 11:30:45 Mark Lee, vous avez écrit :<br>
<div class="im">> /** @} media_player */<br>
> diff --git a/lib/audio.c b/lib/audio.c<br>
> index 27d7cd4..8e7b959 100644<br>
> --- a/lib/audio.c<br>
> +++ b/lib/audio.c<br>
> @@ -42,6 +42,8 @@<br>
> #include "libvlc_internal.h"<br>
> #include "media_player_internal.h"<br>
><br>
> +#include "modules/audio_filter/equalizer_presets.h"<br>
> +<br>
> /*<br>
> * Remember to release the returned audio_output_t since it is locked at<br>
> * the end of this function.<br>
> @@ -507,3 +509,153 @@ int libvlc_audio_set_delay( libvlc_media_player_t<br>
> *p_mi, int64_t i_delay ) }<br>
> return ret;<br>
> }<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_get_preset_count : Get the number of<br>
> equalizer presets +<br>
> **************************************************************************<br>
> ***/ +unsigned libvlc_audio_equalizer_get_preset_count( void )<br>
<div class="im">> +{<br>
> + return NB_PRESETS;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_get_preset_name : Get the name for a preset<br>
> +<br>
> **************************************************************************<br>
> ***/ +const char *libvlc_audio_equalizer_get_preset_name( int i_index ) +{<br>
<div class="im">> + if ( i_index < 0 || i_index >= NB_PRESETS )<br>
> + return NULL;<br>
<br>
</div>You should use unsigned for consistency with ..._count and to simplify.<br>
<div class="im"><br></div></blockquote><div>OK.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +<br>
> + return preset_list_text[ i_index ];<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_get_band_count : Get the number of<br>
<div class="im">> equalizer frequency bands +<br>
> **************************************************************************<br>
</div>> ***/ +unsigned libvlc_audio_equalizer_get_band_count( void )<br>
<div class="im">> +{<br>
> + return EQZ_BANDS_MAX;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_get_band_frequency : Get the frequency for<br>
> a band +<br>
> **************************************************************************<br>
> ***/ +float libvlc_audio_equalizer_get_band_frequency( int i_index )<br>
<div class="im">> +{<br>
> + if ( i_index < 0 || i_index >= EQZ_BANDS_MAX )<br>
> + return -1;<br>
> +<br>
> + return f_vlc_frequency_table_10b[ i_index ];<br>
> +}<br>
<br>
</div>Same here and later.<br>
<br></blockquote><div>OK.<br><br> > +<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +/*************************************************************************<br>
> **** + * libvlc_audio_equalizer_new : Create a new audio equalizer with<br>
> zeroed values +<br>
> **************************************************************************<br>
> ***/ +libvlc_equalizer_t *libvlc_audio_equalizer_new( void )<br>
<div class="im">> +{<br>
> + libvlc_equalizer_t *p_equalizer;<br>
> +<br>
> + p_equalizer = malloc( sizeof( *p_equalizer ) );<br>
> + if ( p_equalizer == NULL )<br>
> + return NULL;<br>
> +<br>
> + p_equalizer->f_preamp = 0.0;<br>
> +<br>
> + for ( int i = 0; i < EQZ_BANDS_MAX; i++ )<br>
> + p_equalizer->f_amp[ i ] = 0.0;<br>
> +<br>
> + return p_equalizer;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_new_from_preset : Create a new audio<br>
<div class="im">> equalizer based on a preset +<br>
> **************************************************************************<br>
</div>> ***/ +libvlc_equalizer_t *libvlc_audio_equalizer_new_from_preset( int<br>
<div class="im">> i_index ) +{<br>
> + libvlc_equalizer_t *p_equalizer;<br>
> +<br>
> + if ( i_index < 0 || i_index >= NB_PRESETS )<br>
> + return NULL;<br>
> +<br>
> + p_equalizer = libvlc_audio_equalizer_new();<br>
> + if ( !p_equalizer )<br>
> + return NULL;<br>
> +<br>
> + p_equalizer->f_preamp = eqz_preset_10b[ i_index ].f_preamp;<br>
> +<br>
> + for ( int i = 0; i < EQZ_BANDS_MAX; i++ )<br>
> + p_equalizer->f_amp[ i ] = eqz_preset_10b[ i_index ].f_amp[ i ];<br>
> +<br>
> + return p_equalizer;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_release : Release a previously created<br>
> equalizer +<br>
> **************************************************************************<br>
> ***/ +void libvlc_audio_equalizer_release( libvlc_equalizer_t *p_equalizer<br>
<div class="im">> ) +{<br>
> + if ( p_equalizer )<br>
> + free( p_equalizer );<br>
<br>
</div>if() is not needed. Refer to ISO C.<br>
<br></blockquote><div>OK.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +}<br>
> +<br>
> +/*************************************************************************<br>
> **** + * libvlc_audio_equalizer_set_preamp : Set the preamp value for an<br>
> equalizer +<br>
> **************************************************************************<br>
> ***/ +int libvlc_audio_equalizer_set_preamp( libvlc_equalizer_t<br>
<div class="im">> *p_equalizer, float f_preamp ) +{<br>
> + if ( !p_equalizer )<br>
> + return -1;<br>
> +<br>
> + if ( f_preamp < -20.0 )<br>
<br>
</div>Use 'f' postfix to avoid conversions to/from double.<br>
<div class="im"><br></div></blockquote><div>OK.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> + f_preamp = -20.0;<br>
> + else if ( f_preamp > 20.0 )<br>
> + f_preamp = 20.0;<br>
> +<br>
> + p_equalizer->f_preamp = f_preamp;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_get_preamp : Get the preamp value for an<br>
> equalizer +<br>
> **************************************************************************<br>
> ***/ +float libvlc_audio_equalizer_get_preamp( libvlc_equalizer_t<br>
<div class="im">> *p_equalizer ) +{<br>
> + if ( !p_equalizer )<br>
> + return 0;<br>
<br>
</div>0.f would be more consistent.<br>
<div class="im"><br></div></blockquote><div>OK.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +<br>
> + return p_equalizer->f_preamp;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_set_amp_at_index : Set the amplification<br>
<div class="im">> value for an equalizer band +<br>
> **************************************************************************<br>
</div>> ***/ +int libvlc_audio_equalizer_set_amp_at_index( libvlc_equalizer_t<br>
<div class="im">> *p_equalizer, float f_amp, int i_band ) +{<br>
> + if ( !p_equalizer )<br>
> + return -1;<br>
> +<br>
> + if ( i_band < 0 || i_band >= EQZ_BANDS_MAX )<br>
> + return -1;<br>
> +<br>
> + if ( f_amp < -20.0 )<br>
> + f_amp = -20.0;<br>
> + else if ( f_amp > 20.0 )<br>
> + f_amp = 20.0;<br>
> +<br>
> + p_equalizer->f_amp[ i_band ] = f_amp;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/*************************************************************************<br>
</div>> **** + * libvlc_audio_equalizer_get_amp_at_index : Get the amplification<br>
<div class="im">> value for an equalizer band +<br>
> **************************************************************************<br>
</div>> ***/ +float libvlc_audio_equalizer_get_amp_at_index( libvlc_equalizer_t<br>
<div class="im">> *p_equalizer, int i_band ) +{<br>
> + if ( !p_equalizer )<br>
> + return 0;<br>
> +<br>
> + if ( i_band < 0 || i_band >= EQZ_BANDS_MAX )<br>
> + return 0;<br>
> +<br>
> + return p_equalizer->f_amp[ i_band ];<br>
> +}<br>
<br>
</div><div><div class="h5">> diff --git a/lib/media_player.c b/lib/media_player.c<br>
> index a41b8c7..f2cb46d 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,49 @@ 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>
> + if ( !p_mi )<br>
> + return -1;<br>
<br>
</div></div>Is this useful to check in practice?<br>
<div class="im"><br></div></blockquote><div>I suppose not. It's not done everywhere else so I should remove it.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> +<br>
> + p_mi->p_equalizer = p_equalizer;<br>
> +<br>
> + if ( p_equalizer )<br>
> + {<br>
> + aout_EnableFilter( p_mi, "equalizer", true );<br>
> +<br>
> + var_SetFloat( p_mi, "equalizer-preamp", p_equalizer->f_preamp );<br>
> +<br>
> + // This implementation slightly adapted from<br>
> modules/audio_filter/equalizer.c<br>
> + char *psz_bands = NULL;<br>
> + for ( int i = 0; i < EQZ_BANDS_MAX; i++ )<br>
> + {<br>
> + lldiv_t d;<br>
> + char *psz;<br>
> +<br>
> + d = lldiv( p_equalizer->f_amp[i] * 10000000, 10000000 );<br>
> + if ( asprintf( &psz, "%s %lld.%07llu", psz_bands ? psz_bands :<br>
> "", d.quot, d.rem ) == -1 )<br>
<br>
</div>Does plain %f not work?<br>
<div class="im"><br></div></blockquote><div>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> + {<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>
> + else<br>
> + aout_EnableFilter( p_mi, "equalizer", false );<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +libvlc_equalizer_t *libvlc_media_player_get_equalizer(<br>
> libvlc_media_player_t *p_mi ) +{<br>
> + if ( !p_mi )<br>
> + return NULL;<br>
> +<br>
> + return p_mi->p_equalizer;<br>
> +}<br>
<br>
</div><div class="im">> diff --git a/src/audio_output/common.c b/src/audio_output/common.c<br>
> index c50883b..25f6ad0 100644<br>
> --- a/src/audio_output/common.c<br>
> +++ b/src/audio_output/common.c<br>
> @@ -88,6 +88,9 @@ audio_output_t *aout_New( vlc_object_t * p_parent )<br>
> var_Create (aout, "mute", VLC_VAR_BOOL | VLC_VAR_DOINHERIT);<br>
> var_AddCallback (aout, "mute", var_Copy, p_parent);<br>
><br>
> + var_AddCallback (p_parent, "equalizer-preamp", var_Copy, aout );<br>
> + var_AddCallback (p_parent, "equalizer-bands", var_Copy, aout );<br>
> +<br>
<br>
</div>You need to ensure the aout variables exist, and initialized, to do that.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> /* Visualizations */<br>
> var_Create (aout, "visual", VLC_VAR_STRING | VLC_VAR_HASCHOICE);<br>
> text.psz_string = _("Visualizations");<br>
> @@ -197,6 +200,10 @@ void aout_Destroy (audio_output_t *aout)<br>
> var_DelCallback (aout, "mute", var_Copy, aout->p_parent);<br>
> var_SetFloat (aout, "volume", -1.f);<br>
> var_DelCallback (aout, "volume", var_Copy, aout->p_parent);<br>
> +<br>
> + var_DelCallback (aout->p_parent, "equalizer-bands", var_Copy, aout);<br>
> + var_DelCallback (aout->p_parent, "equalizer-preamp", var_Copy, aout);<br>
> +<br>
> vlc_object_release (aout);<br>
> }<br>
<br>
</div></div></blockquote></div><br>Thanks again for the review.<br>