Hello!

On Tue, Dec 04, 2012 at 09:35:07AM +0400, ivan babrou wrote:

[...]

> @@ -932,8 +953,27 @@ transparent:
> return NULL;
> }
>
> - ox /= 2;
> - oy /= 2;
> + crop_offset_x = conf->crop_offset_x;
> + crop_offset_y = conf->crop_offset_y;

You may want to use shorter local variable names here.

[...]

> @@ -1149,10 +1189,27 @@ ngx_http_image_filter_value(ngx_str_t *value)
> return (ngx_uint_t) -1;
> }
>
> - n = ngx_atoi(value->data, value->len);
> + if (ngx_strcmp(value->data, "center") == 0) {
> + return NGX_HTTP_IMAGE_OFFSET_CENTER;
> +
> + } else if (ngx_strcmp(value->data, "left") == 0) {
> + return NGX_HTTP_IMAGE_OFFSET_LEFT;
> +
> + } else if (ngx_strcmp(value->data, "right") == 0) {
> + return NGX_HTTP_IMAGE_OFFSET_RIGHT;
> +
> + } else if (ngx_strcmp(value->data, "top") == 0) {
> + return NGX_HTTP_IMAGE_OFFSET_TOP;
>
> - if (n > 0) {
> - return (ngx_uint_t) n;
> + } else if (ngx_strcmp(value->data, "bottom") == 0) {
> + return NGX_HTTP_IMAGE_OFFSET_BOTTOM;
> +
> + } else {
> + n = ngx_atoi(value->data, value->len);
> +
> + if (n > 0) {
> + return (ngx_uint_t) n;
> + }
> }

I'm not happy with this change as it degrades performance of other
values evaluation. It probably may be left as a single function,
but I would rather fallback to all this string matching logic only
if the value doesn't happen to be a number, i.e. if ngx_atoi()
fails.

I also don't really see how you are going to distinguish constants
returned from actual values specified by numbers.

[...]

> @@ -1223,6 +1282,17 @@ ngx_http_image_filter_merge_conf(ngx_conf_t
> *cf, void *parent, void *child)
> ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,
> 1 * 1024 * 1024);
>
> + ngx_conf_merge_uint_value(conf->crop_offset_x,
> prev->crop_offset_x, NGX_HTTP_IMAGE_OFFSET_CENTER);
> + ngx_conf_merge_uint_value(conf->crop_offset_y,
> prev->crop_offset_y, NGX_HTTP_IMAGE_OFFSET_CENTER);
> +
> + if (conf->oxcv == NULL) {
> + conf->oxcv = prev->oxcv;
> + }
> +
> + if (conf->oycv == NULL) {
> + conf->oycv = prev->oycv;
> + }

I believe I've already outlined in the previous review that this
merge logic is wrong, as well as a style problem with lines longer
than 80 chars here.

[...]

--
Maxim Dounin
http://nginx.com/support.html

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel