image_filter enhancement

[Maxim Dounin]

Hello!

On Thu, Dec 20, 2012 at 09:01:38PM +0400, ivan babrou wrote:

> I've send patch for configuration semantics in separate thread.
> 
> Here's lastest version of my patch that may be applied after. Is there
> something else that I should fix?

I would like to see clarification on how are you going to handle 
just bare numbers specified.  As of now the code seems to don't do 
any special processing and seems to assume all values are correct, 
which might not be true and will result in some mostly arbitray 
crop offset selection.  It might make sense to actually support 
percents as in Maxim Bublis's patch[1].

[1] http://mailman.nginx.org/pipermail/nginx-devel/2012-April/002156.html

[...]

> @@ -1151,7 +1188,24 @@ ngx_http_image_filter_value(ngx_str_t *value)
> 
>      n = ngx_atoi(value->data, value->len);
> 
> -    if (n > 0) {
> +    if (n == NGX_ERROR) {
> +        if (ngx_strncmp(value->data, "left", value->len) == 0) {
> +            return NGX_HTTP_IMAGE_OFFSET_LEFT;
> +
> +        } else if (ngx_strncmp(value->data, "right", value->len) == 0) {
> +            return NGX_HTTP_IMAGE_OFFSET_RIGHT;
> +
> +        } else if (ngx_strncmp(value->data, "top", value->len) == 0) {
> +            return NGX_HTTP_IMAGE_OFFSET_TOP;
> +
> +        } else if (ngx_strncmp(value->data, "bottom", value->len) == 0) {
> +            return NGX_HTTP_IMAGE_OFFSET_BOTTOM;
> +
> +        } else {
> +            return NGX_HTTP_IMAGE_OFFSET_CENTER;
> +        }
> +
> +    } else if (n > 0) {
>          return (ngx_uint_t) n;
>      }
> 

The ngx_strncmp() checks are incorrect, see here:
http://mailman.nginx.org/pipermail/nginx-devel/2012-December/003065.html

You may also move them below "if (n > 0)" the check for better 
readability.

> @@ -1175,6 +1229,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;

The "crop_" prefix probably should be dropped here for better 
readability (and to match other names in the code like "oxcv").

[...]

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