From f723f756fd23f6eb2b19353177f36b5d02c7cba3 Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 14:22:47 +0200 Subject: [PATCH 01/12] Photo::scaleImageSquare was giving an inexistent variable to scaleImage. --- include/Photo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/Photo.php b/include/Photo.php index de29dfb67..1a6594966 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -365,7 +365,7 @@ class Photo { if($this->is_imagick()) { $this->image->setFirstIterator(); do { - $this->image->resizeImage($max, $max, imagick::FILTER_LANCZOS, 1, false); + $this->image->resizeImage($dim, $dim, imagick::FILTER_LANCZOS, 1, false); } while ($this->image->nextImage()); return; } From 5d0bd98d2a7c3c0aa1198c5f261d85c09685db28 Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 14:50:19 +0200 Subject: [PATCH 02/12] Photo::getType() now uses the accurate Imagick::getImageMimeType() --- include/Photo.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index 1a6594966..a9cef6a90 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -124,8 +124,7 @@ class Photo { return FALSE; if($this->is_imagick()) { - // This should do the trick (see supportedTypes above) - return 'image/'. $this->getExt(); + return $this->image->getImageMimeType(); } return $this->type; } From 0acb983be44dc1ca9431d680e6a7595c73eaf132 Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 15:17:40 +0200 Subject: [PATCH 03/12] guess_image_type now uses Imagick::getImageMimeType() --- include/Photo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/Photo.php b/include/Photo.php index a9cef6a90..c8a0ac119 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -600,7 +600,7 @@ function guess_image_type($filename, $fromcurl=false) { * we won't be tricked by a manipulated extension */ $image = new Imagick($filename); - $type = 'image/'. strtolower($image->getImageFormat()); + $type = $image->getImageMimeType(); } else { $ext = pathinfo($filename, PATHINFO_EXTENSION); $types = Photo::supportedTypes(); From 689e3028bfc18438ce5681a2b2c92d3f0686b0d9 Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 15:24:41 +0200 Subject: [PATCH 04/12] Use getImageFormat to know the image format in the constructor. Since we don't load the image with Imagick constructor, we need to use the [get|set]imageXXX methods. We should be able to handle the image as if it was loaded from Imagick constructor, so that we don't get lost between the image and its frames. Also added a debug log so that we get some info on unhandled mime types. --- include/Photo.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index c8a0ac119..bba460356 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -47,7 +47,7 @@ class Photo { $this->image->readImageBlob($data); // If it is a gif, it may be animated, get it ready for any future operations - if($this->image->getFormat() !== "GIF") $this->image = $this->image->coalesceImages(); + if($this->image->getImageFormat() !== "GIF") $this->image = $this->image->coalesceImages(); $this->ext = strtolower($this->image->getImageFormat()); } else { @@ -460,8 +460,10 @@ class Photo { $quality = get_config('system','jpeg_quality'); if((! $quality) || ($quality > 100)) $quality = JPEG_QUALITY; - if($this->is_imagick()) + if($this->is_imagick()) { $this->image->setImageFormat('jpeg'); + logger('Photo: imageString: Unhandled mime type ('. $this->getType() .'), Imagick format is "'. $this->image->getImageFormat() .'"', LOGGER_DEBUG); + } else imagejpeg($this->image,NULL,$quality); } From 83075c2f27b41fabfc106669d4bac2ad3b24116c Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 16:12:47 +0200 Subject: [PATCH 05/12] Set the Imagick image format in the constructor, so we can uset [get|set](image)XXX methods as intended --- include/Photo.php | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index bba460356..830f5213e 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -46,8 +46,14 @@ class Photo { $this->image = new Imagick(); $this->image->readImageBlob($data); + /** + * Setup the image to the format of the one we just loaded, + * we'll change it to something else if we need to at the time we save it + */ + $this->image->setFormat($this->image->getImageFormat()); + // If it is a gif, it may be animated, get it ready for any future operations - if($this->image->getImageFormat() !== "GIF") $this->image = $this->image->coalesceImages(); + if($this->image->getFormat() !== "GIF") $this->image = $this->image->coalesceImages(); $this->ext = strtolower($this->image->getImageFormat()); } else { @@ -111,8 +117,8 @@ class Photo { if(!$this->is_valid()) return FALSE; - /* Clean it */ if($this->is_imagick()) { + /* Clean it */ $this->image = $this->image->deconstructImages(); return $this->image; } @@ -147,7 +153,7 @@ class Photo { * If it is not animated, there will be only one iteration here, * so don't bother checking */ - // Don't forget to go back to the first frame for any further operation + // Don't forget to go back to the first frame $this->image->setFirstIterator(); do { $this->image->resizeImage($max, $max, imagick::FILTER_LANCZOS, 1, true); @@ -461,17 +467,17 @@ class Photo { if((! $quality) || ($quality > 100)) $quality = JPEG_QUALITY; if($this->is_imagick()) { - $this->image->setImageFormat('jpeg'); - logger('Photo: imageString: Unhandled mime type ('. $this->getType() .'), Imagick format is "'. $this->image->getImageFormat() .'"', LOGGER_DEBUG); + $this->image->setFormat('jpeg'); + logger('Photo: imageString: Unhandled mime type ('. $this->getType() .'), Imagick format is "'. $this->image->getFormat() .'"', LOGGER_DEBUG); } else imagejpeg($this->image,NULL,$quality); } if($this->is_imagick()) { if($quality !== FALSE) { - // Do we need to iterate for animations? - $this->image->setImageCompressionQuality($quality); - $this->image->stripImage(); + // Do we need to iterate for animations? + $this->image->setCompressionQuality($quality); + $this->image->stripImage(); } $string = $this->image->getImagesBlob(); From ab484ebc76722624bb8823e9de459c9ce026465a Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 16:19:35 +0200 Subject: [PATCH 06/12] getExt should not use Imagick format, mapping ext and mime type is more accurate. --- include/Photo.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index 830f5213e..4ac84d901 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -4,7 +4,6 @@ if(! class_exists("Photo")) { class Photo { private $image; - private $ext; /** * Put back gd stuff, not everybody have Imagick @@ -41,6 +40,7 @@ class Photo { public function __construct($data, $type=null) { $this->imagick = class_exists('Imagick'); + $this->types = $this->supportedTypes(); if($this->is_imagick()) { $this->image = new Imagick(); @@ -54,10 +54,7 @@ class Photo { // If it is a gif, it may be animated, get it ready for any future operations if($this->image->getFormat() !== "GIF") $this->image = $this->image->coalesceImages(); - - $this->ext = strtolower($this->image->getImageFormat()); } else { - $this->types = $this->supportedTypes(); if (!array_key_exists($type,$this->types)){ $type='image/jpeg'; } @@ -139,9 +136,7 @@ class Photo { if(!$this->is_valid()) return FALSE; - if($this->is_imagick()) - return $this->ext; - return $this->types[$this->type]; + return $this->types[$this->getType()]; } public function scaleImage($max) { From c0d3d50fb58667e03a80a25676092afd3cf630d4 Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 16:22:06 +0200 Subject: [PATCH 07/12] Don't forget to clean frames before saving --- include/Photo.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/Photo.php b/include/Photo.php index 4ac84d901..06b90bb88 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -475,6 +475,8 @@ class Photo { $this->image->stripImage(); } + /* Clean it */ + $this->image = $this->image->deconstructImages(); $string = $this->image->getImagesBlob(); } else { $string = ob_get_contents(); From 7a1f15c8dae30a69b7879d0c46eafee788098c5a Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 16:27:22 +0200 Subject: [PATCH 08/12] Always coalesce, if it is not a multi-frame image it won't hurt anyway --- include/Photo.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index 06b90bb88..862547387 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -52,8 +52,8 @@ class Photo { */ $this->image->setFormat($this->image->getImageFormat()); - // If it is a gif, it may be animated, get it ready for any future operations - if($this->image->getFormat() !== "GIF") $this->image = $this->image->coalesceImages(); + // Always coalesce, if it is not a multi-frame image it won't hurt anyway + $this->image = $this->image->coalesceImages(); } else { if (!array_key_exists($type,$this->types)){ $type='image/jpeg'; From 62cd4dcb77ef5b07b7c53c6f2a39849474254c56 Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 17:07:13 +0200 Subject: [PATCH 09/12] Set the type to what it will be saved to in the constructor --- include/Photo.php | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index 862547387..982e435f1 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -38,28 +38,41 @@ class Photo { return $t; } + /** + * Maps Mime types to Imagick formats + */ + static function get_FormatsMap() { + $m = array( + 'image/jpeg' => 'JPG', + 'image/png' => 'PNG', + 'image/gif' => 'GIF' + ); + return $m; + } + public function __construct($data, $type=null) { $this->imagick = class_exists('Imagick'); $this->types = $this->supportedTypes(); + if (!array_key_exists($type,$this->types)){ + $type='image/jpeg'; + } + $this->type = $type; if($this->is_imagick()) { $this->image = new Imagick(); $this->image->readImageBlob($data); /** - * Setup the image to the format of the one we just loaded, - * we'll change it to something else if we need to at the time we save it + * Setup the image to the format it will be saved to */ - $this->image->setFormat($this->image->getImageFormat()); + $map = $this->get_FormatsMap(); + $format = $map[$type]; + $this->image->setFormat($format); // Always coalesce, if it is not a multi-frame image it won't hurt anyway $this->image = $this->image->coalesceImages(); } else { - if (!array_key_exists($type,$this->types)){ - $type='image/jpeg'; - } $this->valid = false; - $this->type = $type; $this->image = @imagecreatefromstring($data); if($this->image !== FALSE) { $this->width = imagesx($this->image); @@ -126,9 +139,6 @@ class Photo { if(!$this->is_valid()) return FALSE; - if($this->is_imagick()) { - return $this->image->getImageMimeType(); - } return $this->type; } @@ -457,14 +467,9 @@ class Photo { // We change nothing here, do we? break; default: - // Convert to jpeg by default $quality = get_config('system','jpeg_quality'); if((! $quality) || ($quality > 100)) $quality = JPEG_QUALITY; - if($this->is_imagick()) { - $this->image->setFormat('jpeg'); - logger('Photo: imageString: Unhandled mime type ('. $this->getType() .'), Imagick format is "'. $this->image->getFormat() .'"', LOGGER_DEBUG); - } else imagejpeg($this->image,NULL,$quality); } @@ -640,11 +645,7 @@ function import_profile_photo($photo,$uid,$cid) { $filename = basename($photo); $img_str = fetch_url($photo,true); - if($this->is_imagick()) $type = null; - else { - // guess mimetype from headers or filename - $type = guess_image_type($photo,true); - } + $type = guess_image_type($photo,true); $img = new Photo($img_str, $type); if($img->is_valid()) { From a3df0d9817cd8c3e7f8b011ed68bd9528132d7bc Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 17:09:18 +0200 Subject: [PATCH 10/12] get_FormatsMap doesn't need to be static --- include/Photo.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index 982e435f1..0fa020728 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -38,18 +38,6 @@ class Photo { return $t; } - /** - * Maps Mime types to Imagick formats - */ - static function get_FormatsMap() { - $m = array( - 'image/jpeg' => 'JPG', - 'image/png' => 'PNG', - 'image/gif' => 'GIF' - ); - return $m; - } - public function __construct($data, $type=null) { $this->imagick = class_exists('Imagick'); $this->types = $this->supportedTypes(); @@ -99,6 +87,18 @@ class Photo { return $this->imagick; } + /** + * Maps Mime types to Imagick formats + */ + public function get_FormatsMap() { + $m = array( + 'image/jpeg' => 'JPG', + 'image/png' => 'PNG', + 'image/gif' => 'GIF' + ); + return $m; + } + public function is_valid() { if($this->is_imagick()) return ($this->image !== FALSE); From 36f600aaf2a9c5c39a23ded592a09d1b845b058e Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 17:27:09 +0200 Subject: [PATCH 11/12] Moved compression setup in constructor for Imagick --- include/Photo.php | 80 +++++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/include/Photo.php b/include/Photo.php index 0fa020728..b72853624 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -59,6 +59,32 @@ class Photo { // Always coalesce, if it is not a multi-frame image it won't hurt anyway $this->image = $this->image->coalesceImages(); + + /** + * setup the compression here, so we'll do it only once + */ + switch($this->getType()){ + case "image/png": + $quality = get_config('system','png_quality'); + if((! $quality) || ($quality > 9)) + $quality = PNG_QUALITY; + /** + * From http://www.imagemagick.org/script/command-line-options.php#quality: + * + * 'For the MNG and PNG image formats, the quality value sets + * the zlib compression level (quality / 10) and filter-type (quality % 10). + * The default PNG "quality" is 75, which means compression level 7 with adaptive PNG filtering, + * unless the image has a color map, in which case it means compression level 7 with no PNG filtering' + */ + $quality = $quality * 10; + $this->image->setCompressionQuality($quality); + break; + case "image/jpeg": + $quality = get_config('system','jpeg_quality'); + if((! $quality) || ($quality > 100)) + $quality = JPEG_QUALITY; + $this->image->setCompressionQuality($quality); + } } else { $this->valid = false; $this->image = @imagecreatefromstring($data); @@ -435,58 +461,32 @@ class Photo { if(!$this->is_valid()) return FALSE; + if($this->is_imagick()) { + /* Clean it */ + $this->image = $this->image->deconstructImages(); + $string = $this->image->getImagesBlob(); + return $string; + } + $quality = FALSE; - /** - * Hmmm, for Imagick - * we should do the conversion/compression at the initialisation i think - * This method may be called several times, - * and there is no need to do that more than once - */ - - if(!$this->is_imagick()) ob_start(); + ob_start() switch($this->getType()){ case "image/png": $quality = get_config('system','png_quality'); if((! $quality) || ($quality > 9)) $quality = PNG_QUALITY; - if($this->is_imagick()) { - /** - * From http://www.imagemagick.org/script/command-line-options.php#quality: - * - * 'For the MNG and PNG image formats, the quality value sets - * the zlib compression level (quality / 10) and filter-type (quality % 10). - * The default PNG "quality" is 75, which means compression level 7 with adaptive PNG filtering, - * unless the image has a color map, in which case it means compression level 7 with no PNG filtering' - */ - $quality = $quality * 10; - } else imagepng($this->image,NULL, $quality); + imagepng($this->image,NULL, $quality); break; - case "image/gif": - // We change nothing here, do we? - break; - default: + case "image/jpeg": $quality = get_config('system','jpeg_quality'); if((! $quality) || ($quality > 100)) $quality = JPEG_QUALITY; - else imagejpeg($this->image,NULL,$quality); - } - - if($this->is_imagick()) { - if($quality !== FALSE) { - // Do we need to iterate for animations? - $this->image->setCompressionQuality($quality); - $this->image->stripImage(); - } - - /* Clean it */ - $this->image = $this->image->deconstructImages(); - $string = $this->image->getImagesBlob(); - } else { - $string = ob_get_contents(); - ob_end_clean(); + imagejpeg($this->image,NULL,$quality); } + $string = ob_get_contents(); + ob_end_clean(); return $string; } @@ -603,7 +603,7 @@ function guess_image_type($filename, $fromcurl=false) { } if (is_null($type)){ // Guessing from extension? Isn't that... dangerous? - if($this->is_imagick()) { + if(class_exists('Imagick')) { /** * Well, this not much better, * but at least it comes from the data inside the image, From ba76af348872d47c3949269eeb4b7a7effcf226e Mon Sep 17 00:00:00 2001 From: Domovoy Date: Sun, 22 Jul 2012 17:50:58 +0200 Subject: [PATCH 12/12] Forgot a ; --- include/Photo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/Photo.php b/include/Photo.php index b72853624..1f751c77f 100644 --- a/include/Photo.php +++ b/include/Photo.php @@ -470,7 +470,7 @@ class Photo { $quality = FALSE; - ob_start() + ob_start(); switch($this->getType()){ case "image/png":