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

[clement chesnin]

Hello !

Thanks for the comments, here is a new version...
Some things about your comments :
>
> +       if (!(p_sys->TCPconnection = net_ConnectTCP(p_this,p_sys->address,p_sys-
>>port))) {
> +        free(p_sys);
> +               return VLC_EGENERIC;
> +       }
>
> The error value is -1, not 0. That said, I wonder why you use TCP instead of
> an intra-process mechanism.

This allowed use to clearly separate the audio and video part.

>
> +       char *message = (char*)malloc(255*sizeof(char));
>
> sizeof(char) is fundamentally always one.
> 'char message[255];' would be much simpler here.

Char array can not be dynamically modified

>
> +       nbChannels = aout_FormatNbChannels( &p_filter->fmt_in.audio );
> +       if (p_sys->nbChannels != nbChannels) {
> +               /*free(p_sys->value);
> +               p_sys->value = (int*)malloc(nbChannels * sizeof(int));
> +               for (i=0; i<nbChannels; i++) {
> +                       p_sys->value[i] = 0;
> +               }*/
> +               p_sys->nbChannels = nbChannels;
> +       }
>
> The if statement is useless here.

OK

>
> +                       /* 5 - compare it to the threshold */
> +                       sprintf(message,"@audiobargraph_v audiobargraph_v-alarm ");
> +                       if (sum < p_sys->alarm_threshold) {
> +                               sprintf(message,"%s1\n",message);
> +                       } else {
> +                               sprintf(message,"%s0\n",message);
> +                       }
>
> This is undefined use of sprintf() and will fail on some platforms.
>
> +       if (p_sys->bargraph) {
> +               /* 6 - sent the message with the values for the BarGraph */
> +               if ((nbChannels > 0) && (p_sys->counter%(p_sys->bargraph_repetition) == 0))
> {
> +                       sprintf(message,"@audiobargraph_v audiobargraph_v-i_values ");
> +                       for (i=0; i<(nbChannels-1); i++) {
> +                               sprintf(message,"%s%f:", message, i_value[i]);
> +                       }
> +                       sprintf(message,"%s%f\n", message, i_value[nbChannels-1]);
>
> Same problem as above.

Modified

>
> +    p_sys->p_blend = NULL;
> +    if( !b_sub )
> +    {
> +
> +        p_sys->p_blend = filter_NewBlend( VLC_OBJECT(p_filter),
> +                                          &p_filter->fmt_in.video );
> +        if( !p_sys->p_blend )
> +        {
> +            free( p_sys );
> +            return VLC_EGENERIC;
>
> You're leaking p_barGraph here.

OK

>
> +       res = strtok(i_values, delim);
> +       while (res != NULL) {
> +               nbChannels++;
> +               p_BarGraph->i_values = (int*)realloc(p_BarGraph->i_values,
> nbChannels*sizeof(int));
> +               p_BarGraph->i_values[nbChannels-1] = __MAX( __MIN( atof(res)*p_BarGraph-
>>scale, p_BarGraph->scale ), 0 );
> +               p_BarGraph->nbChannels = nbChannels;
> +               res = strtok(NULL, delim);
> +       }
>
> strtok() is not thread-safe. Use strtok_r() instead.
>
> +               res = strtok(i_values, delim);
> +               while (res != NULL) {
> +                       p_BarGraph->nbChannels++;
> +                       p_BarGraph->i_values = (int*)realloc(p_BarGraph->i_values, p_BarGraph-
>>nbChannels*sizeof(int));
> +                       p_BarGraph->i_values[p_BarGraph->nbChannels-1] = __MAX( __MIN(
> atof(res)*p_BarGraph->scale, p_BarGraph->scale ), 0 );
> +                       res = strtok(NULL, delim);
> +               }
>
> Same as above. You could probably factor this code.

strtok_r is not a standard function. I factored and modified it...

Thanks again,

/Clement.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: audiobargraph-181109.patch
Type: application/octet-stream
Size: 44976 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091118/99b5eb8b/attachment.obj>