r83778 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83777‎ | r83778 | r83779 >
Date:19:32, 12 March 2011
Author:btongminh
Status:resolved (Comments)
Tags:
Comment:
(bug 14706) Added support for the Imagick PHP extension. Based on patch by Leslie Hoare.
Scaler type is "imext". Rotation is supported.
Logic mostly copied from transformImagemagick() and ported to Imagick calls.
Resizing animated gifs is broken; it only shows the first frame and I can't find out why it does not work, but otherwise it is fully working.
Modified paths:
  • /trunk/phase3/CREDITS (modified) (history)
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/media/Bitmap.php (modified) (history)

Diff [purge]

Index: trunk/phase3/CREDITS
@@ -34,6 +34,7 @@
3535 * Jon Harald Søby
3636 * Juliano F. Ravasi
3737 * Leon Weber
 38+* Leslie Hoare
3839 * Marco Schuster
3940 * Matěj Grabovský
4041 * Matt Johnston
Index: trunk/phase3/includes/media/Bitmap.php
@@ -143,6 +143,8 @@
144144 case 'custom':
145145 $err = $this->transformCustom( $image, $scalerParams );
146146 break;
 147+ case 'imext':
 148+ $err = $this->transformImageMagickExt( $image, $scalerParams );
147149 case 'gd':
148150 default:
149151 $err = $this->transformGd( $image, $scalerParams );
@@ -184,6 +186,8 @@
185187 $scaler = 'custom';
186188 } elseif ( function_exists( 'imagecreatetruecolor' ) ) {
187189 $scaler = 'gd';
 190+ } elseif ( class_exists( 'Imagick' ) ) {
 191+ $scaler = 'imext';
188192 } else {
189193 $scaler = 'client';
190194 }
@@ -209,7 +213,7 @@
210214 return new ThumbnailImage( $image, $image->getURL(),
211215 $params['clientWidth'], $params['clientHeight'], $params['srcPath'] );
212216 }
213 -
 217+
214218 /**
215219 * Transform an image using ImageMagick
216220 *
@@ -301,7 +305,92 @@
302306
303307 return false; # No error
304308 }
 309+
 310+ /**
 311+ * Transform an image using the Imagick PHP extension
 312+ *
 313+ * @param $image File File associated with this thumbnail
 314+ * @param $params array Array with scaler params
 315+ *
 316+ * @return MediaTransformError Error object if error occured, false (=no error) otherwise
 317+ */
 318+ protected function transformImageMagickExt( $image, $params ) {
 319+ global $wgSharpenReductionThreshold, $wgSharpenParameter, $wgMaxAnimatedGifArea;
 320+
 321+ try {
 322+ $im = new Imagick();
 323+ $im->readImage( $params['srcPath'] );
 324+
 325+ if ( $params['mimeType'] == 'image/jpeg' ) {
 326+ // Sharpening, see bug 6193
 327+ if ( ( $params['physicalWidth'] + $params['physicalHeight'] )
 328+ / ( $params['srcWidth'] + $params['srcHeight'] )
 329+ < $wgSharpenReductionThreshold ) {
 330+ // Hack, since $wgSharpenParamater is written specifically for the command line convert
 331+ list( $radius, $sigma ) = explode( 'x', $wgSharpenParameter );
 332+ $im->sharpenImage( $radius, $sigma );
 333+ }
 334+ $im->setCompressionQuality( 80 );
 335+ } elseif( $params['mimeType'] == 'image/png' ) {
 336+ $im->setCompressionQuality( 95 );
 337+ } elseif ( $params['mimeType'] == 'image/gif' ) {
 338+ if ( $this->getImageArea( $image, $params['srcWidth'],
 339+ $params['srcHeight'] ) > $wgMaxAnimatedGifArea ) {
 340+ // Extract initial frame only; we're so big it'll
 341+ // be a total drag. :P
 342+ $im->setImageScene( 0 );
 343+ } elseif ( $this->isAnimatedImage( $image ) ) {
 344+ // Coalesce is needed to scale animated GIFs properly (bug 1017).
 345+ $im = $im->coalesceImages();
 346+ }
 347+ }
 348+
 349+ $rotation = $this->getRotation( $image );
 350+ if ( $rotation == 90 || $rotation == 270 ) {
 351+ // We'll resize before rotation, so swap the dimensions again
 352+ $width = $params['physicalHeight'];
 353+ $height = $params['physicalWidth'];
 354+ } else {
 355+ $width = $params['physicalWidth'];
 356+ $height = $params['physicalHeight'];
 357+ }
 358+
 359+ $im->setImageBackgroundColor( new ImagickPixel( 'white' ) );
 360+
 361+ // Call Imagick::thumbnailImage on each frame
 362+ foreach ( $im as $i => $frame ) {
 363+ if ( !$frame->thumbnailImage( $width, $height, /* fit */ false ) ) {
 364+ return $this->getMediaTransformError( $params, "Error scaling frame $i" );
 365+ }
 366+ }
 367+ $im->setImageDepth( 8 );
 368+
 369+ if ( $rotation ) {
 370+ if ( !$im->rotateImage( new ImagickPixel( 'white' ), $rotation ) ) {
 371+ return $this->getMediaTransformError( $params, "Error rotating $rotation degrees" );
 372+ }
 373+ }
 374+
 375+ if ( $this->isAnimatedImage( $image ) ) {
 376+ wfDebug( __METHOD__ . ": Writing animated thumbnail\n" );
 377+ // This is broken somehow... can't find out how to fix it
 378+ $result = $im->writeImages( $params['dstPath'], true );
 379+ } else {
 380+ $result = $im->writeImage( $params['dstPath'] );
 381+ }
 382+ if ( !$result ) {
 383+ return $this->getMediaTransformError( $params,
 384+ "Unable to write thumbnail to {$params['dstPath']}" );
 385+ }
305386
 387+ } catch ( ImagickException $e ) {
 388+ return $this->getMediaTransformError( $params, $e->getMessage() );
 389+ }
 390+
 391+ return false;
 392+
 393+ }
 394+
306395 /**
307396 * Transform an image using a custom command
308397 *
@@ -703,6 +792,9 @@
704793 case 'im':
705794 # ImageMagick supports autorotation
706795 return true;
 796+ case 'imext':
 797+ # Imagick::rotateImage
 798+ return true;
707799 case 'gd':
708800 # GD's imagerotate function is used to rotate images, but not
709801 # all precompiled PHP versions have that function
Index: trunk/phase3/RELEASE-NOTES
@@ -102,6 +102,7 @@
103103 several wikis.
104104 * When $wgAllowMicrodataAttributes is true, all itemtypes are allowed, not just
105105 the three that were defined in the original specification.
 106+* (bug 14706) Added support for the Imagick PHP extension.
106107
107108 === Bug fixes in 1.18 ===
108109 * (bug 23119) WikiError class and subclasses are now marked as deprecated

Follow-up revisions

RevisionCommit summaryAuthorDate
r83785Fix switch fail in r83778: add missing break.btongminh21:35, 12 March 2011
r83792Follow-up r83778: Need to de-rotate, so use 360 - angle instead of angle.btongminh22:50, 12 March 2011

Comments

#Comment by Bryan (talk | contribs)   19:34, 12 March 2011

According to [1] animated gifs should just work...

#Comment by Reedy (talk | contribs)   20:07, 12 March 2011

Without specifically looking that looks like a lot of duplication. I'm guessin it's cause it's all completely different from shelling out :(

#Comment by Bryan (talk | contribs)   20:10, 12 March 2011

I don't like it either, but I don't see how to properly fix this, without wrapping the shell out into an Imagick compatible wrapper.

#Comment by Reedy (talk | contribs)   20:12, 12 March 2011

Mhmmm. Seems probably a waste of effort for little gain. Though, are there likely to be over Imagemagick ways? :/

I'm guessing we'll just have to put up and shut up...

#Comment by Bryan (talk | contribs)   11:51, 13 March 2011

Could be a proper way to fix bug 19073 though.

#Comment by Nikerabbit (talk | contribs)   21:29, 12 March 2011

No break?

+			case 'imext':
+				$err = $this->transformImageMagickExt( $image, $scalerParams );
 			case 'gd':
#Comment by Bryan (talk | contribs)   21:36, 12 March 2011

Thanks, fixed in r83785.

Status & tagging log