image_filter enhancement

[Maxim Dounin]

Hello!

On Tue, Dec 04, 2012 at 09:35:07AM +0400, ivan babrou wrote:

[...]

> @@ -932,8 +953,27 @@ transparent:
>                  return NULL;
>              }
> 
> -            ox /= 2;
> -            oy /= 2;
> +            crop_offset_x = conf->crop_offset_x;
> +            crop_offset_y = conf->crop_offset_y;

You may want to use shorter local variable names here.

[...]

> @@ -1149,10 +1189,27 @@ ngx_http_image_filter_value(ngx_str_t *value)
>          return (ngx_uint_t) -1;
>      }
> 
> -    n = ngx_atoi(value->data, value->len);
> +    if (ngx_strcmp(value->data, "center") == 0) {
> +        return NGX_HTTP_IMAGE_OFFSET_CENTER;
> +
> +    } else if (ngx_strcmp(value->data, "left") == 0) {
> +        return NGX_HTTP_IMAGE_OFFSET_LEFT;
> +
> +    } else if (ngx_strcmp(value->data, "right") == 0) {
> +        return NGX_HTTP_IMAGE_OFFSET_RIGHT;
> +
> +    } else if (ngx_strcmp(value->data, "top") == 0) {
> +        return NGX_HTTP_IMAGE_OFFSET_TOP;
> 
> -    if (n > 0) {
> -        return (ngx_uint_t) n;
> +    } else if (ngx_strcmp(value->data, "bottom") == 0) {
> +        return NGX_HTTP_IMAGE_OFFSET_BOTTOM;
> +
> +    } else {
> +        n = ngx_atoi(value->data, value->len);
> +
> +        if (n > 0) {
> +            return (ngx_uint_t) n;
> +        }
>      }

I'm not happy with this change as it degrades performance of other 
values evaluation.  It probably may be left as a single function, 
but I would rather fallback to all this string matching logic only 
if the value doesn't happen to be a number, i.e. if ngx_atoi() 
fails.

I also don't really see how you are going to distinguish constants 
returned from actual values specified by numbers.

[...]

> @@ -1223,6 +1282,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->oxcv == NULL) {
> +        conf->oxcv = prev->oxcv;
> +    }
> +
> +    if (conf->oycv == NULL) {
> +        conf->oycv = prev->oycv;
> +    }

I believe I've already outlined in the previous review that this 
merge logic is wrong, as well as a style problem with lines longer 
than 80 chars here.

[...]

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