[FFmpeg-devel] [PATCH] libavcodec/libx264.c: distinguish between x264 parameter errors

[Erik Slagter]

On 24-06-11 02:11, Baptiste Coudurier wrote:
> On 06/22/2011 11:58 AM, Erik Slagter wrote:

[ .. ]

>> I don't understand what you're trying to say here. The function you're
>> describing here is functionally equivalent to my version.
>>
>> And yes, the goal of having fewer lines was clear to me, sorry if I
>> didn't phrase it clearly. I just don't understand why lesser lines should
>> always be a goal, especially when the source starts to look like assembly
>> and the generated code is the same...

[ .. ]

> It's not about that. It's about coding style uniformity.

[ .. ]

> Please address my comments.

Can you please be more specific? The last version already has half of the
amount of lines. If you mean the usage of "OPT_STR" (as a #define), I can't
find any other reference in the code.

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index cc5b983..4bbeb0f 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -198,14 +198,18 @@ static void check_default_settings(AVCodecContext *avctx)
      }
  }

-#define OPT_STR(opt, param)                                             \
-    do {                                                                \
-        if (param && x264_param_parse(&x4->params, opt, param) < 0) {   \
-            av_log(avctx, AV_LOG_ERROR,                                 \
-                   "bad value for '%s': '%s'\n", opt, param);           \
-            return -1;                                                  \
-        }                                                               \
-    } while (0);                                                        \
+static int opt_str(AVCodecContext *avctx, X264Context *x4, const char *opt, const char *param)
+{
+    if(!param)
+        return(1);
+
+    switch(x264_param_parse(&x4->params, opt, param))
+    {
+        case(X264_PARAM_BAD_NAME): { av_log(avctx, AV_LOG_ERROR, "no such option: \"%s\"\n", opt); return(0); }
+        case(X264_PARAM_BAD_VALUE): { av_log(avctx, AV_LOG_ERROR, "bad value: \"%s\" for option \"%s\"\n", param, opt); return(0); }
+        default: { return(1); }
+    }
+}

  static av_cold int X264_init(AVCodecContext *avctx)
  {
@@ -308,7 +312,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
      x4->params.p_log_private        = avctx;
      x4->params.i_log_level          = X264_LOG_DEBUG;

-    OPT_STR("weightp", x4->weightp);
+    if(!opt_str(avctx, x4, "weightp", x4->weightp))
+        return(-1);

      x4->params.b_intra_refresh      = avctx->flags2 & CODEC_FLAG2_INTRA_REFRESH;
      x4->params.rc.i_bitrate         = avctx->bit_rate       / 1000;
@@ -328,7 +333,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
          }
      }

-    OPT_STR("stats", x4->stats);
+    if(!opt_str(avctx, x4, "stats", x4->stats))
+        return(-1);

      // if neither crf nor cqp modes are selected we have to enable the RC
      // we do it this way because we cannot check if the bitrate has been set
@@ -341,14 +347,16 @@ static av_cold int X264_init(AVCodecContext *avctx)
              (float)avctx->rc_initial_buffer_occupancy / avctx->rc_buffer_size;
      }

-    OPT_STR("level", x4->level);
+    if(!opt_str(avctx, x4, "level", x4->level))
+        return(-1);

      if(x4->x264opts){
          const char *p= x4->x264opts;
          while(p){
              char param[256]={0}, val[256]={0};
              sscanf(p, "%255[^:=]=%255[^:]", param, val);
-            OPT_STR(param, val);
+            if(!opt_str(avctx, x4, param, val))
+                return(-1);
              p= strchr(p, ':');
              p+=!!p;
          }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5110 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110624/fc0868ac/attachment.p7s>