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