image_filter enhancement
ivan babrou
ibobrik at gmail.com
Tue Dec 4 14:58:17 UTC 2012
>
> 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.
As I see other config variables are buggy too. Should I fix them too?
Anyway, this is latest version of my patch where I fixed other issues:
diff --git a/ngx_http_image_filter_module.c b/ngx_http_image_filter_module.c
index c853c33..778fcd3 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 *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 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,
+ 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->crop_offset_x);
+ offset_y = ngx_http_image_filter_get_value(r, conf->oycv,
+
conf->crop_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",
@@ -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_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;
+
+ } else if (ngx_strcmp(value->data, "bottom") == 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 +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;
return conf;
}
@@ -1223,6 +1279,19 @@ 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;
+ }
+
return NGX_CONF_OK;
}
@@ -1474,6 +1543,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->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;
+ 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->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 4 December 2012 17:08, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 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 at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
--
Regards, Ian Babrou
http://bobrik.name http://twitter.com/ibobrik skype:i.babrou
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20121204/9d5aa5df/attachment-0001.html>
More information about the nginx-devel
mailing list