[vlc-devel] [PATCH] input: set OSD message when changing rate via hotkeys to display the current rate (v2)

[Laurent Aimar]

Hi,

> +static void DisplayRate( input_thread_t *p_input, float f_rate )
> +{
> +    char psz_msg[14 + 1];
> +    char psz_spd[7 + 1];
> +    strncpy( psz_msg, _("Speed: "), sizeof(psz_msg) );
> +    snprintf( psz_spd, sizeof(psz_spd), _("%.2fx"), f_rate );
> +    strncat( psz_msg, psz_spd, 7 );
 Why not using a simple snprintf for the whole ?
It will be simpler and will avoid security risks (strncat/strncpy are
usually really bad as they don't always add the terminal '\0').

> diff --git a/src/input/input.c b/src/input/input.c
> index f4081cc..75ab43e 100644
> --- a/src/input/input.c
> +++ b/src/input/input.c
> @@ -1773,60 +1773,12 @@ static bool Control( input_thread_t *p_input,
>          case INPUT_CONTROL_SET_RATE_SLOWER:
>          case INPUT_CONTROL_SET_RATE_FASTER:
 As the support for slower/faster are moved to var.c, 
INPUT_CONTROL_SET_RATE_SLOWER/FASTER should be removed from here
(and their declarations should also be removed).

> +            /* Moved the implementation of INPUT_CONTROL_SET_RATE_SLOWER/FASTER
> +             * in /src/input/var.c */
 This comment is not useful (git log/blame in case someone need to find it).

>  #include <vlc_common.h>
> +#include <math.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  
> @@ -565,23 +566,66 @@ static int RateCallback( vlc_object_t *p_this, char const *psz_cmd,
>  {
>      input_thread_t *p_input = (input_thread_t*)p_this;
>      VLC_UNUSED(oldval); VLC_UNUSED(p_data);
> +    
> +    static const int ppi_factor[][2] = {
> +        {1,64}, {1,32}, {1,16}, {1,8}, {1,4}, {1,3}, {1,2}, {2,3},
> +        {1,1},
> +        {3,2}, {2,1}, {3,1}, {4,1}, {8,1}, {16,1}, {32,1}, {64,1},
> +        {0,0}
> +    };
> +    
> +    int i;
> +    int i_idx;
> +    float f_rate = var_GetFloat( p_input, "rate" );
> +    float f_error;
> +    
> +    /* Determine the factor closest to the current rate */
> +    f_error = 1E20;
> +    i_idx = -1;
> +    for( i = 0; ppi_factor[i][0] != 0; i++ )
> +    {
> +        const float f_test_r = (float)ppi_factor[i][0] / (float)ppi_factor[i][1];
 One cast is enough.
> +        const float f_test_e = fabs( fabs( f_rate ) - f_test_r );
> +        if( f_test_e < f_error )
> +        {
> +            i_idx = i;
> +            f_error = f_test_e;
> +        }
> +    }
>  
> -    /* Problem with this way: the "rate" variable is updated after the
> -     * input thread did the change */
> -
> +    if( i_idx < 0 || ppi_factor[i_idx][0] == 0 )
> +        return VLC_EBADVAR;
 You should keep the original assert, this situation cannot happen.

> +    
> +    vlc_value_t val;
> +    const float f_rate_min = (float)INPUT_RATE_DEFAULT / INPUT_RATE_MAX;
> +    const float f_rate_max = (float)INPUT_RATE_DEFAULT / INPUT_RATE_MIN;
> +    
>      if( !strcmp( psz_cmd, "rate-slower" ) )
>      {
> -        input_ControlPush( p_input, INPUT_CONTROL_SET_RATE_SLOWER, NULL );
> +        if( ppi_factor[i_idx+1][0] > 0 )
i_idx > 0 is the right test.
> +            val.f_float = (float)ppi_factor[i_idx-1][0] / (float)ppi_factor[i_idx-1][1];
 One cast.

> +        else
> +            val.f_float = f_rate_min;
> +        
> +        var_SetFloat( p_input, "rate", val.f_float );
 You have forgot to apply the sign of the rate.
>      }
>      else if( !strcmp( psz_cmd, "rate-faster" ) )
>      {
> -        input_ControlPush( p_input, INPUT_CONTROL_SET_RATE_FASTER, NULL );
> +        if( ppi_factor[i_idx+1][0] > 0 )
> +            val.f_float = (float)ppi_factor[i_idx+1][0] / (float)ppi_factor[i_idx+1][1];
 One cast.
> +        else
> +            val.f_float = f_rate_max;
> +        
> +        var_SetFloat( p_input, "rate", val.f_float );
 You have forgot to apply the sign of the rate.
>      }
>      else
>      {
> +        /* Problem with this way: the "rate" variable is updated after the
> +         * input thread did the change */
>          newval.i_int = INPUT_RATE_DEFAULT / newval.f_float;
>          input_ControlPush( p_input, INPUT_CONTROL_SET_RATE, &newval );
>      }
> +    
>      return VLC_SUCCESS;
>  }

Regards,

-- 
fenrir