Page MenuHomePhabricator

Some custom generated thumbnails get massivily cropped
Closed, ResolvedPublic

Description

The issue was highlighted by a user reporting a broken panorama view. See the original report in German. At the end it turned out, that thumbs.php massively crops images while resizing it as preparation for the panorama tool.

See the following examples:
Original: https://commons.wikimedia.org/wiki/File:Stendal-Marktplatz-mit-Rathaus-und-St-Marien-IMG_5262-11x3B-360Gx180G-PanoS-19-05-2023.jpg
w3900: https://commons.wikimedia.org/w/thumb.php?w=3900&f=Stendal-Marktplatz-mit-Rathaus-und-St-Marien-IMG_5262-11x3B-360Gx180G-PanoS-19-05-2023.jpg (fine)
w4000: https://commons.wikimedia.org/w/thumb.php?w=4000&f=Stendal-Marktplatz-mit-Rathaus-und-St-Marien-IMG_5262-11x3B-360Gx180G-PanoS-19-05-2023.jpg (weirdly cropped)

It does not seem to happen with a different version of that image ( that already got a bit cropped ):
Original: https://commons.wikimedia.org/wiki/File:Stendal-Marktplatz-mit-Rathaus-und-St-Marien-IMG_5262-11x3B-eTHI3-GBE-360Gx180G-PanoS-wbk-19-05-2023.jpg
w3900: https://commons.wikimedia.org/w/thumb.php?w=3900&f=Stendal-Marktplatz-mit-Rathaus-und-St-Marien-IMG_5262-11x3B-eTHI3-GBE-360Gx180G-PanoS-wbk-19-05-2023.jpg (fine)
w4000: https://commons.wikimedia.org/w/thumb.php?w=4000&f=Stendal-Marktplatz-mit-Rathaus-und-St-Marien-IMG_5262-11x3B-eTHI3-GBE-360Gx180G-PanoS-wbk-19-05-2023.jpg (fine)

When I import the former version of the file to my local dev wiki and generate a thumb with the same parameters there's no issue. Could currently not upload it to the beta cluster to test it there.

Not sure if related to T282385: Certain PNG thumbnail sizes get cropped (one or two pixels missing)

Event Timeline

This could be related to the latest migration. The user that reported the broken panoramic views says, that this happened around May 2023.

Can also be that the metadata of the specific original is broken in one or more ways, and newer libraries interpret it different than the old ones. We see this all the time.

Can also be that the metadata of the specific original is broken in one or more ways, and newer libraries interpret it different than the old ones. We see this all the time.

Can you give me a link to one of these cases, or is there a ticket related to that?

I also wondered where I could best play with the production's Thumbor setup to test with different input and narrow down the issue.

I also wondered where I could best play with the production's Thumbor setup to test with different input and narrow down the issue.

Okay that was easy. Reproducible with the default docker setup and the config example in https://gerrit.wikimedia.org/g/operations/software/thumbor-plugins

Findings so far:

  • I can reproduce the issue with any jpeg that size it seems.
    • e.g. I have a jpeg with the 14532x7266px and try to shrink it to 4000px width it will get cropped [1]
  • More shrinking will lead to a working result
    • e.g. I have a jpeg with the 14532x7266px and try to shrink it to 3000px width it will not get cropped [2]
  • Smaller images can be shrinked more
    • e.g. I have a jpeg with the 14000x7000px and try to shrink it to 4000px width it will not get cropped [3]
  • Metadata or contents of the jpeg do not matter. I just created an almost white jpeg with that size.

[1] http://localhost:8800/thumbor/unsafe/4000x/14532x7266px.jpg
[2] http://localhost:8800/thumbor/unsafe/3000x/14532x7266px.jpg
[3] http://localhost:8800/thumbor/unsafe/4000x/14000x7000px.jpg

WMDE-Fisch added a project: Regression.
thumbor-plugins-thumbor-1  | 2023-09-28 23:57:28 thumbor:DEBUG [ShellRunner] Command: ['/usr/bin/timeout', '--foreground', '60', '/usr/bin/convert', '-define', 'tiff:exif-properties=no', '-define', 'jpeg:size=8000x4000', '-resize', '4000x2000^', '-gravity', 'center', '-extent', '4000x2000', '-quality', '79', '-sampling-factor', '4:2:0', '-interlace', 'Plane', '/tmp/tmp692tp6k6[0]', 'jpg:-']
thumbor-plugins-thumbor-1  | 2023-09-28 23:57:30 thumbor:DEBUG [ShellRunner] Stdout: <too long to display (1934930 bytes)>
thumbor-plugins-thumbor-1  | 2023-09-28 23:57:30 thumbor:DEBUG [ShellRunner] Stderr: b"convert: cache resources exhausted `/tmp/tmp692tp6k6' @ error/cache.c/OpenPixelCache/4083.\n"
thumbor-plugins-thumbor-1  | 2023-09-28 23:57:30 thumbor:DEBUG [ShellRunner] Return code: 1

I think the resize operator fails, but the extent operator continues anyway using the source image as its input. The extent operator either crops or fills with a background colour. The overall exit status is 1, but Thumbor uses the result anyway:

if returncode != 0:
    # T179200 ImageMagick may return a non-zero exit code while having
    # actually rendered a thumbnail of a semi-broken file
    if self.is_valid_thumbnail(result):
        self.debug('[IM] errored but still rendered: %s' % stderr)
    else:
        # Haven't been able to find a test file that meets this criteria
        ShellRunner.rm_f(self.image.name)  # pragma: no cover
        raise ImageMagickException('Failed to convert image %s' % stderr)  # pragma: no cover

The referenced bug was fixed another way, so this workaround may be doing more harm than good.

# The ^ + gravity + extent trick is necessary to ensure that we get a thumbnail
# of exactly the width we've requested. In some edge cases a tiny fraction
# of the image might be cropped out. This is unavoidable with ImageMagick
# See http://www.imagemagick.org/Usage/resize/ for details

MW core fixes this problem by appending an exclamation mark to the size. Refer convert geometry:

We see that ImageMagick is very good about preserving aspect ratios of images, to prevent distortion of your favorite photos and images. But you might really want the dimensions to be 100x200, thereby stretching the image. In this case just tell ImageMagick you really mean it (!) by appending an exclamation operator to the geometry. This will force the image size to exactly what you specify. So, for example, if you specify 100x200! the dimensions will become exactly 100x200 (giving a small, vertically elongated wizard).

We are only talking about fixing a 1px rounding error. I think it would be safer, faster, and likely generate a better visual result if we followed MW core and used ! instead of extent.

As for the actual failure, with convert -debug 'Cache,Resource' we can see it exceeding the disk limit of 1 GB:

2023-09-29T00:51:41+00:00 0:00.010 0.010u 6.9.10 Resource convert-im6.q16[20]: resource.c/AcquireMagickResource/395/Resource
  Disk: 805.584MiB/805.584MiB/1GiB
...
2023-09-29T00:51:42+00:00 0:01.000 0.980u 6.9.10 Resource convert-im6.q16[20]: resource.c/AcquireMagickResource/395/Resource
  Disk: 232512000B/805.584MiB/1GiB

A really incredible amount of scratch space for a 106 Mpx input image. It's apparently trying to allocate a source-sized 16bpp RGBA buffer and a buffer with the source width and the destination height (14532x2000). Back in the day I compiled Q8 binaries so that it would use half the memory. I can understand not wanting to bother with that anymore. Maybe now we can just double all the limits instead.

$ convert -list resource
Resource limits:
  Width: 16KP
  Height: 16KP
  List length: 18.446744EP
  Area: 128MP
  Memory: 256MiB
  Map: 512MiB
  Disk: 1GiB
  File: 786432
  Thread: 12
  Throttle: 0
  Time: unlimited

A 256MB memory limit seems stingy when the container has a 2GB limit. Thumbor seems to run without the -processes command line option, implying a maximum of one convert process per container. Maybe we can raise that to 1.7GB, and raise the disk limit to 5GB.

Change 961968 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[operations/software/thumbor-plugins@master] Don't ignore imagemagick exit status

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

The limit comes from the Debian package. Prior to the k8s transition, we used puppet imagemagick::install which installed a policy.xml with all the resource limits commented out.

Change 961968 merged by jenkins-bot:

[operations/software/thumbor-plugins@master] Don't ignore imagemagick exit status

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

Change 963690 had a related patch set uploaded (by Hnowlan; author: Hnowlan):

[operations/deployment-charts@master] thumbor: bump version

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

Change 963690 merged by jenkins-bot:

[operations/deployment-charts@master] thumbor: bump version

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

Change 964889 had a related patch set uploaded (by Hnowlan; author: Hnowlan):

[operations/deployment-charts@master] thumbor: bump version

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

Change 964889 merged by jenkins-bot:

[operations/deployment-charts@master] thumbor: bump version

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

Removing limits and fixing the invalid error handling appears to be rendering these images correctly

Thanks so much. Works fine for the use cases in question. I'll mark this resolved.