Is there anything else I should fix?


On 7 January 2013 17:01, ivan babrou <ibobrik@gmail.com> wrote:

> Btw, here's latest version with zero-termination and other fixes:
>
> diff --git a/ngx_http_image_filter_module.c
> b/ngx_http_image_filter_module.c
> index 3aee1a4..b086e3c 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 offset_x;
> + ngx_uint_t offset_y;
>
> ngx_flag_t transparency;
>
> ngx_http_complex_value_t *wcv;
> ngx_http_complex_value_t *hcv;
> + ngx_http_complex_value_t *oxcv;
> + ngx_http_complex_value_t *oycv;
> 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 offset_x;
> + ngx_uint_t 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,
> + offset_x, offset_y;
> u_char *out;
> ngx_buf_t *b;
> ngx_uint_t resize;
> @@ -932,8 +953,24 @@ transparent:
> return NULL;
> }
>
> - ox /= 2;
> - oy /= 2;
> + offset_x = ngx_http_image_filter_get_value(r, conf->oxcv,
> + conf->offset_x);
> + offset_y = ngx_http_image_filter_get_value(r, conf->oycv,
> + conf->offset_y);
> +
> + if (offset_x == NGX_HTTP_IMAGE_OFFSET_LEFT) {
> + ox = 0;
> +
> + } else if (offset_x == NGX_HTTP_IMAGE_OFFSET_CENTER) {
> + ox /= 2;
> + }
> +
> + if (offset_y == NGX_HTTP_IMAGE_OFFSET_TOP) {
> + oy = 0;
> +
> + } else if (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",
> @@ -1141,17 +1178,43 @@ ngx_http_image_filter_get_value(ngx_http_request_t
> *r,
>
>
> static ngx_uint_t
> -ngx_http_image_filter_value(ngx_str_t *value)
> +ngx_http_image_filter_value(ngx_str_t *v)
> {
> ngx_int_t n;
>
> - if (value->len == 1 && value->data[0] == '-') {
> + if (v->len == 1 && v->data[0] == '-') {
> return (ngx_uint_t) -1;
> }
>
> - n = ngx_atoi(value->data, value->len);
> + n = ngx_atoi(v->data, v->len);
> +
> + if (n == NGX_ERROR) {
> +
> + if (v->len == sizeof("left") - 1
> + && ngx_strncmp(v->data, "left", v->len) == 0)
> + {
> + return NGX_HTTP_IMAGE_OFFSET_LEFT;
> +
> + } else if (v->len == sizeof("right") - 1
> + && ngx_strncmp(v->data, "right", sizeof("right") - 1)
> == 0)
> + {
> + return NGX_HTTP_IMAGE_OFFSET_RIGHT;
> +
> + } else if (v->len == sizeof("top") - 1
> + && ngx_strncmp(v->data, "top", sizeof("top") - 1) == 0)
> + {
> + return NGX_HTTP_IMAGE_OFFSET_TOP;
>
> - if (n > 0) {
> + } else if (v->len == sizeof("bottom") - 1
> + && ngx_strncmp(v->data, "bottom", sizeof("bottom") -
> 1) == 0)
> + {
> + return NGX_HTTP_IMAGE_OFFSET_BOTTOM;
> +
> + } else {
> + return NGX_HTTP_IMAGE_OFFSET_CENTER;
> + }
> +
> + } else if (n > 0) {
> return (ngx_uint_t) n;
> }
>
> @@ -1175,6 +1238,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->offset_x = NGX_CONF_UNSET_UINT;
> + conf->offset_y = NGX_CONF_UNSET_UINT;
>
> return conf;
> }
> @@ -1230,6 +1295,24 @@ 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);
>
> + if (conf->offset_x == NGX_CONF_UNSET_UINT) {
> + ngx_conf_merge_uint_value(conf->offset_x, prev->offset_x,
> + NGX_HTTP_IMAGE_OFFSET_CENTER);
> +
> + if (conf->oxcv == NULL) {
> + conf->oxcv = prev->oxcv;
> + }
> + }
> +
> + if (conf->offset_y == NGX_CONF_UNSET_UINT) {
> + ngx_conf_merge_uint_value(conf->offset_y, prev->offset_y,
> + NGX_HTTP_IMAGE_OFFSET_CENTER);
> +
> + if (conf->oycv == NULL) {
> + conf->oycv = prev->oycv;
> + }
> + }
> +
> return NGX_CONF_OK;
> }
>
> @@ -1481,6 +1564,66 @@ 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) {
> + imcf->offset_x = ngx_http_image_filter_value(&value[1]);
> +
> + } else {
> + imcf->oxcv = ngx_palloc(cf->pool,
> sizeof(ngx_http_complex_value_t));
> + if (imcf->oxcv == NULL) {
> + return NGX_CONF_ERROR;
> + }
> +
> + *imcf->oxcv = 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) {
> + imcf->offset_y = ngx_http_image_filter_value(&value[2]);
> +
> + } else {
> + imcf->oycv = ngx_palloc(cf->pool,
> sizeof(ngx_http_complex_value_t));
> + if (imcf->oycv == NULL) {
> + return NGX_CONF_ERROR;
> + }
> +
> + *imcf->oycv = cv;
> + }
> +
> + return NGX_CONF_OK;
> +}
> +
> +
> static ngx_int_t
> ngx_http_image_filter_init(ngx_conf_t *cf)
> {
>
>
>
> On 25 December 2012 20:05, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
>> Hello!
>>
>> On Thu, Dec 20, 2012 at 09:01:38PM +0400, ivan babrou wrote:
>>
>> > I've send patch for configuration semantics in separate thread.
>> >
>> > Here's lastest version of my patch that may be applied after. Is there
>> > something else that I should fix?
>>
>> I would like to see clarification on how are you going to handle
>> just bare numbers specified. As of now the code seems to don't do
>> any special processing and seems to assume all values are correct,
>> which might not be true and will result in some mostly arbitray
>> crop offset selection. It might make sense to actually support
>> percents as in Maxim Bublis's patch[1].
>>
>> [1] http://mailman.nginx.org/pipermail/nginx-devel/2012-April/002156.html
>>
>> [...]
>>
>> > @@ -1151,7 +1188,24 @@ ngx_http_image_filter_value(ngx_str_t *value)
>> >
>> > n = ngx_atoi(value->data, value->len);
>> >
>> > - if (n > 0) {
>> > + if (n == NGX_ERROR) {
>> > + if (ngx_strncmp(value->data, "left", value->len) == 0) {
>> > + return NGX_HTTP_IMAGE_OFFSET_LEFT;
>> > +
>> > + } else if (ngx_strncmp(value->data, "right", value->len) == 0)
>> {
>> > + return NGX_HTTP_IMAGE_OFFSET_RIGHT;
>> > +
>> > + } else if (ngx_strncmp(value->data, "top", value->len) == 0) {
>> > + return NGX_HTTP_IMAGE_OFFSET_TOP;
>> > +
>> > + } else if (ngx_strncmp(value->data, "bottom", value->len) ==
>> 0) {
>> > + return NGX_HTTP_IMAGE_OFFSET_BOTTOM;
>> > +
>> > + } else {
>> > + return NGX_HTTP_IMAGE_OFFSET_CENTER;
>> > + }
>> > +
>> > + } else if (n > 0) {
>> > return (ngx_uint_t) n;
>> > }
>> >
>>
>> The ngx_strncmp() checks are incorrect, see here:
>> http://mailman.nginx.org/pipermail/nginx-devel/2012-December/003065.html
>>
>> You may also move them below "if (n > 0)" the check for better
>> readability.
>>
>> > @@ -1175,6 +1229,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;
>>
>> The "crop_" prefix probably should be dropped here for better
>> readability (and to match other names in the code like "oxcv").
>>
>> [...]
>>
>> --
>> 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
>



--
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