[vlc-devel] [PATCH] avcodec: encoder: Fix win32 build

[Filip Roséen]

Hi Hugo,

On 2017-04-10 17:39, Hugo Beauzée-Luyssen wrote:

> By removing some long time deprecated options
> ---
>  modules/codec/avcodec/encoder.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/modules/codec/avcodec/encoder.c b/modules/codec/avcodec/encoder.c
> index 29627011bd..b6691d38b6 100644
> --- a/modules/codec/avcodec/encoder.c
> +++ b/modules/codec/avcodec/encoder.c
> @@ -555,6 +555,8 @@ int OpenEncoder( vlc_object_t *p_this )
>          }
>      }
>      free( psz_val );
> +    AVDictionary *options = NULL;
> +    char option_buff[128];
>  
>      if( p_enc->fmt_in.i_cat == VIDEO_ES )
>      {
> @@ -586,7 +588,8 @@ int OpenEncoder( vlc_object_t *p_this )
>          p_context->lumi_masking = p_sys->f_lumi_masking;
>          p_context->dark_masking = p_sys->f_dark_masking;
>          p_context->p_masking = p_sys->f_p_masking;
> -        p_context->border_masking = p_sys->f_border_masking;
> +        sprintf(option_buff, "%f", p_sys->f_border_masking);

Personally I would rest a little easier if `snprintf` was used instead
of `sprintf`, not that I am aware of any platform where the
string-representation of a `float` can yield a string longer than
`128`, but better safe than sorry.

> +        av_dict_set(&options, "border_mask", option_buff, 0);
>  
>          if( p_sys->i_key_int > 0 )
>              p_context->gop_size = p_sys->i_key_int;
> @@ -699,12 +702,16 @@ int OpenEncoder( vlc_object_t *p_this )
>          if( p_sys->i_qmin > 0 )
>          {
>              p_context->qmin = p_sys->i_qmin;
> -            p_context->mb_lmin = p_context->lmin = p_sys->i_qmin * FF_QP2LAMBDA;
> +            p_context->mb_lmin = p_sys->i_qmin * FF_QP2LAMBDA;
> +            sprintf(option_buff, "%d", p_context->mb_lmin);
> +            av_dict_set(&options, "lmin", option_buff, 0);
>          }
>          if( p_sys->i_qmax > 0 )
>          {
>              p_context->qmax = p_sys->i_qmax;
> -            p_context->mb_lmax = p_context->lmax = p_sys->i_qmax * FF_QP2LAMBDA;
> +            p_context->mb_lmax = p_sys->i_qmax * FF_QP2LAMBDA;
> +            sprintf(option_buff, "%d", p_context->mb_lmax);
> +            av_dict_set(&options, "lmax", option_buff, 0);
>          }
>          p_context->max_qdiff = 3;
>  
> @@ -717,7 +724,7 @@ int OpenEncoder( vlc_object_t *p_this )
>          }
>          else
>          {
> -            p_context->rc_qsquish = 1.0;
> +            av_dict_set(&options, "qsquish", "1.0", 0);
>              /* Default to 1/2 second buffer for given bitrate unless defined otherwise*/
>              if( !p_sys->i_rc_buffer_size )
>              {
> @@ -731,7 +738,8 @@ int OpenEncoder( vlc_object_t *p_this )
>              /* This is from ffmpeg's ffmpeg.c : */
>              p_context->rc_initial_buffer_occupancy
>                  = p_sys->i_rc_buffer_size * 3/4;
> -            p_context->rc_buffer_aggressivity = p_sys->f_rc_buffer_aggressivity;
> +            sprintf(option_buff, "%f", p_sys->f_rc_buffer_aggressivity);
> +            av_dict_set(&options, "rc_buffer_aggressivity", option_buff, 0);
>          }
>      }
>      else if( p_enc->fmt_in.i_cat == AUDIO_ES )
> @@ -868,19 +876,25 @@ int OpenEncoder( vlc_object_t *p_this )
>              if( !var_GetInteger( p_enc, ENC_CFG_PREFIX "qmin" ) )
>              {
>                  p_context->qmin = 10;
> -                p_context->mb_lmin = p_context->lmin = 10 * FF_QP2LAMBDA;
> +                p_context->mb_lmin = 10 * FF_QP2LAMBDA;
> +                sprintf(option_buff, "%d", p_context->mb_lmin);
> +                av_dict_set(&options, "lmin", option_buff, 0);
>              }
>  
>              if( !var_GetInteger( p_enc, ENC_CFG_PREFIX "qmax" ) )
>              {
>                  p_context->qmax = 42;
> -                p_context->mb_lmax = p_context->lmax = 42 * FF_QP2LAMBDA;
> +                p_context->mb_lmax = 42 * FF_QP2LAMBDA;
> +                sprintf(option_buff, "%d", p_context->mb_lmax);
> +                av_dict_set(&options, "lmax", option_buff, 0);
>              }
>  
>          } else if( !var_GetInteger( p_enc, ENC_CFG_PREFIX "qmin" ) )
>          {
>                  p_context->qmin = 1;
> -                p_context->mb_lmin = p_context->lmin = FF_QP2LAMBDA;
> +                p_context->mb_lmin = FF_QP2LAMBDA;
> +                sprintf(option_buff, "%d", p_context->mb_lmin);
> +                av_dict_set(&options, "lmin", option_buff, 0);
>          }

Further thoughts:

 - Since the usage of `sprintf` + `av_dict_set` is used in so many
   places, maybe it is worth introducing a helper that does both in
   one call?

 - It would be appropriate to check whether `av_dict_set` was indeed
   successful where it is called, since it does return an error-code.

> @@ -913,7 +927,6 @@ int OpenEncoder( vlc_object_t *p_this )
>  
>      int ret;
>      char *psz_opts = var_InheritString(p_enc, ENC_CFG_PREFIX "options");
> -    AVDictionary *options = NULL;
>      if (psz_opts) {
>          options = vlc_av_get_options(psz_opts);

Leak of the previously created `AVDictionary` as
`sout-avcodec-options`, and other options now resulting in a write to
`options`, are not mutually exclusive?

>          free(psz_opts);
> -- 
> 2.11.0
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170410/0400e70f/attachment.html>