#4115 closed (wontfix)
contrib.thumbnails
Reported by: | Chris Beaven | Owned by: | nobody |
---|---|---|---|
Component: | Contrib apps | Version: | dev |
Severity: | Keywords: | ||
Cc: | brosner@…, nick.lane.au@…, frankie@…, orestis@…, paolo@…, jshaffer2112@…, jesse.lovelace@…, stuff4riqui@…, donspauldingii@…, sebastien.fievet@…, scott@… | Triage Stage: | Design decision needed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The idea has been bantered about in the dev group, so I thought I'd put together something to start the ball rolling at least.
Attachments (13)
Change History (64)
by , 18 years ago
Attachment: | contrib.thumbnails.patch added |
---|
comment:1 by , 18 years ago
Good initiative Chris.
Some time ago I gave a shot at creating a set of generic views to interface with PIL in a RESTful manner. Maybe it's a nice idea to consider adding generic views to your contrib.
Regards,
bin
comment:2 by , 18 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 18 years ago
Cc: | added |
---|
comment:9 by , 17 years ago
One suggestion to make this more useful (at least to me): We've found at the Journal-World that the default PIL JPG compression results in really crappy, artifacted images. After a good deal of playing, we decided that 85 was the ideal mix of filesize and image quality for us. I'd suggest the save function the "quality=85" argument:
thumbnail.save(self.filename, "JPEG", quality=85)
Obviously this is personal preference, so maybe it should be a setting, instead. In any case, I've never heard of any designer that is satisfied with the quality of the default JPG compression.
Great work!
comment:10 by , 17 years ago
Here's the next cut.
- I've added in Jeff's suggestion of being able to provide the JPEG quality.
- Following on from the group discussion, I added in a helper template tag which creates the
<img />
tag with the correct width & height for you.
follow-up: 14 comment:12 by , 17 years ago
The size is going to be returned incorrectly if a thumbnail is created. Here's a simple function I created to cache and return the actual size.
I'd recommend calling this in the img_tag filter.
size_cache = get_cache('locmem:///')
_SIZE_CACHE_TIMEOUT = 60 * 60 * 60 * 24 * 31 # 1 month
def get_actual_size(self):
"""
When an image is scaled, the dimensions of the image may not equal
the supplied. Example 240x240 -> 240x96 actual size depending on the aspect
ratio. This uses the cache to store the actual size instead of calling it each
time an image is displayed. Normal use case is for image tags.
"""
size = size_cache.get(self.filename)
if size:
return size
try:
img = Image.open(self.filename)
size_cache.set(self.filename, im.size, _SIZE_CACHE_TIMEOUT)
return img.size
except IOError, msg:
raise ThumbnailInvalidImage(msg)
actual_size = property(get_actual_size)
comment:13 by , 17 years ago
Ooops. Let's try that last code set again.
This should be put into the Thumbnail object.
size_cache = get_cache('locmem:///') _SIZE_CACHE_TIMEOUT = 60 * 60 * 60 * 24 * 31 # 1 month def get_actual_size(self): """ When an image is scaled, the dimensions of the image may not equal the supplied. Example 240x240 -> 240x96 actual size depending on the aspect ratio. This uses the cache to store the actual size instead of calling it each time an image is displayed. Normal use case is for image tags. """ size = size_cache.get(self.filename) if size: return size try: img = Image.open(self.filename) size_cache.set(self.filename, im.size, _SIZE_CACHE_TIMEOUT) return img.size except IOError, msg: raise ThumbnailInvalidImage(msg) actual_size = property(get_actual_size)
comment:14 by , 17 years ago
Chris Moffitt said: The size is going to be returned incorrectly if a thumbnail is created.
I think you're missing where the size
is coming from.
thumbnail.thumbnail.size
will return the correct size because the thumbnail
property gets the PIL image object internally.
You do raise the valid question whether we should cache this rather than require a fs operation, but it does work the way it is now.
by , 17 years ago
Attachment: | contrib.thumbnails.3.patch added |
---|
With tests (mostly Chris Moffitt's work)
comment:15 by , 17 years ago
Needs tests: | unset |
---|
comment:16 by , 17 years ago
Taking a quick look on the patch if you do not have PIL installed the test will immediately fail (ImportError), but PIL is an optional dependency for django so the test just just say "Skipping this test because there's no PIL installed".
Just an idea :)
by , 17 years ago
Attachment: | contrib.thumbnails.4.patch added |
---|
Tests now inside of thumbnails.contrib (it's the new way) and it'll ignore the tests if PIL import fails
comment:18 by , 17 years ago
crop() had some errors with non-quadratic sizes here. It worked if crop_ratio was <= 1, but failed with crop_ratio > 1. I tried to fix this, leading to this code:
def crop(thumbnail): """ Crop the image down to the same ratio as `size` """ img = thumbnail.original_image size = thumbnail.size if img.size[0] < size[0] or img.size[1] < size[1]: raise ThumbnailTooSmall('Image must be at least %s wide and %s high' % size) image_x, image_y = img.size crop_ratio = size[0] / float(size[1]) image_ratio = image_x / float(image_y) if crop_ratio < image_ratio: # x needs to shrink top = 0 bottom = image_y if crop_ratio <= 1: crop_width = int(image_y * crop_ratio) else: crop_width = int(image_y / crop_ratio) left = (image_x - crop_width) // 2 right = left + crop_width else: # y needs to shrink left = 0 right = image_x if crop_ratio <= 1: crop_height = int(image_x * crop_ratio) else: crop_height = int(image_x / crop_ratio) top = (image_y - crop_height) // 2 bottom = top + crop_height img = img.crop((left, top, right, bottom)) return img.resize(size, Image.ANTIALIAS)
by , 17 years ago
Attachment: | contrib.thumbnails.5.patch added |
---|
working tests this time, and fixed crop method
comment:19 by , 17 years ago
Hi, I've found 2 little bugs ;)
If the image size is shorter than the expected one, thumbnail filter should raise a ThumbnailTooShort exception (methods.py:L10), it try a variable interpolation with the 2 itens list variable "size", the size is a list, not a tuple, should be converted to a tuple elsewhere (maybe in the base.py:L18).
If the image isn't a JPEG, thumbnails try to append ".jpg" to the end of filename, the string don't have a "append" method, should use += syntax (see the base.py:L34).
I will send a patch fixing these errors but now I am having problems with svn :D
Thanks
comment:20 by , 17 years ago
I don't see why you need to convert a list to a tuple - it'll still do the same thing.
Regarding the appending of the filename and checking for both .jpg
and .jpeg
(case insensitively), that makes sense. Patch coming...
by , 17 years ago
Attachment: | contrib.thumbnails.6.patch added |
---|
Don't append .jpg unless we really need to.
by , 17 years ago
Attachment: | contrib.thumbnails.7.patch added |
---|
Fixing a small bug if size is passed as a list (must be converted to a tuple so it can be used as string formatting arguments)
comment:22 by , 17 years ago
Cc: | added |
---|
comment:23 by , 17 years ago
Cc: | added |
---|
comment:24 by , 17 years ago
Please incorporate following fix. The error happened on my own application.
--- thumbnails.py.orig 2007-07-11 23:30:09.000000000 +0200 +++ thumbnails.py 2007-07-11 23:30:56.000000000 +0200 @@ -84,6 +84,9 @@ except OSError: # Couldn't read the original file. return '' + except IOError: + # Looks like it wasn't a file at all. + return '' # Generate the thumbnail. try:
comment:25 by , 17 years ago
Cc: | added |
---|
comment:26 by , 17 years ago
Another quick fix to make it possible to thumbnail images which aren't RGB.
--- base.py.orig 2007-07-28 13:06:58.000000000 +0100 +++ base.py 2007-07-28 13:10:17.000000000 +0100 @@ -53,6 +53,8 @@ original = Image.open(StringIO(data)) except IOError, msg: raise ThumbnailInvalidImage(msg) + if original.mode not in ("L", "RGB"): + original = original.convert("RGB") self._original_image = original thumbnail = self.method() self._thumbnail = thumbnail
comment:27 by , 17 years ago
Just a note that Fredrik Lundh felt the patch (with the previous comment's change) looked resaonable. So we aren't doing any PIL silliness here.
comment:28 by , 17 years ago
Following up myself here; I have two minor nits:
- orphansandoligarchs makes a good point here; converting to RGB in case it's neither grayscale or colour is a good thing
- PIL uses lazy loading of image data, which means that the first method that actually uses the pixels may fail as well. It could be worth moving the conversion and the method call into the try/except IOError part.
comment:29 by , 17 years ago
Ahem. And one somewhat larger:
- The thumbnail method modifies the source image in place, which means that using the "scale" method messes up the original image.
comment:30 by , 17 years ago
I have modified patch set 7 to include the recommended updates from Frederik. I also updated the tests so that they pass now given the current naming scheme.
I've also attached the sample image used by the tests.
comment:31 by , 17 years ago
To make filename with non-ascii characters to work, I guess this is since the unicode branch merge, I changed return thumbnail to return unicode(thumbnail) in thumbnails.py.
comment:32 by , 17 years ago
You meant "force_unicode", right? Python's "unicode" constructor throws an exception if you run it on a non-ASCII and don't specify an encoding.
comment:34 by , 17 years ago
Fredrik: actuallty not, smart_unicode and force_unicode throws error here: 'ascii' codec can't encode character...
by , 17 years ago
Attachment: | contrib.thumbnails.9.patch added |
---|
reverted to my latest proper tests and added unicode safety
by , 17 years ago
Attachment: | contrib.thumbnails.10.patch added |
---|
comment:36 by , 17 years ago
Image URLs need to be properly formatted using django.utils.http.urlquote, e.g. ' '
converted to '%20'
.
by , 17 years ago
Attachment: | contrib.thumbnails.11.patch added |
---|
make thumbnail.url use urlquote (+ some formatting tweaks)
comment:37 by , 17 years ago
Hmm, the latest patch urlquotes the base URL too, which breaks if you have "http://blah.com/" as your MEDIA_URL. It should assume that MEDIA_URL is already urlquoted, and only urlquote the image filename.
by , 17 years ago
Attachment: | contrib.thumbnails.12.patch added |
---|
Take 2 on quoting the thumbnail url (thanks for the critiques, esaj)
comment:38 by , 17 years ago
Cc: | added |
---|
comment:39 by , 17 years ago
Cc: | added |
---|
comment:40 by , 17 years ago
Cc: | added |
---|
comment:41 by , 17 years ago
The current patch seems to be generating the thumbnail one time too many times (line 84 thumbnail.py) and then again (line 89). The first call isn't wrapped in a try statement, so it fails with a ThumbnailTooSmall exception if the image is smaller than the specified thumbnail size.
by , 17 years ago
Attachment: | contrib.thumbnails.13.patch added |
---|
Fix accidentally duplicated thumbnail creation, and img_tag filter incorrectly using file
instead of filename
comment:43 by , 17 years ago
Cc: | added |
---|
comment:44 by , 17 years ago
A few thoughts:
If an original image is smaller than the requested thumbnail size, ThumnailTooSmall is raised. This makes sense as upscaling the image would increase its size.
I propose that instead of raising ThumbnailTooSmall, we return the original image instead and add a mechanism to use HTML to do the upscaling. This mechanism should be able to decide if is best to use either height or width so the aspect ratio gets preserved.
I think we could make a wrapper around the thumbnail and img_tag templatetags. Perhaps change the current thumbnail to be thumbnail_url instead?
Any comments?
comment:45 by , 17 years ago
I just noticed that it seems to scale the image even if it already has the correct dimensions, which (in my case) may unnecessarily add artefacts to a jpeg image. Instead, the image should just be copied as-is if it already has the correct dimensions.
comment:46 by , 17 years ago
@esaj:
Creating a hard- or symlink if possible would be the better solution (me thinks). Not really portable to win32 though.
comment:47 by , 17 years ago
If installing this patch on Debian, it should go in: /usr/share/python-support/python-django/django/contrib/thumbnails/
But the same directory structure should be made in: /var/lib/python-support/python2.4/django/contrib/thumbnails/
with symlinks to each of the .py files, as with the other django.contrib directories found there.
follow-up: 49 comment:48 by , 17 years ago
With regards to ThumbnailTooSmall, if one is using the thumbnail template filters for general-purpose resizing, then returning nothing to the template causes it to fail really ungracefully. I agree that returning the original image (with proper size info) is much, much better than failing.
This could be achieved by just returning the original image data instead of resized data. I personally think it's okay to replicate the image on disk with the expected filename as this will actually be of a smaller filesize than a generated thumbnail. It would be better to just return a Thumbnail object with the relevant data for the original image. The first option is easier to implement:
# diff methods.py methods.py.orig 10c10,11 < return img --- > raise ThumbnailTooSmall('Image should be at least %s wide or %s high' % > tuple(size)) 21c22,23 < return img --- > raise ThumbnailTooSmall('Image must be at least %s wide and %s high' % > tuple(size)) 30c32,33 < return img --- > raise ThumbnailTooSmall('Image must be at least %s wide and %s high' % > tuple(size))
comment:49 by , 17 years ago
patch version, sorry:
--- methods.py.orig 2007-12-01 10:55:46.000000000 +0000 +++ methods.py 2007-12-01 10:56:34.000000000 +0000 @@ -7,8 +7,7 @@ img = thumbnail.original_image size = thumbnail.size if img.size[0] < size[0] and img.size[1] < size[1]: - raise ThumbnailTooSmall('Image should be at least %s wide or %s high' % - tuple(size)) + return img # .thumbnail() method modifies in-place, so create a copy. img = img.copy() img.thumbnail(size, Image.ANTIALIAS) @@ -19,8 +18,7 @@ img = thumbnail.original_image size = thumbnail.size if img.size[0] < size[0] or img.size[1] < size[1]: - raise ThumbnailTooSmall('Image must be at least %s wide and %s high' % - tuple(size)) + return img # Just use ImageOps.fit (added in PIL 1.1.3) return ImageOps.fit(img, size, method=Image.ANTIALIAS) @@ -29,6 +27,5 @@ img = thumbnail.original_image size = thumbnail.size if img.size[0] < size[0] or img.size[1] < size[1]: - raise ThumbnailTooSmall('Image must be at least %s wide and %s high' % - tuple(size)) + return img return img.resize(size, Image.ANTIALIAS)
comment:50 by , 17 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
I think this is one of those projects that ought to be taken over to Google Code (or whereever) to get some real-world use before we consider adding it to Django.
comment:51 by , 17 years ago
It is as of a few days ago... I wanted to finalise things with sorl before I wrote here but jacob has forced my hand :)
I've taken a role in the development team of sorl-thumbnail: http://code.google.com/p/sorl-thumbnail/
Currently I am working on a branch of it which will refactor quite a lot of the code, making it similar in functionality to this ticket. It is mostly complete, there are just a few issues to resolve, like should it be a tag or a filter.
But anyway, that is where my thumbnails attention is going to be pointed. I recommend those who have followed this ticket to pay attention to that project.
First cut... no tests but read the documentation for the low-down