Opened 17 years ago

Closed 16 years ago

Last modified 16 years ago

#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)

contrib.thumbnails.patch (27.3 KB ) - added by Chris Beaven 17 years ago.
First cut... no tests but read the documentation for the low-down
contrib.thumbnails.2.patch (20.7 KB ) - added by Chris Beaven 17 years ago.
Next cut
contrib.thumbnails.3.patch (24.1 KB ) - added by Chris Beaven 17 years ago.
With tests (mostly Chris Moffitt's work)
contrib.thumbnails.4.patch (23.5 KB ) - added by Chris Beaven 17 years ago.
Tests now inside of thumbnails.contrib (it's the new way) and it'll ignore the tests if PIL import fails
contrib.thumbnails.5.patch (23.2 KB ) - added by Chris Beaven 17 years ago.
working tests this time, and fixed crop method
contrib.thumbnails.6.patch (23.3 KB ) - added by Chris Beaven 17 years ago.
Don't append .jpg unless we really need to.
contrib.thumbnails.7.patch (23.3 KB ) - added by Chris Beaven 17 years ago.
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)
contrib.thumbnails.8.patch (25.6 KB ) - added by chris.moffitt@… 17 years ago.
Updated patch set
contrib.thumbnails.9.patch (21.0 KB ) - added by Chris Beaven 17 years ago.
reverted to my latest proper tests and added unicode safety
contrib.thumbnails.10.patch (24.3 KB ) - added by Chris Beaven 17 years ago.
contrib.thumbnails.11.patch (24.3 KB ) - added by Chris Beaven 16 years ago.
make thumbnail.url use urlquote (+ some formatting tweaks)
contrib.thumbnails.12.patch (24.5 KB ) - added by Chris Beaven 16 years ago.
Take 2 on quoting the thumbnail url (thanks for the critiques, esaj)
contrib.thumbnails.13.patch (23.7 KB ) - added by Chris Beaven 16 years ago.
Fix accidentally duplicated thumbnail creation, and img_tag filter incorrectly using file instead of filename

Download all attachments as: .zip

Change History (64)

by Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.patch added

First cut... no tests but read the documentation for the low-down

comment:1 by bin, 17 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 Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:3 by brosner <brosner@…>, 17 years ago

Cc: brosner@… added

in reply to:  description comment:4 by Mitja, 17 years ago

See ThumbNails for discussion.

comment:5 by nick.lane.au@…, 17 years ago

Cc: nick.lane.au@… added

Good idea, and I like the implementation.

comment:6 by frankie@…, 17 years ago

Cc: frankie@… added

+1 for inclusion

comment:7 by orestis@…, 17 years ago

Cc: orestis@… added
Needs tests: set

+1 on this, I'll begin testing it...

comment:8 by jeff@…, 17 years ago

+1 fo' sho'. Good stuff!

comment:9 by jeff@…, 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!

by Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.2.patch added

Next cut

comment:10 by Chris Beaven, 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.

comment:11 by Chris Beaven, 17 years ago

I meant a helper template filter. It's named img_tag.

comment:12 by Chris Moffitt <chris.moffitt@…>, 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 Chris Moffitt <chris.moffitt@…>, 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)

in reply to:  12 comment:14 by Chris Beaven, 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 Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.3.patch added

With tests (mostly Chris Moffitt's work)

comment:15 by Chris Beaven, 17 years ago

Needs tests: unset

comment:16 by Marc Fargas <telenieko@…>, 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 :)

comment:17 by Chris Beaven, 17 years ago

Darn, yea I meant to do that and forgot.

by Chris Beaven, 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 David Danier <goliath.mailinglist@…>, 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 Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.5.patch added

working tests this time, and fixed crop method

comment:19 by michelts@…, 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 Chris Beaven, 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 Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.6.patch added

Don't append .jpg unless we really need to.

by Chris Beaven, 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:21 by Chris Beaven, 17 years ago

I see the tuple/list bug now, fixed in .7

comment:22 by anonymous, 17 years ago

Cc: paolo@… added

comment:23 by John Shaffer <jshaffer2112@…>, 17 years ago

Cc: jshaffer2112@… added

comment:24 by christophrauch@…, 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 anonymous, 17 years ago

Cc: jesse.lovelace@… added

comment:26 by orphansandoligarchs@…, 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 Malcolm Tredinnick, 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 Fredrik Lundh <fredrik@…>, 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 Fredrik Lundh <fredrik@…>, 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 chris.moffitt@…, 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.

by chris.moffitt@…, 17 years ago

Attachment: contrib.thumbnails.8.patch added

Updated patch set

comment:31 by sorl, 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 Fredrik Lundh <fredrik@…>, 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:33 by James Bennett, 17 years ago

See also #5476.

comment:34 by sorl, 17 years ago

Fredrik: actuallty not, smart_unicode and force_unicode throws error here: 'ascii' codec can't encode character...

by Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.9.patch added

reverted to my latest proper tests and added unicode safety

comment:35 by jesse.lovelace@…, 17 years ago

This last patch doesn't have the templatetags dir.

by Chris Beaven, 17 years ago

Attachment: contrib.thumbnails.10.patch added

comment:36 by Jason Davies, 16 years ago

Image URLs need to be properly formatted using django.utils.http.urlquote, e.g. ' ' converted to '%20'.

by Chris Beaven, 16 years ago

Attachment: contrib.thumbnails.11.patch added

make thumbnail.url use urlquote (+ some formatting tweaks)

comment:37 by Jason Davies, 16 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 Chris Beaven, 16 years ago

Attachment: contrib.thumbnails.12.patch added

Take 2 on quoting the thumbnail url (thanks for the critiques, esaj)

comment:38 by anonymous, 16 years ago

Cc: stuff4riqui@… added

comment:39 by donspaulding, 16 years ago

Cc: donspauldingii@… added

comment:40 by anonymous, 16 years ago

Cc: sebastien.fievet@… added

comment:41 by Eric B <ebartels@…>, 16 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 Chris Beaven, 16 years ago

Attachment: contrib.thumbnails.13.patch added

Fix accidentally duplicated thumbnail creation, and img_tag filter incorrectly using file instead of filename

comment:42 by Chris Beaven, 16 years ago

Thanks Eric, all fixed.

comment:43 by Scott Barr <scott@…>, 16 years ago

Cc: scott@… added

comment:44 by Christoph Rauch <christoph.rauch@…>, 16 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 Jason Davies, 16 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 smokey, 16 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 Kellen, 16 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.

comment:48 by Kellen, 16 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))

in reply to:  48 comment:49 by Kellen, 16 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 Jacob, 16 years ago

Resolution: wontfix
Status: newclosed

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 Chris Beaven, 16 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.

Note: See TracTickets for help on using tickets.
Back to Top