<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <meta http-equiv="Content-Style-Type" content="text/css" />
  <meta name="generator" content="pandoc" />
  <title></title>
  <style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Hugo,</p>
<p>On 2017-04-10 17:39, Hugo BeauzĂ©e-Luyssen wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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);</code></pre>
</blockquote>
<p>Personally I would rest a little easier if <code>snprintf</code> was used instead of <code>sprintf</code>, not that I am aware of any platform where the string-representation of a <code>float</code> can yield a string longer than <code>128</code>, but better safe than sorry.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +        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);
          }</code></pre>
</blockquote>
<p>Further thoughts:</p>
<ul>
<li><p>Since the usage of <code>sprintf</code> + <code>av_dict_set</code> is used in so many places, maybe it is worth introducing a helper that does both in one call?</p></li>
<li><p>It would be appropriate to check whether <code>av_dict_set</code> was indeed successful where it is called, since it does return an error-code.</p></li>
</ul>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><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);</code></pre>
</blockquote>
<p>Leak of the previously created <code>AVDictionary</code> as <code>sout-avcodec-options</code>, and other options now resulting in a write to <code>options</code>, are not mutually exclusive?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code>          free(psz_opts);
 -- 
 2.11.0

 _______________________________________________
 vlc-devel mailing list
 To unsubscribe or modify your subscription options:
 https://mailman.videolan.org/listinfo/vlc-devel</code></pre>
</blockquote>
</body>
</html>