https://bugzilla.wikimedia.org/show_bug.cgi?id=23258
Tim Starling <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |[email protected] --- Comment #2 from Tim Starling <[email protected]> 2010-06-22 04:24:06 UTC --- Review follows. I'm not sure who is still involved in this project so I'm just putting my comments here. I don't know why this is an extension. Can it be moved to the core, replacing the barely-functional TiffHandler there? Is there any reason we have to use exiv to read EXIF data? We already have functions for reading EXIF data from TIFF files in the MediaWiki core, in Exif.php. The "cache" in PagedTiffImage::retrieveMetaData() seems to just be a way to completely break image uploads for a given filename for a day, I can't see any practical purpose for it. But it's a moot point if the whole file is going to be deleted in favour of Exif.php. I've read a bit about VIPS, I see that this extension uses it. VIPS has the ability to scale arbitrarily sized TIFF files with limited RAM, which would be a useful property for us. But it only works if the whole pipeline is PIO rather than WIO. This means that the input TIFF file must be tiled (RowsPerStrip > 16), and only PIO transformation functions may be used. PagedTiffHandler currently uses im_resize_linear which is WIO. If it used the PIO function im_shrink instead, then the RAM usage would be limited. im_resize_linear will look nicer when a small TIFF image is resized to a slightly smaller image. That's not the expected use case for PagedTiffHandler on Wikimedia. im_shrink will look OK when the thumbnail is several times smaller than the source image, which is the case we expect to be dealing with. To limit memory usage in VIPS, please have PagedTiffHandler error out if it tries to scale a source image with area greater than $wgMaxImageArea and RowsPerStrip less than 16. And use im_shrink. Ideally I would like to see two-stage conversion, so that very large (>100 Mpx) images can be scaled down with im_shrink to somewhat larger than web size, say 4 Mpx, once only per upload. Then the 4 Mpx intermediate image would be used to generate the ~0.1 Mpx thumbnail images requested by editors. The final step could be done with ImageMagick, or with VIPS with a transformation similar to the one ImageMagick uses. It's a common misconception that downscaling requires interpolation. Really it needs blurring followed by downsampling followed by sharpening, that's what ImageMagick does. It's only upscaling (or downscaling by a very small factor) that needs interpolation. im_shrink uses a very simple kind of blurring, specifically a block average. The key to high quality downscaling is to choose an appropriate window function to do the blur. ImageMagick uses a Lanczos filter by default, see: http://www.imagemagick.org/script/command-line-options.php#filter In PagedTiffHandler.php: $wgTiffErrorCacheTTL = 84600; Is this meant to be 86400, the number of seconds in a day? In PagedTiffHandler_body.php: // @todo check height ## MediaWiki doesn't have a MagicWord-ID for height, appears to be a bug. ^SU It's not a bug. Width and height are specified using two numbers separated by an "x", e.g. 100x100px. There is a special case for this at Parser.php line 4689. If the img_width magic word parameter matches this format, then a height parameter will automatically be passed through to the handler. * TODO: The purpose of this function is not yet fully clear. */ function getScriptParams( $params ) { # # wtf?? ^DK // FIXME: This function is unused, seems to be useless, // and could be replaced with an array_intersect() call The function is used to specify which parameters to File::transform() should be passed through to thumb.php, in the case where the configuration specifies thumb.php is to be used (e.g. $wgThumbnailScriptPath !== false). You should pass through the same parameters as in makeParamString(). The code you have here would be fine if the fixme comments were removed, except for a detail about the lossy parameter, see below. PagedTiffHandler::normaliseParams() seems to be duplicating code from ImageHandler::normaliseParams(). I'm not sure why this is necessary. DjVuHandler gets away with not overriding normaliseParams() at all. If there is a bug in ImageHandler::normaliseParams(), you should fix it, rather than forking it. If all you're doing is validating the lossy parameter, then you can do that with code like: function normaliseParams( $image, &$params ) { if ( !parent::normaliseParams( $image, $params ) ) { return false; } // ... handle lossy parameter... } If you normalise the lossy parameter properly at this point, you avoid the need to duplicate the code in makeParamString(), getScriptParams() and getThumbExtension(). Also, you resolve a bug due to makeParamString() not matching getThumbExtension() in the way it handles a missing parameter. function normaliseParams( $image, &$params ) { if ( !parent::normaliseParams( $image, $params ) ) { return false; } if ( isset( $params['lossy'] ) ) { if ( in_array( $params['lossy'], array( 1, '1', 'true', 'lossy' ) ) ) { $params['lossy'] = 'lossy'; } else { $params['lossy'] = 'lossless'; } } else { $data = $this->getMetaArray( $image ); if ( ( strtolower( $data['page_data'][$params['page']]['alpha'] ) == 'true' ) ) { $params['lossy'] = 'lossless'; } else { $params['lossy'] = 'lossy'; } } return true; } In PagedTiffHandler::doTransform: please remove the error suppression (@) operators. normaliseParams() should return false if the width and height are missing. Error suppression operators have lots of implementation issues in PHP, they are evil and should almost never be used. PagedTiffHandler::getThumbType() is missing and the parent is not returning the correct type. Please fix this by modifying File::thumbName() to pass handler parameters through to MediaHandler::getThumbType() as a third parameter, so that your lossy/lossless feature can work. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug. You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
