<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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>
<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>
<div class="im"><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></blockquote><div><br></div><div style>I assume that ngx_atoi will return NGX_ERROR if it's not number. If number is ok, <span style="color:rgb(80,0,80)">(ngx_uint_t) n will be returned. If it's not a number, string checks will be performed. If even crop offset value is wrong then </span><span style="color:rgb(80,0,80)">NGX_HTTP_IMAGE_OFFSET_CENTER will be returned which is default behaviour and zero at the same time. So if you pass incorrect number and expect a number then you'll just get zero. Just like it was before.</span></div>
<div style><span style="color:rgb(80,0,80)"><br></span></div><div style><span style="color:rgb(80,0,80)">Cropping with offsets that is specified in percents seems more specific task. I don't think this is required.</span></div>
<div><br></div><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">
[...]<br>
<div class="im"><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 class="im"><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 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
</div></div>