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

[clement chesnin]

Hello !

Thanks for the tips !
Here is a new version made with git, including the changes you
suggested. Please tell us if there are things to change or tweak...

Clément.

2009/10/21 Rémi Denis-Courmont <remi at remlab.net>:
>        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/
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: audiobargraph.patch
Type: application/octet-stream
Size: 4869584 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091116/2e6f73b8/attachment.obj>