Page MenuHomePhabricator

File::transform() creates thumbnail with wrong size when mustRender() is true
Open, HighPublic

Description

Example. The API returns whatever urlwidth parameter is passed as the thumbnail width (as long as it's not smaller than the original width).

The size logic in TransformationalImageHandler::doTransform seems completely wrong: File::transform will pass in blindly whatever size it was instructed to target, then clientWidth is set to that size, while physicalWidth is correctly set to the file width by normaliseParams, but clientWidth is what eventually gets passed to the ThumbnailImage object. (I suppose this is for the client-side scaling option that nobody ever uses?)

The API then tries to fix this by checking if the thumb URL is equal to the original URL and ignore the size claimed by ThumbnailImage in that case (cf rMWb744fdba237f: * (bug 23834) Invalid "thumbwidth" and "thumbheight" in "imageinfo" query when…) which is a poor workaround that fails for EXIF rotations and will probably fail for more in the future.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Causes all kinds of quirks outside the API as well:

  • thumbnails in file history get overmagnified for small images
  • thumbnail wikikcode does not respect size limit for EXIF-rotated images. Full image wikicode does not respect size limit for any image. (test)
  • some gallery types get messed up for small EXIF-rotated images
dr0ptp4kt triaged this task as Medium priority.Aug 7 2017, 4:33 PM
dr0ptp4kt moved this task from Untriaged to Next up on the Multimedia board.
dr0ptp4kt subscribed.

Next step on this would be to further investigate and scope level of effort before committing to work.

Change 375393 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/core@master] Don’t pass bigger-than-source-image dimensions to ThumbnailImage

https://gerrit.wikimedia.org/r/375393

MarkTraceur raised the priority of this task from Medium to High.Sep 11 2017, 4:21 PM
MarkTraceur moved this task from Doing to Needs code review on the Multimedia board.

I looked at the code, I would say it's fine to go, but I don't have any real opinion on whether to do it this way or some other way. The only question that comes to mind is, what happens when we try to pass in an SVG (which can be scaled without concern)?

Code looks ok to me too (apart from the failing test) but we need feedback on what the desired behaviour is - the image doesn't get resized, but in the file history or if you include it in a page with [[File:x.jpg|500px]] then height and width in the <img> tags will cause it to be scaled up in the browser. Is this something we need to address? @Ramsey-WMF ?

The current patch still needs work (doesn't even pass tests atm) if we want to pursue this.
I'm just not sure about where to take this.

Do we actually want [[File:xyz.jpg|500px]] files to be displayed at less than 500px, even if it was explicitly set for 500px? (and as Mark points out, some types can scale up quite well)
@Tgr since you raised this: any chance you could elaborate on how exactly this should turn out?

Do we actually want [[File:xyz.jpg|500px]] files to be displayed at less than 500px, even if it was explicitly set for 500px?

IMO yes, unless File::isVectorized returns true. Can't really imagine any use case for showing files in larger-than-life sizes (I guess you could do it for single-color images or simple gradients without looking horrible, but those can done with CSS easily).

On the other hand, if the user says "500px" and doesn't get 500px, they're probably going to be more surprised than if they say "500px" and get an ugly scaled up image. Especially since existing behavior is to give them the browser-scaled image.

@Ramsey-WMF ... we need a decision on this to move forward

Here's my take on it (apologies for the wall of text, but I like thoroughness). Short version: I think we should keep the functionality of File:xyz.jpg|XXXpx even if the params are larger than the original image size.

Long, explained version:

Tgr is correct that the current implementation does make things look ugly in many instances. However, there are reasonable cases where the current functionality could be useful, like getting size consistency with a group of images that are vastly different sizes

As an example, for the AdChoices icon in Gergő's test above, the original image we have on Commons is 75x79. If I'm putting together a group of ad-related logos and I want the thumbs to be a nice uniform size, I probably don't want to be limited to 75 pixels for all of them. I'd rather bump up the AdChoices image a bit to match the others.

To illustrate, I put together a test page with various ad logos: https://commons.wikimedia.org/wiki/User:RIsler_(WMF)/testthumb

On that page, we have the AdChoices icon scaled up to 170px, the tm logo scaled down from 1003px, and the rpa logo scaled up from 167px to 240px. The resulting images look presentable as a group, and this is desired/expected behavior from the perspective of someone just trying to make things look organized, if not perfect. But it would not be possible if we didn't allow File:xyz.jpg|XXXpx to work if the desired dimensions are bigger than the original size.

As a user, I would expect this to work for Thumbnails too, to a certain degree. It's fine if there's a max limit to thumbnails (they're thumbs after all), but I'd expect to be able to tweak thumbnail sizes within reason. Current functionality doesn't allow me to do that (as seen on my test page), which is kind of a bummer. That tiny AdChoices thumbnail just looks weird compared to the others

Now, we could easily say, "Well if you want to do something like that, use a gallery." And I did include a gallery on that test page as well. But there are a few issues with that:

  • I don't get the fine control over sizing that I get with a series of simple file includes
  • You'll notice that the AdChoices image in the gallery is scaled up (which is expected functionality), so at least one system is in fact already making images bigger than their original size when appropriate, so you might as well let me (the user) do it on my own.

For file revision thumbnails, there's probably no need to have those be adjustable unless we're talking about vector images (even then, use cases here are limited). But I'd like to retain the upscaling ability we already have for files, and possibly even extend that to work for thumbs as well (with a max size limit).

Another possible option is to honor the specified size for how much space the element takes up but do not scale up the image, ie. use something like object-fit: none to keep the image smaller than the img element. (For the File::transform patch that's probably the same as allowing stretching; it is only concerned with passing the file URL and the size to the client.)

Another possible option is to honor the specified size for how much space the element takes up but do not scale up the image, ie. use something like object-fit: none to keep the image smaller than the img element. (For the File::transform patch that's probably the same as allowing stretching; it is only concerned with passing the file URL and the size to the client.)

I'm not sure if that addresses the aesthetic issues involved though. A box of a certain size with a tiny image in the middle is still something that looks odd/wrong.

Change 375393 abandoned by Matthias Mullie:
Don’t pass bigger-than-source-image dimensions to ThumbnailImage

https://gerrit.wikimedia.org/r/375393