image_filter enhancement

[Maxim Dounin]

Hello!

On Mon, Nov 12, 2012 at 09:09:50PM +0400, ivan babrou wrote:

> Here's the patch.

[...]

> @@ -150,6 +159,13 @@ static ngx_command_t  ngx_http_image_filter_commands[] = {
>        offsetof(ngx_http_image_filter_conf_t, buffer_size),
>        NULL },
> 
> +    { ngx_string("image_filter_offset"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      ngx_http_image_filter_offset,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      0,
> +      NULL },
> +
>        ngx_null_command
>  };

>From here it looks like the patch needs more work.  Though 
functionality proposed is probably needed, as it's second patch 
for the crop offset posted this year.  See here for the one posted 
previously by Maxim Bublis:

http://mailman.nginx.org/pipermail/nginx-devel/2012-April/002156.html

Maxim's aproach is a bit better in some aspectes as a) it calls 
crop offset as crop offset, and b) it allows to configure crop 
offsets via variables.  On the other hand, his patch asks for 
cleaner code and more user-friendly configuration, as "left top" is 
much easier to understand compared to "0 0".

Overral, it will be probably fine to have both Maxim Bublis's and 
your patches merged to have powerfull yet simple to configure 
functionality.  From user perspective it would be fine to have 
syntax similar to background-position CSS property
(http://www.w3.org/TR/CSS21/colors.html#propdef-background-position).

> 
> @@ -911,14 +927,12 @@ transparent:
> 
>          if ((ngx_uint_t) dx > ctx->max_width) {
>              ox = dx - ctx->max_width;
> -
>          } else {
>              ox = 0;
>          }
> 
>          if ((ngx_uint_t) dy > ctx->max_height) {
>              oy = dy - ctx->max_height;
> -
>          } else {
>              oy = 0;
>          }

Just a side note: please avoid unrelated changes.

[...]

> @@ -1078,6 +1101,7 @@ ngx_http_image_out(ngx_http_request_t *r,
> ngx_uint_t type, gdImagePtr img,
> 
>      out = NULL;
> 
> +
>      switch (type) {
> 
>      case NGX_HTTP_IMAGE_JPEG:

Same here.

[...]

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