r99912 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r99911‎ | r99912 | r99913 >
Date:20:39, 15 October 2011
Author:btongminh
Status:resolved (Comments)
Tags:
Comment:
Per bug 28135:
* Set JPEG comment
* Add comment about the options array
* Automatically set $wgEnableAutoRotation to true
Modified paths:
  • /trunk/extensions/VipsScaler/VipsScaler.php (modified) (history)
  • /trunk/extensions/VipsScaler/VipsScaler_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/VipsScaler/VipsScaler_body.php
@@ -45,6 +45,11 @@
4646 }
4747 }
4848
 49+ # Set comment
 50+ if ( !empty( $options['setcomment'] ) && isset( $params['comment'] ) ) {
 51+ self::setJpegComment( $params['dstPath'], $params['comment'] );
 52+ }
 53+
4954 # Set the output variable
5055 $mto = new ThumbnailImage( $file, $params['dstUrl'],
5156 $params['clientWidth'], $params['clientHeight'], $params['dstPath'] );
@@ -205,6 +210,22 @@
206211 }
207212
208213 /**
 214+ * Sets the JPEG comment on a file using exiv2.
 215+ * Requires $wgExiv2Command to be setup properly.
 216+ *
 217+ * @param string $fileName File where the comment needs to be set
 218+ * @param string $comment The comment
 219+ */
 220+ public static function setJpegComment( $fileName, $comment ) {
 221+ global $wgExiv2Command;
 222+
 223+ wfShellExec( wfEscapeShellArg( $wgExiv2Command ) . ' mo -c '
 224+ . wfEscapeShellArg( $comment ) . ' '
 225+ . wfEscapeShellArg( $fileName )
 226+ );
 227+ }
 228+
 229+ /**
209230 * Return the appropriate im_XXX2vips handler for this file
210231 * @param File $file
211232 * @return mixed String or false
Index: trunk/extensions/VipsScaler/VipsScaler.php
@@ -36,7 +36,21 @@
3737 # Download vips from http://www.vips.ecs.soton.ac.uk/
3838 $wgVipsCommand = 'vips';
3939
40 -# Options and conditions for images to be scaled with this scaler
 40+/* Options and conditions for images to be scaled with this scaler.
 41+ * Set to an array of arrays. The inner array contains a condition array, which
 42+ * contains a list of conditions that the image should pass for it to be scaled
 43+ * with vips. Conditions are mimeType, minArea, maxArea, minShrinkFactor,
 44+ * maxShrinkFactor. The other items in the array are options. Options available
 45+ * are:
 46+ * - sharpen: Set to an array with keys 'radius' and 'sigma', which are
 47+ * parameters to gaussian sharpen matrix.
 48+ * - preconvert: Convert the file to a .v file first, which costs some space,
 49+ * but saves memory on the actual downsize
 50+ * - bilinear: Use im_resize_linear instead of im_shrink
 51+ * - convolution: Apply specified convolution matrix
 52+ * - setcomment: Add an exif comment specifying the source of the file.
 53+ * Requires $wgExiv2Command to be set properly.
 54+ */
4155 $wgVipsOptions = array(
4256 # Sharpen jpeg files which are shrunk more than 1.2
4357 array(
@@ -62,3 +76,8 @@
6377 ),
6478 );
6579
 80+# This scaler support rotation
 81+$wgEnableAutoRotation = true;
 82+
 83+
 84+

Follow-up revisions

RevisionCommit summaryAuthorDate
r101375Partial revert of r99912: remove $wgEnableAutoRotation settingbtongminh18:18, 31 October 2011
r101678Follow-up r99911, r99912: Hook into new BitmapHandlerCheckImageArea hookbtongminh20:51, 2 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r99911Per bug 28135, disable $wgMaxImageArea check when transforming using a hook, ...btongminh20:36, 15 October 2011

Comments

#Comment by Tim Starling (talk | contribs)   05:36, 24 October 2011
+# This scaler support rotation
+$wgEnableAutoRotation = true;

It's not appropriate to set core configuration globals in an extension setup file like this. If the user puts $wgEnableAutoRotation = false; in their LocalSettings.php before VipsScaler is registered, it means they really don't want auto-rotation, regardless of whether it is supported.

You could add a hook to BitmapHandler::canRotate(), but the problem is that BitmapHandler::canRotate() doesn't specify any particular image, but VipsScaler whether will act on the BitmapHandlerTransform hook depends on what image is being scaled.

My advice is to remove that line of code for now, and to consider a better architecture for MediaWiki 1.19. Perhaps $wgEnableAutoRotation could be true by default, instead of null. Then BitmapHandler::getRotation() would return zero if the handler cannot rotate the given file, instead of just screwing up everything like it does now if $wgEnableAutoRotation is true incorrectly.

#Comment by Bryan (talk | contribs)   18:18, 31 October 2011
#Comment by Hashar (talk | contribs)   14:40, 3 November 2011

back to new per Brayn follow up (r101375)

Status & tagging log