Last modified: 2011-09-20 22:16:28 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T33024, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 31024 - Auto-rotating images is broken on test2.wikipedia.org but works fine on my local checkout
Auto-rotating images is broken on test2.wikipedia.org but works fine on my lo...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
File management (Other open bugs)
1.18.x
All All
: Highest normal (vote)
: ---
Assigned To: Brion Vibber
http://test2.wikipedia.org/wiki/File:...
:
Depends on:
Blocks: 6672 29068
  Show dependency treegraph
 
Reported: 2011-09-20 03:06 UTC by Bawolff (Brian Wolff)
Modified: 2011-09-20 22:16 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Bawolff (Brian Wolff) 2011-09-20 03:06:36 UTC
http://test2.wikipedia.org/wiki/File:Portrait-rotated.jpg

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
Comment 1 Bryan Tong Minh 2011-09-20 10:32:51 UTC
Different scaler? GD instead of ImageMagick?
Comment 2 p858snake 2011-09-20 10:34:47 UTC
A different scaler shouldn't result in MW chucking out a 450x600 image (and resizing via css) when it should be 600x800.
Comment 3 Roan Kattouw 2011-09-20 11:08:18 UTC
It seems the preview page points to http://upload.wikimedia.org/wikipedia/test2/thumb/c/c1/Portrait-rotated.jpg/600px-Portrait-rotated.jpg , 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?
Comment 4 Brion Vibber 2011-09-20 16:27:27 UTC
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 <http://test2.wikipedia.org/wiki/File:Portrait-rotated.jpg> -- 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?
Comment 5 Brion Vibber 2011-09-20 18:24:12 UTC
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.

Sigh.
Comment 6 Brion Vibber 2011-09-20 20:53:30 UTC
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.
Comment 7 Brion Vibber 2011-09-20 22:16:28 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links