Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added progress callback when save_all is used #7435

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions Tests/test_file_apng.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from io import BytesIO

import pytest

from PIL import Image, ImageSequence, PngImagePlugin
Expand Down Expand Up @@ -663,6 +665,36 @@ def test_apng_save_blend(tmp_path):
assert im.getpixel((0, 0)) == (0, 255, 0, 255)


def test_save_all_progress():
out = BytesIO()
progress = []

def callback(filename, frame_number, n_frames):
progress.append((filename, frame_number, n_frames))

Image.new("RGB", (1, 1)).save(out, "PNG", save_all=True, progress=callback)
assert progress == [(0, 1, 1)]

out = BytesIO()
progress = []

with Image.open("Tests/images/apng/single_frame.png") as im:
with Image.open("Tests/images/apng/delay.png") as im2:
im.save(
out, "PNG", save_all=True, append_images=[im, im2], progress=callback
)

assert progress == [
(0, 1, 7),
(1, 2, 7),
(2, 3, 7),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say... if the frames are counted from 1, why count the images from 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm triggering these callbacks when frames are completed, not when they are started.

My intention is for (0, 1, 7) to communicate "We're in the first image. We've completed the first frame, there are seven frames in total".
Then (2, 7, 7) communicates "We're in the third image. We've completed seven frames, there are seven frames in total, so we're done"

If I started counting the frames from 0, the last value would be (2, 6, 7). That looks less to me like save_all is finished.

I recognise this could be a subject of debate.

Copy link

@LeXofLeviafan LeXofLeviafan Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…The question was “Why count the images from 0?”

I.e. why are the images numbered starting from 0? You're numbering frames starting from 1 – it would be consistent to use the same numbering scheme for images.

(And yes, I know that due to how pointer arithmetic works, arrays in C are indexed starting from 0, but that's an implementation detail and it's kind of ridiculous to drag it into higher-level languages just because; counting things is meant to be started from 1 – that's what the number means in the first place.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to explain that my goal is not to number the frames from 1, but rather to clearly communicate when progress is completed, because the frame count will be compared with the total number of frames. The image count will not.

arrays in C are indexed starting from 0, but that's an implementation detail and it's kind of ridiculous to drag it into higher-level languages

Python arrays are also indexed from 0.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image count will not.

And yet counting from 0 would be inconsistent with the other counting used within same data set.

Python arrays are also indexed from 0.

Which makes no sense since Python is a high-level language (and there's some consistency issues caused by this arbitrary choice to imitate C array indexing, but that's neither here nor there).

Besides, the images aren't coming from a list as far as the user is concerned (well, there's append_images, but going by that logic you'd have to start from -1 since the first input image is not in this list in the first place… except that -1 is a valid index in Python, so it probably would be None instead).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a commit naming each piece of information within the returned data. It is now "image_index" and "completed_frames". Hopefully that clarifies that one is the Pythonic zero-based index of the image in progress, and the other is the number of frames that have been finished.

(2, 4, 7),
(2, 5, 7),
(2, 6, 7),
(2, 7, 7),
]


def test_seek_after_close():
im = Image.open("Tests/images/apng/delay.png")
im.seek(1)
Expand Down
23 changes: 23 additions & 0 deletions Tests/test_file_gif.py
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,29 @@ def test_roundtrip_save_all_1(tmp_path):
assert reloaded.getpixel((0, 0)) == 255


def test_save_all_progress():
out = BytesIO()
progress = []

def callback(filename, frame_number, n_frames):
progress.append((filename, frame_number, n_frames))

Image.new("RGB", (1, 1)).save(out, "GIF", save_all=True, progress=callback)
assert progress == [(0, 1, 1)]

out = BytesIO()
progress = []

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

expected = [(0, 1, 32)]
for i in range(31):
expected.append((1, i + 2, 32))
assert progress == expected


@pytest.mark.parametrize(
"path, mode",
(
Expand Down
25 changes: 25 additions & 0 deletions Tests/test_file_mpo.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,3 +278,28 @@ def test_save_all():
# Test that a single frame image will not be saved as an MPO
jpg = roundtrip(im, save_all=True)
assert "mp" not in jpg.info


def test_save_all_progress():
out = BytesIO()
progress = []

def callback(filename, frame_number, n_frames):
progress.append((filename, frame_number, n_frames))

Image.new("RGB", (1, 1)).save(out, "MPO", save_all=True, progress=callback)
assert progress == [(0, 1, 1)]

out = BytesIO()
progress = []

with Image.open("Tests/images/sugarshack.mpo") as im:
with Image.open("Tests/images/frozenpond.mpo") as im2:
im.save(out, "MPO", save_all=True, append_images=[im2], progress=callback)

assert progress == [
(0, 1, 4),
(0, 2, 4),
(1, 3, 4),
(1, 4, 4),
]
31 changes: 28 additions & 3 deletions Tests/test_file_pdf.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import io
import os
import os.path
import tempfile
import time
from io import BytesIO

import pytest

Expand Down Expand Up @@ -169,6 +169,31 @@ def im_generator(ims):
assert os.path.getsize(outfile) > 0


def test_save_all_progress():
out = BytesIO()
progress = []

def callback(filename, frame_number, n_frames):
progress.append((filename, frame_number, n_frames))

Image.new("RGB", (1, 1)).save(out, "PDF", save_all=True, progress=callback)
assert progress == [(0, 1, 1)]

out = BytesIO()
progress = []

with Image.open("Tests/images/sugarshack.mpo") as im:
with Image.open("Tests/images/frozenpond.mpo") as im2:
im.save(out, "PDF", save_all=True, append_images=[im2], progress=callback)

assert progress == [
(0, 1, 4),
(0, 2, 4),
(1, 3, 4),
(1, 4, 4),
]


def test_multiframe_normal_save(tmp_path):
# Test saving a multiframe image without save_all
with Image.open("Tests/images/dispose_bgnd.gif") as im:
Expand Down Expand Up @@ -323,12 +348,12 @@ def test_pdf_info(tmp_path):

def test_pdf_append_to_bytesio():
im = hopper("RGB")
f = io.BytesIO()
f = BytesIO()
im.save(f, format="PDF")
initial_size = len(f.getvalue())
assert initial_size > 0
im = hopper("P")
f = io.BytesIO(f.getvalue())
f = BytesIO(f.getvalue())
im.save(f, format="PDF", append=True)
assert len(f.getvalue()) > initial_size

Expand Down
28 changes: 27 additions & 1 deletion Tests/test_file_tiff.py
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ def test_palette(self, mode, tmp_path):
with Image.open(outfile) as reloaded:
assert_image_equal(im.convert("RGB"), reloaded.convert("RGB"))

def test_tiff_save_all(self):
def test_save_all(self):
mp = BytesIO()
with Image.open("Tests/images/multipage.tiff") as im:
im.save(mp, format="tiff", save_all=True)
Expand Down Expand Up @@ -688,6 +688,32 @@ def im_generator(ims):
with Image.open(mp) as reread:
assert reread.n_frames == 3

def test_save_all_progress(self):
out = BytesIO()
progress = []

def callback(filename, frame_number, n_frames):
progress.append((filename, frame_number, n_frames))

Image.new("RGB", (1, 1)).save(out, "TIFF", save_all=True, progress=callback)
assert progress == [(0, 1, 1)]

out = BytesIO()
progress = []

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

assert progress == [
(0, 1, 4),
(1, 2, 4),
(1, 3, 4),
(1, 4, 4),
]

def test_saving_icc_profile(self, tmp_path):
# Tests saving TIFF with icc_profile set.
# At the time of writing this will only work for non-compressed tiffs
Expand Down
30 changes: 27 additions & 3 deletions Tests/test_file_webp.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import io
import re
import sys
import warnings
from io import BytesIO

import pytest

Expand Down Expand Up @@ -102,10 +102,10 @@ def test_write_rgb(self, tmp_path):
def test_write_method(self, tmp_path):
self._roundtrip(tmp_path, self.rgb_mode, 12.0, {"method": 6})

buffer_no_args = io.BytesIO()
buffer_no_args = BytesIO()
hopper().save(buffer_no_args, format="WEBP")

buffer_method = io.BytesIO()
buffer_method = BytesIO()
hopper().save(buffer_method, format="WEBP", method=6)
assert buffer_no_args.getbuffer() != buffer_method.getbuffer()

Expand All @@ -122,6 +122,30 @@ def test_save_all(self, tmp_path):
reloaded.seek(1)
assert_image_similar(im2, reloaded, 1)

@skip_unless_feature("webp_anim")
def test_save_all_progress(self):
out = BytesIO()
progress = []

def callback(filename, frame_number, n_frames):
progress.append((filename, frame_number, n_frames))

Image.new("RGB", (1, 1)).save(out, "WEBP", save_all=True, progress=callback)
assert progress == [(0, 1, 1)]

out = BytesIO()
progress = []

with Image.open("Tests/images/iss634.webp") as im:
im2 = Image.new("RGB", im.size)
im.save(out, "WEBP", save_all=True, append_images=[im2], progress=callback)

expected = []
for i in range(42):
expected.append((0, i + 1, 43))
expected.append((1, 43, 43))
assert progress == expected

def test_icc_profile(self, tmp_path):
self._roundtrip(tmp_path, self.rgb_mode, 12.5, {"icc_profile": None})
if _webp.HAVE_WEBPANIM:
Expand Down
14 changes: 12 additions & 2 deletions src/PIL/GifImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
# See the README file for information on usage and redistribution.
#

import itertools
import math
import os
import subprocess
Expand Down Expand Up @@ -578,10 +577,17 @@ def _write_multiple_frames(im, fp, palette):
duration = im.encoderinfo.get("duration")
disposal = im.encoderinfo.get("disposal", im.info.get("disposal"))

progress = im.encoderinfo.get("progress")
imSequences = [im] + list(im.encoderinfo.get("append_images", []))
if progress:
n_frames = 0
for imSequence in imSequences:
n_frames += getattr(imSequence, "n_frames", 1)

im_frames = []
frame_count = 0
background_im = None
for imSequence in itertools.chain([im], im.encoderinfo.get("append_images", [])):
for i, imSequence in enumerate(imSequences):
for im_frame in ImageSequence.Iterator(imSequence):
# a copy is required here since seek can still mutate the image
im_frame = _normalize_mode(im_frame.copy())
Expand Down Expand Up @@ -611,6 +617,8 @@ def _write_multiple_frames(im, fp, palette):
# This frame is identical to the previous frame
if encoderinfo.get("duration"):
previous["encoderinfo"]["duration"] += encoderinfo["duration"]
if progress:
progress(i, frame_count, n_frames)
continue
if encoderinfo.get("disposal") == 2:
if background_im is None:
Expand All @@ -624,6 +632,8 @@ def _write_multiple_frames(im, fp, palette):
else:
bbox = None
im_frames.append({"im": im_frame, "bbox": bbox, "encoderinfo": encoderinfo})
if progress:
progress(i, frame_count, n_frames)

if len(im_frames) > 1:
for frame_data in im_frames:
Expand Down
15 changes: 13 additions & 2 deletions src/PIL/MpoImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
# See the README file for information on usage and redistribution.
#

import itertools
import os
import struct

Expand All @@ -42,6 +41,7 @@ def _save(im, fp, filename):


def _save_all(im, fp, filename):
progress = im.encoderinfo.get("progress")
append_images = im.encoderinfo.get("append_images", [])
if not append_images:
try:
Expand All @@ -50,11 +50,19 @@ def _save_all(im, fp, filename):
animated = False
if not animated:
_save(im, fp, filename)
if progress:
progress(0, 1, 1)
return

mpf_offset = 28
offsets = []
for imSequence in itertools.chain([im], append_images):
imSequences = [im] + list(append_images)
if progress:
frame_number = 0
n_frames = 0
for imSequence in imSequences:
n_frames += getattr(imSequence, "n_frames", 1)
for i, imSequence in enumerate(imSequences):
for im_frame in ImageSequence.Iterator(imSequence):
if not offsets:
# APP2 marker
Expand All @@ -73,6 +81,9 @@ def _save_all(im, fp, filename):
else:
im_frame.save(fp, "JPEG")
offsets.append(fp.tell() - offsets[-1])
if progress:
frame_number += 1
progress(i, frame_number, n_frames)

ifd = TiffImagePlugin.ImageFileDirectory_v2()
ifd[0xB000] = b"0100"
Expand Down
5 changes: 4 additions & 1 deletion src/PIL/PdfImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,9 @@ def _save(im, fp, filename, save_all=False):
# catalog and list of pages
existing_pdf.write_catalog()

progress = im.encoderinfo.get("progress")
page_number = 0
for im_sequence in ims:
for i, im_sequence in enumerate(ims):
im_pages = ImageSequence.Iterator(im_sequence) if save_all else [im_sequence]
for im in im_pages:
image_ref, procset = _write_image(im, filename, existing_pdf, image_refs)
Expand Down Expand Up @@ -281,6 +282,8 @@ def _save(im, fp, filename, save_all=False):
existing_pdf.write_obj(contents_refs[page_number], stream=page_contents)

page_number += 1
if progress:
progress(i, page_number, number_of_pages)

#
# trailer
Expand Down
Loading