Page MenuHomePhabricator

Auto-rotating images is broken on but works fine on my local checkout
Closed, ResolvedPublic


The image at that page should be 600x800 px. However, its resized to 450x600 px which is the right aspect ratio but too small. It's then upscaled by attributes on the <img> tag, which makes it look crapy.

I have no idea why this is happening. It works fine on my local checkout of REL1_18 and my checkout of 1.18wmf1

Version: 1.18.x
Severity: normal



Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 21 2014, 11:53 PM
bzimport set Reference to bz31024.

Bryan.TongMinh wrote:

Different scaler? GD instead of ImageMagick?

A different scaler shouldn't result in MW chucking out a 450x600 image (and resizing via css) when it should be 600x800.

It seems the preview page points to , and then the scaler takes 600px to be the limit for the greatest dimension (width, in this case, rather than height). Which of these two things is wrong? Should the URL be 800px- instead of 600px- , or should the scaler always look at the height rather than the greatest dimension?

The given width is the output width. You should end up with an image that's 600px wide.

Conceptually, the original image's size is 768x1024px -- and it *SHOULD* be reported that way consistently throughout MediaWiki. If it's still showing the size as 1024x768 on the file page -- which it is -- then there may be something wrong internally.

Perhaps not all the fixes on trunk got backported to 1.18, or perhaps it's still not 100% working on trunk, or perhaps something's still kinda messed up in the installation or the scalers are calling 1.17 code or some weird mix?

Ok, the behavior on trunk and 1.18 seems to match in these broken ways:

  • when uploading on Special:Upload, the preview thumbnail is rotated correctly, but width/height are reported unrotated (WRONG)
  • File: page incorrectly reports unrotated width/height (WRONG)
  • under default thumbnailing config, it produces a 600x800px image which is too big to fit within the 800x600px default File: page image size maximum (labeled as 600px wide, but correctly sized to that)

If I set $wgThumbnailScriptPath to '/trunk/thumb.php' (to match my dir), remopve the thumbnails, and then refresh I get:

  • File: page asks for thumb.php?f=Portrait-rotated_X.jpg&width=600 which produces a 450x600px image
  • but the File: page still sizes it to 600x800px

So..... there's a butt-ton of wrong stuff going on, but you can repro the problem easily by running the thumbnails through thumb.php.


Partway through fixing this; have updated the unit tests to check the reported image size correctly again (regressed in r92246) and also to actually run some thumbnail transformations and check the resulting sizes.

It looks like the prior code wasn't actually setting the image's reported width/height to match the rotation, it was just fiddling things around later on when you asked for thumbnails and such, then getting a bit confuzzled.

This explains the problem reported here:

  • ImagePage asks for a thumbnail that fits in 800x600
  • BitmapHandler ends up returning a MediaTransform specifying 600x800 (wrong size limit, right aspect ratio)
  • the delayed transform uses a URL for 600px-whatever
  • that hits 404 handler turning into thumb.php?w=600
  • BitmapHandler ends up returning a MediaTransform for 450x600 (wrong size limit, right aspect ratio)
  • the 450x600 image gets returned for the 600px file spot

Working on a fix to normalize how rotated images are handled along these lines:

  • swap the width/height to fit the logical size in ExifBitmapHandler::getImageSize, so it gets saved and used with logical size
  • change the thumbnailing bits in BitmapHandler to handle the rotation by, if necessary, swapping widths/heights back and forth in the work stuff

This should allow the various box-sizing code to avoid having to think about rotation at all -- since it only cares about logical size -- and should also make exif-rotated images transparent to remote API clients (eg InstantCommons / RemoteApiRepo) -- they'll see the logical size, request the logical size, and get back thumb URLs matching the logical size.

r97651, r97656, r97659 updated the test cases...

r97671 fixes the bugs on trunk. Now checks file metadata at ExifBitmapHandler::getImageSize() time and sets the width/height to the correct logical size, which simplifies things a lot. :)

We only now need to swap coords back when generating the output thumbnail, which seems to work nicely. This also gets rotated images working correctly over ForeignApiRepo -- under the previous code, things got rather confusing as it would switch coordinates around and wasn't compatible between 1.18 and 1.17.

New code with application to 1.18 should keep things clean.