r107351 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r107350‎ | r107351 | r107352 >
Date:00:46, 27 December 2011
Author:mah
Status:resolved (Comments)
Tags:
Comment:
* First simple XCF thumbnailing. Convert from ImageMagick has buggy
support for XCF, though, so one of the solutions at
http://stackoverflow.com/questions/5794640/how-to-convert-xcf-to-png-using-gimp-from-the-command-line
is probably better.

* File::maybeDoTransform() since (at least) r106752 neglects what
BitmapHandler::getThumbType() returns, so add support for that.

* Add $wgImageMagickIdentifyCommand to avoid writing a parser for XCF
metadata.
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/filerepo/file/File.php (modified) (history)
  • /trunk/phase3/includes/media/Bitmap.php (modified) (history)
  • /trunk/phase3/includes/media/XCF.php (added) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/file/File.php
@@ -778,8 +778,11 @@
779779 wfDebug( __METHOD__ . " forcing rendering per flag File::RENDER_FORCE\n" );
780780 }
781781
782 - // Create a temp FS file with the same extension
783 - $tmpFile = TempFSFile::factory( 'transform_', $this->getExtension() );
 782+ // Create a temp FS file with the same extension and the thumbnail
 783+ $extension = $this->getExtension();
 784+ list( $thumbExt, $thumbMime ) = $this->handler->getThumbType(
 785+ $extension, $this->getMimeType(), $params );
 786+ $tmpFile = TempFSFile::factory( 'transform_', $this->getExtension() . '.' . $thumbExt );
784787 if ( !$tmpFile ) {
785788 return new MediaTransformError( 'thumbnail_error',
786789 $params['width'], 0, wfMsg( 'thumbnail-temp-create' ) );
Index: trunk/phase3/includes/media/Bitmap.php
@@ -302,6 +302,8 @@
303303 $animation_post = '-fuzz 5% -layers optimizeTransparency';
304304 }
305305 }
 306+ } elseif ( $params['mimeType'] == 'image/x-xcf' ) {
 307+ $animation_post = '-layers merge';
306308 }
307309
308310 // Use one thread only, to avoid deadlock bugs on OOM
Index: trunk/phase3/includes/media/XCF.php
@@ -0,0 +1,93 @@
 2+<?php
 3+/**
 4+ * Handler for Microsoft's bitmap format
 5+ *
 6+ * @file
 7+ * @ingroup Media
 8+ */
 9+
 10+/**
 11+ * Handler for Microsoft's bitmap format; getimagesize() doesn't
 12+ * support these files
 13+ *
 14+ * @ingroup Media
 15+ */
 16+class XCFHandler extends BitmapHandler {
 17+
 18+ /**
 19+ * Render files as PNG
 20+ *
 21+ * @param $ext
 22+ * @param $mime
 23+ * @param $params
 24+ * @return array
 25+ */
 26+ function getThumbType( $ext, $mime, $params = null ) {
 27+ return array( 'png', 'image/png' );
 28+ }
 29+
 30+ /**
 31+ * Get width and height from the bmp header.
 32+ *
 33+ * @param $image
 34+ * @param $filename
 35+ * @return array
 36+ */
 37+ function getImageSize( $image, $filename ) {
 38+ return self::getXCFMetaData( $filename );
 39+ }
 40+
 41+ static function getXCFMetaData( $filename ) {
 42+ global $wgImageMagickIdentifyCommand;
 43+
 44+ $md = false;
 45+ $cmd = wfEscapeShellArg( $wgImageMagickIdentifyCommand ) . ' -verbose ' . wfEscapeShellArg( $filename );
 46+ wfDebug( __METHOD__ . ": Running $cmd \n" );
 47+ $retval = '';
 48+ $return = wfShellExec( $cmd, $retval );
 49+
 50+ if( $retval == 0 ) {
 51+ $colorspace = preg_match_all( '/ *Colorspace: RGB/', $return, $match );
 52+ $frameCount = preg_match_all( '/ *Geometry: ([0-9]+x[0-9]+)\+[+0-9]*/', $return, $match );
 53+ wfDebug( __METHOD__ . ": Got $frameCount matches\n" );
 54+
 55+ /* if( $frameCount == 1 ) { */
 56+ /* preg_match( '/([0-9]+)x([0-9]+)/sm', $match[1][0], $m ); */
 57+ /* $sizeX = $m[1]; */
 58+ /* $sizeY = $m[2]; */
 59+ /* } else { */
 60+ $sizeX = 0;
 61+ $sizeY = 0;
 62+
 63+ foreach( $match[1] as $res ) {
 64+ preg_match( '/([0-9]+)x([0-9]+)/sm', $res, $m );
 65+ if( $m[1] > $sizeX ) {
 66+ $sizeX = $m[1];
 67+ }
 68+ if( $m[2] > $sizeY ) {
 69+ $sizeY = $m[2];
 70+ }
 71+ }
 72+ /* } */
 73+
 74+ wfDebug( __METHOD__ . ": Found $sizeX x $sizeY x $frameCount \n" );
 75+ $md['frameCount'] = $frameCount;
 76+ $md[0] = $sizeX;
 77+ $md[1] = $sizeY;
 78+ $md[2] = null;
 79+ $md[3] = "height=\"$sizeY\" width=\"$sizeX\"";
 80+ $md['mime'] = 'image/x-xcf';
 81+ $md['channels'] = $colorspace == 1 ? 3 : 4;
 82+ }
 83+ return $md;
 84+ }
 85+
 86+ /**
 87+ * Must use "im" for XCF
 88+ *
 89+ * @return string
 90+ */
 91+ protected static function getScalerType( $dstPath, $checkDstPath = true ) {
 92+ return "im";
 93+ }
 94+}
Property changes on: trunk/phase3/includes/media/XCF.php
___________________________________________________________________
Added: svn:eol-syle
195 + native
Index: trunk/phase3/includes/DefaultSettings.php
@@ -620,6 +620,7 @@
621621 'image/tiff' => 'TiffHandler',
622622 'image/x-ms-bmp' => 'BmpHandler',
623623 'image/x-bmp' => 'BmpHandler',
 624+ 'image/x-xcf' => 'XCFHandler',
624625 'image/svg+xml' => 'SvgHandler', // official
625626 'image/svg' => 'SvgHandler', // compat
626627 'image/vnd.djvu' => 'DjVuHandler', // official
@@ -638,6 +639,8 @@
639640 $wgUseImageMagick = false;
640641 /** The convert command shipped with ImageMagick */
641642 $wgImageMagickConvertCommand = '/usr/bin/convert';
 643+/** The identify command shipped with ImageMagick */
 644+$wgImageMagickIdentifyCommand = '/usr/bin/identify';
642645
643646 /** Sharpening parameter to ImageMagick */
644647 $wgSharpenParameter = '0x0.4';

Follow-up revisions

RevisionCommit summaryAuthorDate
r107355r107351: forgot to commit the changes to AutoLoader ... also update a commentmah02:05, 27 December 2011
r107379alphabetize includes/media autoloaders + some whitespacemah13:34, 27 December 2011
r107382Followup r107351 and r107381...reedy15:10, 27 December 2011
r107447re Aaron's comment on r107351: remove double extension from temporary filemah22:39, 27 December 2011
r108024XCF format: code style/comment...hashar13:37, 4 January 2012
r108141rewrite getXCFMetaData() to get ride of im identify...hashar14:48, 5 January 2012
r108862XCF: force rendering...hashar22:09, 13 January 2012
r108965Release note for ug 17959...hashar11:05, 15 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r106752Merged FileBackend branch. Manually avoiding merging the many prop-only chang...aaron03:52, 20 December 2011

Comments

#Comment by MarkAHershberger (talk | contribs)   00:51, 27 December 2011

Should have added a ref for Bug #17959

#Comment by Reedy (talk | contribs)   01:02, 27 December 2011

A new class, in a new file... Needs to go in the AutoLoader! :)

#Comment by Bawolff (talk | contribs)   01:23, 27 December 2011

Minor issue:

Handler for Microsoft's bitmap format
#Comment by Aaron Schulz (talk | contribs)   18:34, 27 December 2011

What is the point of changing maybeDoTransform() as above? It also now adds the thumb extension even if it's the same as the source file extension.

Probably the temp file should only have one extension equal to FileBackend::extensionFromPath( $thumbPath ).

#Comment by MarkAHershberger (talk | contribs)   22:41, 27 December 2011

Thanks, this part confused me. Next time I'm stuck in the entrails of this area, you may be getting a call from me. ;)

The point is to get the extension to match the type being converted to. ImageMagick uses the extension to determine the type to create and in the old code it was being passed the XCF extension.

Changed to your suggestion in r107447.

#Comment by Brion VIBBER (talk | contribs)   22:54, 3 January 2012

Needs test case & data file to confirm that the image size getting works. Ideally should be able to read the size from the file header directly instead of shelling out to ImageMagick.

Also... this should probably be an extension rather than core.

#Comment by Hashar (talk | contribs)   14:12, 4 January 2012

It is probably safer to just rely on imagemagick to read metadata. I am not sure it is worth the time to implements our own metadata reader for XCF.

If we support XCF format out of the box, it might help spreading that file format :D An extension will probably not receive enough attention and will end up decaying somewhere in our svn repo :-/

#Comment by Hashar (talk | contribs)   14:24, 4 January 2012

Imagemagick implementation for the XCF file format at:

http://www.google.com/codesearch#vKN9r9MhgKg/coders/xcf.c&q=imagemagick%20xcf

#Comment by Bawolff (talk | contribs)   18:47, 4 January 2012

Why exactly would we want to spread this format. Its not really a very good format for users, its more like the source code for an image, which well an important thing, isn't that important for most people.

#Comment by MarkAHershberger (talk | contribs)   19:12, 4 January 2012

I think have a free source format available is a way to encourage re-use... it really fits into the free culture mindset of the Commons

#Comment by Bawolff (talk | contribs)   01:38, 5 January 2012

It certainly makes sense for commons, I meant why are we trying to spread it to the average re-user of MediaWiki.

#Comment by Hashar (talk | contribs)   12:18, 5 January 2012

Part of "The Plan"

#Comment by MarkAHershberger (talk | contribs)   17:05, 4 January 2012

I know I said it was probably ok with me if you made this an extension, but I would now rather you didn't. I'd like these sort of files to be widely available in MediaWiki.

I'll try to add test cases today.

#Comment by Hashar (talk | contribs)   14:50, 5 January 2012

I have implement an header reader in r108141. Thus we are no more using ImageMagick identify.

Marking resolved. Removing tags hashar & hexmode

Status & tagging log