<div dir="ltr">Is there anything else I should fix?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On 7 January 2013 17:01, ivan babrou <span dir="ltr"><<a href="mailto:ibobrik@gmail.com" target="_blank">ibobrik@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Btw, here's latest version with zero-termination and other fixes:<div><br></div><div><div>diff --git a/ngx_http_image_filter_module.c b/ngx_http_image_filter_module.c</div>

<div>index 3aee1a4..b086e3c 100644</div><div class="im">
<div>--- a/ngx_http_image_filter_module.c</div><div>+++ b/ngx_http_image_filter_module.c</div><div>@@ -32,6 +32,11 @@</div><div> #define NGX_HTTP_IMAGE_GIF       2</div><div> #define NGX_HTTP_IMAGE_PNG       3</div><div>

 </div>
<div>+#define NGX_HTTP_IMAGE_OFFSET_CENTER    0</div><div>+#define NGX_HTTP_IMAGE_OFFSET_LEFT      1</div><div>+#define NGX_HTTP_IMAGE_OFFSET_RIGHT     2</div><div>+#define NGX_HTTP_IMAGE_OFFSET_TOP       3</div><div>+#define NGX_HTTP_IMAGE_OFFSET_BOTTOM    4</div>


<div> </div><div> #define NGX_HTTP_IMAGE_BUFFERED  0x08</div><div> </div><div>@@ -43,11 +48,15 @@ typedef struct {</div><div>     ngx_uint_t                   angle;</div><div>     ngx_uint_t                   jpeg_quality;</div>


<div>     ngx_uint_t                   sharpen;</div></div><div>+    ngx_uint_t                   offset_x;</div><div>+    ngx_uint_t                   offset_y;</div><div class="im"><div> </div><div>     ngx_flag_t                   transparency;</div>


<div> </div><div>     ngx_http_complex_value_t    *wcv;</div><div>     ngx_http_complex_value_t    *hcv;</div><div>+    ngx_http_complex_value_t    *oxcv;</div><div>+    ngx_http_complex_value_t    *oycv;</div><div>     ngx_http_complex_value_t    *acv;</div>


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


<div>     ngx_uint_t                   max_height;</div></div><div>+    ngx_uint_t                   offset_x;</div><div>+    ngx_uint_t                   offset_y;</div><div><div class="h5"><div>     ngx_uint_t                   angle;</div>

<div> </div>
<div>     ngx_uint_t                   phase;</div><div>@@ -110,6 +121,8 @@ static char *ngx_http_image_filter_jpeg_quality(ngx_conf_t *cf,</div><div>     ngx_command_t *cmd, void *conf);</div><div> static char *ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,</div>


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


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


+    { ngx_string("image_filter_crop_offset"),</div><div>+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,</div><div>+      ngx_http_image_filter_offset,</div><div>+      NGX_HTTP_LOC_CONF_OFFSET,</div>


<div>+      0,</div><div>+      NULL },</div><div>+</div><div>       ngx_null_command</div><div> };</div><div> </div><div>@@ -737,7 +757,8 @@ ngx_http_image_resize(ngx_http_request_t *r, ngx_http_image_filter_ctx_t *ctx)</div>


<div> {</div><div>     int                            sx, sy, dx, dy, ox, oy, ax, ay, size,</div><div>                                    colors, palette, transparent, sharpen,</div><div>-                                   red, green, blue, t;</div>


<div>+                                   red, green, blue, t,</div><div>+                                   offset_x, offset_y;</div><div>     u_char                        *out;</div><div>     ngx_buf_t                     *b;</div>


<div>     ngx_uint_t                     resize;</div><div>@@ -932,8 +953,24 @@ transparent:</div><div>                 return NULL;</div><div>             }</div><div> </div><div>-            ox /= 2;</div><div>-            oy /= 2;</div>


<div>+            offset_x = ngx_http_image_filter_get_value(r, conf->oxcv,</div></div></div><div>+                                                       conf->offset_x);</div><div class="im"><div>+            offset_y = ngx_http_image_filter_get_value(r, conf->oycv,</div>


</div><div>+                                                       conf->offset_y);</div><div class="im"><div>+</div><div>+            if (offset_x == NGX_HTTP_IMAGE_OFFSET_LEFT) {</div><div>+                ox = 0;</div>

<div>+</div><div>+            } else if (offset_x == NGX_HTTP_IMAGE_OFFSET_CENTER) {</div>
<div>+                ox /= 2;</div><div>+            }</div><div>+</div><div>+            if (offset_y == NGX_HTTP_IMAGE_OFFSET_TOP) {</div><div>+                oy = 0;</div><div>+</div><div>+            } else if (offset_y == NGX_HTTP_IMAGE_OFFSET_CENTER) {</div>


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


</div><div>@@ -1141,17 +1178,43 @@ ngx_http_image_filter_get_value(ngx_http_request_t *r,</div><div> </div><div> </div><div> static ngx_uint_t</div><div>-ngx_http_image_filter_value(ngx_str_t *value)</div><div>+ngx_http_image_filter_value(ngx_str_t *v)</div>


<div> {</div><div>     ngx_int_t  n;</div><div> </div><div>-    if (value->len == 1 && value->data[0] == '-') {</div><div>+    if (v->len == 1 && v->data[0] == '-') {</div><div>


         return (ngx_uint_t) -1;</div><div>     }</div><div> </div><div>-    n = ngx_atoi(value->data, value->len);</div><div>+    n = ngx_atoi(v->data, v->len);</div><div class="im"><div>+</div><div>+    if (n == NGX_ERROR) {</div>


<div>+</div></div><div>+        if (v->len == sizeof("left") - 1</div><div>+            && ngx_strncmp(v->data, "left", v->len) == 0)</div><div class="im"><div>+        {</div><div>+            return NGX_HTTP_IMAGE_OFFSET_LEFT;</div>


<div>+</div></div><div>+        } else if (v->len == sizeof("right") - 1</div><div>+                   && ngx_strncmp(v->data, "right", sizeof("right") - 1) == 0)</div><div class="im">

<div>+        {</div>
<div>+            return NGX_HTTP_IMAGE_OFFSET_RIGHT;</div><div>+</div></div><div>+        } else if (v->len == sizeof("top") - 1</div><div>+                   && ngx_strncmp(v->data, "top", sizeof("top") - 1) == 0)</div>


<div>+        {</div><div>+            return NGX_HTTP_IMAGE_OFFSET_TOP;</div><div> </div><div>-    if (n > 0) {</div><div>+        } else if (v->len == sizeof("bottom") - 1</div><div>+                   && ngx_strncmp(v->data, "bottom", sizeof("bottom") - 1) == 0)</div>

<div class="im">
<div>+        {</div><div>+            return NGX_HTTP_IMAGE_OFFSET_BOTTOM;</div><div>+</div><div>+        } else {</div><div>+            return NGX_HTTP_IMAGE_OFFSET_CENTER;</div><div>+        }</div><div>+</div><div>+    } else if (n > 0) {</div>


<div>         return (ngx_uint_t) n;</div><div>     }</div><div> </div></div><div>@@ -1175,6 +1238,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)</div><div class="im"><div>     conf->angle = NGX_CONF_UNSET_UINT;</div>

<div>     conf->transparency = NGX_CONF_UNSET;</div>
<div>     conf->buffer_size = NGX_CONF_UNSET_SIZE;</div></div><div>+    conf->offset_x = NGX_CONF_UNSET_UINT;</div><div>+    conf->offset_y = NGX_CONF_UNSET_UINT;</div><div> </div><div>     return conf;</div><div>

 }</div>
<div>@@ -1230,6 +1295,24 @@ ngx_http_image_filter_merge_conf(ngx_conf_t *cf, void *parent, void *child)</div><div class="im"><div>     ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,</div><div>                               1 * 1024 * 1024);</div>


<div> </div></div><div>+    if (conf->offset_x == NGX_CONF_UNSET_UINT) {</div><div>+        ngx_conf_merge_uint_value(conf->offset_x, prev->offset_x,</div><div class="im"><div>+                                  NGX_HTTP_IMAGE_OFFSET_CENTER);</div>


<div>+</div><div>+        if (conf->oxcv == NULL) {</div><div>+            conf->oxcv = prev->oxcv;</div><div>+        }</div><div>+    }</div><div>+</div></div><div>+    if (conf->offset_y == NGX_CONF_UNSET_UINT) {</div>


<div>+        ngx_conf_merge_uint_value(conf->offset_y, prev->offset_y,</div><div class="im"><div>+                                  NGX_HTTP_IMAGE_OFFSET_CENTER);</div><div>+</div><div>+        if (conf->oycv == NULL) {</div>

<div>
+            conf->oycv = prev->oycv;</div><div>+        }</div><div>+    }</div><div>+</div><div>     return NGX_CONF_OK;</div><div> }</div><div> </div></div><div>@@ -1481,6 +1564,66 @@ ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,</div>

<div class="im">
<div> }</div><div> </div><div> </div><div>+static char *</div><div>+ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,</div><div>+    void *conf)</div><div>+{</div><div>+    ngx_http_image_filter_conf_t *imcf = conf;</div>


<div>+</div><div>+    ngx_str_t                         *value;</div><div>+    ngx_http_complex_value_t           cv;</div><div>+    ngx_http_compile_complex_value_t   ccv;</div><div>+</div><div>+    value = cf->args->elts;</div>


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

+    ccv.complex_value = &cv;</div>
<div>+</div></div><div class="im"><div>+    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {</div><div>+        return NGX_CONF_ERROR;</div><div>+    }</div><div>+</div><div>+    if (cv.lengths == NULL) {</div></div>

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


<div>+        }</div><div>+</div><div>+        *imcf->oxcv = cv;</div><div>+    }</div><div>+</div><div>+    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));</div><div>+</div><div>+    <a href="http://ccv.cf" target="_blank">ccv.cf</a> = cf;</div>


<div>+    ccv.value = &value[2];</div><div>+    ccv.complex_value = &cv;</div><div>+</div></div><div class="im"><div>+    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {</div><div>+        return NGX_CONF_ERROR;</div>

<div>+    }</div>
<div>+</div><div>+    if (cv.lengths == NULL) {</div></div><div>+        imcf->offset_y = ngx_http_image_filter_value(&value[2]);</div><div class="im"><div>+</div><div>+    } else {</div><div>+        imcf->oycv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));</div>


<div>+        if (imcf->oycv == NULL) {</div><div>+            return NGX_CONF_ERROR;</div><div>+        }</div><div>+</div><div>+        *imcf->oycv = cv;</div><div>+    }</div><div>+</div><div>+    return NGX_CONF_OK;</div>


<div>+}</div><div>+</div><div>+</div><div> static ngx_int_t</div><div> ngx_http_image_filter_init(ngx_conf_t *cf)</div><div> {</div></div></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">

<div class="im">On 25 December 2012 20:05, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>
</div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div><br>
On Thu, Dec 20, 2012 at 09:01:38PM +0400, ivan babrou wrote:<br>
<br>
> I've send patch for configuration semantics in separate thread.<br>
><br>
> Here's lastest version of my patch that may be applied after. Is there<br>
> something else that I should fix?<br>
<br>
</div>I would like to see clarification on how are you going to handle<br>
just bare numbers specified.  As of now the code seems to don't do<br>
any special processing and seems to assume all values are correct,<br>
which might not be true and will result in some mostly arbitray<br>
crop offset selection.  It might make sense to actually support<br>
percents as in Maxim Bublis's patch[1].<br>
<br>
[1] <a href="http://mailman.nginx.org/pipermail/nginx-devel/2012-April/002156.html" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2012-April/002156.html</a><br>
<br>
[...]<br>
<div><br>
> @@ -1151,7 +1188,24 @@ ngx_http_image_filter_value(ngx_str_t *value)<br>
><br>
>      n = ngx_atoi(value->data, value->len);<br>
><br>
> -    if (n > 0) {<br>
> +    if (n == NGX_ERROR) {<br>
> +        if (ngx_strncmp(value->data, "left", value->len) == 0) {<br>
> +            return NGX_HTTP_IMAGE_OFFSET_LEFT;<br>
> +<br>
> +        } else if (ngx_strncmp(value->data, "right", value->len) == 0) {<br>
> +            return NGX_HTTP_IMAGE_OFFSET_RIGHT;<br>
> +<br>
> +        } else if (ngx_strncmp(value->data, "top", value->len) == 0) {<br>
> +            return NGX_HTTP_IMAGE_OFFSET_TOP;<br>
> +<br>
> +        } else if (ngx_strncmp(value->data, "bottom", value->len) == 0) {<br>
> +            return NGX_HTTP_IMAGE_OFFSET_BOTTOM;<br>
> +<br>
> +        } else {<br>
> +            return NGX_HTTP_IMAGE_OFFSET_CENTER;<br>
> +        }<br>
> +<br>
> +    } else if (n > 0) {<br>
>          return (ngx_uint_t) n;<br>
>      }<br>
><br>
<br>
</div>The ngx_strncmp() checks are incorrect, see here:<br>
<a href="http://mailman.nginx.org/pipermail/nginx-devel/2012-December/003065.html" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2012-December/003065.html</a><br>
<br>
You may also move them below "if (n > 0)" the check for better<br>
readability.<br>
<div><br>
> @@ -1175,6 +1229,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)<br>
>      conf->angle = NGX_CONF_UNSET_UINT;<br>
>      conf->transparency = NGX_CONF_UNSET;<br>
>      conf->buffer_size = NGX_CONF_UNSET_SIZE;<br>
> +    conf->crop_offset_x = NGX_CONF_UNSET_UINT;<br>
> +    conf->crop_offset_y = NGX_CONF_UNSET_UINT;<br>
<br>
</div>The "crop_" prefix probably should be dropped here for better<br>
readability (and to match other names in the code like "oxcv").<br>
<br>
[...]<br>
<div><div><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" target="_blank">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></div></div><br><br clear="all"><div class="im"><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
</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
</div>