diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index b6f11e0979..8b5ae95ea0 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -44,13 +44,17 @@ class StaticContent(object): return self.location.category == 'thumbnail' @staticmethod - def generate_thumbnail_name(original_name, dimensions=None): + def generate_thumbnail_name(original_name, dimensions=None, extension=None): """ - original_name: Name of the asset (typically its location.name) - dimensions: `None` or a tuple of (width, height) in pixels + - extension: `None` or desired filename extension of the thumbnail """ + if extension is None: + extension = XASSET_THUMBNAIL_TAIL_NAME + name_root, ext = os.path.splitext(original_name) - if not ext == XASSET_THUMBNAIL_TAIL_NAME: + if not ext == extension: name_root = name_root + ext.replace(u'.', u'-') if dimensions: @@ -59,7 +63,7 @@ class StaticContent(object): return u"{name_root}{extension}".format( name_root=name_root, - extension=XASSET_THUMBNAIL_TAIL_NAME, + extension=extension, ) @staticmethod @@ -330,9 +334,10 @@ class ContentStore(object): pixels. It defaults to None. """ thumbnail_content = None + is_svg = content.content_type == 'image/svg+xml' # use a naming convention to associate originals with the thumbnail thumbnail_name = StaticContent.generate_thumbnail_name( - content.location.name, dimensions=dimensions + content.location.name, dimensions=dimensions, extension='.svg' if is_svg else None ) thumbnail_file_location = StaticContent.compute_location( content.location.course_key, thumbnail_name, is_thumbnail=True @@ -340,28 +345,42 @@ class ContentStore(object): # if we're uploading an image, then let's generate a thumbnail so that we can # serve it up when needed without having to rescale on the fly - if content.content_type is not None and content.content_type.split('/')[0] == 'image': - try: + try: + if is_svg: + # for svg simply store the provided svg file, since vector graphics should be good enough + # for downscaling client-side + if tempfile_path is None: + thumbnail_file = StringIO.StringIO(content.data) + else: + with open(tempfile_path) as f: + thumbnail_file = StringIO.StringIO(f.read()) + thumbnail_content = StaticContent(thumbnail_file_location, thumbnail_name, + 'image/svg+xml', thumbnail_file) + self.save(thumbnail_content) + elif content.content_type is not None and content.content_type.split('/')[0] == 'image': # use PIL to do the thumbnail generation (http://www.pythonware.com/products/pil/) # My understanding is that PIL will maintain aspect ratios while restricting # the max-height/width to be whatever you pass in as 'size' # @todo: move the thumbnail size to a configuration setting?!? if tempfile_path is None: - im = Image.open(StringIO.StringIO(content.data)) + source = StringIO.StringIO(content.data) else: - im = Image.open(tempfile_path) + source = tempfile_path - # I've seen some exceptions from the PIL library when trying to save palletted - # PNG files to JPEG. Per the google-universe, they suggest converting to RGB first. - im = im.convert('RGB') - - if not dimensions: - dimensions = (128, 128) - - im.thumbnail(dimensions, Image.ANTIALIAS) + # We use the context manager here to avoid leaking the inner file descriptor + # of the Image object -- this way it gets closed after we're done with using it. thumbnail_file = StringIO.StringIO() - im.save(thumbnail_file, 'JPEG') - thumbnail_file.seek(0) + with Image.open(source) as image: + # I've seen some exceptions from the PIL library when trying to save palletted + # PNG files to JPEG. Per the google-universe, they suggest converting to RGB first. + thumbnail_image = image.convert('RGB') + + if not dimensions: + dimensions = (128, 128) + + thumbnail_image.thumbnail(dimensions, Image.ANTIALIAS) + thumbnail_image.save(thumbnail_file, 'JPEG') + thumbnail_file.seek(0) # store this thumbnail as any other piece of content thumbnail_content = StaticContent(thumbnail_file_location, thumbnail_name, @@ -369,9 +388,11 @@ class ContentStore(object): self.save(thumbnail_content) - except Exception, e: - # log and continue as thumbnails are generally considered as optional - logging.exception(u"Failed to generate thumbnail for {0}. Exception: {1}".format(content.location, str(e))) + except Exception, exc: # pylint: disable=broad-except + # log and continue as thumbnails are generally considered as optional + logging.exception( + u"Failed to generate thumbnail for {0}. Exception: {1}".format(content.location, str(exc)) + ) return thumbnail_content, thumbnail_file_location diff --git a/common/lib/xmodule/xmodule/tests/test_content.py b/common/lib/xmodule/xmodule/tests/test_content.py index 4d151e20e3..834be425e6 100644 --- a/common/lib/xmodule/xmodule/tests/test_content.py +++ b/common/lib/xmodule/xmodule/tests/test_content.py @@ -3,6 +3,7 @@ import os import unittest import ddt +from mock import Mock, patch from path import Path as path from xmodule.contentstore.content import StaticContent, StaticContentStream @@ -58,6 +59,7 @@ class Content(object): def __init__(self, location, content_type): self.location = location self.content_type = content_type + self.data = None class FakeGridFsItem(object): @@ -84,6 +86,17 @@ class FakeGridFsItem(object): return chunk +class MockImage(Mock): + """ + This class pretends to be PIL.Image for purposes of thumbnails testing. + """ + def __enter__(self): + return self + + def __exit__(self, *args): + self.close() + + @ddt.ddt class ContentTest(unittest.TestCase): def test_thumbnail_none(self): @@ -103,11 +116,43 @@ class ContentTest(unittest.TestCase): ) @ddt.unpack def test_generate_thumbnail_image(self, original_filename, thumbnail_filename): - contentStore = ContentStore() + content_store = ContentStore() content = Content(AssetLocation(u'mitX', u'800', u'ignore_run', u'asset', original_filename), None) - (thumbnail_content, thumbnail_file_location) = contentStore.generate_thumbnail(content) + (thumbnail_content, thumbnail_file_location) = content_store.generate_thumbnail(content) self.assertIsNone(thumbnail_content) - self.assertEqual(AssetLocation(u'mitX', u'800', u'ignore_run', u'thumbnail', thumbnail_filename), thumbnail_file_location) + self.assertEqual( + AssetLocation(u'mitX', u'800', u'ignore_run', u'thumbnail', thumbnail_filename), + thumbnail_file_location + ) + + @patch('xmodule.contentstore.content.Image') + def test_image_is_closed_when_generating_thumbnail(self, image_class_mock): + # We used to keep the Image's file descriptor open when generating a thumbnail. + # It should be closed after being used. + mock_image = MockImage() + image_class_mock.open.return_value = mock_image + + content_store = ContentStore() + content = Content(AssetLocation(u'mitX', u'800', u'ignore_run', u'asset', "monsters.jpg"), "image/jpeg") + content.data = 'mock data' + content_store.generate_thumbnail(content) + self.assertTrue(image_class_mock.open.called, "Image.open not called") + self.assertTrue(mock_image.close.called, "mock_image.close not called") + + def test_store_svg_as_thumbnail(self): + # We had a bug that caused generate_thumbnail to attempt to pass SVG to PIL to generate a thumbnail. + # SVG files should be stored in original form for thumbnail purposes. + content_store = ContentStore() + content_store.save = Mock() + thumbnail_filename = u'test.svg' + content = Content(AssetLocation(u'mitX', u'800', u'ignore_run', u'asset', u'test.svg'), 'image/svg+xml') + content.data = 'mock svg file' + (thumbnail_content, thumbnail_file_location) = content_store.generate_thumbnail(content) + self.assertEqual(thumbnail_content.data.read(), b'mock svg file') + self.assertEqual( + AssetLocation(u'mitX', u'800', u'ignore_run', u'thumbnail', thumbnail_filename), + thumbnail_file_location + ) def test_compute_location(self): # We had a bug that __ got converted into a single _. Make sure that substitution of INVALID_CHARS (like space) @@ -115,7 +160,10 @@ class ContentTest(unittest.TestCase): asset_location = StaticContent.compute_location( SlashSeparatedCourseKey('mitX', '400', 'ignore'), 'subs__1eo_jXvZnE .srt.sjson' ) - self.assertEqual(AssetLocation(u'mitX', u'400', u'ignore', u'asset', u'subs__1eo_jXvZnE_.srt.sjson', None), asset_location) + self.assertEqual( + AssetLocation(u'mitX', u'400', u'ignore', u'asset', u'subs__1eo_jXvZnE_.srt.sjson', None), + asset_location + ) def test_get_location_from_path(self): asset_location = StaticContent.get_location_from_path(u'/c4x/a/b/asset/images_course_image.jpg')