Code

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4115 closed (wontfix)

contrib.thumbnails

Reported by: SmileyChris Owned by: nobody
Component: Contrib apps Version: master
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: UI/UX:

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 SmileyChris 7 years ago.
First cut... no tests but read the documentation for the low-down
contrib.thumbnails.2.patch (20.7 KB) - added by SmileyChris 7 years ago.
Next cut
contrib.thumbnails.3.patch (24.1 KB) - added by SmileyChris 7 years ago.
With tests (mostly Chris Moffitt's work)
contrib.thumbnails.4.patch (23.5 KB) - added by SmileyChris 7 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 SmileyChris 7 years ago.
working tests this time, and fixed crop method
contrib.thumbnails.6.patch (23.3 KB) - added by SmileyChris 7 years ago.
Don't append .jpg unless we really need to.
contrib.thumbnails.7.patch (23.3 KB) - added by SmileyChris 7 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@… 7 years ago.
Updated patch set
contrib.thumbnails.9.patch (21.0 KB) - added by SmileyChris 7 years ago.
reverted to my latest proper tests and added unicode safety
contrib.thumbnails.10.patch (24.3 KB) - added by SmileyChris 7 years ago.
contrib.thumbnails.11.patch (24.3 KB) - added by SmileyChris 7 years ago.
make thumbnail.url use urlquote (+ some formatting tweaks)
contrib.thumbnails.12.patch (24.5 KB) - added by SmileyChris 7 years ago.
Take 2 on quoting the thumbnail url (thanks for the critiques, esaj)
contrib.thumbnails.13.patch (23.7 KB) - added by SmileyChris 7 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)

Changed 7 years ago by SmileyChris

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

comment:1 Changed 7 years ago by bin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 7 years ago by brosner <brosner@…>

  • Cc brosner@… added

comment:4 in reply to: ↑ description Changed 7 years ago by Mitja

See ThumbNails for discussion.

comment:5 Changed 7 years ago by nick.lane.au@…

  • Cc nick.lane.au@… added

Good idea, and I like the implementation.

comment:6 Changed 7 years ago by frankie@…

  • Cc frankie@… added

+1 for inclusion

comment:7 Changed 7 years ago by orestis@…

  • Cc orestis@… added
  • Needs tests set

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

comment:8 Changed 7 years ago by jeff@…

+1 fo' sho'. Good stuff!

comment:9 Changed 7 years ago by jeff@…

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!

Changed 7 years ago by SmileyChris

Next cut

comment:10 Changed 7 years ago by SmileyChris

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 Changed 7 years ago by SmileyChris

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

comment:12 follow-up: Changed 7 years ago by Chris Moffitt <chris.moffitt@…>

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 Changed 7 years ago by Chris Moffitt <chris.moffitt@…>

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 in reply to: ↑ 12 Changed 7 years ago by SmileyChris

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.

Changed 7 years ago by SmileyChris

With tests (mostly Chris Moffitt's work)

comment:15 Changed 7 years ago by SmileyChris

  • Needs tests unset

comment:16 Changed 7 years ago by Marc Fargas <telenieko@…>

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 Changed 7 years ago by SmileyChris

Darn, yea I meant to do that and forgot.

Changed 7 years ago by SmileyChris

Tests now inside of thumbnails.contrib (it's the new way) and it'll ignore the tests if PIL import fails

comment:18 Changed 7 years ago by David Danier <goliath.mailinglist@…>

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) 

Changed 7 years ago by SmileyChris

working tests this time, and fixed crop method

comment:19 Changed 7 years ago by michelts@…

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 Changed 7 years ago by SmileyChris

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...

Changed 7 years ago by SmileyChris

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

Changed 7 years ago by SmileyChris

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 Changed 7 years ago by SmileyChris

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

comment:22 Changed 7 years ago by anonymous

  • Cc paolo@… added

comment:23 Changed 7 years ago by John Shaffer <jshaffer2112@…>

  • Cc jshaffer2112@… added

comment:24 Changed 7 years ago by christophrauch@…

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 Changed 7 years ago by anonymous

  • Cc jesse.lovelace@… added

comment:26 Changed 7 years ago by orphansandoligarchs@…

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 Changed 7 years ago by mtredinnick

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 Changed 7 years ago by Fredrik Lundh <fredrik@…>

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 Changed 7 years ago by Fredrik Lundh <fredrik@…>

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 Changed 7 years ago by chris.moffitt@…

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.

Changed 7 years ago by chris.moffitt@…

Updated patch set

comment:31 Changed 7 years ago by sorl

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 Changed 7 years ago by Fredrik Lundh <fredrik@…>

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 Changed 7 years ago by ubernostrum

See also #5476.

comment:34 Changed 7 years ago by sorl

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

Changed 7 years ago by SmileyChris

reverted to my latest proper tests and added unicode safety

comment:35 Changed 7 years ago by jesse.lovelace@…

This last patch doesn't have the templatetags dir.

Changed 7 years ago by SmileyChris

comment:36 Changed 7 years ago by esaj

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

Changed 7 years ago by SmileyChris

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

comment:37 Changed 7 years ago by esaj

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.

Changed 7 years ago by SmileyChris

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

comment:38 Changed 7 years ago by anonymous

  • Cc stuff4riqui@… added

comment:39 Changed 7 years ago by donspaulding

  • Cc donspauldingii@… added

comment:40 Changed 7 years ago by anonymous

  • Cc sebastien.fievet@… added

comment:41 Changed 7 years ago by Eric B <ebartels@…>

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.

Changed 7 years ago by SmileyChris

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

comment:42 Changed 7 years ago by SmileyChris

Thanks Eric, all fixed.

comment:43 Changed 7 years ago by Scott Barr <scott@…>

  • Cc scott@… added

comment:44 Changed 7 years ago by Christoph Rauch <christoph.rauch@…>

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 Changed 7 years ago by esaj

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 Changed 7 years ago by smokey

@esaj:

Creating a hard- or symlink if possible would be the better solution (me thinks). Not really portable to win32 though.

comment:47 Changed 7 years ago by Kellen

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 follow-up: Changed 7 years ago by Kellen

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 in reply to: ↑ 48 Changed 7 years ago by Kellen

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 Changed 7 years ago by jacob

  • Resolution set to wontfix
  • Status changed from new to 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 Changed 7 years ago by SmileyChris

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.