r87099 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87098‎ | r87099 | r87100 >
Date:20:18, 28 April 2011
Author:mah
Status:ok (Comments)
Tags:
Comment:
Provisional fix for Bug #28631 to remove artifacts from GIF. This
seems better in my (very) limited testing than leaving it in, but I'd
like to get more tests.
Modified paths:
  • /trunk/phase3/includes/media/Bitmap.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/media/Bitmap.php
——@@ -27,12 +27,12 @@
2828 $mimeType = $image->getMimeType();
2929 $srcWidth = $image->getWidth( $params['page'] );
3030 $srcHeight = $image->getHeight( $params['page'] );
31 -
 31+
3232 if ( self::canRotate() ) {
3333 $rotation = $this->getRotation( $image );
3434 if ( $rotation == 90 || $rotation == 270 ) {
3535 wfDebug( __METHOD__ . ": Swapping width and height because the file will be rotated $rotation degrees\n" );
36 -
 36+
3737 $width = $params['width'];
3838 $params['width'] = $params['height'];
3939 $params['height'] = $width;
——@@ -136,7 +136,7 @@
137137 wfDebug( __METHOD__ . ": Unable to create thumbnail destination directory, falling back to client scaling\n" );
138138 return $this->getClientScalingThumbnailImage( $image, $scalerParams );
139139 }
140 -
 140+
141141 # Try a hook
142142 $mto = null;
143143 wfRunHooks( 'BitmapHandlerTransform', array( $this, $image, &$scalerParams, &$mto ) );
——@@ -144,7 +144,7 @@
145145 wfDebug( __METHOD__ . ": Hook to BitmapHandlerTransform created an mto\n" );
146146 $scaler = 'hookaborted';
147147 }
148 -
 148+
149149 switch ( $scaler ) {
150150 case 'hookaborted':
151151 # Handled by the hook above
——@@ -181,16 +181,16 @@
182182 $scalerParams['clientHeight'], $dstPath );
183183 }
184184 }
185 -
 185+
186186 /**
187 - * Returns which scaler type should be used. Creates parent directories
 187+ * Returns which scaler type should be used. Creates parent directories
188188 * for $dstPath and returns 'client' on error
189 - *
 189+ *
190190 * @return string client,im,custom,gd
191191 */
192192 protected static function getScalerType( $dstPath, $checkDstPath = true ) {
193193 global $wgUseImageResize, $wgUseImageMagick, $wgCustomConvertCommand;
194 -
 194+
195195 if ( !$dstPath && $checkDstPath ) {
196196 # No output path available, client side scaling only
197197 $scaler = 'client';
——@@ -207,7 +207,7 @@
208208 } else {
209209 $scaler = 'client';
210210 }
211 -
 211+
212212 if ( $scaler != 'client' && $dstPath ) {
213213 if ( !wfMkdirParents( dirname( $dstPath ) ) ) {
214214 # Unable to create a path for the thumbnail
——@@ -229,7 +229,7 @@
230230 return new ThumbnailImage( $image, $image->getURL(),
231231 $params['clientWidth'], $params['clientHeight'], $params['srcPath'] );
232232 }
233 -
 233+
234234 /**
235235 * Transform an image using ImageMagick
236236 *
——@@ -277,7 +277,7 @@
278278 // We optimize the output, but -optimize is broken,
279279 // use optimizeTransparency instead (bug 11822)
280280 if ( version_compare( $this->getMagickVersion(), "6.3.5" ) >= 0 ) {
281 - $animation_post = '-fuzz 5% -layers optimizeTransparency +map';
 281+ $animation_post = '-fuzz 5% -layers optimizeTransparency';
282282 }
283283 }
284284 }
——@@ -321,10 +321,10 @@
322322
323323 return false; # No error
324324 }
325 -
 325+
326326 /**
327327 * Transform an image using the Imagick PHP extension
328 - *
 328+ *
329329 * @param $image File File associated with this thumbnail
330330 * @param $params array Array with scaler params
331331 *
——@@ -332,11 +332,11 @@
333333 */
334334 protected function transformImageMagickExt( $image, $params ) {
335335 global $wgSharpenReductionThreshold, $wgSharpenParameter, $wgMaxAnimatedGifArea;
336 -
 336+
337337 try {
338338 $im = new Imagick();
339339 $im->readImage( $params['srcPath'] );
340 -
 340+
341341 if ( $params['mimeType'] == 'image/jpeg' ) {
342342 // Sharpening, see bug 6193
343343 if ( ( $params['physicalWidth'] + $params['physicalHeight'] )
——@@ -360,7 +360,7 @@
361361 $im = $im->coalesceImages();
362362 }
363363 }
364 -
 364+
365365 $rotation = $this->getRotation( $image );
366366 if ( $rotation == 90 || $rotation == 270 ) {
367367 // We'll resize before rotation, so swap the dimensions again
——@@ -368,11 +368,11 @@
369369 $height = $params['physicalWidth'];
370370 } else {
371371 $width = $params['physicalWidth'];
372 - $height = $params['physicalHeight'];
 372+ $height = $params['physicalHeight'];
373373 }
374 -
 374+
375375 $im->setImageBackgroundColor( new ImagickPixel( 'white' ) );
376 -
 376+
377377 // Call Imagick::thumbnailImage on each frame
378378 foreach ( $im as $i => $frame ) {
379379 if ( !$frame->thumbnailImage( $width, $height, /* fit */ false ) ) {
——@@ -380,13 +380,13 @@
381381 }
382382 }
383383 $im->setImageDepth( 8 );
384 -
 384+
385385 if ( $rotation ) {
386386 if ( !$im->rotateImage( new ImagickPixel( 'white' ), 360 - $rotation ) ) {
387387 return $this->getMediaTransformError( $params, "Error rotating $rotation degrees" );
388388 }
389389 }
390 -
 390+
391391 if ( $this->isAnimatedImage( $image ) ) {
392392 wfDebug( __METHOD__ . ": Writing animated thumbnail\n" );
393393 // This is broken somehow... can't find out how to fix it
——@@ -395,16 +395,16 @@
396396 $result = $im->writeImage( $params['dstPath'] );
397397 }
398398 if ( !$result ) {
399 - return $this->getMediaTransformError( $params,
 399+ return $this->getMediaTransformError( $params,
400400 "Unable to write thumbnail to {$params['dstPath']}" );
401401 }
402402
403403 } catch ( ImagickException $e ) {
404 - return $this->getMediaTransformError( $params, $e->getMessage() );
 404+ return $this->getMediaTransformError( $params, $e->getMessage() );
405405 }
406 -
 406+
407407 return false;
408 -
 408+
409409 }
410410
411411 /**
——@@ -453,7 +453,7 @@
454454 }
455455 /**
456456 * Get a MediaTransformError with error 'thumbnail_error'
457 - *
 457+ *
458458 * @param $params array Parameter array as passed to the transform* functions
459459 * @param $errMsg string Error message
460460 * @return MediaTransformError
——@@ -507,7 +507,7 @@
508508 }
509509
510510 $src_image = call_user_func( $loader, $params['srcPath'] );
511 -
 511+
512512 $rotation = function_exists( 'imagerotate' ) ? $this->getRotation( $image ) : 0;
513513 if ( $rotation == 90 || $rotation == 270 ) {
514514 # We'll resize before rotation, so swap the dimensions again
——@@ -515,7 +515,7 @@
516516 $height = $params['physicalWidth'];
517517 } else {
518518 $width = $params['physicalWidth'];
519 - $height = $params['physicalHeight'];
 519+ $height = $params['physicalHeight'];
520520 }
521521 $dst_image = imagecreatetruecolor( $width, $height );
522522
——@@ -538,13 +538,13 @@
539539 $width, $height,
540540 imagesx( $src_image ), imagesy( $src_image ) );
541541 }
542 -
 542+
543543 if ( $rotation % 360 != 0 && $rotation % 90 == 0 ) {
544544 $rot_image = imagerotate( $dst_image, $rotation, 0 );
545545 imagedestroy( $dst_image );
546546 $dst_image = $rot_image;
547547 }
548 -
 548+
549549 imagesavealpha( $dst_image, true );
550550
551551 call_user_func( $saveType, $dst_image, $params['dstPath'] );
——@@ -671,9 +671,9 @@
672672 }
673673
674674 /**
675 - * Try to read out the orientation of the file and return the angle that
 675+ * Try to read out the orientation of the file and return the angle that
676676 * the file needs to be rotated to be viewed
677 - *
 677+ *
678678 * @param $file File
679679 * @return int 0, 90, 180 or 270
680680 */
——@@ -701,7 +701,7 @@
702702
703703 /**
704704 * Returns whether the current scaler supports rotation (im and gd do)
705 - *
 705+ *
706706 * @return bool
707707 */
708708 public static function canRotate() {
——@@ -722,11 +722,11 @@
723723 return false;
724724 }
725725 }
726 -
 726+
727727 /**
728 - * Rerurns whether the file needs to be rendered. Returns true if the
 728+ * Rerurns whether the file needs to be rendered. Returns true if the
729729 * file requires rotation and we are able to rotate it.
730 - *
 730+ *
731731 * @param $file File
732732 * @return bool
733733 */

Sign-offs

UserFlagDate
Bryaninspected20:21, 1 May 2011
Catropeinspected10:57, 13 August 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r945571.17wmf1: MFT r87099, r90729, r93141, r93517, r93891, r94155, r94277, r94372catrope20:12, 15 August 2011

Comments

#Comment by MarkAHershberger (talk | contribs)   20:22, 28 April 2011

Sorry for all the w/s changes. I usually check for that before checking in. This is the only diff of substance: 

@@ -277,7 +277,7 @@
				// We optimize the output, but -optimize is broken,
				// use optimizeTransparency instead (bug 11822)
				if ( version_compare( $this->getMagickVersion(), "6.3.5" ) >= 0 ) {
-					$animation_post = '-fuzz 5% -layers optimizeTransparency +map';
+					$animation_post = '-fuzz 5% -layers optimizeTransparency';
				}
			}
#Comment by Hashar (talk | contribs)   13:21, 8 June 2011

Just for the context, this only apply when matching the three conditions: - image/gif - animated 

 imagemagick >= 6.3.5

The options which were passed to ImageMagick were: 

- fuzz 5%
- layers optimizeTransparency
- +map

+map is supposed to look at the color map and generate the minimum map needed to represent the image. I do not think this parameter is the root cause although removing it might fix the issue as a side effect.

I believe the root cause is the 'fuzz 5%' associated to the optimize transparency switch.

From the documentation at http://www.imagemagick.org/script/command-line-options.php :

optimize-transparency

Given a GIF animation, replace any pixel in the sub-frame overlay images with transparency, if it does not change the resulting animation by more than the current -fuzz factor.
This should allow a existing frame optimized GIF animation to compress into a smaller file size due to larger areas of one (transparent) color rather than a pattern of multiple colors repeating the current disposed image of the last frame.


I am wondering if removing the fuzz option will be enough. Might be worth checking a newer imagemagick version and/or open an upstream bug.

#Comment by TheDJ (talk | contribs)   13:26, 8 June 2011

I don't remember exactly anymore, but I think that the fuzz factor is a very large influence on the compression factor of the animated gif filesize. And since that is the primary reason for handling these imagemagick options seperately, something to double check before making changes.

Status & tagging log