image_filter enhancement

Maxim Dounin mdounin at mdounin.ru
Tue Nov 27 16:03:28 UTC 2012


Hello!

On Tue, Nov 27, 2012 at 12:10:13AM +0400, ivan babrou wrote:

> I tried to rework patch. Now it supports configuration through
> variables. Variables ox and oy renamed to crop_offset_x and
> crop_offset_y.
> 
> I'm confused about this line:
> +    value.data[value.len] = 0;
> 
> Probably I don't understand something about memory management in nginx.

See below.

> 
> 
> diff --git a/ngx_http_image_filter_module.c b/ngx_http_image_filter_module.c
> index c853c33..15f8d17 100644
> --- a/ngx_http_image_filter_module.c
> +++ b/ngx_http_image_filter_module.c
> @@ -32,6 +32,14 @@
>  #define NGX_HTTP_IMAGE_GIF       2
>  #define NGX_HTTP_IMAGE_PNG       3
> 
> +#define NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL 0
> +#define NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL 1
> +
> +#define NGX_HTTP_IMAGE_OFFSET_CENTER    0
> +#define NGX_HTTP_IMAGE_OFFSET_LEFT      1
> +#define NGX_HTTP_IMAGE_OFFSET_RIGHT     2
> +#define NGX_HTTP_IMAGE_OFFSET_TOP       3
> +#define NGX_HTTP_IMAGE_OFFSET_BOTTOM    4
> 
>  #define NGX_HTTP_IMAGE_BUFFERED  0x08
> 
> @@ -43,11 +51,15 @@ typedef struct {
>      ngx_uint_t                   angle;
>      ngx_uint_t                   jpeg_quality;
>      ngx_uint_t                   sharpen;
> +    ngx_uint_t                   crop_offset_x;
> +    ngx_uint_t                   crop_offset_y;
> 
>      ngx_flag_t                   transparency;
> 
>      ngx_http_complex_value_t    *wcv;
>      ngx_http_complex_value_t    *hcv;
> +    ngx_http_complex_value_t    *oxv;
> +    ngx_http_complex_value_t    *oyv;

Probably you mean "oxcv", "oycv" - "cv" here is abbreviation of 
"complex value".

>      ngx_http_complex_value_t    *acv;
>      ngx_http_complex_value_t    *jqcv;
>      ngx_http_complex_value_t    *shcv;
> @@ -66,6 +78,8 @@ typedef struct {
>      ngx_uint_t                   height;
>      ngx_uint_t                   max_width;
>      ngx_uint_t                   max_height;
> +    ngx_uint_t                   crop_offset_x;
> +    ngx_uint_t                   crop_offset_y;
>      ngx_uint_t                   angle;
> 
>      ngx_uint_t                   phase;
> @@ -98,6 +112,15 @@ static u_char
> *ngx_http_image_out(ngx_http_request_t *r, ngx_uint_t type,
>  static void ngx_http_image_cleanup(void *data);
>  static ngx_uint_t ngx_http_image_filter_get_value(ngx_http_request_t *r,
>      ngx_http_complex_value_t *cv, ngx_uint_t v);
> +static void
> +ngx_http_image_filter_crop_offset(ngx_http_request_t *r,
> +    ngx_http_complex_value_t *cv, ngx_uint_t t, ngx_uint_t *v);
> +static void
> +ngx_http_image_filter_vertical_crop_offset(ngx_str_t *value,
> +    ngx_uint_t *v);
> +static void
> +ngx_http_image_filter_horizontal_crop_offset(ngx_str_t *value,
> +    ngx_uint_t *v);
>  static ngx_uint_t ngx_http_image_filter_value(ngx_str_t *value);
> 
> 
> @@ -110,6 +133,8 @@ static char
> *ngx_http_image_filter_jpeg_quality(ngx_conf_t *cf,
>      ngx_command_t *cmd, void *conf);
>  static char *ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,
>      void *conf);
> +static char *ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,
> +    void *conf);
>  static ngx_int_t ngx_http_image_filter_init(ngx_conf_t *cf);
> 
> 
> @@ -150,6 +175,13 @@ static ngx_command_t  ngx_http_image_filter_commands[] = {
>        offsetof(ngx_http_image_filter_conf_t, buffer_size),
>        NULL },
> 
> +    { ngx_string("image_filter_offset"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      ngx_http_image_filter_offset,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      0,
> +      NULL },
> +
>        ngx_null_command
>  };
> 
> @@ -529,6 +561,22 @@ ngx_http_image_process(ngx_http_request_t *r)
>          return NULL;
>      }
> 
> +    if (conf->oxv == NULL) {
> +        ctx->crop_offset_x = conf->crop_offset_x;
> +    } else {
> +        ngx_http_image_filter_crop_offset(r, conf->oxv,
> +
> NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL,
> +                                          &ctx->crop_offset_x);
> +    }
> +
> +    if (conf->oyv == NULL) {
> +        ctx->crop_offset_y = conf->crop_offset_y;
> +    } else {
> +        ngx_http_image_filter_crop_offset(r, conf->oyv,
> +                                          NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL,
> +                                          &ctx->crop_offset_y);
> +    }
> +

1) You want to follow pattern in the previous code, i.e.
   foo = ngx_http_image_filter_get_value(r, conf->foocv, conf->foo);

   (With additional support of top/bottom/left/right literals.)

2) I don't think you need to calculate offsets to decide whether 
   image is ok as is or we need to resize it.  Hence it's should 
   be ok to move the code to where it's needed.

3) With (2) fixed you probably don't need variables in ctx 
   anymore.

>      if (rc == NGX_OK
>          && ctx->width <= ctx->max_width
>          && ctx->height <= ctx->max_height
> @@ -932,8 +980,17 @@ transparent:
>                  return NULL;
>              }
> 
> -            ox /= 2;
> -            oy /= 2;
> +            if (ctx->crop_offset_x == NGX_HTTP_IMAGE_OFFSET_LEFT) {
> +                ox = 0;
> +            } else if (ctx->crop_offset_x == NGX_HTTP_IMAGE_OFFSET_CENTER) {
> +                ox /= 2;
> +            }
> +
> +            if (ctx->crop_offset_y == NGX_HTTP_IMAGE_OFFSET_TOP) {
> +                oy = 0;
> +            } else if (ctx->crop_offset_y == NGX_HTTP_IMAGE_OFFSET_CENTER) {
> +                oy /= 2;
> +            }

Note: this needs empty lines before "} else if ..." to match 
style.  It should be changed anyway though, due to reasons 
outlined above.

> 
>              ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>                             "image crop: %d x %d @ %d x %d",
> @@ -1139,6 +1196,52 @@ ngx_http_image_filter_get_value(ngx_http_request_t *r,
>      return ngx_http_image_filter_value(&val);
>  }
> 
> +static void
> +ngx_http_image_filter_crop_offset(ngx_http_request_t *r,
> +    ngx_http_complex_value_t *cv, ngx_uint_t t, ngx_uint_t *v)
> +{
> +    ngx_str_t value;
> +
> +    if (ngx_http_complex_value(r, cv, &value) != NGX_OK) {
> +        return;
> +    }
> +
> +    value.data[value.len] = 0;

This is incorrect and will result in memory corruption.

If you need null-terminated string, right aproach is to set 
ccv.zero flag while calling ngx_http_compile_complex_value().  On 
the other hand, it might be good idea to don't assume 
null-termination instead.

> +
> +    if (t == NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL) {
> +        ngx_http_image_filter_horizontal_crop_offset(&value, v);
> +    } else if (t == NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL) {
> +        ngx_http_image_filter_vertical_crop_offset(&value, v);
> +    }
> +}
> +
> +
> +static void
> +ngx_http_image_filter_horizontal_crop_offset(ngx_str_t *value,
> +    ngx_uint_t *v)
> +{
> +    if (ngx_strcmp(value->data, "center") == 0) {
> +        *v = NGX_HTTP_IMAGE_OFFSET_CENTER;
> +    } else if (ngx_strcmp(value->data, "left") == 0) {
> +        *v = NGX_HTTP_IMAGE_OFFSET_LEFT;
> +    } else if (ngx_strcmp(value->data, "right") == 0) {
> +        *v = NGX_HTTP_IMAGE_OFFSET_RIGHT;
> +    }
> +}
> +
> +static void
> +ngx_http_image_filter_vertical_crop_offset(ngx_str_t *value,
> +    ngx_uint_t *v)
> +{
> +    if (ngx_strcmp(value->data, "center") == 0) {
> +        *v = NGX_HTTP_IMAGE_OFFSET_CENTER;
> +    } else if (ngx_strcmp(value->data, "top") == 0) {
> +        *v = NGX_HTTP_IMAGE_OFFSET_TOP;
> +    } else if (ngx_strcmp(value->data, "bottom") == 0) {
> +        *v = NGX_HTTP_IMAGE_OFFSET_BOTTOM;
> +    }
> +}
> +

I don't think we need so many functions to do this simple task.  
It would be good idea simplify this.

The NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL and 
NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL seem to be unneeded, just a 
boolean value would do the trick.

>  static ngx_uint_t
>  ngx_http_image_filter_value(ngx_str_t *value)
> @@ -1175,6 +1278,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)
>      conf->angle = NGX_CONF_UNSET_UINT;
>      conf->transparency = NGX_CONF_UNSET;
>      conf->buffer_size = NGX_CONF_UNSET_SIZE;
> +    conf->crop_offset_x = NGX_CONF_UNSET_UINT;
> +    conf->crop_offset_y = NGX_CONF_UNSET_UINT;
> 
>      return conf;
>  }
> @@ -1223,6 +1328,17 @@ ngx_http_image_filter_merge_conf(ngx_conf_t
> *cf, void *parent, void *child)
>      ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,
>                                1 * 1024 * 1024);
> 
> +    ngx_conf_merge_uint_value(conf->crop_offset_x,
> prev->crop_offset_x, NGX_HTTP_IMAGE_OFFSET_CENTER);
> +    ngx_conf_merge_uint_value(conf->crop_offset_y,
> prev->crop_offset_y, NGX_HTTP_IMAGE_OFFSET_CENTER);
> +
> +    if (conf->oxv == NULL) {
> +        conf->oxv = prev->oxv;
> +    }
> +
> +    if (conf->oyv == NULL) {
> +        conf->oyv = prev->oyv;
> +    }
> +

Style: please keep lines shorter than 80 chars.

Additionally, looking at the code I tend to think it's incorrect.  
That is, it's pattern you've followed is incorrect, not your code.  
E.g. the following config will result in $arg_q incorrectly used 
for quality in /image/, instead of "80" explicitly set:

    image_filter_jpeg_quality $arg_q;

    location /image/ {
        image_filter crop $arg_w $arg_h;
        image_filter_jpeg_quality 80;
    }

This needs fixing.

>      return NGX_CONF_OK;
>  }
> 
> @@ -1474,6 +1590,64 @@ ngx_http_image_filter_sharpen(ngx_conf_t *cf,
> ngx_command_t *cmd,
>  }
> 
> 
> +static char *
> +ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,
> +    void *conf)
> +{
> +    ngx_http_image_filter_conf_t *imcf = conf;
> +
> +    ngx_str_t                         *value;
> +    ngx_http_complex_value_t           cv;
> +    ngx_http_compile_complex_value_t   ccv;
> +
> +    value = cf->args->elts;
> +
> +    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
> +
> +    ccv.cf = cf;
> +    ccv.value = &value[1];
> +    ccv.complex_value = &cv;
> +
> +    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> +        return NGX_CONF_ERROR;
> +    }
> +
> +    if (cv.lengths == NULL) {
> +        ngx_http_image_filter_horizontal_crop_offset(&value[1],
> &imcf->crop_offset_x);
> +    } else {
> +        imcf->oxv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));
> +        if (imcf->oxv == NULL) {
> +            return NGX_CONF_ERROR;
> +        }
> +
> +        *imcf->oxv = cv;
> +    }
> +
> +    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
> +
> +    ccv.cf = cf;
> +    ccv.value = &value[2];
> +    ccv.complex_value = &cv;
> +
> +    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> +        return NGX_CONF_ERROR;
> +    }
> +
> +    if (cv.lengths == NULL) {
> +        ngx_http_image_filter_vertical_crop_offset(&value[2],
> &imcf->crop_offset_y);
> +    } else {
> +        imcf->oyv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));
> +        if (imcf->oyv == NULL) {
> +            return NGX_CONF_ERROR;
> +        }
> +
> +        *imcf->oyv = cv;
> +    }
> +
> +    return NGX_CONF_OK;
> +}
> +
> +
>  static ngx_int_t
>  ngx_http_image_filter_init(ngx_conf_t *cf)
>  {
> 
> 
> On 15 November 2012 03:47, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > offsets via variables.  On the other hand, his patch asks for
> 
> 
> 
> --
> Regards, Ian Babrou
> http://bobrik.name http://twitter.com/ibobrik skype:i.babrou
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Maxim Dounin
http://nginx.com/support.html



More information about the nginx-devel mailing list