Preserve XMP metadata when resizing images, 16bit and 32bit resize support, multithreaded processing#1830
Conversation
|
Also adds support for resizing multispectral datasets (16bit, 32bit) as it looks like support for these was recently added in Pillow. |
There was a problem hiding this comment.
Pull request overview
Preserves EXIF/XMP metadata when resizing uploaded images (including higher bit-depth TIFFs) to address “Resize does not preserve XMP metadata” (#1828).
Changes:
- Upgrades Pillow and adjusts image resizing logic to preserve XMP/EXIF, using
exiftoolas a fallback. - Expands which image extensions are considered for resizing.
- Adds/extends API task tests to validate EXIF/XMP preservation (including a new multispectral TIFF fixture) and installs
exiftoolin the Docker image.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Updates Pillow version to enable improved metadata/bit-depth support. |
| package.json | Bumps app version. |
| app/tests/test_api_task.py | Adds assertions verifying EXIF/XMP preservation after resize (uses exiftool). |
| app/models/task.py | Reworks resize_image to preserve XMP/EXIF and adds exiftool fallback; expands resize file matching to include PNG. |
| app/fixtures/tiny_drone_image_multispec.tif | Adds a TIFF fixture containing metadata for regression coverage. |
| Dockerfile | Installs exiftool to support metadata-copy fallback and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/tests/test_api_task.py
Outdated
|
|
||
| # Exiftool agrees | ||
| resized_exif_dump = extract_exif(resized_task.task_path("tiny_drone_image.jpg")) | ||
| self.assertTrue('''GPS Latitude : 41 deg 13' 34.93" N''' in img1_exif_dump) |
There was a problem hiding this comment.
The EXIF assertion is checking img1_exif_dump instead of the resized image’s dump (resized_exif_dump), so this test can pass even if metadata isn’t preserved on the resized file. Assert against resized_exif_dump here.
| self.assertTrue('''GPS Latitude : 41 deg 13' 34.93" N''' in img1_exif_dump) | |
| self.assertTrue('''GPS Latitude : 41 deg 13' 34.93" N''' in resized_exif_dump) |
app/models/task.py
Outdated
| if xmp is not None and exif is not None: | ||
| params['xmp'] = xmp | ||
| params['exif'] = exif | ||
| else: | ||
| # This is either a TIFF or an image for which | ||
| # PIL's simple EXIF/XMP handling code is insufficient | ||
| exiftool = shutil.which('exiftool') | ||
| if not exiftool: | ||
| raise Exception("Exiftool missing, but needed") |
There was a problem hiding this comment.
resize_image only uses Pillow to write metadata when BOTH XMP and EXIF are present; if either is missing (common for many JPEGs), the code requires exiftool and will fail resizing when it’s not installed. Consider preserving whichever metadata is available via Pillow (set params['exif'] and/or params['xmp'] independently) and only falling back to exiftool for formats/metadata cases Pillow can’t handle.
app/models/task.py
Outdated
| max_side = max(width, height) | ||
| if max_side < resize_to: | ||
| logger.warning('You asked to make {} bigger ({} --> {}), but we are not going to do that.'.format(image_path, max_side, resize_to)) | ||
| return {'path': image_path, 'resize_ratio': 1} |
There was a problem hiding this comment.
The early return when max_side < resize_to bypasses the done callback, which is used by Task.resize_images() to advance progress and cancellation checks. Call the callback before returning (with a 1.0 ratio result) to keep progress/cancel behavior consistent.
| return {'path': image_path, 'resize_ratio': 1} | |
| retval = {'path': image_path, 'resize_ratio': 1} | |
| if done is not None: | |
| done(retval) | |
| return retval |
| except Exception as e: | ||
| logger.warning("Cannot resize {}: {}.".format(image_path, str(e))) |
There was a problem hiding this comment.
Catching a blanket Exception here will also swallow programming errors (e.g. NameError/TypeError) and make failures harder to diagnose. Prefer catching the specific Pillow/IO/subprocess exceptions you expect (and let unexpected exceptions surface) while still performing the same cleanup.
| plugin_signals.task_resizing_images.send_robust(sender=self.__class__, task_id=self.id) | ||
|
|
||
| images_path = self.find_all_files_matching(r'.*\.(jpe?g|tiff?)$') | ||
| images_path = self.find_all_files_matching(r'.*\.(jpe?g|tiff?|png)$') |
There was a problem hiding this comment.
resize_images() now includes PNGs in the resize set, but the method docstring/comment above still describes resizing JPGs and mentions lack of TIFF support. Update the docstring to match the actual supported extensions/behavior (and confirm PNG is intended).
| # Xmp and exif are preserved | ||
| self.assertTrue(im.info.get('xmp') is not None) | ||
| self.assertEqual(im.info.get('xmp'), img1_xmp) | ||
| self.assertTrue(im.info.get('exif') is not None) |
There was a problem hiding this comment.
assertTrue(a is not b) cannot provide an informative message. Using assertIsNot(a, b) instead will give more informative messages.
| self.assertTrue(im.info.get('exif') is not None) | |
| self.assertIsNotNone(im.info.get('exif')) |
app/tests/test_api_task.py
Outdated
|
|
||
| # Exiftool agrees | ||
| resized_exif_dump = extract_exif(resized_task.task_path("tiny_drone_image.jpg")) | ||
| self.assertTrue('''GPS Latitude : 41 deg 13' 34.93" N''' in img1_exif_dump) |
There was a problem hiding this comment.
assertTrue(a in b) cannot provide an informative message. Using assertIn(a, b) instead will give more informative messages.
| self.assertTrue('''GPS Latitude : 41 deg 13' 34.93" N''' in img1_exif_dump) | ||
|
|
||
| resized_xmp_dump = extract_xmp(resized_task.task_path("tiny_drone_image.jpg")) | ||
| self.assertTrue('<sensefly:CamID>11</sensefly:CamID>' in resized_xmp_dump) |
There was a problem hiding this comment.
assertTrue(a in b) cannot provide an informative message. Using assertIn(a, b) instead will give more informative messages.
| # EXIF/XMP bytes are preserved | ||
| ms_resized_exif = subprocess.run(['exiftool', resized_task.task_path("tiny_drone_image_multispec.tif")], | ||
| capture_output=True, text=True, check=True).stdout.strip() | ||
| self.assertTrue('''GPS Latitude : 50 deg 58' 53.02" N''' in ms_resized_exif) |
There was a problem hiding this comment.
assertTrue(a in b) cannot provide an informative message. Using assertIn(a, b) instead will give more informative messages.
|
|
||
| ms_resized_xmp = subprocess.run(['exiftool', '-xmp', '-b', resized_task.task_path("tiny_drone_image_multispec.tif")], | ||
| capture_output=True, text=True, check=True).stdout.strip() | ||
| self.assertTrue('MicaSense:CaptureId="CqaClXcmhKgplT0yPCLE"' in ms_resized_xmp) |
There was a problem hiding this comment.
assertTrue(a in b) cannot provide an informative message. Using assertIn(a, b) instead will give more informative messages.
Closes #1828