[vlc-devel] [PATCH] avcodec: encoder: Fix win32 build
Hugo Beauzée-Luyssen
hugo at beauzee.fr
Tue Apr 11 09:47:21 CEST 2017
On Mon, Apr 10, 2017, at 08:24 PM, Filip Roséen wrote:
> Hi Hugo,
>
Hey Filip,
> 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.
>
Agreed
> > + 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?
>
I thought of that, and seing the remaining amount of deprecated option,
that makes sense.
> - It would be appropriate to check whether `av_dict_set` was indeed
> successful where it is called, since it does return an error-code.
Fair enough, though I'm not sure what can be done in that case.
I went for displaying a warning, but opinion/patch welcome. It seems
harsh to fail in that case IMHO.
>
> > @@ -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?
Oh my thanks for catching that!
>
> > free(psz_opts);
> > --
> > 2.11.0
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
--
Hugo Beauzée-Luyssen
hugo at beauzee.fr
More information about the vlc-devel
mailing list