Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard

Add progress callback when save_all is used

Open radarhere opened this issue 2 years ago • 12 comments

Resolves #7433

The issue requests the ability to use a callback function to monitor progress when calling save_all. This function is triggered after processing each frame, with the filename of the current image (or None if the filename is not available), the number of frames processed so far, and the total number of frames.

from PIL import Image

def callback(filename, frame_count, n_frames):
    print((filename, frame_count, n_frames))

with Image.open("Tests/images/hopper.gif") as im:
    with Image.open("Tests/images/dispose_bgnd.gif") as im2:
        im.save("out.gif", save_all=True, append_images=[im2], progress=callback)

gives

('Tests/images/hopper.gif', 1, 6)
('Tests/images/dispose_bgnd.gif', 2, 6)
('Tests/images/dispose_bgnd.gif', 3, 6)
('Tests/images/dispose_bgnd.gif', 4, 6)
('Tests/images/dispose_bgnd.gif', 5, 6)
('Tests/images/dispose_bgnd.gif', 6, 6)

radarhere avatar Oct 02 '23 03:10 radarhere

An idea: use the same callback for generating frames: pass current im object (which could be an image without bitmap in memory) and check if complete image is returned.

homm avatar Oct 02 '23 10:10 homm

Sorry, I'm not sure I understand. Are you suggesting this

from PIL import Image

colors_left = ["#f00", "#0f0"]
def callback():
    if colors_left:
        return Image.new("RGB", (1, 1), frames_left.pop(0))

im = Image.new("RGB", (1, 1))
im.save("out.png", save_all=True, progress=callback)

with the intention that it allow multiframe images to be saved without needing all images to be created beforehand and so reduce memory impact?

radarhere avatar Oct 02 '23 10:10 radarhere

In general, yes. Depending on current code flow, it could be like this, or require some informations about the frames (like size and mode).

homm avatar Oct 02 '23 10:10 homm

APNG considers the modes of all frames when saving - #6610. I don't know what an elegant way of getting that particular information from the user at the beginning would look like, and I find it hard to imagine that users would expect that they have to know that at the beginning.

radarhere avatar Oct 02 '23 11:10 radarhere

allow multiframe images to be saved without needing all images to be created beforehand and so reduce memory impact

I imagine if this is ever implemented, it should not be exposed in a workaround-ish way, and it probably should be implemented as a format-specific method if it can only be supported for specific formats (e.g. GifImagePlugin.fromFrames(generator)).

I'm also pretty sure such a feature is out of scope for this particular pull-request.

LeXofLeviafan avatar Oct 02 '23 15:10 LeXofLeviafan

I'm also pretty sure such a feature is out of scope for this particular pull-request.

It out of scope of your needs, but is closely related to current PR since it may or may not share the same API and it is better to discuss it here.

homm avatar Oct 02 '23 18:10 homm

APNG considers the modes of all frames when saving

We can restrict modes of generated frames only to the same as the initial image.

homm avatar Oct 02 '23 18:10 homm

It out of scope of your needs, but is closely related to current PR since it may or may not share the same API and it is better to discuss it here.

You're conflating the concerns of "keeping track of progress" and "generating input data for the operation", based solely on the fact that both are related to "iteration over frames". I don't believe such features should even intersect in their implementation, let alone complicating each other.

In fact, I'm pretty sure that the correct way of implementing the feature you're requesting would be by supporting a generator as a value passed in append_images without immediately converting it to a list (it would be requested to yield a new image after the previous one got processed) – doing it this way would be idiomatic for Python; and the only way it would in any way affect the functionality of this pull-request, is that it would no longer be possible to pre-calculate the number of frames if append_images is a generator.

def to_gif_memheavy (frames, filename):
    first, _frames = Image.open(frames[0]), frames[1:]
    # append_images is a list with pre-loaded data
    first.save(filename, 'GIF', save_all=True, append_images=[Image.open(s) for s in _frames])

def to_gif_memlow (frames, filename):
    first, _frames = Image.open(frames[0]), frames[1:]
    # append_images is a generator and loads images one-at-a-time
    first.save(filename, 'GIF', save_all=True, append_images=(Image.open(s) for s in _frames))

LeXofLeviafan avatar Oct 02 '23 20:10 LeXofLeviafan

Having iterators support you can track progress without any callbacks.

homm avatar Oct 03 '23 09:10 homm

Having iterators support you can track progress without any callbacks.

Only in the sole specific case when composing an animated image from separate single-frame images that you load one-by-one. Which means, when combining animated images, you'd only get updates per image, not per frame (and the simple usecase of "saving/converting a single animated image" would get zero progress tracking support).

Not to mention you're conflating concerns of "providing input" and "progress tracking" again. Why would the user need to be forced to deal with both when he only needs one of these things? And why would the user code dealing with one of these need to also deal with the other? If the user wants to track frame progress, he passes a callback that gets invoked on every frame (e.g. print). If the user wants to load input images one-at-a-time, he passes a generator instead of a list. He can easily do one of these things, both, or neither. Simple, intuitive, unentangled.

LeXofLeviafan avatar Oct 03 '23 11:10 LeXofLeviafan