From d5a96ee0ff240968124a0416e502c2fb54e4c408 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Tue, 19 Jan 2016 22:05:17 +0000 Subject: [PATCH 1/2] Preserve EXIF orientation data for profile images. When creating profile images from an uploaded file, ensure that EXIF orientation information is preserved, so profile images appear right-side-up. https://openedx.atlassian.net/browse/MA-1559 --- .../djangoapps/profile_images/exceptions.py | 15 ++ .../core/djangoapps/profile_images/images.py | 189 ++++++++++++------ .../profile_images/tests/helpers.py | 9 +- .../profile_images/tests/test_images.py | 71 +++++-- .../core/djangoapps/profile_images/views.py | 3 +- requirements/edx/base.txt | 1 + 6 files changed, 202 insertions(+), 86 deletions(-) create mode 100644 openedx/core/djangoapps/profile_images/exceptions.py diff --git a/openedx/core/djangoapps/profile_images/exceptions.py b/openedx/core/djangoapps/profile_images/exceptions.py new file mode 100644 index 0000000000..ca63fced8b --- /dev/null +++ b/openedx/core/djangoapps/profile_images/exceptions.py @@ -0,0 +1,15 @@ +""" +Exceptions related to the handling of profile images. +""" + + +class ImageValidationError(Exception): + """ + Exception to use when the system rejects a user-supplied source image. + """ + @property + def user_message(self): + """ + Translate the developer-facing exception message for API clients. + """ + return self.message diff --git a/openedx/core/djangoapps/profile_images/images.py b/openedx/core/djangoapps/profile_images/images.py index a60b138056..190ea9c318 100644 --- a/openedx/core/djangoapps/profile_images/images.py +++ b/openedx/core/djangoapps/profile_images/images.py @@ -3,13 +3,16 @@ Image file manipulation functions related to profile images. """ from cStringIO import StringIO from collections import namedtuple +from contextlib import closing from django.conf import settings from django.core.files.base import ContentFile from django.utils.translation import ugettext as _ +import piexif from PIL import Image from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_storage +from .exceptions import ImageValidationError ImageType = namedtuple('ImageType', ('extensions', 'mimetypes', 'magic')) @@ -33,42 +36,47 @@ IMAGE_TYPES = { } -def user_friendly_size(size): +def create_profile_images(image_file, profile_image_names): """ - Convert size in bytes to user friendly size. + Generates a set of image files based on image_file and stores them + according to the sizes and filenames specified in `profile_image_names`. Arguments: - size (int): size in bytes + + image_file (file): + The uploaded image file to be cropped and scaled to use as a + profile image. The image is cropped to the largest possible square, + and centered on this image. + + profile_image_names (dict): + A dictionary that maps image sizes to file names. The image size + is an integer representing one side of the equilateral image to be + created. Returns: - user friendly size + + None """ - units = [_('bytes'), _('KB'), _('MB')] - i = 0 - while size >= 1024: - size /= 1024 - i += 1 - return u'{} {}'.format(size, units[i]) + storage = get_profile_image_storage() + + original = Image.open(image_file) + image = _set_color_mode_to_rgb(original) + image = _crop_image_to_square(image) + + for size, name in profile_image_names.items(): + scaled = _scale_image(image, size) + exif = _get_corrected_exif(scaled, original) + with closing(_create_image_file(scaled, exif)) as scaled_image_file: + storage.save(name, scaled_image_file) -def get_valid_file_types(): +def remove_profile_images(profile_image_names): """ - Return comma separated string of valid file types. + Physically remove the image files specified in `profile_image_names` """ - return ', '.join([', '.join(IMAGE_TYPES[ft].extensions) for ft in IMAGE_TYPES.keys()]) - - -class ImageValidationError(Exception): - """ - Exception to use when the system rejects a user-supplied source image. - """ - @property - def user_message(self): - """ - Translate the developer-facing exception message for API clients. - """ - # pylint: disable=translation-of-non-string - return _(self.message) + storage = get_profile_image_storage() + for name in profile_image_names.values(): + storage.delete(name) def validate_uploaded_image(uploaded_file): @@ -86,14 +94,14 @@ def validate_uploaded_image(uploaded_file): file_upload_too_large = _( u'The file must be smaller than {image_max_size} in size.' ).format( - image_max_size=user_friendly_size(settings.PROFILE_IMAGE_MAX_BYTES) + image_max_size=_user_friendly_size(settings.PROFILE_IMAGE_MAX_BYTES) ) raise ImageValidationError(file_upload_too_large) elif uploaded_file.size < settings.PROFILE_IMAGE_MIN_BYTES: file_upload_too_small = _( u'The file must be at least {image_min_size} in size.' ).format( - image_min_size=user_friendly_size(settings.PROFILE_IMAGE_MIN_BYTES) + image_min_size=_user_friendly_size(settings.PROFILE_IMAGE_MIN_BYTES) ) raise ImageValidationError(file_upload_too_small) @@ -103,7 +111,7 @@ def validate_uploaded_image(uploaded_file): if not filetype: file_upload_bad_type = _( u'The file must be one of the following types: {valid_file_types}.' - ).format(valid_file_types=get_valid_file_types()) + ).format(valid_file_types=_get_valid_file_types()) raise ImageValidationError(file_upload_bad_type) filetype = filetype[0] @@ -127,51 +135,106 @@ def validate_uploaded_image(uploaded_file): uploaded_file.seek(0) -def _get_scaled_image_file(image_obj, size): +def _crop_image_to_square(image): """ - Given a PIL.Image object, get a resized copy using `size` (square) and - return a file-like object containing the data saved as a JPEG. + Given a PIL.Image object, return a copy cropped to a square around the + center point with each side set to the size of the smaller dimension. + """ + width, height = image.size + if width != height: + side = width if width < height else height + left = (width - side) // 2 + top = (height - side) // 2 + right = (width + side) // 2 + bottom = (height + side) // 2 + image = image.crop((left, top, right, bottom)) + return image - Note that the file object returned is a django ContentFile which holds - data in memory (not on disk). + +def _set_color_mode_to_rgb(image): + """ + Given a PIL.Image object, return a copy with the color mode set to RGB. + """ + return image.convert('RGB') + + +def _scale_image(image, side_length): + """ + Given a PIL.Image object, get a resized copy with each side being + `side_length` pixels long. The scaled image will always be square. + """ + return image.resize((side_length, side_length), Image.ANTIALIAS) + + +def _create_image_file(image, exif): + """ + Given a PIL.Image object, create and return a file-like object containing + the data saved as a JPEG. + + Note that the file object returned is a django ContentFile which holds data + in memory (not on disk). """ - if image_obj.mode != "RGB": - image_obj = image_obj.convert("RGB") - scaled = image_obj.resize((size, size), Image.ANTIALIAS) string_io = StringIO() - scaled.save(string_io, format='JPEG') + + # The if/else dance below is required, because PIL raises an exception if + # you pass None as the value of the exif kwarg. + if exif is None: + image.save(string_io, format='JPEG') + else: + image.save(string_io, format='JPEG', exif=exif) + image_file = ContentFile(string_io.getvalue()) return image_file -def create_profile_images(image_file, profile_image_names): +def _get_corrected_exif(image, original): """ - Generates a set of image files based on image_file and - stores them according to the sizes and filenames specified - in `profile_image_names`. + If the original image contains exif data, use that data to + preserve image orientation in the new image. """ - image_obj = Image.open(image_file) - - # first center-crop the image if needed (but no scaling yet). - width, height = image_obj.size - if width != height: - side = width if width < height else height - image_obj = image_obj.crop(((width - side) / 2, (height - side) / 2, (width + side) / 2, (height + side) / 2)) - - storage = get_profile_image_storage() - for size, name in profile_image_names.items(): - scaled_image_file = _get_scaled_image_file(image_obj, size) - # Store the file. - try: - storage.save(name, scaled_image_file) - finally: - scaled_image_file.close() + if 'exif' in original.info: + image_exif = image.info.get('exif', piexif.dump({})) + original_exif = original.info['exif'] + image_exif = _update_exif_orientation(image_exif, _get_exif_orientation(original_exif)) + return image_exif -def remove_profile_images(profile_image_names): +def _update_exif_orientation(exif, orientation): """ - Physically remove the image files specified in `profile_image_names` + Given an exif value and an integer value 1-8, reflecting a valid value for + the exif orientation, return a new exif with the orientation set. """ - storage = get_profile_image_storage() - for name in profile_image_names.values(): - storage.delete(name) + exif_dict = piexif.load(exif) + exif_dict['0th'][piexif.ImageIFD.Orientation] = orientation + return piexif.dump(exif_dict) + + +def _get_exif_orientation(exif): + """Return the orientation value for the given Image object""" + exif_dict = piexif.load(exif) + return exif_dict['0th'].get(piexif.ImageIFD.Orientation) + + +def _get_valid_file_types(): + """ + Return comma separated string of valid file types. + """ + return ', '.join([', '.join(IMAGE_TYPES[ft].extensions) for ft in IMAGE_TYPES.keys()]) + + +def _user_friendly_size(size): + """ + Convert size in bytes to user friendly size. + + Arguments: + size (int): size in bytes + + Returns: + user friendly size + """ + units = [_('bytes'), _('KB'), _('MB')] + i = 0 + while size >= 1024 and i < len(units): + size /= 1024 + i += 1 + return u'{} {}'.format(size, units[i]) diff --git a/openedx/core/djangoapps/profile_images/tests/helpers.py b/openedx/core/djangoapps/profile_images/tests/helpers.py index e1bf2cf834..42578f1bf2 100644 --- a/openedx/core/djangoapps/profile_images/tests/helpers.py +++ b/openedx/core/djangoapps/profile_images/tests/helpers.py @@ -7,10 +7,11 @@ from tempfile import NamedTemporaryFile from django.core.files.uploadedfile import UploadedFile from PIL import Image +import piexif @contextmanager -def make_image_file(dimensions=(320, 240), extension=".jpeg", force_size=None): +def make_image_file(dimensions=(320, 240), extension=".jpeg", force_size=None, orientation=None): """ Yields a named temporary file created with the specified image type and options. @@ -24,7 +25,11 @@ def make_image_file(dimensions=(320, 240), extension=".jpeg", force_size=None): image = Image.new('RGB', dimensions, "green") image_file = NamedTemporaryFile(suffix=extension) try: - image.save(image_file) + if orientation and orientation in xrange(1, 9): + exif_bytes = piexif.dump({'0th': {piexif.ImageIFD.Orientation: orientation}}) + image.save(image_file, exif=exif_bytes) + else: + image.save(image_file) if force_size is not None: image_file.seek(0, os.SEEK_END) bytes_to_pad = force_size - image_file.tell() diff --git a/openedx/core/djangoapps/profile_images/tests/test_images.py b/openedx/core/djangoapps/profile_images/tests/test_images.py index 52f877be77..34efb2b6df 100644 --- a/openedx/core/djangoapps/profile_images/tests/test_images.py +++ b/openedx/core/djangoapps/profile_images/tests/test_images.py @@ -15,12 +15,13 @@ import ddt import mock from PIL import Image +from ..exceptions import ImageValidationError from ..images import ( create_profile_images, - ImageValidationError, remove_profile_images, validate_uploaded_image, - get_valid_file_types, + _get_exif_orientation, + _get_valid_file_types, ) from .helpers import make_image_file, make_uploaded_file @@ -33,7 +34,7 @@ class TestValidateUploadedImage(TestCase): """ FILE_UPLOAD_BAD_TYPE = ( u'The file must be one of the following types: {valid_file_types}.'.format( - valid_file_types=get_valid_file_types() + valid_file_types=_get_valid_file_types() ) ) @@ -146,24 +147,54 @@ class TestGenerateProfileImages(TestCase): 100: "hundred.jpg", 1000: "thousand.jpg", } - mock_storage = mock.Mock() with make_uploaded_file(dimensions=dimensions, extension=extension, content_type=content_type) as uploaded_file: - with mock.patch( - "openedx.core.djangoapps.profile_images.images.get_profile_image_storage", - return_value=mock_storage, - ): - create_profile_images(uploaded_file, requested_sizes) - names_and_files = [v[0] for v in mock_storage.save.call_args_list] - actual_sizes = {} - for name, file_ in names_and_files: - # get the size of the image file and ensure it's square jpeg - with closing(Image.open(file_)) as image_obj: - width, height = image_obj.size - self.assertEqual(width, height) - self.assertEqual(image_obj.format, 'JPEG') - actual_sizes[width] = name - self.assertEqual(requested_sizes, actual_sizes) - mock_storage.save.reset_mock() + names_and_files = self._create_mocked_profile_images(uploaded_file, requested_sizes) + actual_sizes = {} + self.assertEqual(len(names_and_files), 3) + for name, file_ in names_and_files: + # get the size of the image file and ensure it's square jpeg + with closing(Image.open(file_)) as image_obj: + width, height = image_obj.size + self.assertEqual(width, height) + self.assertEqual(image_obj.format, 'JPEG') + actual_sizes[width] = name + self.assertEqual(requested_sizes, actual_sizes) + + def test_jpeg_with_exif_orientation(self): + requested_images = {10: "ten.jpg"} + rotate_90_clockwise = 8 # Value used in EXIF Orientation field. + with make_image_file(orientation=rotate_90_clockwise, extension='.jpg') as imfile: + names_and_files = self._create_mocked_profile_images(imfile, requested_images) + self.assertEqual(len(names_and_files), 1) + for file_ in [nf[1] for nf in names_and_files]: + with closing(Image.open(file_)) as image_obj: + self.assertEqual(image_obj.format, 'JPEG') + self.assertIn('exif', image_obj.info) + self.assertEqual(_get_exif_orientation(image_obj.info['exif']), rotate_90_clockwise) + + def test_jpeg_without_exif_orientation(self): + requested_images = {10: "ten.jpg"} + with make_image_file(extension='.jpg') as imfile: + names_and_files = self._create_mocked_profile_images(imfile, requested_images) + self.assertEqual(len(names_and_files), 1) + for file_ in [nf[1] for nf in names_and_files]: + with closing(Image.open(file_)) as image_obj: + self.assertEqual(image_obj.format, 'JPEG') + self.assertNotIn('exif', image_obj.info) + + def _create_mocked_profile_images(self, image_file, requested_images): + """ + Create image files with mocked-out storage. Return a list of tuples + consisting of the name and content of the file. + """ + mock_storage = mock.Mock() + with mock.patch( + "openedx.core.djangoapps.profile_images.images.get_profile_image_storage", + return_value=mock_storage, + ): + create_profile_images(image_file, requested_images) + names_and_files = [v[0] for v in mock_storage.save.call_args_list] + return names_and_files @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS') diff --git a/openedx/core/djangoapps/profile_images/views.py b/openedx/core/djangoapps/profile_images/views.py index 5fa4685631..7d967254a4 100644 --- a/openedx/core/djangoapps/profile_images/views.py +++ b/openedx/core/djangoapps/profile_images/views.py @@ -22,8 +22,9 @@ from openedx.core.lib.api.parsers import TypedFileUploadParser from openedx.core.lib.api.permissions import IsUserInUrl, IsUserInUrlOrStaff from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_names, set_has_profile_image +from .exceptions import ImageValidationError from .images import ( - IMAGE_TYPES, validate_uploaded_image, create_profile_images, remove_profile_images, ImageValidationError + IMAGE_TYPES, validate_uploaded_image, create_profile_images, remove_profile_images ) log = logging.getLogger(__name__) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index ff6bb9e8b0..2729b142bf 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -54,6 +54,7 @@ nose-xunitmp==0.3.2 oauthlib==0.7.2 paramiko==1.9.0 path.py==7.2 +piexif==1.0.2 Pillow==2.7.0 polib==1.0.3 pycrypto>=2.6 From 2b6ae74885f3929fb802a6142c07c4ac536c4971 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Mon, 25 Jan 2016 19:22:58 +0000 Subject: [PATCH 2/2] fixup: dedupe test code. --- .../core/djangoapps/profile_images/images.py | 5 +- .../profile_images/tests/helpers.py | 2 +- .../profile_images/tests/test_images.py | 60 +++++++++++-------- 3 files changed, 39 insertions(+), 28 deletions(-) diff --git a/openedx/core/djangoapps/profile_images/images.py b/openedx/core/djangoapps/profile_images/images.py index 190ea9c318..5d0c7bdd0a 100644 --- a/openedx/core/djangoapps/profile_images/images.py +++ b/openedx/core/djangoapps/profile_images/images.py @@ -210,7 +210,10 @@ def _update_exif_orientation(exif, orientation): def _get_exif_orientation(exif): - """Return the orientation value for the given Image object""" + """ + Return the orientation value for the given Image object, or None if the + value is not set. + """ exif_dict = piexif.load(exif) return exif_dict['0th'].get(piexif.ImageIFD.Orientation) diff --git a/openedx/core/djangoapps/profile_images/tests/helpers.py b/openedx/core/djangoapps/profile_images/tests/helpers.py index 42578f1bf2..d3836017c8 100644 --- a/openedx/core/djangoapps/profile_images/tests/helpers.py +++ b/openedx/core/djangoapps/profile_images/tests/helpers.py @@ -6,8 +6,8 @@ import os from tempfile import NamedTemporaryFile from django.core.files.uploadedfile import UploadedFile -from PIL import Image import piexif +from PIL import Image @contextmanager diff --git a/openedx/core/djangoapps/profile_images/tests/test_images.py b/openedx/core/djangoapps/profile_images/tests/test_images.py index 34efb2b6df..253394b40b 100644 --- a/openedx/core/djangoapps/profile_images/tests/test_images.py +++ b/openedx/core/djangoapps/profile_images/tests/test_images.py @@ -13,6 +13,7 @@ from django.test import TestCase from django.test.utils import override_settings import ddt import mock +import piexif from PIL import Image from ..exceptions import ImageValidationError @@ -127,6 +128,18 @@ class TestGenerateProfileImages(TestCase): """ Test create_profile_images """ + + def check_exif_orientation(self, image, expected_orientation): + """ + Check that the created object is a JPEG and that it has the expected + """ + self.assertEqual(image.format, 'JPEG') + if expected_orientation is not None: + self.assertIn('exif', image.info) + self.assertEqual(_get_exif_orientation(image.info['exif']), expected_orientation) + else: + self.assertIsNone(_get_exif_orientation(image.info.get('exif', piexif.dump({})))) + @ddt.data( *product( ["gif", "jpg", "png"], @@ -148,44 +161,36 @@ class TestGenerateProfileImages(TestCase): 1000: "thousand.jpg", } with make_uploaded_file(dimensions=dimensions, extension=extension, content_type=content_type) as uploaded_file: - names_and_files = self._create_mocked_profile_images(uploaded_file, requested_sizes) + names_and_images = self._create_mocked_profile_images(uploaded_file, requested_sizes) actual_sizes = {} - self.assertEqual(len(names_and_files), 3) - for name, file_ in names_and_files: + for name, image_obj in names_and_images: # get the size of the image file and ensure it's square jpeg - with closing(Image.open(file_)) as image_obj: - width, height = image_obj.size - self.assertEqual(width, height) - self.assertEqual(image_obj.format, 'JPEG') - actual_sizes[width] = name + width, height = image_obj.size + self.assertEqual(width, height) + actual_sizes[width] = name self.assertEqual(requested_sizes, actual_sizes) def test_jpeg_with_exif_orientation(self): - requested_images = {10: "ten.jpg"} + requested_images = {10: "ten.jpg", 100: "hunnert.jpg"} rotate_90_clockwise = 8 # Value used in EXIF Orientation field. with make_image_file(orientation=rotate_90_clockwise, extension='.jpg') as imfile: - names_and_files = self._create_mocked_profile_images(imfile, requested_images) - self.assertEqual(len(names_and_files), 1) - for file_ in [nf[1] for nf in names_and_files]: - with closing(Image.open(file_)) as image_obj: - self.assertEqual(image_obj.format, 'JPEG') - self.assertIn('exif', image_obj.info) - self.assertEqual(_get_exif_orientation(image_obj.info['exif']), rotate_90_clockwise) + for _, image in self._create_mocked_profile_images(imfile, requested_images): + self.check_exif_orientation(image, rotate_90_clockwise) def test_jpeg_without_exif_orientation(self): - requested_images = {10: "ten.jpg"} + requested_images = {10: "ten.jpg", 100: "hunnert.jpg"} with make_image_file(extension='.jpg') as imfile: - names_and_files = self._create_mocked_profile_images(imfile, requested_images) - self.assertEqual(len(names_and_files), 1) - for file_ in [nf[1] for nf in names_and_files]: - with closing(Image.open(file_)) as image_obj: - self.assertEqual(image_obj.format, 'JPEG') - self.assertNotIn('exif', image_obj.info) + for _, image in self._create_mocked_profile_images(imfile, requested_images): + self.check_exif_orientation(image, None) def _create_mocked_profile_images(self, image_file, requested_images): """ - Create image files with mocked-out storage. Return a list of tuples - consisting of the name and content of the file. + Create image files with mocked-out storage. + + Verifies that an image was created for each element in + requested_images, and returns an iterator of 2-tuples representing + those imageswhere each tuple consists of a filename and a PIL.Image + object. """ mock_storage = mock.Mock() with mock.patch( @@ -194,7 +199,10 @@ class TestGenerateProfileImages(TestCase): ): create_profile_images(image_file, requested_images) names_and_files = [v[0] for v in mock_storage.save.call_args_list] - return names_and_files + self.assertEqual(len(names_and_files), len(requested_images)) + for name, file_ in names_and_files: + with closing(Image.open(file_)) as image: + yield name, image @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')