r54284 - Code Review

From MediaWiki.org

Jump to: navigation, search
Repository:MediaWiki
Revision:r54283 | r54284 (on ViewVC) | r54285 >
Date:15:01, 3 August 2009
Author:werdna
Status:resolved (Comments)
Tags:
Comment:(bug 16451) Fix application of scaling limits for animated GIFs.
* Creates a new media handler for GIF files, and extracts metadata such as duration, whether or not the GIF is looped, and the number of frames.
* Uses the extracted metadata for the long file description.
* Considers number of frames in the calculation of image area (multiplies by width and height to get the "time-area", or so to speak).

After this patch is deployed, the work-around to disable GIF scaling on Wikimedia sites can be eliminated.
Modified paths:

Test cases

ParserTests

558 succeeded tests.

Diff [purge]

Index: trunk/phase3/includes/media/Bitmap.php
===================================================================
--- trunk/phase3/includes/media/Bitmap.php	(revision 54283)
+++ trunk/phase3/includes/media/Bitmap.php	(revision 54284)
@@ -22,7 +22,7 @@
 		# JPEG has the handy property of allowing thumbnailing without full decompression, so we make
 		# an exception for it.
 		if ( $mimeType !== 'image/jpeg' &&
-			$srcWidth * $srcHeight > $wgMaxImageArea )
+			$this->getImageArea( $image, $srcWidth, $srcHeight ) > $wgMaxImageArea )
 		{
 			return false;
 		}
@@ -39,6 +39,13 @@
 
 		return true;
 	}
+	
+	
+	// Function that returns the number of pixels to be thumbnailed.
+	// Intended for animated GIFs to multiply by the number of frames.
+	function getImageArea( $image, $width, $height ) {
+		return $width * $height;
+	}
 
 	function doTransform( $image, $dstPath, $dstUrl, $params, $flags = 0 ) {
 		global $wgUseImageMagick, $wgImageMagickConvertCommand, $wgImageMagickTempDir;
Index: trunk/phase3/includes/media/GIFMetadataExtractor.php
===================================================================
--- trunk/phase3/includes/media/GIFMetadataExtractor.php	(revision 0)
+++ trunk/phase3/includes/media/GIFMetadataExtractor.php	(revision 54284)
@@ -0,0 +1,171 @@
+<?php
+/**
+  * GIF frame counter.
+  * Originally written in Perl by Steve Sanbeg.
+  * Ported to PHP by Andrew Garrett
+  * Deliberately not using MWExceptions to avoid external dependencies, encouraging
+  * redistribution.
+  */
+
+class GIFMetadataExtractor {
+	static $gif_frame_sep;
+	static $gif_extension_sep;
+	static $gif_term;
+
+	static function getMetadata( $filename ) {
+		wfProfileIn( __METHOD__ );
+	
+		self::$gif_frame_sep = pack( "C", ord("," ) );
+		self::$gif_extension_sep = pack( "C", ord("!" ) );
+		self::$gif_term = pack( "C", ord(";" ) );
+		
+		$frameCount = 0;
+		$duration = 0.0;
+		$isLooped = false;
+		
+		if (!$filename)
+			throw new Exception( "No file name specified" );
+		elseif ( !file_exists($filename) || is_dir($filename) )
+			throw new Exception( "File $filename does not exist" );
+		
+		$fh = fopen( $filename, 'r' );
+		
+		if (!$fh)
+			throw new Exception( "Unable to open file $filename" );
+		
+		// Check for the GIF header
+		$buf = fread( $fh, 6 );
+		if ( !($buf == 'GIF87a' || $buf == 'GIF89a') ) {
+			throw new Exception( "Not a valid GIF file; header: $buf" );
+		}
+		
+		// Skip over width and height.
+		fread( $fh, 4 );
+		
+		// Read BPP
+		$buf = fread( $fh, 1 );
+		$bpp = self::decodeBPP( $buf );
+		
+		// Skip over background and aspect ratio
+		fread( $fh, 2 );
+		
+		// Skip over the GCT
+		self::readGCT( $fh, $bpp );
+		
+		while( !feof( $fh ) ) {
+			$buf = fread( $fh, 1 );
+			
+			if ($buf == self::$gif_frame_sep) {
+				// Found a frame
+				$frameCount++;
+				
+				## Skip dimensions (Why is this 8 bytes and not 4?)
+				fread( $fh, 8 );
+				
+				## Read BPP
+				$buf = fread( $fh, 1 );
+				$bpp = self::decodeBPP( $buf );
+				
+				## Read GCT
+				self::readGCT( $fh, $bpp );
+				fread( $fh, 1 );
+				self::skipBlock( $fh );	
+			} elseif ( $buf == self::$gif_extension_sep ) {
+				$buf = fread( $fh, 1 );
+				$extension_code = unpack( 'C', $buf );
+				$extension_code = $extension_code[1];
+				
+				if ($extension_code == 0xF9) {
+					// Graphics Control Extension.
+					fread( $fh, 1 ); // Block size
+					fread( $fh, 1 ); // Transparency, disposal method, user input
+					
+					$buf = fread( $fh, 2 ); // Delay, in hundredths of seconds.
+					$delay = unpack( 'v', $buf );
+					$delay = $delay[1];
+					$duration += $delay * 0.01;
+					
+					fread( $fh, 1 ); // Transparent colour index
+					
+					$term = fread( $fh, 1 ); // Should be a terminator
+					$term = unpack( 'C', $term );
+					$term = $term[1];
+					if ($term != 0 )
+						throw new Exception( "Malformed Graphics Control Extension block" );
+				} elseif ($extension_code == 0xFF) {
+					// Application extension (Netscape info about the animated gif)
+					$blockLength = fread( $fh, 1 );
+					$blockLength = unpack( 'C', $blockLength );
+					$blockLength = $blockLength[1];
+					$data = fread( $fh, $blockLength );
+					
+					// NETSCAPE2.0 (application name)
+					if ($blockLength != 11 || $data != 'NETSCAPE2.0') {
+						self::skipBlock();
+						continue;
+					}
+					
+					fread( $fh, 2 ); // Block length and introduction, should be 03 01
+					
+					// Unsigned little-endian integer, loop count or zero for "forever"
+					$loopData = fread( $fh, 2 );
+					$loopData = unpack( 'v', $loopData );
+					$loopCount = $loopData[1];
+					
+					if ($loopCount != 1) {
+						$isLooped = true;
+					}
+					
+					// Read out terminator byte
+					fread( $fh, 1 );
+				}
+			} elseif ( $buf == self::$gif_term ) {
+				break;
+			} else {
+				$byte = unpack( 'C', $buf );
+				$byte = $byte[1];
+				throw new Exception( "At position: ".ftell($fh). ", Unknown byte ".$byte );
+			}
+		}
+		
+		wfProfileOut( __METHOD__ );
+		
+		return array(
+			'frameCount' => $frameCount,
+			'looped' => $isLooped,
+			'duration' => $duration
+		);
+		
+	}
+	
+	static function readGCT( $fh, $bpp ) {
+		if ($bpp > 0) {
+			for( $i=1; $i<=pow(2,$bpp); ++$i ) {
+				fread( $fh, 3 );
+			}
+		}
+	}
+	
+	static function decodeBPP( $data ) {
+		$buf = unpack( 'C', $data );
+		$buf = $buf[1];
+		$bpp = ( $buf & 7 ) + 1;
+		$buf >>= 7;
+		
+		$have_map = $buf & 1;
+		
+		return $have_map ? $bpp : 0;
+	}
+	
+	static function skipBlock( $fh ) {
+		while ( !feof( $fh ) ) {
+			$buf = fread( $fh, 1 );
+			$block_len = unpack( 'C', $buf );
+			$block_len = $block_len[1];
+			if ($block_len == 0)
+				return;
+			fread( $fh, $block_len );
+		}
+	}
+
+}
\ No newline at end of file
Index: trunk/phase3/includes/media/GIF.php
===================================================================
--- trunk/phase3/includes/media/GIF.php	(revision 0)
+++ trunk/phase3/includes/media/GIF.php	(revision 54284)
@@ -0,0 +1,58 @@
+<?php
+/**
+ * @file
+ * @ingroup Media
+ */
+
+/**
+ * Handler for GIF images.
+ *
+ * @ingroup Media
+ */
+class GIFHandler extends BitmapHandler {
+	
+	function getMetadata( $image, $filename ) {
+		if ( !isset($image->parsedGIFMetadata) )
+			$image->parsedGIFMetadata = GIFMetadataExtractor::getMetadata( $filename );
+
+		return serialize($image->parsedGIFMetadata);			
+
+	}
+	
+	function formatMetadata( $image ) {
+		return false;
+	}
+	
+	function getImageArea( $image, $width, $height ) {
+		$metadata = unserialize($image->getMetadata());
+		return $width * $height * $metadata['frameCount'];
+	}
+	
+	function getMetadataType( $image ) {
+		return 'parsed-gif';
+	}
+	
+	function getLongDesc( $image ) {
+		global $wgUser, $wgLang;
+		$sk = $wgUser->getSkin();
+		
+		$metadata = unserialize($image->getMetadata());
+		
+		$info = array();
+		$info[] = $image->getMimeType();
+		$info[] = $sk->formatSize( $image->getSize() );
+		
+		if ($metadata['looped'])
+			$info[] = wfMsgExt( 'file-info-gif-looped', 'parseinline' );
+		
+		if ($metadata['frameCount'] > 1)
+			$info[] = wfMsgExt( 'file-info-gif-frames', 'parseinline', $metadata['frameCount'] );
+		
+		if ($metadata['duration'])
+			$info[] = $wgLang->formatTimePeriod( $metadata['duration'] );
+		
+		$infoString = $wgLang->commaList( $info );
+		
+		return "($infoString)";
+	}
+}
Index: trunk/phase3/includes/AutoLoader.php
===================================================================
--- trunk/phase3/includes/AutoLoader.php	(revision 54283)
+++ trunk/phase3/includes/AutoLoader.php	(revision 54284)
@@ -87,6 +87,8 @@
 	'ForkController' => 'includes/ForkController.php',
 	'FormatExif' => 'includes/Exif.php',
 	'FormOptions' => 'includes/FormOptions.php',
+	'GIFMetadataExtractor' => 'includes/media/GIFMetadataExtractor.php',
+	'GIFHandler' => 'includes/media/GIF.php',
 	'GlobalDependency' => 'includes/CacheDependency.php',
 	'HashBagOStuff' => 'includes/BagOStuff.php',
 	'HashtableReplacer' => 'includes/StringUtils.php',
Index: trunk/phase3/includes/DefaultSettings.php
===================================================================
--- trunk/phase3/includes/DefaultSettings.php	(revision 54283)
+++ trunk/phase3/includes/DefaultSettings.php	(revision 54284)
@@ -2129,7 +2129,7 @@
 $wgMediaHandlers = array(
 	'image/jpeg' => 'BitmapHandler',
 	'image/png' => 'BitmapHandler',
-	'image/gif' => 'BitmapHandler',
+	'image/gif' => 'GIFHandler',
 	'image/tiff' => 'TiffHandler',
 	'image/x-ms-bmp' => 'BmpHandler',
 	'image/x-bmp' => 'BmpHandler',
Index: trunk/phase3/languages/messages/MessagesEn.php
===================================================================
--- trunk/phase3/languages/messages/MessagesEn.php	(revision 54283)
+++ trunk/phase3/languages/messages/MessagesEn.php	(revision 54284)
@@ -3411,6 +3411,8 @@
 'svg-long-desc'        => '(SVG file, nominally $1 × $2 pixels, file size: $3)',
 'show-big-image'       => 'Full resolution',
 'show-big-image-thumb' => '<small>Size of this preview: $1 × $2 pixels</small>',
+'file-info-gif-looped' => 'looped',
+'file-info-gif-frames' => '$1 frames',
 
 # Special:NewFiles
 'newimages'             => 'Gallery of new files',

Follow-up revisions

RevisionCommit summaryAuthorDate
r54285Follow-up r54284: PLURAL keyword for messageraymond15:14, 3 August 2009

Comments

#Comment by Raymond (Talk | contribs)   17:20, 3 August 2009

Seen at translatewiki running r54285.

[03-Aug-2009 17:12:05] PHP Notice: unserialize() [<a href='function.unserialize'>function.unserialize</a>]: Error at offset 0 of 1 bytes in /var/www/w/includes/media/GIF.php on line 27

Unclear what image triggered this notice.

#Comment by Werdna (Talk | contribs)   12:47, 4 August 2009

Possibly cached null metadata, which can't be unserialized for obvious reasons.

#Comment by Nikerabbit (Talk | contribs)   16:13, 4 August 2009

Can you make the cache to fix itself? Obviously it is not doing the right thing now. Btw the value seems to be "0", if that helps.

Status & tagging log

  • 17:25, 19 August 2009 Brion VIBBER (Talk | contribs) changed the status of r54284 [removed: fixme added: resolved]
  • 17:20, 3 August 2009 Raymond (Talk | contribs) changed the status of r54284 [removed: new added: fixme]
Views
Toolbox