I've send patch for configuration semantics in separate thread.<div><br></div><div>Here's lastest version of my patch that may be applied after. Is there something else that I should fix?</div><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..9bb6e12 100644</div><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>

+    ngx_uint_t                   crop_offset_x;</div><div>+    ngx_uint_t                   crop_offset_y;</div><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>+    ngx_uint_t                   crop_offset_x;</div><div>+    ngx_uint_t                   crop_offset_y;</div><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>+                                                       conf->crop_offset_x);</div><div>+            offset_y = ngx_http_image_filter_get_value(r, conf->oycv,</div>

<div>+                                                       conf->crop_offset_y);</div><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>@@ -1151,7 +1188,24 @@ ngx_http_image_filter_value(ngx_str_t *value)</div><div> </div><div>     n = ngx_atoi(value->data, value->len);</div>

<div> </div><div>-    if (n > 0) {</div><div>+    if (n == NGX_ERROR) {</div><div>+        if (ngx_strncmp(value->data, "left", value->len) == 0) {</div><div>+            return NGX_HTTP_IMAGE_OFFSET_LEFT;</div>

<div>+</div><div>+        } else if (ngx_strncmp(value->data, "right", value->len) == 0) {</div><div>+            return NGX_HTTP_IMAGE_OFFSET_RIGHT;</div><div>+</div><div>+        } else if (ngx_strncmp(value->data, "top", value->len) == 0) {</div>

<div>+            return NGX_HTTP_IMAGE_OFFSET_TOP;</div><div>+</div><div>+        } else if (ngx_strncmp(value->data, "bottom", value->len) == 0) {</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>@@ -1175,6 +1229,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)</div><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>+    conf->crop_offset_x = NGX_CONF_UNSET_UINT;</div><div>+    conf->crop_offset_y = NGX_CONF_UNSET_UINT;</div><div> </div><div>     return conf;</div><div> }</div><div>@@ -1230,6 +1286,24 @@ ngx_http_image_filter_merge_conf(ngx_conf_t *cf, void *parent, void *child)</div>

<div>     ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,</div><div>                               1 * 1024 * 1024);</div><div> </div><div>+    if (conf->crop_offset_x == NGX_CONF_UNSET_UINT) {</div>

<div>+        ngx_conf_merge_uint_value(conf->crop_offset_x, prev->crop_offset_x,</div><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>+    if (conf->crop_offset_y == NGX_CONF_UNSET_UINT) {</div><div>+        ngx_conf_merge_uint_value(conf->crop_offset_y, prev->crop_offset_y,</div>

<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>@@ -1481,6 +1555,68 @@ ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,</div><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">ccv.cf</a> = cf;</div><div>+    ccv.value = &value[1];</div><div>+    ccv.complex_value = &cv;</div><div>+    ccv.zero = 1;</div><div>+</div><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>+        imcf->crop_offset_x = ngx_http_image_filter_value(&value[1]);</div><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">ccv.cf</a> = cf;</div><div>

+    ccv.value = &value[2];</div><div>+    ccv.complex_value = &cv;</div><div>+    ccv.zero = 1;</div><div>+</div><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>+        imcf->crop_offset_y = ngx_http_image_filter_value(&value[2]);</div><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><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 18 December 2012 14:53, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div class="im"><br>
On Sun, Dec 16, 2012 at 07:55:39PM +0400, ivan babrou wrote:<br>
<br>
> On 5 December 2012 18:52, Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>> wrote:<br>
><br>
> > Hello!<br>
> ><br>
> > On Tue, Dec 04, 2012 at 06:58:17PM +0400, ivan babrou wrote:<br>
> ><br>
> > > ><br>
> > > > 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.<br>
> > ><br>
> > ><br>
> > > As I see other config variables are buggy too. Should I fix them too?<br>
> ><br>
> > It would be fine to produce a patch which fixes existing config<br>
> > variables, and the patch to add crop offsets on top of it.<br>
><br>
><br>
> Can you give me some good examples to see "good practices" in nginx<br>
> configuration handling?<br>
<br>
</div>I would recommend looking into standard ngx_conf_merge_*_value()<br>
macros from src/core/ngx_conf_file.h, and rolling your code based<br>
on them with needed modifications and/or code reuse.  Something<br>
like this should work:<br>
<br>
    if (conf->jpeg_quality == NGX_CONF_UNSET_UINT) {<br>
        ngx_conf_merge_uint_value(conf->jpeg_quality, prev->jpeg_quality, 75);<br>
<br>
        if (conf->jqcv == NULL) {<br>
            conf->jqcv = prev->jqcv;<br>
<div class="HOEnZb"><div class="h5">        }<br>
    }<br>
<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>