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

[Yuval Tze]

Thanks for your comments!
I'll try to fix it and send it again in the next few days.


2011/3/20 Rémi Denis-Courmont <remi at remlab.net>

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110320/0fc3b82f/attachment.html>