[vlc-devel] [Patch] New audio & video filters for audio bargraph display inside video

[Rémi Denis-Courmont]

	Hello,

>diff -ruN vlc-1.0.2/configure.ac vlc-1.0.2_audiobargraph/configure.ac
>--- vlc-1.0.2/configure.ac	2009-09-19 21:30:22.000000000 +0200
>+++ vlc-1.0.2_audiobargraph/configure.ac	2009-10-19 12:17:13.906250000 +0200

New features should really be diffed against the master branch.

>+    add_integer( CFG_PREFIX "barGraph", 1, NULL, BARGRAPH_TEXT,
>BARGRAPH_LONGTEXT, false )

Conventionally, VLC uses only lower case for configuration items. Separate 
words with dash.

>+struct aout_filter_sys_t
>+{
>+	char*			address;
>+	int 			port;
>+	int 			barGraph;
>+	int 			barGraphRepetition;
>+	int 			silence;
>+	int 			timeWindow;
>+	float 			alarmThreshold;
>+	int 			repetitionTime;
>+	int				TCPconnection;
>+	int 			counter;
>+	int				nbChannels;
>+	ValueDate_t* 	first;
>+	ValueDate_t* 	last;

You're making your life more miserable with pointers. You can allocate those 
two structures directly inside aout_filter_sys_t.

>+    if( ( p_filter->input.i_format != VLC_FOURCC('f','l','3','2') &&
>+          p_filter->input.i_format != VLC_FOURCC('f','i','3','2') ) )

Why do you allow fixed-point? It seems this filter only works with floats.

>+	p_sys->TCPconnection = net_ConnectTCP(p_this,p_sys->address,p_sys->port);

net_ConnectTCP() can fail. You shouldn't blindly write to a socket -later- if 
you don't check that it was created succesfully.

>+    p_filter->pf_do_work = DoWork;
>+    p_filter->b_in_place= 1;

We use pf_audio_filter nowadays. The pf_do_work interface won't work in the 
master branch.

>+	for ( i_line = 20; i_line < scale+20; i_line++ ) {
>+		// black
>+		*(p_pic->p[0].p_pixels +
>+			(scale + 30 - i_line - 1) *
>+			p_pic->p[0].i_pitch + 20) = 0;
...

This piece of code is very repetitive. If you cannot factor it out, you might 
at least want to use a macro.

>+ * vout_sys_t: logo video output method descriptor

This is NOT the logo filter...

>+    /* FIXME missing lock */

Yes indeed...

>+forward:
>+    return var_Set( p_vout, psz_var, newval );

Uuuuuuuuuh, you are setting a variable from the variable callback? How come 
this is not deadlocking?

>+    var_AddCallback( p_filter, "audiobargraph_v-barWidth", BarGraphCallback, 
p_sys );
>+
>+    /* Misc init */
>+    p_filter->pf_sub_filter = Filter;
>+    p_sys->b_need_update = true;

Too late. You already registered a callback that might modify this value 
asynchronously.


>+    vlc_mutex_unlock( &p_BarGraph->lock );
>+
>+    /*  where to locate the bar graph: */
>+    if( p_sys->pos < 0 )

p_sys->pos is modified by a callback. You need to protect concurrent accesses. 
This applies to a few other variables too.

>+static int BarGraphCallback( vlc_object_t *p_this, char const *psz_var,
>+                         vlc_value_t oldval, vlc_value_t newval, void
>*p_data )
>+{
>+    VLC_UNUSED(oldval);
>+    filter_sys_t *p_sys = (filter_sys_t *)p_data;
>+    BarGraph_t *p_BarGraph = p_sys->p_BarGraph;
>+	char* i_values;
>+	char* res = NULL;
>+	char delim[] = ":";
>+
>+    if ( !strcmp( psz_var, "audiobargraph_v-x" ) )
>+    {
>+        p_sys->posx = newval.i_int;

Missing thread synchronization here too.

Best regards,

-- 
Rémi Denis-Courmont
http://www.remlab.net/