<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>