[vlc-devel] [PATCH] subsdelay filter - initial version

[Rémi Denis-Courmont]

A few comments inline...

Le dimanche 20 mars 2011 00:34:25 Yuval Tze, vous avez écrit :
>          struct
>          {
>              subpicture_t * (*pf_filter)    ( filter_t *, mtime_t );
> +            subpicture_t * (*pf_filter2)   ( filter_t *, subpicture_t * );

Given you are introducing that callback for a new plugin class, you should 
probably define a new structure within the callback union instead of extending 
the subpicture structure.

(...)
> +static int SubsdelayCallback( vlc_object_t *p_this, char const
> *psz_var, vlc_value_t oldval, vlc_value_t newval, +        void *p_data )
> +{
> +    filter_sys_t *p_sys = (filter_sys_t *) p_data;
> +
> +    VLC_UNUSED( oldval );
> +
> +    if( !strcmp( psz_var, CFG_MODE ) )
> +    {
> +        p_sys->i_mode = newval.i_int;
> +    }
> +    else if( !strcmp( psz_var, CFG_FACTOR ) )
> +    {
> +        p_sys->i_factor = FLOAT_FACTOR_TO_INT_FACTOR( newval.f_float );
> +    }
> +    else if( !strcmp( psz_var, CFG_OVERLAP ) )
> +    {
> +        p_sys->i_overlap = newval.i_int;
> +    }
> +    else if( !strcmp( psz_var, CFG_MIN_ALPHA ) )
> +    {
> +        p_sys->i_min_alpha = newval.i_int;
> +    }
> +    else if( !strcmp( psz_var, CFG_MIN_STOPS_INTERVAL ) )
> +    {
> +        p_sys->i_min_stops_interval = MILLISEC_TO_MICROSEC( newval.i_int
> );
> +    }
> +    else if( !strcmp( psz_var, CFG_MIN_STOP_START_INTERVAL ) )
> +    {
> +        p_sys->i_min_stop_start_interval = MILLISEC_TO_MICROSEC(
> newval.i_int );
> +    }
> +    else if( !strcmp( psz_var, CFG_MIN_START_STOP_INTERVAL ) )
> +    {
> +        p_sys->i_min_start_stop_interval = MILLISEC_TO_MICROSEC(
> newval.i_int ); +    }
> +    else
> +    {
> +        return VLC_ENOVAR;
> +    }
> +
> +    SubsdelayRecalculateDelays( (filter_t *) p_this );
> +
> +    return VLC_SUCCESS;
> +}

This does not seem to be thread-safe. Keep in mind that the UI thread may call 
this function asynchronously (as far as the video thread is concerned anyway).

Depending on your taste, you might prefer to have one callback per variable 
than a series of strcmp().

> +/*************************************************************************
> **** + * SubsdelayGetWordRank: Calculate single word rank according to its
> length +
> **************************************************************************
> ***/ +static int SubsdelayGetWordRank( int i_length )
> +{
> +    static int p_rank[20] = { 0 };
> +    int i;
> +
> +    if( p_rank[0] == 0 )
> +    {
> +        /* inititalize rank array */
> +        p_rank[0] = 300; /* base rank */
> +        p_rank[1] = p_rank[0];
> +        p_rank[2] = p_rank[1];
> +
> +        for( i = 3; i < 20; i++ )
> +        {
> +            p_rank[i] = (int) ( 1.1 * p_rank[i - 1] );
> +        }

This is not thread-safe, as there could be concurrent instances of the plugin 
from different LibVLC pipelines in the same process. Precomputed values in a 
'static const' table would seem more appropriate. You can still spell the 
original 'formula' as a comment.

(...)
> +/*************************************************************************
> **** + * SubsdelayIsTextEmpty: Check if the text contains spaces only
> +
> **************************************************************************
> ***/ +static bool SubsdelayIsTextEmpty( char *psz_text )
> +{
> +    int i;
> +
> +    if( !psz_text )
> +    {
> +        return false;
> +    }
> +
> +    i = 0;
> +    while ( psz_text[i] != '\0' )
> +    {
> +        if( psz_text[i] != ' ' )
> +        {
> +            return false;
> +        }
> +
> +        i++;
> +    }
> +
> +    return true;
> +}
> +

This could be simplified to:

	psz_text += strspn( psz_text, " " );
	return *psz_text;

Best regards,

-- 
Rémi Denis-Courmont
http://www.remlab.info/
http://fi.linkedin.com/in/remidenis