convert_resize_img should not crash if it can't resize the image

[danschwarz]

Description I'm trying to render an image (Mastodon custom server emoji) into a 2-cell-wide x 1-cell-tall box. Usually this works fine. Sometimes I encounter a server emoji that can't be resized this small. The error occurred much more frequently when I was using a 1x1 cell box, expanding to 2x1 is fine. Expanding beyond that is not going to work for this application.

To Reproduce Steps to reproduce the behavior:

Code snippet: This will constrain the UrwidImage to render in a 1x1 cell:

Nest the below widget cw in whatever testbed app you like. Make sure url_string points to the url of a suitably large image. columns = [] img = Image.open(requests.get(url_string, stream=True).raw) columns.append(urwid.BoxAdapter(UrwidImage(AutoImage(img)), 1)) columns.append(("weight", 9999, urwid.Text(""))) cw = urwid.Columns(columns, dividechars=1, min_width=1)

4. See error File ".../dev/term-image/src/term_image/image/common.py", line 1584, in convert_resize_img raise ValueError("Image size or scale too small") from e

Expected behavior Ideally the image would be clipped to the available space. If it can't be clipped, at least don't crash.

Screenshots If applicable, add screenshots to help explain your problem.

[danschwarz]

On second thought - if the image can't be clipped to the cell bounds - the exception gives me useful info. I can revert to text display of the custom emoji (it looks like : emojiname : ).

[AnonymouX47]

I also encountered this while testing and provided a solution. What do you see to this?

The issue is, in computing the size of the image to fit into a 1 x 1 frame while preserving the aspect ratio, one of the dimensions was computed to be really small and thereby rounded to zero.

I have considered always rounding computed dimension up instead but I have not decided on whether its a good idea. I've tried it before and it make some images go really off their original aspect ratio when small.

Also, I could just use a max(dimension, 1) ? 🤔

Anyways, aside small (zero) size, there are also other errors that could occur when converting an image, which are not reasonably remediable. For instance, if an image was initialized with a PIL image instance with an invalid mode. I can't recreate it right now but I remember it was a certain exception I was encountered that caused me to add an exception handler for conversion.

[AnonymouX47]

I can revert to text display of the custom emoji (it looks like : emojiname : ).

That's another option. And this exactly (i.e leaving the user to handle the exception as they see fit), is why I intentionally left rendering errors unhandled.

You can subclass UrwidImage and handle the exception within render().

Sorry, I don't know why/how I mistakenly edited your comment. 😆

[danschwarz]

It'd be interesting to see how it performs with max (dimension, 1). At such small image sizes, aspect ratio may be a bit distorted, but it won't matter. And the error placeholder widget will provide a nice solution in cases where we get an exception.

[AnonymouX47]

Hmm... Okay.

Do I implement the quick fix now? Or do I just wait and implement the actual solution?

The actual solution is something I had in mind to do already. A means of preserving aspect ratio beyond just the size computation. If you've noticed, images rendered with a small size tend to be way off their original aspect ratio. This is caused by rounding.

The actual image image size computation is done in pixels but when converting from pixels to cells, some precision is lost.

I intend to store the ratio of the computed pixel size to the rounded pixel size along with the size. Then at render-time, that ratio can be used to compute the actual image size, and the image is composited unto a blank image of the rounded size (i.e pixel-equivalent of the image size in cells).

This is not so trivial. So, I wouldn't want to delay this release any longer. Do you think the error placeholder will suffice for now, or I should implement the max(dim, 1)?

[AnonymouX47]

Also, I forgot to mention that imge.scale is another possible cause of a zero render size. I intend to remove this property soon because it's actually standing in the way of some planned features and I almost always can't see any substantial need for it.

My reason for the property was to provide a means by which the aspect ratio of an image can be distorted if wanted but I ask myself every time, who really wants to do so?

[AnonymouX47]

Changed the label because this has actually been the intended behaviour until now.

Also, I I'm considering changing the title because any exception raised within convert_resize_img() can't be handled, the only way is to prevent it e.g by never computing a zero size as I've proposed.

Or what do you think should be done if an exception is raised at that point?

[danschwarz]

Your idea for a long term fix sounds good. I don't know who's going to want to have the widget distort their image. The options available now - display as-provided, or resize-to-fit available space, preserving aspect ratio, seem right.

I'm going to need to catch these exceptions, so overriding render() seems like my near term solution.

On Sat, Feb 25, 2023 at 9:16 PM Toluwaleke Ogundipe < @.***> wrote:

Hmm... Okay.

Do I implement the quick fix now? Or do I just wait and implement the actual solution?

The actual solution is something I had in mind to do already. A means of preserving aspect ratio beyond just the size computation. If you've noticed, images rendered with a small size tend to be way off their original aspect ratio. This is caused by rounding.

The actual image image size computation is done in pixels but when converting from pixels to cells, some precision is lost.

I intend to store the ratio of the computed pixel size to the rounded pixel size along with the size. Then at render-time, that ratio can be used to compute the actual image size, and the image is composited unto a blank image of the rounded size (i.e pixel-equivalent of the image size in cells).

— Reply to this email directly, view it on GitHub https://github.com/AnonymouX47/term-image/issues/75#issuecomment-1445250483, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAY4FJWLEZOE7GTBRIL4EALWZK4JJANCNFSM6AAAAAAVIEDS34 . You are receiving this because you authored the thread.Message ID: @.***>

[AnonymouX47]

I see, good.

Commit 6f6aa390e01a1fad0d7e809144664b2737b81012 actually already covers your case. With the changes, it's now impossible to have a zero size.

As for the scale (which you don't use), I've also made the necessary changes to disallow a zero render size but i can't push until i update the related tests.

[AnonymouX47]

About the image conversion I mentioned yesterday:

from PIL import Image
from term_image.image import *
AutoImage(Image.new("La", (100, 100))).draw()

results in:

. . .

ValueError: conversion from La to L not supported

The above exception was the direct cause of the following exception:

. . .

ValueError: Unable to convert image

IMO, there's obviously nothing that could be done to handle such, the exception just has to be raised.

[AnonymouX47]

As for resize errors, any obvious this ones are now prevented by ensuring the image size, rendered size and hence, render size is never null. The rendered size covers the case of small scale that I mentioned.

This will keep things together until I get to implement the actual solution later. Can't wait to get images with well preserved aspect ratio 😁.

Thanks for the report and suggestion. 🙇🏾‍♂️

[AnonymouX47]

One last question... Please, do you think set_error_placeholder() is still necessary? 🤔