<div dir="ltr">Thanks for your comments!<br>
I'll try to fix it and send it again in the next few days.<br><br><br><div class="gmail_quote">2011/3/20 Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net">remi@remlab.net</a>></span><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
A few comments inline...<br>
<br>
Le dimanche 20 mars 2011 00:34:25 Yuval Tze, vous avez écrit :<br>
<div class="im">>          struct<br>
>          {<br>
>              subpicture_t * (*pf_filter)    ( filter_t *, mtime_t );<br>
> +            subpicture_t * (*pf_filter2)   ( filter_t *, subpicture_t * );<br>
<br>
</div>Given you are introducing that callback for a new plugin class, you should<br>
probably define a new structure within the callback union instead of extending<br>
the subpicture structure.<br>
<br>
(...)<br>
<div class="im">> +static int SubsdelayCallback( vlc_object_t *p_this, char const<br>
> *psz_var, vlc_value_t oldval, vlc_value_t newval, +        void *p_data )<br>
> +{<br>
> +    filter_sys_t *p_sys = (filter_sys_t *) p_data;<br>
> +<br>
> +    VLC_UNUSED( oldval );<br>
> +<br>
> +    if( !strcmp( psz_var, CFG_MODE ) )<br>
> +    {<br>
> +        p_sys->i_mode = newval.i_int;<br>
> +    }<br>
> +    else if( !strcmp( psz_var, CFG_FACTOR ) )<br>
> +    {<br>
> +        p_sys->i_factor = FLOAT_FACTOR_TO_INT_FACTOR( newval.f_float );<br>
> +    }<br>
> +    else if( !strcmp( psz_var, CFG_OVERLAP ) )<br>
> +    {<br>
> +        p_sys->i_overlap = newval.i_int;<br>
> +    }<br>
> +    else if( !strcmp( psz_var, CFG_MIN_ALPHA ) )<br>
> +    {<br>
> +        p_sys->i_min_alpha = newval.i_int;<br>
> +    }<br>
> +    else if( !strcmp( psz_var, CFG_MIN_STOPS_INTERVAL ) )<br>
> +    {<br>
> +        p_sys->i_min_stops_interval = MILLISEC_TO_MICROSEC( newval.i_int<br>
> );<br>
> +    }<br>
> +    else if( !strcmp( psz_var, CFG_MIN_STOP_START_INTERVAL ) )<br>
> +    {<br>
> +        p_sys->i_min_stop_start_interval = MILLISEC_TO_MICROSEC(<br>
> newval.i_int );<br>
> +    }<br>
> +    else if( !strcmp( psz_var, CFG_MIN_START_STOP_INTERVAL ) )<br>
> +    {<br>
> +        p_sys->i_min_start_stop_interval = MILLISEC_TO_MICROSEC(<br>
> newval.i_int ); +    }<br>
> +    else<br>
> +    {<br>
> +        return VLC_ENOVAR;<br>
> +    }<br>
> +<br>
> +    SubsdelayRecalculateDelays( (filter_t *) p_this );<br>
> +<br>
> +    return VLC_SUCCESS;<br>
> +}<br>
<br>
</div>This does not seem to be thread-safe. Keep in mind that the UI thread may call<br>
this function asynchronously (as far as the video thread is concerned anyway).<br>
<br>
Depending on your taste, you might prefer to have one callback per variable<br>
than a series of strcmp().<br>
<br>
> +/*************************************************************************<br>
> **** + * SubsdelayGetWordRank: Calculate single word rank according to its<br>
<div class="im">> length +<br>
> **************************************************************************<br>
> ***/ +static int SubsdelayGetWordRank( int i_length )<br>
> +{<br>
> +    static int p_rank[20] = { 0 };<br>
> +    int i;<br>
> +<br>
> +    if( p_rank[0] == 0 )<br>
> +    {<br>
> +        /* inititalize rank array */<br>
> +        p_rank[0] = 300; /* base rank */<br>
> +        p_rank[1] = p_rank[0];<br>
> +        p_rank[2] = p_rank[1];<br>
> +<br>
> +        for( i = 3; i < 20; i++ )<br>
> +        {<br>
> +            p_rank[i] = (int) ( 1.1 * p_rank[i - 1] );<br>
> +        }<br>
<br>
</div>This is not thread-safe, as there could be concurrent instances of the plugin<br>
from different LibVLC pipelines in the same process. Precomputed values in a<br>
'static const' table would seem more appropriate. You can still spell the<br>
original 'formula' as a comment.<br>
<br>
(...)<br>
> +/*************************************************************************<br>
> **** + * SubsdelayIsTextEmpty: Check if the text contains spaces only<br>
<div class="im">> +<br>
> **************************************************************************<br>
> ***/ +static bool SubsdelayIsTextEmpty( char *psz_text )<br>
> +{<br>
> +    int i;<br>
> +<br>
> +    if( !psz_text )<br>
> +    {<br>
> +        return false;<br>
> +    }<br>
> +<br>
> +    i = 0;<br>
> +    while ( psz_text[i] != '\0' )<br>
> +    {<br>
> +        if( psz_text[i] != ' ' )<br>
> +        {<br>
> +            return false;<br>
> +        }<br>
> +<br>
> +        i++;<br>
> +    }<br>
> +<br>
> +    return true;<br>
> +}<br>
> +<br>
<br>
</div>This could be simplified to:<br>
<br>
        psz_text += strspn( psz_text, " " );<br>
        return *psz_text;<br>
<br>
Best regards,<br>
<font color="#888888"><br>
--<br>
Rémi Denis-Courmont<br>
<a href="http://www.remlab.info/" target="_blank">http://www.remlab.info/</a><br>
<a href="http://fi.linkedin.com/in/remidenis" target="_blank">http://fi.linkedin.com/in/remidenis</a><br>
</font></blockquote></div><br></div>