<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Additionally, looking at the code I tend to think it's incorrect.<br>

That is, it's pattern you've followed is incorrect, not your code.<br>E.g. the following config will result in $arg_q incorrectly used<br>for quality in /image/, instead of "80" explicitly set:<br>    image_filter_jpeg_quality $arg_q;<br>

    location /image/ {<br>        image_filter crop $arg_w $arg_h;<br>        image_filter_jpeg_quality 80;<br>    }<br>This needs fixing.</blockquote><div><br></div><div>As I see other config variables are buggy too. Should I fix them too?</div>

<div class="gmail_extra"><br>Anyway, this is latest version of my patch where I fixed other issues:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">diff --git a/ngx_http_image_filter_module.c b/ngx_http_image_filter_module.c</div>

<div class="gmail_extra">index c853c33..778fcd3 100644</div><div class="gmail_extra">--- a/ngx_http_image_filter_module.c</div><div class="gmail_extra">+++ b/ngx_http_image_filter_module.c</div><div class="gmail_extra">@@ -32,6 +32,11 @@</div>

<div class="gmail_extra"> #define NGX_HTTP_IMAGE_GIF       2</div><div class="gmail_extra"> #define NGX_HTTP_IMAGE_PNG       3</div><div class="gmail_extra"> </div><div class="gmail_extra">+#define NGX_HTTP_IMAGE_OFFSET_CENTER    0</div>

<div class="gmail_extra">+#define NGX_HTTP_IMAGE_OFFSET_LEFT      1</div><div class="gmail_extra">+#define NGX_HTTP_IMAGE_OFFSET_RIGHT     2</div><div class="gmail_extra">+#define NGX_HTTP_IMAGE_OFFSET_TOP       3</div><div class="gmail_extra">

+#define NGX_HTTP_IMAGE_OFFSET_BOTTOM    4</div><div class="gmail_extra"> </div><div class="gmail_extra"> #define NGX_HTTP_IMAGE_BUFFERED  0x08</div><div class="gmail_extra"> </div><div class="gmail_extra">@@ -43,11 +48,15 @@ typedef struct {</div>

<div class="gmail_extra">     ngx_uint_t                   angle;</div><div class="gmail_extra">     ngx_uint_t                   jpeg_quality;</div><div class="gmail_extra">     ngx_uint_t                   sharpen;</div>

<div class="gmail_extra">+    ngx_uint_t                   crop_offset_x;</div><div class="gmail_extra">+    ngx_uint_t                   crop_offset_y;</div><div class="gmail_extra"> </div><div class="gmail_extra">     ngx_flag_t                   transparency;</div>

<div class="gmail_extra"> </div><div class="gmail_extra">     ngx_http_complex_value_t    *wcv;</div><div class="gmail_extra">     ngx_http_complex_value_t    *hcv;</div><div class="gmail_extra">+    ngx_http_complex_value_t    *oxcv;</div>

<div class="gmail_extra">+    ngx_http_complex_value_t    *oycv;</div><div class="gmail_extra">     ngx_http_complex_value_t    *acv;</div><div class="gmail_extra">     ngx_http_complex_value_t    *jqcv;</div><div class="gmail_extra">

     ngx_http_complex_value_t    *shcv;</div><div class="gmail_extra">@@ -66,6 +75,8 @@ typedef struct {</div><div class="gmail_extra">     ngx_uint_t                   height;</div><div class="gmail_extra">     ngx_uint_t                   max_width;</div>

<div class="gmail_extra">     ngx_uint_t                   max_height;</div><div class="gmail_extra">+    ngx_uint_t                   crop_offset_x;</div><div class="gmail_extra">+    ngx_uint_t                   crop_offset_y;</div>

<div class="gmail_extra">     ngx_uint_t                   angle;</div><div class="gmail_extra"> </div><div class="gmail_extra">     ngx_uint_t                   phase;</div><div class="gmail_extra">@@ -110,6 +121,8 @@ static char *ngx_http_image_filter_jpeg_quality(ngx_conf_t *cf,</div>

<div class="gmail_extra">     ngx_command_t *cmd, void *conf);</div><div class="gmail_extra"> static char *ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,</div><div class="gmail_extra">     void *conf);</div>

<div class="gmail_extra">+static char *ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,</div><div class="gmail_extra">+    void *conf);</div><div class="gmail_extra"> static ngx_int_t ngx_http_image_filter_init(ngx_conf_t *cf);</div>

<div class="gmail_extra"> </div><div class="gmail_extra"> </div><div class="gmail_extra">@@ -150,6 +163,13 @@ static ngx_command_t  ngx_http_image_filter_commands[] = {</div><div class="gmail_extra">       offsetof(ngx_http_image_filter_conf_t, buffer_size),</div>

<div class="gmail_extra">       NULL },</div><div class="gmail_extra"> </div><div class="gmail_extra">+    { ngx_string("image_filter_crop_offset"),</div><div class="gmail_extra">+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,</div>

<div class="gmail_extra">+      ngx_http_image_filter_offset,</div><div class="gmail_extra">+      NGX_HTTP_LOC_CONF_OFFSET,</div><div class="gmail_extra">+      0,</div><div class="gmail_extra">+      NULL },</div><div class="gmail_extra">

+</div><div class="gmail_extra">       ngx_null_command</div><div class="gmail_extra"> };</div><div class="gmail_extra"> </div><div class="gmail_extra">@@ -737,7 +757,8 @@ ngx_http_image_resize(ngx_http_request_t *r, ngx_http_image_filter_ctx_t *ctx)</div>

<div class="gmail_extra"> {</div><div class="gmail_extra">     int                            sx, sy, dx, dy, ox, oy, ax, ay, size,</div><div class="gmail_extra">                                    colors, palette, transparent, sharpen,</div>

<div class="gmail_extra">-                                   red, green, blue, t;</div><div class="gmail_extra">+                                   red, green, blue, t,</div><div class="gmail_extra">+                                   offset_x, offset_y;</div>

<div class="gmail_extra">     u_char                        *out;</div><div class="gmail_extra">     ngx_buf_t                     *b;</div><div class="gmail_extra">     ngx_uint_t                     resize;</div><div class="gmail_extra">

@@ -932,8 +953,24 @@ transparent:</div><div class="gmail_extra">                 return NULL;</div><div class="gmail_extra">             }</div><div class="gmail_extra"> </div><div class="gmail_extra">-            ox /= 2;</div>

<div class="gmail_extra">-            oy /= 2;</div><div class="gmail_extra">+            offset_x = ngx_http_image_filter_get_value(r, conf->oxcv,</div><div class="gmail_extra">+                                                       conf->crop_offset_x);</div>

<div class="gmail_extra">+            offset_y = ngx_http_image_filter_get_value(r, conf->oycv,</div><div class="gmail_extra">+                                                       conf->crop_offset_y);</div><div class="gmail_extra">

+</div><div class="gmail_extra">+            if (offset_x == NGX_HTTP_IMAGE_OFFSET_LEFT) {</div><div class="gmail_extra">+                ox = 0;</div><div class="gmail_extra">+</div><div class="gmail_extra">+            } else if (offset_x == NGX_HTTP_IMAGE_OFFSET_CENTER) {</div>

<div class="gmail_extra">+                ox /= 2;</div><div class="gmail_extra">+            }</div><div class="gmail_extra">+</div><div class="gmail_extra">+            if (offset_y == NGX_HTTP_IMAGE_OFFSET_TOP) {</div>

<div class="gmail_extra">+                oy = 0;</div><div class="gmail_extra">+</div><div class="gmail_extra">+            } else if (offset_y == NGX_HTTP_IMAGE_OFFSET_CENTER) {</div><div class="gmail_extra">+                oy /= 2;</div>

<div class="gmail_extra">+            }</div><div class="gmail_extra"> </div><div class="gmail_extra">             ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,</div><div class="gmail_extra">                            "image crop: %d x %d @ %d x %d",</div>

<div class="gmail_extra">@@ -1151,7 +1188,24 @@ ngx_http_image_filter_value(ngx_str_t *value)</div><div class="gmail_extra"> </div><div class="gmail_extra">     n = ngx_atoi(value->data, value->len);</div><div class="gmail_extra">

 </div><div class="gmail_extra">-    if (n > 0) {</div><div class="gmail_extra">+    if (n == NGX_ERROR) {</div><div class="gmail_extra">+        if (ngx_strcmp(value->data, "left") == 0) {</div><div class="gmail_extra">

+            return NGX_HTTP_IMAGE_OFFSET_LEFT;</div><div class="gmail_extra">+</div><div class="gmail_extra">+        } else if (ngx_strcmp(value->data, "right") == 0) {</div><div class="gmail_extra">+            return NGX_HTTP_IMAGE_OFFSET_RIGHT;</div>

<div class="gmail_extra">+</div><div class="gmail_extra">+        } else if (ngx_strcmp(value->data, "top") == 0) {</div><div class="gmail_extra">+            return NGX_HTTP_IMAGE_OFFSET_TOP;</div><div class="gmail_extra">

+</div><div class="gmail_extra">+        } else if (ngx_strcmp(value->data, "bottom") == 0) {</div><div class="gmail_extra">+            return NGX_HTTP_IMAGE_OFFSET_BOTTOM;</div><div class="gmail_extra">+</div>

<div class="gmail_extra">+        } else {</div><div class="gmail_extra">+            return NGX_HTTP_IMAGE_OFFSET_CENTER;</div><div class="gmail_extra">+        }</div><div class="gmail_extra">+</div><div class="gmail_extra">

+    } else if (n > 0) {</div><div class="gmail_extra">         return (ngx_uint_t) n;</div><div class="gmail_extra">     }</div><div class="gmail_extra"> </div><div class="gmail_extra">@@ -1175,6 +1229,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)</div>

<div class="gmail_extra">     conf->angle = NGX_CONF_UNSET_UINT;</div><div class="gmail_extra">     conf->transparency = NGX_CONF_UNSET;</div><div class="gmail_extra">     conf->buffer_size = NGX_CONF_UNSET_SIZE;</div>

<div class="gmail_extra">+    conf->crop_offset_x = NGX_CONF_UNSET_UINT;</div><div class="gmail_extra">+    conf->crop_offset_y = NGX_CONF_UNSET_UINT;</div><div class="gmail_extra"> </div><div class="gmail_extra">     return conf;</div>

<div class="gmail_extra"> }</div><div class="gmail_extra">@@ -1223,6 +1279,19 @@ ngx_http_image_filter_merge_conf(ngx_conf_t *cf, void *parent, void *child)</div><div class="gmail_extra">     ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,</div>

<div class="gmail_extra">                               1 * 1024 * 1024);</div><div class="gmail_extra"> </div><div class="gmail_extra">+    ngx_conf_merge_uint_value(conf->crop_offset_x, prev->crop_offset_x,</div>
<div class="gmail_extra">
+                              NGX_HTTP_IMAGE_OFFSET_CENTER);</div><div class="gmail_extra">+    ngx_conf_merge_uint_value(conf->crop_offset_y, prev->crop_offset_y,</div><div class="gmail_extra">+                              NGX_HTTP_IMAGE_OFFSET_CENTER);</div>

<div class="gmail_extra">+</div><div class="gmail_extra">+    if (conf->oxcv == NULL) {</div><div class="gmail_extra">+        conf->oxcv = prev->oxcv;</div><div class="gmail_extra">+    }</div><div class="gmail_extra">

+</div><div class="gmail_extra">+    if (conf->oycv == NULL) {</div><div class="gmail_extra">+        conf->oycv = prev->oycv;</div><div class="gmail_extra">+    }</div><div class="gmail_extra">+</div><div class="gmail_extra">

     return NGX_CONF_OK;</div><div class="gmail_extra"> }</div><div class="gmail_extra"> </div><div class="gmail_extra">@@ -1474,6 +1543,68 @@ ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,</div><div class="gmail_extra">

 }</div><div class="gmail_extra"> </div><div class="gmail_extra"> </div><div class="gmail_extra">+static char *</div><div class="gmail_extra">+ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,</div><div class="gmail_extra">

+    void *conf)</div><div class="gmail_extra">+{</div><div class="gmail_extra">+    ngx_http_image_filter_conf_t *imcf = conf;</div><div class="gmail_extra">+</div><div class="gmail_extra">+    ngx_str_t                         *value;</div>

<div class="gmail_extra">+    ngx_http_complex_value_t           cv;</div><div class="gmail_extra">+    ngx_http_compile_complex_value_t   ccv;</div><div class="gmail_extra">+</div><div class="gmail_extra">+    value = cf->args->elts;</div>

<div class="gmail_extra">+</div><div class="gmail_extra">+    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));</div><div class="gmail_extra">+</div><div class="gmail_extra">+    <a href="http://ccv.cf">ccv.cf</a> = cf;</div>

<div class="gmail_extra">+    ccv.value = &value[1];</div><div class="gmail_extra">+    ccv.complex_value = &cv;</div><div class="gmail_extra">+    ccv.zero = 1;</div><div class="gmail_extra">+</div><div class="gmail_extra">

+    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {</div><div class="gmail_extra">+        return NGX_CONF_ERROR;</div><div class="gmail_extra">+    }</div><div class="gmail_extra">+</div><div class="gmail_extra">

+    if (cv.lengths == NULL) {</div><div class="gmail_extra">+        imcf->crop_offset_x = ngx_http_image_filter_value(&value[1]);</div><div class="gmail_extra">+</div><div class="gmail_extra">+    } else {</div>
<div class="gmail_extra">
+        imcf->oxcv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));</div><div class="gmail_extra">+        if (imcf->oxcv == NULL) {</div><div class="gmail_extra">+            return NGX_CONF_ERROR;</div>

<div class="gmail_extra">+        }</div><div class="gmail_extra">+</div><div class="gmail_extra">+        *imcf->oxcv = cv;</div><div class="gmail_extra">+    }</div><div class="gmail_extra">+</div><div class="gmail_extra">

+    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));</div><div class="gmail_extra">+</div><div class="gmail_extra">+    <a href="http://ccv.cf">ccv.cf</a> = cf;</div><div class="gmail_extra">+    ccv.value = &value[2];</div>

<div class="gmail_extra">+    ccv.complex_value = &cv;</div><div class="gmail_extra">+    ccv.zero = 1;</div><div class="gmail_extra">+</div><div class="gmail_extra">+    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {</div>

<div class="gmail_extra">+        return NGX_CONF_ERROR;</div><div class="gmail_extra">+    }</div><div class="gmail_extra">+</div><div class="gmail_extra">+    if (cv.lengths == NULL) {</div><div class="gmail_extra">+        imcf->crop_offset_y = ngx_http_image_filter_value(&value[2]);</div>

<div class="gmail_extra">+</div><div class="gmail_extra">+    } else {</div><div class="gmail_extra">+        imcf->oycv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));</div><div class="gmail_extra">+        if (imcf->oycv == NULL) {</div>

<div class="gmail_extra">+            return NGX_CONF_ERROR;</div><div class="gmail_extra">+        }</div><div class="gmail_extra">+</div><div class="gmail_extra">+        *imcf->oycv = cv;</div><div class="gmail_extra">

+    }</div><div class="gmail_extra">+</div><div class="gmail_extra">+    return NGX_CONF_OK;</div><div class="gmail_extra">+}</div><div class="gmail_extra">+</div><div class="gmail_extra">+</div><div class="gmail_extra">

 static ngx_int_t</div><div class="gmail_extra"> ngx_http_image_filter_init(ngx_conf_t *cf)</div><div class="gmail_extra"> {</div><div><br></div><br><br><div class="gmail_quote">On 4 December 2012 17:08, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hello!<br>
<br>
On Tue, Dec 04, 2012 at 09:35:07AM +0400, ivan babrou wrote:<br>
<br>
[...]<br>
<div class="im"><br>
> @@ -932,8 +953,27 @@ transparent:<br>
>                  return NULL;<br>
>              }<br>
><br>
> -            ox /= 2;<br>
> -            oy /= 2;<br>
> +            crop_offset_x = conf->crop_offset_x;<br>
> +            crop_offset_y = conf->crop_offset_y;<br>
<br>
</div>You may want to use shorter local variable names here.<br>
<br>
[...]<br>
<div><div class="h5"><br>
> @@ -1149,10 +1189,27 @@ ngx_http_image_filter_value(ngx_str_t *value)<br>
>          return (ngx_uint_t) -1;<br>
>      }<br>
><br>
> -    n = ngx_atoi(value->data, value->len);<br>
> +    if (ngx_strcmp(value->data, "center") == 0) {<br>
> +        return NGX_HTTP_IMAGE_OFFSET_CENTER;<br>
> +<br>
> +    } else if (ngx_strcmp(value->data, "left") == 0) {<br>
> +        return NGX_HTTP_IMAGE_OFFSET_LEFT;<br>
> +<br>
> +    } else if (ngx_strcmp(value->data, "right") == 0) {<br>
> +        return NGX_HTTP_IMAGE_OFFSET_RIGHT;<br>
> +<br>
> +    } else if (ngx_strcmp(value->data, "top") == 0) {<br>
> +        return NGX_HTTP_IMAGE_OFFSET_TOP;<br>
><br>
> -    if (n > 0) {<br>
> -        return (ngx_uint_t) n;<br>
> +    } else if (ngx_strcmp(value->data, "bottom") == 0) {<br>
> +        return NGX_HTTP_IMAGE_OFFSET_BOTTOM;<br>
> +<br>
> +    } else {<br>
> +        n = ngx_atoi(value->data, value->len);<br>
> +<br>
> +        if (n > 0) {<br>
> +            return (ngx_uint_t) n;<br>
> +        }<br>
>      }<br>
<br>
</div></div>I'm not happy with this change as it degrades performance of other<br>
values evaluation.  It probably may be left as a single function,<br>
but I would rather fallback to all this string matching logic only<br>
if the value doesn't happen to be a number, i.e. if ngx_atoi()<br>
fails.<br>
<br>
I also don't really see how you are going to distinguish constants<br>
returned from actual values specified by numbers.<br>
<br>
[...]<br>
<div class="im"><br>
> @@ -1223,6 +1282,17 @@ ngx_http_image_filter_merge_conf(ngx_conf_t<br>
> *cf, void *parent, void *child)<br>
>      ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,<br>
>                                1 * 1024 * 1024);<br>
><br>
> +    ngx_conf_merge_uint_value(conf->crop_offset_x,<br>
> prev->crop_offset_x, NGX_HTTP_IMAGE_OFFSET_CENTER);<br>
> +    ngx_conf_merge_uint_value(conf->crop_offset_y,<br>
> prev->crop_offset_y, NGX_HTTP_IMAGE_OFFSET_CENTER);<br>
> +<br>
> +    if (conf->oxcv == NULL) {<br>
> +        conf->oxcv = prev->oxcv;<br>
> +    }<br>
> +<br>
> +    if (conf->oycv == NULL) {<br>
> +        conf->oycv = prev->oycv;<br>
> +    }<br>
<br>
</div>I believe I've already outlined in the previous review that this<br>
merge logic is wrong, as well as a style problem with lines longer<br>
than 80 chars here.<br>
<br>
[...]<br>
<div class=""><div class="h5"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.com/support.html" target="_blank">http://nginx.com/support.html</a><br>
<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br>Regards, Ian Babrou<br><a href="http://bobrik.name" target="_blank">http://bobrik.name</a> <a href="http://twitter.com/ibobrik" target="_blank">http://twitter.com/ibobrik</a> skype:i.babrou<br>


</div>