Wikimedia Bugzilla is closed!

I will be wokring on this bug.

Created attachment 10253 [details]
*Basic* implementation of Rotate image API.

*Please add the patch in the next comment to check the API *

I have written a very basic API for rotating the image(only multiples of 90 degrees). Please check it and suggest me what changes I have to do.

Example- ../api.php?action=rotateimage&title=Image1.jpg&angle=90

Created attachment 10254 [details]
Patch to be used with the above code

*Include this patch with the above attachment*

Ok, so

General:
Any and all patches need to have their code something like our style guidelines. It doesn't need to be perfect, but somewhere near. See https://www.mediawiki.org/wiki/Manual:Coding_conventions - We also have a tool that will sort most of the issues available at http://svn.wikimedia.org/svnroot/mediawiki/trunk/tools/code-utils/stylize.php

You also seem to be using a mix of space and tabs for leading whitespace. Don't do that, just use tabs, see the coding conventions.

Please submit all additions as patches see https://www.mediawiki.org/wiki/Patches#Submit_your_changes - Even new files. All modified/created files can be submitted as one patch.

You will also need to set svn:keywords Id on the new API file (this is for getVersion)

As well as adding the new class to ApiMain.php, it also needs adding to includes/Autoloader.php in the API section.

Api Stuff:

Your initializeParams seems slightly useless, just do this all in execute(), and only put things into class variables if it's really needed. For your current code, you could just pass it as parameters.

You shouldn't be attempting to reinvent the wheel when you're dealing with parameters. Like you've used ApiBase::PARAM_REQUIRED, you can set ApiBase::PARAM_ISMULTI => true, and the api will do the building and splitting for you, giving you an array to work with.

Image Stuff:
I don't think that saving it into the uploadstash is a sensible idea. IMHO (at least), a rotated image is essentially a thumbnail, so why not treat it as such?

MediaWiki Image Url: https://commons.wikimedia.org/wiki/File:%C3%96tlingen_-_Panoramaansicht_klein.jpg

Physical Url: https://upload.wikimedia.org/wikipedia/commons/a/a2/%C3%96tlingen_-_Panoramaansicht_klein.jpg

Thumbnail url: https://upload.wikimedia.org/wikipedia/commons/thumb/a/a2/%C3%96tlingen_-_Panoramaansicht_klein.jpg/800px-%C3%96tlingen_-_Panoramaansicht_klein.jpg

Could we not use something similar, and insert the rotation amount after the pixels, but before the actual image name?

You're also reinventing the wheel here, directly using specific rotate methods. Don't do that. MediaWiki allows usage of different pieces of external software to do thumbnailing. See https://www.mediawiki.org/wiki/Manual:Image_Administration

MediaWiki currently has limited rotation support, allowing images that have metadata suggesting the image is upside down to be displayed the correct way. See Bitmap::transformImageMagick() and the global $wgEnableAutoRotation for the automatic rotation.

if ( $rotation % 360 != 0 && $rotation % 90 == 0 ) {
	$rot_image = imagerotate( $dst_image, $rotation, 0 );
	imagedestroy( $dst_image );
	$dst_image = $rot_image;
}

If the current functionality in the image classes isn't enough, add to it, but add it there, not to the API itself.

We also have:

public static function canRotate() {
	$scaler = self::getScalerType( null, false );
	switch ( $scaler ) {
		case 'im':
			# ImageMagick supports autorotation
			return true;
		case 'imext':
			# Imagick::rotateImage
			return true;
		case 'gd':
			# GD's imagerotate function is used to rotate images, but not
			# all precompiled PHP versions have that function
			return function_exists( 'imagerotate' );
		default:
			# Other scalers don't support rotation
			return false;
	}
}
	
Which you can use in your code, and stop with an error if rotation isn't available

The limitation of the 90 rotate (though, per the mediawiki code you shouldn't rotate by 360 degrees multiples either) is alright, though would be nice if we could do arbitary rotations of any degress.

I'd also recommend adding a direction parameter - default to clockwise, but allow anticlockwise also


If you need more information/help about the file code, we have a few developers who are rather file orientated

(In reply to comment #4)
> Image Stuff:
> I don't think that saving it into the uploadstash is a sensible idea. IMHO (at
> least), a rotated image is essentially a thumbnail, so why not treat it as
> such?
>
I think the while point of this bug was to upload a new, rotated version, but I could be wrong in that.

(In reply to comment #5)
> (In reply to comment #4)
> > Image Stuff:
> > I don't think that saving it into the uploadstash is a sensible idea. IMHO (at
> > least), a rotated image is essentially a thumbnail, so why not treat it as
> > such?
> >
> I think the while point of this bug was to upload a new, rotated version, but I
> could be wrong in that.

To be honest, I really can't remember. In theory, both would make sense? Our 404 thumbhandler could be made to handle rotation too

Patch is reviewed.

We need some way of making it clearer when a patch has been reviewed, but needs more work.  Per Sam's comment, there's a lot of work necessary to get this into shape for merging in.  Bagariavivek, is this work that you are up to?  If not, we should find someone else to finish off this work.

MediaWiki (or Wikimedia Commons) needs a built-in image editor (bug 37732). A built-in image editor would presumably include the ability to rotate images.

Is this how we want this to be implemented? Where each activity is an API action with some kind of UI wrapper? I'm worried that enough consideration isn't being taken here.

pushed a patch to implement a rotate api to
https://gerrit.wikimedia.org/r/#/c/44005/

Patch was merged two days ago.
Is there anything else needed here or can this be closed as FIXED?

Image rotation really needs to be done on the image scalers. We have the image scalers because image scaling is memory intensive, and 90° rotation is irreducibly memory intensive, so it makes sense to break it out in the same way. Also, the required packages for image rotation are pretty much the same as for scaling.

It could be done with a curl request to the same API module, with an extra "no proxy" parameter to avoid loops.

It would be nice if the rotation used lossless rotation for jpeg files (jpegtran), if possible.

JpegHandler::rotate (includes/media/Jpeg.php) uses jpegtrans if its installed.

As I understand it, this bug is fixed: MediaWiki has an API to rotate images. However, it is currently disabled on the WMF cluster.

Tim's comment 12 appears to relate to the WMF cluster only.

(In reply to This, that and the other from comment #15)
> As I understand it, this bug is fixed: MediaWiki has an API to rotate
> images. However, it is currently disabled on the WMF cluster.
> 
> Tim's comment 12 appears to relate to the WMF cluster only.

I would consider this bug open until such a time we have a solution that is acceptable to be turned on on wmf cluster.