Reworked again. Looks much better now! :)

I renamed configuration directive to image_filter_crop_offset.

diff --git a/ngx_http_image_filter_module.c b/ngx_http_image_filter_module.c
index c853c33..4d0ce95 100644
--- a/ngx_http_image_filter_module.c
+++ b/ngx_http_image_filter_module.c
@@ -32,6 +32,11 @@
#define NGX_HTTP_IMAGE_GIF 2
#define NGX_HTTP_IMAGE_PNG 3

+#define NGX_HTTP_IMAGE_OFFSET_CENTER 0
+#define NGX_HTTP_IMAGE_OFFSET_LEFT 1
+#define NGX_HTTP_IMAGE_OFFSET_RIGHT 2
+#define NGX_HTTP_IMAGE_OFFSET_TOP 3
+#define NGX_HTTP_IMAGE_OFFSET_BOTTOM 4

#define NGX_HTTP_IMAGE_BUFFERED 0x08

@@ -43,11 +48,15 @@ typedef struct {
ngx_uint_t angle;
ngx_uint_t jpeg_quality;
ngx_uint_t sharpen;
+ ngx_uint_t crop_offset_x;
+ ngx_uint_t crop_offset_y;

ngx_flag_t transparency;

ngx_http_complex_value_t *wcv;
ngx_http_complex_value_t *hcv;
+ ngx_http_complex_value_t *oxv;
+ ngx_http_complex_value_t *oyv;
ngx_http_complex_value_t *acv;
ngx_http_complex_value_t *jqcv;
ngx_http_complex_value_t *shcv;
@@ -66,6 +75,8 @@ typedef struct {
ngx_uint_t height;
ngx_uint_t max_width;
ngx_uint_t max_height;
+ ngx_uint_t crop_offset_x;
+ ngx_uint_t crop_offset_y;
ngx_uint_t angle;

ngx_uint_t phase;
@@ -110,6 +121,8 @@ static char
*ngx_http_image_filter_jpeg_quality(ngx_conf_t *cf,
ngx_command_t *cmd, void *conf);
static char *ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,
void *conf);
+static char *ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,
+ void *conf);
static ngx_int_t ngx_http_image_filter_init(ngx_conf_t *cf);


@@ -150,6 +163,13 @@ static ngx_command_t ngx_http_image_filter_commands[] = {
offsetof(ngx_http_image_filter_conf_t, buffer_size),
NULL },

+ { ngx_string("image_filter_crop_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
};

@@ -737,7 +757,8 @@ ngx_http_image_resize(ngx_http_request_t *r,
ngx_http_image_filter_ctx_t *ctx)
{
int sx, sy, dx, dy, ox, oy, ax, ay, size,
colors, palette, transparent, sharpen,
- red, green, blue, t;
+ red, green, blue, t,
+ crop_offset_x, crop_offset_y;
u_char *out;
ngx_buf_t *b;
ngx_uint_t resize;
@@ -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;
+
+ crop_offset_x = ngx_http_image_filter_get_value(r, conf->oxv,
+ crop_offset_x);
+ crop_offset_y = ngx_http_image_filter_get_value(r, conf->oyv,
+ crop_offset_y);
+
+ if (crop_offset_x == NGX_HTTP_IMAGE_OFFSET_LEFT) {
+ ox = 0;
+
+ } else if (crop_offset_x == NGX_HTTP_IMAGE_OFFSET_CENTER) {
+ ox /= 2;
+ }
+
+ if (crop_offset_y == NGX_HTTP_IMAGE_OFFSET_TOP) {
+ oy = 0;
+
+ } else if (crop_offset_y == NGX_HTTP_IMAGE_OFFSET_CENTER) {
+ oy /= 2;
+ }

ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
"image crop: %d x %d @ %d x %d",
@@ -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;
+ }
}

return 0;
@@ -1175,6 +1232,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)
conf->angle = NGX_CONF_UNSET_UINT;
conf->transparency = NGX_CONF_UNSET;
conf->buffer_size = NGX_CONF_UNSET_SIZE;
+ conf->crop_offset_x = NGX_CONF_UNSET_UINT;
+ conf->crop_offset_y = NGX_CONF_UNSET_UINT;

return conf;
}
@@ -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->oxv == NULL) {
+ conf->oxv = prev->oxv;
+ }
+
+ if (conf->oyv == NULL) {
+ conf->oyv = prev->oyv;
+ }
+
return NGX_CONF_OK;
}

@@ -1474,6 +1544,68 @@ ngx_http_image_filter_sharpen(ngx_conf_t *cf,
ngx_command_t *cmd,
}


+static char *
+ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,
+ void *conf)
+{
+ ngx_http_image_filter_conf_t *imcf = conf;
+
+ ngx_str_t *value;
+ ngx_http_complex_value_t cv;
+ ngx_http_compile_complex_value_t ccv;
+
+ value = cf->args->elts;
+
+ ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
+
+ ccv.cf = cf;
+ ccv.value = &value[1];
+ ccv.complex_value = &cv;
+ ccv.zero = 1;
+
+ if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
+ if (cv.lengths == NULL) {
+ imcf->crop_offset_x = ngx_http_image_filter_value(&value[1]);
+
+ } else {
+ imcf->oxv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));
+ if (imcf->oxv == NULL) {
+ return NGX_CONF_ERROR;
+ }
+
+ *imcf->oxv = cv;
+ }
+
+ ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
+
+ ccv.cf = cf;
+ ccv.value = &value[2];
+ ccv.complex_value = &cv;
+ ccv.zero = 1;
+
+ if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
+ if (cv.lengths == NULL) {
+ imcf->crop_offset_y = ngx_http_image_filter_value(&value[2]);
+
+ } else {
+ imcf->oyv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));
+ if (imcf->oyv == NULL) {
+ return NGX_CONF_ERROR;
+ }
+
+ *imcf->oyv = cv;
+ }
+
+ return NGX_CONF_OK;
+}
+
+
static ngx_int_t
ngx_http_image_filter_init(ngx_conf_t *cf)
{

On 27 November 2012 20:03, Maxim Dounin <mdounin@mdounin.ru> wrote:
> Hello!
>
> On Tue, Nov 27, 2012 at 12:10:13AM +0400, ivan babrou wrote:
>
>> I tried to rework patch. Now it supports configuration through
>> variables. Variables ox and oy renamed to crop_offset_x and
>> crop_offset_y.
>>
>> I'm confused about this line:
>> + value.data[value.len] = 0;
>>
>> Probably I don't understand something about memory management in nginx.
>
> See below.
>
>>
>>
>> diff --git a/ngx_http_image_filter_module.c b/ngx_http_image_filter_module.c
>> index c853c33..15f8d17 100644
>> --- a/ngx_http_image_filter_module.c
>> +++ b/ngx_http_image_filter_module.c
>> @@ -32,6 +32,14 @@
>> #define NGX_HTTP_IMAGE_GIF 2
>> #define NGX_HTTP_IMAGE_PNG 3
>>
>> +#define NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL 0
>> +#define NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL 1
>> +
>> +#define NGX_HTTP_IMAGE_OFFSET_CENTER 0
>> +#define NGX_HTTP_IMAGE_OFFSET_LEFT 1
>> +#define NGX_HTTP_IMAGE_OFFSET_RIGHT 2
>> +#define NGX_HTTP_IMAGE_OFFSET_TOP 3
>> +#define NGX_HTTP_IMAGE_OFFSET_BOTTOM 4
>>
>> #define NGX_HTTP_IMAGE_BUFFERED 0x08
>>
>> @@ -43,11 +51,15 @@ typedef struct {
>> ngx_uint_t angle;
>> ngx_uint_t jpeg_quality;
>> ngx_uint_t sharpen;
>> + ngx_uint_t crop_offset_x;
>> + ngx_uint_t crop_offset_y;
>>
>> ngx_flag_t transparency;
>>
>> ngx_http_complex_value_t *wcv;
>> ngx_http_complex_value_t *hcv;
>> + ngx_http_complex_value_t *oxv;
>> + ngx_http_complex_value_t *oyv;
>
> Probably you mean "oxcv", "oycv" - "cv" here is abbreviation of
> "complex value".
>
>> ngx_http_complex_value_t *acv;
>> ngx_http_complex_value_t *jqcv;
>> ngx_http_complex_value_t *shcv;
>> @@ -66,6 +78,8 @@ typedef struct {
>> ngx_uint_t height;
>> ngx_uint_t max_width;
>> ngx_uint_t max_height;
>> + ngx_uint_t crop_offset_x;
>> + ngx_uint_t crop_offset_y;
>> ngx_uint_t angle;
>>
>> ngx_uint_t phase;
>> @@ -98,6 +112,15 @@ static u_char
>> *ngx_http_image_out(ngx_http_request_t *r, ngx_uint_t type,
>> static void ngx_http_image_cleanup(void *data);
>> static ngx_uint_t ngx_http_image_filter_get_value(ngx_http_request_t *r,
>> ngx_http_complex_value_t *cv, ngx_uint_t v);
>> +static void
>> +ngx_http_image_filter_crop_offset(ngx_http_request_t *r,
>> + ngx_http_complex_value_t *cv, ngx_uint_t t, ngx_uint_t *v);
>> +static void
>> +ngx_http_image_filter_vertical_crop_offset(ngx_str_t *value,
>> + ngx_uint_t *v);
>> +static void
>> +ngx_http_image_filter_horizontal_crop_offset(ngx_str_t *value,
>> + ngx_uint_t *v);
>> static ngx_uint_t ngx_http_image_filter_value(ngx_str_t *value);
>>
>>
>> @@ -110,6 +133,8 @@ static char
>> *ngx_http_image_filter_jpeg_quality(ngx_conf_t *cf,
>> ngx_command_t *cmd, void *conf);
>> static char *ngx_http_image_filter_sharpen(ngx_conf_t *cf, ngx_command_t *cmd,
>> void *conf);
>> +static char *ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,
>> + void *conf);
>> static ngx_int_t ngx_http_image_filter_init(ngx_conf_t *cf);
>>
>>
>> @@ -150,6 +175,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
>> };
>>
>> @@ -529,6 +561,22 @@ ngx_http_image_process(ngx_http_request_t *r)
>> return NULL;
>> }
>>
>> + if (conf->oxv == NULL) {
>> + ctx->crop_offset_x = conf->crop_offset_x;
>> + } else {
>> + ngx_http_image_filter_crop_offset(r, conf->oxv,
>> +
>> NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL,
>> + &ctx->crop_offset_x);
>> + }
>> +
>> + if (conf->oyv == NULL) {
>> + ctx->crop_offset_y = conf->crop_offset_y;
>> + } else {
>> + ngx_http_image_filter_crop_offset(r, conf->oyv,
>> + NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL,
>> + &ctx->crop_offset_y);
>> + }
>> +
>
> 1) You want to follow pattern in the previous code, i.e.
> foo = ngx_http_image_filter_get_value(r, conf->foocv, conf->foo);
>
> (With additional support of top/bottom/left/right literals.)
>
> 2) I don't think you need to calculate offsets to decide whether
> image is ok as is or we need to resize it. Hence it's should
> be ok to move the code to where it's needed.
>
> 3) With (2) fixed you probably don't need variables in ctx
> anymore.
>
>> if (rc == NGX_OK
>> && ctx->width <= ctx->max_width
>> && ctx->height <= ctx->max_height
>> @@ -932,8 +980,17 @@ transparent:
>> return NULL;
>> }
>>
>> - ox /= 2;
>> - oy /= 2;
>> + if (ctx->crop_offset_x == NGX_HTTP_IMAGE_OFFSET_LEFT) {
>> + ox = 0;
>> + } else if (ctx->crop_offset_x == NGX_HTTP_IMAGE_OFFSET_CENTER) {
>> + ox /= 2;
>> + }
>> +
>> + if (ctx->crop_offset_y == NGX_HTTP_IMAGE_OFFSET_TOP) {
>> + oy = 0;
>> + } else if (ctx->crop_offset_y == NGX_HTTP_IMAGE_OFFSET_CENTER) {
>> + oy /= 2;
>> + }
>
> Note: this needs empty lines before "} else if ..." to match
> style. It should be changed anyway though, due to reasons
> outlined above.
>
>>
>> ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>> "image crop: %d x %d @ %d x %d",
>> @@ -1139,6 +1196,52 @@ ngx_http_image_filter_get_value(ngx_http_request_t *r,
>> return ngx_http_image_filter_value(&val);
>> }
>>
>> +static void
>> +ngx_http_image_filter_crop_offset(ngx_http_request_t *r,
>> + ngx_http_complex_value_t *cv, ngx_uint_t t, ngx_uint_t *v)
>> +{
>> + ngx_str_t value;
>> +
>> + if (ngx_http_complex_value(r, cv, &value) != NGX_OK) {
>> + return;
>> + }
>> +
>> + value.data[value.len] = 0;
>
> This is incorrect and will result in memory corruption.
>
> If you need null-terminated string, right aproach is to set
> ccv.zero flag while calling ngx_http_compile_complex_value(). On
> the other hand, it might be good idea to don't assume
> null-termination instead.
>
>> +
>> + if (t == NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL) {
>> + ngx_http_image_filter_horizontal_crop_offset(&value, v);
>> + } else if (t == NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL) {
>> + ngx_http_image_filter_vertical_crop_offset(&value, v);
>> + }
>> +}
>> +
>> +
>> +static void
>> +ngx_http_image_filter_horizontal_crop_offset(ngx_str_t *value,
>> + ngx_uint_t *v)
>> +{
>> + if (ngx_strcmp(value->data, "center") == 0) {
>> + *v = NGX_HTTP_IMAGE_OFFSET_CENTER;
>> + } else if (ngx_strcmp(value->data, "left") == 0) {
>> + *v = NGX_HTTP_IMAGE_OFFSET_LEFT;
>> + } else if (ngx_strcmp(value->data, "right") == 0) {
>> + *v = NGX_HTTP_IMAGE_OFFSET_RIGHT;
>> + }
>> +}
>> +
>> +static void
>> +ngx_http_image_filter_vertical_crop_offset(ngx_str_t *value,
>> + ngx_uint_t *v)
>> +{
>> + if (ngx_strcmp(value->data, "center") == 0) {
>> + *v = NGX_HTTP_IMAGE_OFFSET_CENTER;
>> + } else if (ngx_strcmp(value->data, "top") == 0) {
>> + *v = NGX_HTTP_IMAGE_OFFSET_TOP;
>> + } else if (ngx_strcmp(value->data, "bottom") == 0) {
>> + *v = NGX_HTTP_IMAGE_OFFSET_BOTTOM;
>> + }
>> +}
>> +
>
> I don't think we need so many functions to do this simple task.
> It would be good idea simplify this.
>
> The NGX_HTTP_IMAGE_OFFSET_TYPE_HORIZONTAL and
> NGX_HTTP_IMAGE_OFFSET_TYPE_VERTICAL seem to be unneeded, just a
> boolean value would do the trick.
>
>> static ngx_uint_t
>> ngx_http_image_filter_value(ngx_str_t *value)
>> @@ -1175,6 +1278,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)
>> conf->angle = NGX_CONF_UNSET_UINT;
>> conf->transparency = NGX_CONF_UNSET;
>> conf->buffer_size = NGX_CONF_UNSET_SIZE;
>> + conf->crop_offset_x = NGX_CONF_UNSET_UINT;
>> + conf->crop_offset_y = NGX_CONF_UNSET_UINT;
>>
>> return conf;
>> }
>> @@ -1223,6 +1328,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->oxv == NULL) {
>> + conf->oxv = prev->oxv;
>> + }
>> +
>> + if (conf->oyv == NULL) {
>> + conf->oyv = prev->oyv;
>> + }
>> +
>
> Style: please keep lines shorter than 80 chars.
>
> Additionally, looking at the code I tend to think it's incorrect.
> That is, it's pattern you've followed is incorrect, not your code.
> E.g. the following config will result in $arg_q incorrectly used
> for quality in /image/, instead of "80" explicitly set:
>
> image_filter_jpeg_quality $arg_q;
>
> location /image/ {
> image_filter crop $arg_w $arg_h;
> image_filter_jpeg_quality 80;
> }
>
> This needs fixing.
>
>> return NGX_CONF_OK;
>> }
>>
>> @@ -1474,6 +1590,64 @@ ngx_http_image_filter_sharpen(ngx_conf_t *cf,
>> ngx_command_t *cmd,
>> }
>>
>>
>> +static char *
>> +ngx_http_image_filter_offset(ngx_conf_t *cf, ngx_command_t *cmd,
>> + void *conf)
>> +{
>> + ngx_http_image_filter_conf_t *imcf = conf;
>> +
>> + ngx_str_t *value;
>> + ngx_http_complex_value_t cv;
>> + ngx_http_compile_complex_value_t ccv;
>> +
>> + value = cf->args->elts;
>> +
>> + ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
>> +
>> + ccv.cf = cf;
>> + ccv.value = &value[1];
>> + ccv.complex_value = &cv;
>> +
>> + if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
>> + return NGX_CONF_ERROR;
>> + }
>> +
>> + if (cv.lengths == NULL) {
>> + ngx_http_image_filter_horizontal_crop_offset(&value[1],
>> &imcf->crop_offset_x);
>> + } else {
>> + imcf->oxv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));
>> + if (imcf->oxv == NULL) {
>> + return NGX_CONF_ERROR;
>> + }
>> +
>> + *imcf->oxv = cv;
>> + }
>> +
>> + ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
>> +
>> + ccv.cf = cf;
>> + ccv.value = &value[2];
>> + ccv.complex_value = &cv;
>> +
>> + if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
>> + return NGX_CONF_ERROR;
>> + }
>> +
>> + if (cv.lengths == NULL) {
>> + ngx_http_image_filter_vertical_crop_offset(&value[2],
>> &imcf->crop_offset_y);
>> + } else {
>> + imcf->oyv = ngx_palloc(cf->pool, sizeof(ngx_http_complex_value_t));
>> + if (imcf->oyv == NULL) {
>> + return NGX_CONF_ERROR;
>> + }
>> +
>> + *imcf->oyv = cv;
>> + }
>> +
>> + return NGX_CONF_OK;
>> +}
>> +
>> +
>> static ngx_int_t
>> ngx_http_image_filter_init(ngx_conf_t *cf)
>> {
>>
>>
>> On 15 November 2012 03:47, Maxim Dounin <mdounin@mdounin.ru> wrote:
>> > offsets via variables. On the other hand, his patch asks for
>>
>>
>>
>> --
>> Regards, Ian Babrou
>> http://bobrik.name http://twitter.com/ibobrik skype:i.babrou
>>
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> --
> Maxim Dounin
> http://nginx.com/support.html
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



--
Regards, Ian Babrou
http://bobrik.name http://twitter.com/ibobrik skype:i.babrou

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