From a3586cab16340bb14e2fa499345ec5a0128543e0 Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Fri, 7 Jun 2013 15:35:16 +0200 Subject: [PATCH 1/8] More detailed tracking of errors when d/l images Easier to troubleshoot in case of wrong redirects, or plain origin server errors. --- src/mod_dims.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/mod_dims.c b/src/mod_dims.c index c8674bc..ac4b387 100755 --- a/src/mod_dims.c +++ b/src/mod_dims.c @@ -580,6 +580,28 @@ dims_fetch_remote_image(dims_request_rec *d, const char *url) if(response_code == 404) { d->status = DIMS_FILE_NOT_FOUND; + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, d->r, + "Received a 404 NOT FOUND on request: %s ", + d->r->uri); + } + + /* This shouldn't happen with CURLOPT_FOLLOWLOCATION => 1 */ + else if(response_code == 301 || response_code == 302) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, d->r, + "Received a %d REDIRECT on request: %s ", + response_code, d->r->uri); + } + + else if(response_code >= 500) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, d->r, + "Received a %d SERVER ERROR on request: %s ", + response_code, d->r->uri); + } + + else { + ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, d->r, + "Received a %d non-ok status on request: %s ", + response_code, d->r->uri); } free(fetch_url); From 3223eb555078b5e97d2030ef1a3c9bcf8e474221 Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Fri, 7 Jun 2013 16:05:45 +0200 Subject: [PATCH 2/8] New ops, liquid-rescale and gravity. We use(d) these operations for My Opera and Discover. --- src/mod_dims.c | 2 ++ src/mod_dims.h | 4 +++- src/mod_dims_ops.c | 23 +++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/mod_dims.c b/src/mod_dims.c index c8674bc..59d9b57 100755 --- a/src/mod_dims.c +++ b/src/mod_dims.c @@ -1480,6 +1480,8 @@ dims_init(apr_pool_t *p, apr_pool_t *plog, apr_pool_t* ptemp, server_rec *s) apr_hash_set(ops, "autolevel", APR_HASH_KEY_STRING, dims_autolevel_operation); apr_hash_set(ops, "rotate", APR_HASH_KEY_STRING, dims_rotate_operation); apr_hash_set(ops, "invert", APR_HASH_KEY_STRING, dims_invert_operation); + apr_hash_set(ops, "liquid_rescale", APR_HASH_KEY_STRING, dims_liquid_rescale_operation); + apr_hash_set(ops, "gravity", APR_HASH_KEY_STRING, dims_gravity_operation); /* Init APR's atomic functions */ status = apr_atomic_init(p); diff --git a/src/mod_dims.h b/src/mod_dims.h index 0294763..7f3b064 100644 --- a/src/mod_dims.h +++ b/src/mod_dims.h @@ -75,7 +75,9 @@ dims_operation_func dims_autolevel_operation, dims_rotate_operation, dims_invert_operation, - dims_legacy_crop_operation; + dims_legacy_crop_operation, + dims_liquid_rescale_operation, + dims_gravity_operation; struct dims_config_rec { int download_timeout; diff --git a/src/mod_dims_ops.c b/src/mod_dims_ops.c index a56a045..57d70bd 100644 --- a/src/mod_dims_ops.c +++ b/src/mod_dims_ops.c @@ -316,3 +316,26 @@ dims_legacy_thumbnail_operation (dims_request_rec *d, char *args, char **err) { return DIMS_SUCCESS; } +apr_status_t +dims_liquid_rescale_operation (dims_request_rec *d, char *args, char **err) { + MagickStatusType flags; + RectangleInfo rec; + + ExceptionInfo ex_info; + flags = ParseGravityGeometry(GetImageFromMagickWand(d->wand), args, &rec, &ex_info); + if(!(flags & AllValues)) { + *err = "Parsing liquid resize geometry failed"; + return DIMS_FAILURE; + } + + MAGICK_CHECK(MagickLiquidRescaleImage(d->wand, rec.width, rec.height, 3, 25), d); + + return DIMS_SUCCESS; +} + +apr_status_t +dims_gravity_operation (dims_request_rec *d, char *args, char **err) { + int gravity = ParseMagickOption(MagickGravityOptions,MagickFalse,args); + MAGICK_CHECK(MagickSetImageGravity(d->wand, gravity), d); + return DIMS_SUCCESS; +} From 6d863f821269b0e4f7c7ecda4f4793f503fdd6d7 Mon Sep 17 00:00:00 2001 From: Terje Kristensen Date: Fri, 7 Jun 2013 16:26:33 +0200 Subject: [PATCH 3/8] Fixes some whitespace problem with crop Sadly, I don't remember what was the problem anymore. Refers to our internal commit 39fc429258dcc76554d015a286105289b18487b0. --- src/mod_dims_ops.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mod_dims_ops.c b/src/mod_dims_ops.c index a56a045..dc617ed 100644 --- a/src/mod_dims_ops.c +++ b/src/mod_dims_ops.c @@ -130,6 +130,7 @@ dims_crop_operation (dims_request_rec *d, char *args, char **err) { MagickStatusType flags; RectangleInfo rec; ExceptionInfo ex_info; + char *page = "0x0+0+0"; flags = ParseGravityGeometry(GetImageFromMagickWand(d->wand), args, &rec, &ex_info); if(!(flags & AllValues)) { @@ -138,6 +139,7 @@ dims_crop_operation (dims_request_rec *d, char *args, char **err) { } MAGICK_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y), d); + MAGICK_CHECK(MagickResetImagePage(d->wand, page), d); return DIMS_SUCCESS; } From e9e8dc65dcf25bdfad194d92166036bf1c8ea76b Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Fri, 7 Jun 2013 17:31:42 +0200 Subject: [PATCH 4/8] Added background operation Useful when converting PNG pictures to JPEG and at the same time changing the default black color. Maybe we could sample the picture colors and decide a suitable background color too. --- src/mod_dims.c | 1 + src/mod_dims.h | 3 ++- src/mod_dims_ops.c | 8 ++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/mod_dims.c b/src/mod_dims.c index c8674bc..b9f66f8 100755 --- a/src/mod_dims.c +++ b/src/mod_dims.c @@ -1480,6 +1480,7 @@ dims_init(apr_pool_t *p, apr_pool_t *plog, apr_pool_t* ptemp, server_rec *s) apr_hash_set(ops, "autolevel", APR_HASH_KEY_STRING, dims_autolevel_operation); apr_hash_set(ops, "rotate", APR_HASH_KEY_STRING, dims_rotate_operation); apr_hash_set(ops, "invert", APR_HASH_KEY_STRING, dims_invert_operation); + apr_hash_set(ops, "background", APR_HASH_KEY_STRING, dims_background_operation); /* Init APR's atomic functions */ status = apr_atomic_init(p); diff --git a/src/mod_dims.h b/src/mod_dims.h index 0294763..07eba0b 100644 --- a/src/mod_dims.h +++ b/src/mod_dims.h @@ -75,7 +75,8 @@ dims_operation_func dims_autolevel_operation, dims_rotate_operation, dims_invert_operation, - dims_legacy_crop_operation; + dims_legacy_crop_operation, + dims_background_operation; struct dims_config_rec { int download_timeout; diff --git a/src/mod_dims_ops.c b/src/mod_dims_ops.c index a56a045..58f41fc 100644 --- a/src/mod_dims_ops.c +++ b/src/mod_dims_ops.c @@ -316,3 +316,11 @@ dims_legacy_thumbnail_operation (dims_request_rec *d, char *args, char **err) { return DIMS_SUCCESS; } +apr_status_t +dims_background_operation (dims_request_rec *d, char *args, char **err) { + PixelWand *bgcolor = NewPixelWand(); + PixelSetColor(bgcolor, args); + MAGICK_CHECK(MagickSetImageBackgroundColor(d->wand, bgcolor), d); + DestroyPixelWand(pxWand); + return DIMS_SUCCESS; +} From 18a205acc952ec9600be9a610db4b2cc092336a5 Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Fri, 7 Jun 2013 18:53:51 +0200 Subject: [PATCH 5/8] Fixup background-op. --- src/mod_dims_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mod_dims_ops.c b/src/mod_dims_ops.c index 58f41fc..ef26230 100644 --- a/src/mod_dims_ops.c +++ b/src/mod_dims_ops.c @@ -321,6 +321,6 @@ dims_background_operation (dims_request_rec *d, char *args, char **err) { PixelWand *bgcolor = NewPixelWand(); PixelSetColor(bgcolor, args); MAGICK_CHECK(MagickSetImageBackgroundColor(d->wand, bgcolor), d); - DestroyPixelWand(pxWand); + DestroyPixelWand(bgcolor); return DIMS_SUCCESS; } From 8a751684f485d9a023f38615c32fd4294ccb2195 Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Fri, 7 Jun 2013 19:44:19 +0200 Subject: [PATCH 6/8] Background applies correctly now. It's necessary to apply the background filter early in the URL chain. If you define a URL like: /resize/240x>/background/red/ then the picture will result with a red background but it will be the original size. I'm not familiar enough with ImageMagick to understand why at this point, so just put your background filter at the beginning. /background/grey/resize/240x>/ This will work. --- src/mod_dims_ops.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/mod_dims_ops.c b/src/mod_dims_ops.c index ef26230..2766f9c 100644 --- a/src/mod_dims_ops.c +++ b/src/mod_dims_ops.c @@ -318,9 +318,21 @@ dims_legacy_thumbnail_operation (dims_request_rec *d, char *args, char **err) { apr_status_t dims_background_operation (dims_request_rec *d, char *args, char **err) { + MagickWand *flat; PixelWand *bgcolor = NewPixelWand(); PixelSetColor(bgcolor, args); + MAGICK_CHECK(MagickSetImageBackgroundColor(d->wand, bgcolor), d); + + /* These steps are necessary or the background color + will be applied but then discarded */ + MAGICK_CHECK(flat = MagickMergeImageLayers(d->wand, FlattenLayer), d); + + /* XXX Appropriate way to replace d->wand? */ + DestroyMagickWand(d->wand); + d->wand = flat; + DestroyPixelWand(bgcolor); + return DIMS_SUCCESS; } From 1d54eb6eab99bbdae498473268a855765e3b0f94 Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Fri, 14 Jun 2013 13:01:09 +0200 Subject: [PATCH 7/8] ParseMagickOption() has been renamed In ImageMagick 6.8.5, there's no ParseMagickOption(). That function was renamed to ParseCommandOption(). http://trac.imagemagick.org/changeset/4281/ImageMagick/trunk/magick/image.c --- src/mod_dims_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mod_dims_ops.c b/src/mod_dims_ops.c index 255af3d..1db5891 100644 --- a/src/mod_dims_ops.c +++ b/src/mod_dims_ops.c @@ -337,7 +337,7 @@ dims_liquid_rescale_operation (dims_request_rec *d, char *args, char **err) { apr_status_t dims_gravity_operation (dims_request_rec *d, char *args, char **err) { - int gravity = ParseMagickOption(MagickGravityOptions,MagickFalse,args); + int gravity = ParseCommandOption(MagickGravityOptions,MagickFalse,args); MAGICK_CHECK(MagickSetImageGravity(d->wand, gravity), d); return DIMS_SUCCESS; } From 6e16c75ca4d136e6f3cfa63671173edefe6cd169 Mon Sep 17 00:00:00 2001 From: Cosimo Streppone Date: Tue, 18 Jun 2013 17:43:30 +0200 Subject: [PATCH 8/8] Implement a DimsImageMagickThreadLimit directive Should be the same as doing MAGICK_THREAD_LIMIT, but done at module initialisation time rather than at request time. --- src/mod_dims.c | 16 ++++++++++++++++ src/mod_dims.h | 1 + 2 files changed, 17 insertions(+) diff --git a/src/mod_dims.c b/src/mod_dims.c index a255a29..92d7541 100755 --- a/src/mod_dims.c +++ b/src/mod_dims.c @@ -292,6 +292,17 @@ dims_config_set_imagemagick_disk_size(cmd_parms *cmd, void *dummy, const char *a return NULL; } + +static const char * +dims_config_set_imagemagick_thread_limit(cmd_parms *cmd, void *dummy, const char *arg) +{ + dims_config_rec *config = (dims_config_rec *) ap_get_module_config( + cmd->server->module_config, &dims_module); + config->thread_limit = atol(arg); + + return NULL; +} + static const char * dims_config_set_secretkeyExpiryPeriod(cmd_parms *cmd, void *dummy, const char *arg) { @@ -1484,6 +1495,7 @@ dims_init(apr_pool_t *p, apr_pool_t *plog, apr_pool_t* ptemp, server_rec *s) MagickSetResourceLimit(DiskResource, config->disk_size); MagickSetResourceLimit(MemoryResource, config->memory_size); MagickSetResourceLimit(MapResource, config->map_size); + MagickSetResourceLimit(ThreadResource, config->thread_limit); ops = apr_hash_make(p); apr_hash_set(ops, "strip", APR_HASH_KEY_STRING, dims_strip_operation); @@ -1689,6 +1701,10 @@ static const command_rec dims_commands[] = dims_config_set_imagemagick_disk_size, NULL, RSRC_CONF, "Maximum amount of disk space in megabytes to use for the pixel cache." "The default is 1024mb."), + AP_INIT_TAKE1("DimsImagemagickThreadLimit", + dims_config_set_imagemagick_thread_limit, NULL, RSRC_CONF, + "Maximum number of threads that should be used by ImageMagick." + "The default is ImageMagick default."), AP_INIT_TAKE1("DimsSecretMaxExpiryPeriod", dims_config_set_secretkeyExpiryPeriod, NULL, RSRC_CONF, "How long in the future (in seconds) can the expiry date on the URL be requesting. 0 = forever" diff --git a/src/mod_dims.h b/src/mod_dims.h index e98a64e..d1d87bd 100644 --- a/src/mod_dims.h +++ b/src/mod_dims.h @@ -96,6 +96,7 @@ struct dims_config_rec { MagickSizeType memory_size; MagickSizeType map_size; MagickSizeType disk_size; + MagickSizeType thread_limit; int curl_queue_size; char *secret_key;