Fix T51678: 16bit greyscale PNG images thumbnailing fails

generate_local_thumbnails() now uses pathlib and f-string formatting too,
making the code a lot simpler. Furthermore, I removed unused bits of
resize_and_crop() and simplified the rest.

pillar/api/file_storage/__init__.py

@@ -25,12 +25,11 @@ from flask import url_for, helpers
 from pillar.api import utils
 from pillar.api.file_storage_backends.gcs import GoogleCloudStorageBucket, \
     GoogleCloudStorageBlob
-from pillar.api.utils import remove_private_keys
+from pillar.api.utils import remove_private_keys, imaging
 from pillar.api.utils.authorization import require_login, \
     user_matches_roles
 from pillar.api.utils.cdn import hash_file_path
 from pillar.api.utils.encoding import Encoder
-from pillar.api.utils.imaging import generate_local_thumbnails
 from pillar.api.file_storage_backends import default_storage_backend, Bucket
 from pillar.auth import current_user
 
@@ -97,8 +96,9 @@ def _process_image(bucket: Bucket,
 
     # Generate previews
     log.info('Generating thumbnails for file %s', file_id)
-    src_file['variations'] = generate_local_thumbnails(src_file['name'],
-                                                       local_file.name)
+    local_path = pathlib.Path(local_file.name)
+    name_base = pathlib.Path(src_file['name']).stem
+    src_file['variations'] = imaging.generate_local_thumbnails(name_base, local_path)
 
     # Send those previews to Google Cloud Storage.
     log.info('Uploading %i thumbnails for file %s to Google Cloud Storage '

pillar/api/utils/imaging.py

@@ -1,54 +1,61 @@
-import os
 import json
+import typing
+
+import os
+import pathlib
 import subprocess
+
 from PIL import Image
 from flask import current_app
 
+# Images with these modes will be thumbed to PNG, others to JPEG.
+MODES_FOR_PNG = {'RGBA', 'LA'}
 
-# TODO: refactor to use pathlib.Path and f-strings.
-def generate_local_thumbnails(name_base, src):
+
+def generate_local_thumbnails(fp_base: str, src: pathlib.Path):
     """Given a source image, use Pillow to generate thumbnails according to the
     application settings.
 
-    :param name_base: the thumbnail will get a field 'name': '{basename}-{thumbsize}.jpg'
-    :type name_base: str
+    :param fp_base: the thumbnail will get a field
+        'file_path': '{fp_base}-{thumbsize}.{ext}'
     :param src: the path of the image to be thumbnailed
-    :type src: str
     """
 
     thumbnail_settings = current_app.config['UPLOADS_LOCAL_STORAGE_THUMBNAILS']
     thumbnails = []
 
-    save_to_base, _ = os.path.splitext(src)
-    name_base, _ = os.path.splitext(name_base)
-
     for size, settings in thumbnail_settings.items():
-        dst = '{0}-{1}{2}'.format(save_to_base, size, '.jpg')
-        name = '{0}-{1}{2}'.format(name_base, size, '.jpg')
+        im = Image.open(src)
+        extra_args = {}
+
+        # If the source image has transparency, save as PNG
+        if im.mode in MODES_FOR_PNG:
+            suffix = '.png'
+            imformat = 'PNG'
+        else:
+            suffix = '.jpg'
+            imformat = 'JPEG'
+            extra_args = {'quality': 95}
+        dst = src.with_name(f'{src.stem}-{size}{suffix}')
 
         if settings['crop']:
-            resize_and_crop(src, dst, settings['size'])
-            width, height = settings['size']
+            im = resize_and_crop(im, settings['size'])
         else:
-            im = Image.open(src)
             im.thumbnail(settings['size'], resample=Image.LANCZOS)
+        width, height = im.size
 
-            # If the source image has transparency, save as PNG
-            if im.mode == 'RGBA':
-                im.save(dst, format='PNG', optimize=True)
-            else:
-                im.save(dst, format='JPEG', optimize=True, quality=95)
-
-            width, height = im.size
+        if imformat == 'JPEG':
+            im = im.convert('RGB')
+        im.save(dst, format=imformat, optimize=True, **extra_args)
 
         thumb_info = {'size': size,
-                      'file_path': name,
-                      'local_path': dst,
-                      'length': os.stat(dst).st_size,
+                      'file_path': f'{fp_base}-{size}{suffix}',
+                      'local_path': str(dst),
+                      'length': dst.stat().st_size,
                       'width': width,
                       'height': height,
                       'md5': '',
-                      'content_type': 'image/jpeg'}
+                      'content_type': f'image/{imformat.lower()}'}
 
         if size == 't':
             thumb_info['is_public'] = True
@@ -58,63 +65,40 @@ def generate_local_thumbnails(name_base, src):
     return thumbnails
 
 
-def resize_and_crop(img_path, modified_path, size, crop_type='middle'):
-    """
-    Resize and crop an image to fit the specified size. Thanks to:
-    https://gist.github.com/sigilioso/2957026
+def resize_and_crop(img: Image, size: typing.Tuple[int, int]) -> Image:
+    """Resize and crop an image to fit the specified size.
 
-    args:
-    img_path: path for the image to resize.
-    modified_path: path to store the modified image.
-    size: `(width, height)` tuple.
-    crop_type: can be 'top', 'middle' or 'bottom', depending on this
-    value, the image will cropped getting the 'top/left', 'middle' or
-    'bottom/right' of the image to fit the size.
-    raises:
-    Exception: if can not open the file in img_path of there is problems
-    to save the image.
-    ValueError: if an invalid `crop_type` is provided.
+    Thanks to: https://gist.github.com/sigilioso/2957026
 
+    :param img: opened PIL.Image to work on
+    :param size: `(width, height)` tuple.
     """
     # If height is higher we resize vertically, if not we resize horizontally
-    img = Image.open(img_path).convert('RGB')
     # Get current and desired ratio for the images
-    img_ratio = img.size[0] / float(img.size[1])
-    ratio = size[0] / float(size[1])
+    cur_w, cur_h = img.size  # current
+    img_ratio = cur_w / cur_h
+
+    w, h = size  # desired
+    ratio = w / h
+
     # The image is scaled/cropped vertically or horizontally depending on the ratio
     if ratio > img_ratio:
-        img = img.resize((size[0], int(round(size[0] * img.size[1] / img.size[0]))),
-                         Image.ANTIALIAS)
-        # Crop in the top, middle or bottom
-        if crop_type == 'top':
-            box = (0, 0, img.size[0], size[1])
-        elif crop_type == 'middle':
-            box = (0, int(round((img.size[1] - size[1]) / 2)), img.size[0],
-                   int(round((img.size[1] + size[1]) / 2)))
-        elif crop_type == 'bottom':
-            box = (0, img.size[1] - size[1], img.size[0], img.size[1])
-        else:
-            raise ValueError('ERROR: invalid value for crop_type')
+        uncropped_h = (w * cur_h) // cur_w
+        img = img.resize((w, uncropped_h), Image.ANTIALIAS)
+        box = (0, (uncropped_h - h) // 2,
+               w, (uncropped_h + h) // 2)
         img = img.crop(box)
     elif ratio < img_ratio:
-        img = img.resize((int(round(size[1] * img.size[0] / img.size[1])), size[1]),
-                         Image.ANTIALIAS)
-        # Crop in the top, middle or bottom
-        if crop_type == 'top':
-            box = (0, 0, size[0], img.size[1])
-        elif crop_type == 'middle':
-            box = (int(round((img.size[0] - size[0]) / 2)), 0,
-                   int(round((img.size[0] + size[0]) / 2)), img.size[1])
-        elif crop_type == 'bottom':
-            box = (img.size[0] - size[0], 0, img.size[0], img.size[1])
-        else:
-            raise ValueError('ERROR: invalid value for crop_type')
+        uncropped_w = (h * cur_w) // cur_h
+        img = img.resize((uncropped_w, h), Image.ANTIALIAS)
+        box = ((uncropped_w - w) // 2, 0,
+               (uncropped_w + w) // 2, h)
         img = img.crop(box)
     else:
-        img = img.resize((size[0], size[1]),
-                         Image.ANTIALIAS)
+        img = img.resize((w, h), Image.ANTIALIAS)
+
     # If the scale is the same, we do not need to crop
-    img.save(modified_path, "JPEG")
+    return img
 
 
 def get_video_data(filepath):

tests/test_api/images/README.txt

@@ -0,0 +1,2 @@
+Images courtesy of Blender Cloud
+https://cloud.blender.org/

tests/test_api/test_imaging.py

@@ -0,0 +1,292 @@
+import pathlib
+import shutil
+import tempfile
+
+from pillar.tests import AbstractPillarTest
+
+
+class ThumbnailTest(AbstractPillarTest):
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.image_path = pathlib.Path(__file__).with_name('images')
+
+    def setUp(self, **kwargs):
+        super().setUp(**kwargs)
+        self._tmp = tempfile.TemporaryDirectory()
+        self.tmp = pathlib.Path(self._tmp.name)
+
+    def tearDown(self):
+        super().tearDown()
+        self._tmp.cleanup()
+
+    def _tmpcopy(self, image_fname: str) -> pathlib.Path:
+        src = self.image_path / image_fname
+        dst = self.tmp / image_fname
+        shutil.copy(str(src), str(dst))
+        return dst
+
+    def _thumb_test(self, source):
+        from PIL import Image
+        from pillar.api.utils import imaging
+
+        with self.app.app_context():
+            # Almost same as in production, but less different sizes.
+            self.app.config['UPLOADS_LOCAL_STORAGE_THUMBNAILS'] = {
+                's': {'size': (90, 90), 'crop': True},
+                'b': {'size': (160, 160), 'crop': True},
+                't': {'size': (160, 160), 'crop': False},
+                'm': {'size': (320, 320), 'crop': False},
+            }
+
+            thumbs = imaging.generate_local_thumbnails('มัสมั่น', source)
+
+        # Remove the length field, it is can be hard to predict.
+        for t in thumbs:
+            t.pop('length')
+
+        # Verify that the images can be loaded and have the advertised size.
+        for t in thumbs:
+            local_path = pathlib.Path(t['local_path'])
+            im = Image.open(local_path)
+            self.assertEqual((t['width'], t['height']), im.size)
+
+        return thumbs
+
+    def test_thumbgen_jpg(self):
+        source = self._tmpcopy('512x512-8bit-rgb.jpg')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.jpg',
+                 'local_path': str(source.with_name('512x512-8bit-rgb-s.jpg')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.jpg',
+                 'local_path': str(source.with_name('512x512-8bit-rgb-b.jpg')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.jpg',
+                 'local_path': str(source.with_name('512x512-8bit-rgb-t.jpg')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/jpeg',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.jpg',
+                 'local_path': str(source.with_name('512x512-8bit-rgb-m.jpg')),
+                 'width': 320, 'height': 320,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+            ],
+            thumbs)
+
+    def test_thumbgen_vertical(self):
+        source = self._tmpcopy('300x512-8bit-rgb.jpg')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.jpg',
+                 'local_path': str(source.with_name('300x512-8bit-rgb-s.jpg')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.jpg',
+                 'local_path': str(source.with_name('300x512-8bit-rgb-b.jpg')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.jpg',
+                 'local_path': str(source.with_name('300x512-8bit-rgb-t.jpg')),
+                 'width': 93, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/jpeg',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.jpg',
+                 'local_path': str(source.with_name('300x512-8bit-rgb-m.jpg')),
+                 'width': 187, 'height': 320,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+            ],
+            thumbs)
+
+    def test_thumbgen_png_alpha(self):
+        source = self._tmpcopy('512x512-8bit-rgba.png')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.png',
+                 'local_path': str(source.with_name('512x512-8bit-rgba-s.png')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.png',
+                 'local_path': str(source.with_name('512x512-8bit-rgba-b.png')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.png',
+                 'local_path': str(source.with_name('512x512-8bit-rgba-t.png')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.png',
+                 'local_path': str(source.with_name('512x512-8bit-rgba-m.png')),
+                 'width': 320, 'height': 320,
+                 'md5': '',
+                 'content_type': 'image/png'},
+            ],
+            thumbs)
+
+    def test_thumbgen_png_greyscale_alpha(self):
+        source = self._tmpcopy('512x512-8bit-grey-alpha.png')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.png',
+                 'local_path': str(source.with_name('512x512-8bit-grey-alpha-s.png')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.png',
+                 'local_path': str(source.with_name('512x512-8bit-grey-alpha-b.png')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.png',
+                 'local_path': str(source.with_name('512x512-8bit-grey-alpha-t.png')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.png',
+                 'local_path': str(source.with_name('512x512-8bit-grey-alpha-m.png')),
+                 'width': 320, 'height': 320,
+                 'md5': '',
+                 'content_type': 'image/png'},
+            ],
+            thumbs)
+
+    def test_thumbgen_png_16bit(self):
+        source = self._tmpcopy('512x256-16bit-rgb.png')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.png',
+                 'local_path': str(source.with_name('512x256-16bit-rgb-s.png')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.png',
+                 'local_path': str(source.with_name('512x256-16bit-rgb-b.png')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.png',
+                 'local_path': str(source.with_name('512x256-16bit-rgb-t.png')),
+                 'width': 160, 'height': 80,
+                 'md5': '',
+                 'content_type': 'image/png',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.png',
+                 'local_path': str(source.with_name('512x256-16bit-rgb-m.png')),
+                 'width': 320, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png'},
+            ],
+            thumbs)
+
+    def test_thumbgen_png_16bit_grey(self):
+        source = self._tmpcopy('512x256-16bit-grey.png')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.jpg',
+                 'local_path': str(source.with_name('512x256-16bit-grey-s.jpg')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.jpg',
+                 'local_path': str(source.with_name('512x256-16bit-grey-b.jpg')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.jpg',
+                 'local_path': str(source.with_name('512x256-16bit-grey-t.jpg')),
+                 'width': 160, 'height': 80,
+                 'md5': '',
+                 'content_type': 'image/jpeg',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.jpg',
+                 'local_path': str(source.with_name('512x256-16bit-grey-m.jpg')),
+                 'width': 320, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/jpeg'},
+            ],
+            thumbs)
+
+    def test_thumbgen_png_16bit_greyscale_alpha(self):
+        source = self._tmpcopy('512x256-16bit-grey-alpha.png')
+        thumbs = self._thumb_test(source)
+
+        self.assertEqual(
+            [
+                {'size': 's',
+                 'file_path': 'มัสมั่น-s.png',
+                 'local_path': str(source.with_name('512x256-16bit-grey-alpha-s.png')),
+                 'width': 90, 'height': 90,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 'b',
+                 'file_path': 'มัสมั่น-b.png',
+                 'local_path': str(source.with_name('512x256-16bit-grey-alpha-b.png')),
+                 'width': 160, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png'},
+                {'size': 't',
+                 'file_path': 'มัสมั่น-t.png',
+                 'local_path': str(source.with_name('512x256-16bit-grey-alpha-t.png')),
+                 'width': 160, 'height': 80,
+                 'md5': '',
+                 'content_type': 'image/png',
+                 'is_public': True},
+                {'size': 'm',
+                 'file_path': 'มัสมั่น-m.png',
+                 'local_path': str(source.with_name('512x256-16bit-grey-alpha-m.png')),
+                 'width': 320, 'height': 160,
+                 'md5': '',
+                 'content_type': 'image/png'},
+            ],
+            thumbs)