matthewwithanm / django-imagekit

Automated image processing for Django. Currently v4.0
http://django-imagekit.rtfd.org/
BSD 3-Clause "New" or "Revised" License
2.26k stars 276 forks source link

ImageSpec randomly regenerates after request. #453

Closed octavio2895 closed 6 years ago

octavio2895 commented 6 years ago

Hi! So I made a custom processor that fitted my needs to resize the canvas if the image ratio is beyond a certain limit. Here's my custom processor:

class RatioFixer(object):
    """
    Resizes the image canvas so that a target ratio (width/height) is achieved.
    """

    def __init__(self, ratio, color=None, anchor=None, x=None, y=None):
        """
        :param ratio: The target ratio (width/height).
        :param color: The background color to use for padding.
        :param anchor: Specifies the position of the original image on the new
            canvas. Valid values are:
            - Anchor.TOP_LEFT
            - Anchor.TOP
            - Anchor.TOP_RIGHT
            - Anchor.LEFT
            - Anchor.CENTER
            - Anchor.RIGHT
            - Anchor.BOTTOM_LEFT
            - Anchor.BOTTOM
            - Anchor.BOTTOM_RIGHT
            You may also pass a tuple that indicates the position in
            percentages. For example, ``(0, 0)`` corresponds to "top left",
            ``(0.5, 0.5)`` to "center" and ``(1, 1)`` to "bottom right". This is
            basically the same as using percentages in CSS background positions.
        """
        if x is not None or y is not None:
            if anchor:
                raise Exception('You may provide either an anchor or x and y'
                                ' coordinate, but not both.')
            else:
                self.x, self.y = x or 0, y or 0
                self.anchor = None
        else:
            self.anchor = anchor or Anchor.CENTER
            self.x = self.y = None

        self.ratio = round(ratio, 1)
        self.color = color or (255, 255, 255, 0)
        self.ratio_bot = round(self.ratio - .2, 1)
        self.ratio_top = round(self.ratio + .2, 1)

    def process(self, img):
        original_width, original_height = img.size
        original_ratio = round(original_width / original_height, 1)

        if original_ratio > self.ratio_top:
            self.height = int(original_width/self.ratio)
            self.width = original_width
        elif original_ratio < self.ratio_bot:
            self.width = int(original_height*self.ratio)
            self.height = original_height
        else:
            self.height = original_height
            self.width = original_width
            return img
        if self.anchor:
            anchor = Anchor.get_tuple(self.anchor)
            trim_x, trim_y = self.width - original_width, \
                             self.height - original_height
            x = int(float(trim_x) * float(anchor[0]))
            y = int(float(trim_y) * float(anchor[1]))
        else:
            x, y = self.x, self.y

        new_img = Image.new('RGBA', (self.width, self.height), self.color)
        new_img.paste(img, (x, y))
        return new_img`

Its a modified version of the ResizeCanvas processor that comes with PilKit.

I noticed that with this new processor images will randomly re-generate (twice) which increases the loading time and fills up the cache. I'm pretty sure there's something wrong with my processor since it worked well previously but its not obvious to me.

octavio2895 commented 6 years ago

I rewrote the custom processor

class RatioFixer2(object):

    def __init__(self, width, height, color=None, anchor=None, x=None, y=None):
        if x is not None or y is not None:
            if anchor:
                raise Exception('You may provide either an anchor or x and y'
                                ' coordinate, but not both.')
            else:
                self.x, self.y = x or 0, y or 0
                self.anchor = None
        else:
            self.anchor = anchor or Anchor.CENTER
            self.x = self.y = None

        self.width = width
        self.height = height
        self.color = color or (255, 255, 255, 0)

    def process(self, img):
        original_width, original_height = img.size
        ratio = min(float(self.width) / original_width, float(self.height) / original_height)
        new_width, new_height = (int(round(original_width * ratio)), int(round(original_height * ratio)))
        img = Resize(new_width, new_height, upscale=True).process(img)
        if self.anchor:
            anchor = Anchor.get_tuple(self.anchor)
            trim_x, trim_y = self.width - new_width, self.height - new_height
            x = int(float(trim_x) * float(anchor[0]))
            y = int(float(trim_y) * float(anchor[1]))
        else:
            x, y = self.x, self.y
        new_img = Image.new('RGBA', (self.width, self.height), self.color)
        new_img.paste(img, (x, y))
        return new_img`

The regenerating seems to have stopped and works perfectly. I'm still wondering what caused the regeneration on my first approach?

vstoykov commented 6 years ago

I'm glad that you solved the problem.

From the two versions I see that in the first one (the broken one) it's not thread safe. I'm not sure how you are using the code and if your code is deployed in multithreaded environment but you should not change any state of the processor in process method (you should not assign anything to self). You can do that only in __init__. Because if two threads share the same processor instance and are processing different images at the same time then one thread can change the values of self.width and self.height after they where changed by other thread but before the other thread was able to use them.

This is what I see at first glance.

P.P. I edited your comments to fix the code blocks.

octavio2895 commented 6 years ago

@vstoykov Oh, that makes sense! Thanks a lot. Sorry for the newbie question I'm new to Django and python in general.