libvips / pyvips

python binding for libvips using cffi
MIT License
642 stars 49 forks source link

Progress callbacks? #98

Closed erdmann closed 4 years ago

erdmann commented 5 years ago

Hi John,

I see that it's possible to set the VIPS_PROGRESS environment variable to enable some kind of feedback during long-running operations, but I'm curious whether there exists a functionality in pyvips whereby I can register a callback function similar to what appears here: https://github.com/kleisauke/net-vips/issues/31. In addition to enabling customized display of the progress (for example, using tqdm or tqdm_notebook within Jupyter), it would presumably also enable a means to allow the user to cancel an operation like in nip2.

Many thanks in advance for any tips you can provide.

jcupitt commented 5 years ago

It's not possible in pyvips right now, but it wouldn't be hard to add. Let's tag this as an enhancement.

erdmann commented 5 years ago

That would be great. If I can help, please let me know.

kleisauke commented 5 years ago

There's a work in progress branch here (commit https://github.com/libvips/pyvips/commit/1d8b4f3574e56eb0a61797e2824f9361a5d54bf6). I guess it's already working.

Perhaps the callback here: https://github.com/libvips/pyvips/blob/1d8b4f3574e56eb0a61797e2824f9361a5d54bf6/pyvips/gobject.py#L13

Should be switched to "new style callback" in API mode, just like: https://github.com/libvips/pyvips/blob/f4d9334d2e3085b4b058129f14ac17a7872b109b/pyvips/__init__.py#L131-L133 https://github.com/libvips/pyvips/blob/f4d9334d2e3085b4b058129f14ac17a7872b109b/pyvips/decls.py#L50-L51 Because the original callbacks are slower to invoke and have the same issue as libffi's callbacks. See the warning on the CFFI site.

jcupitt commented 5 years ago

Oh heh I'd forgotten about that, thanks Kleis. Yes, it'll be a bit slow as written, though I don't know if it'd be significant.

jcupitt commented 5 years ago

I think they've make their warning about ffi.callback() stronger since I last read it. OK, we should use extern Python instead.

We'll need to check threading too: The callbacks from signal_connect can come from another thread. Hopefully the GIL will make this safe, but it would need testing.

jcupitt commented 5 years ago

I fixed it up a bit. This test program:

#!/usr/bin/python3

import sys
import pyvips

def preeval_cb(image, progress):
    print('preeval:')

def eval_cb(image, progress):
    print('{}%, eta {}s'.format(progress.percent, progress.eta))

def posteval_cb(image, progress):
    print('posteval:')

image = pyvips.Image.new_from_file(sys.argv[1], access='sequential')
image.set_progress(True)
image.signal_connect('preeval', preeval_cb)
image.signal_connect('eval', eval_cb)
image.signal_connect('posteval', posteval_cb)
image.write_to_file(sys.argv[2]) 

Does this:

john@yingna ~/try $ ./signal.py ~/pics/k2.jpg x.jpg
preeval:
7%, eta 0s
7%, eta 0s
8%, eta 0s
9%, eta 0s
10%, eta 0s
10%, eta 0s
11%, eta 0s
12%, eta 0s
13%, eta 0s
14%, eta 0s
14%, eta 0s
Segmentation fault (core dumped)

I'm not quite sure why it segvs, probably just after the first GC. It needs swapping to new-style callbacks.

jcupitt commented 5 years ago

I found the segv: there was a ref missing. Fixed in https://github.com/libvips/pyvips/commit/74ca396500a0832f1de77588f10ae845b28e82c7

jcupitt commented 4 years ago

I added kill support too. Here's a better test program:

#!/usr/bin/python3

import pyvips

def print_progress(progress):
    print(f'run = {progress.run}, eta = {progress.eta}, '
        f'percent = {progress.percent} tpels = {progress.tpels}, '
        f'npels = {progress.npels}')

def preeval_cb(image, progress):
    print('preeval:')
    print_progress( progress)

def eval_cb(image, progress):
    print('eval:')
    print_progress( progress)
    image.set_kill(True)

def posteval_cb(image, progress):
    print('posteval:')
    print_progress( progress)

image = pyvips.Image.black(10, 10000)
image.set_progress(True)
image.signal_connect('preeval', preeval_cb)
image.signal_connect('eval', eval_cb)
image.signal_connect('posteval', posteval_cb)
image.copy_memory()

If you run this, you'll see:

$ ./signal.py
preeval:
run = 0, eta = 0, percent = 0 tpels = 100000, npels = 0
eval:
run = 0, eta = 0, percent = 0 tpels = 100000, npels = 320
eval:
run = 0, eta = 0, percent = 1 tpels = 100000, npels = 1280
eval:
run = 0, eta = 0, percent = 1 tpels = 100000, npels = 1280
eval:
run = 0, eta = 0, percent = 1 tpels = 100000, npels = 1440
posteval:
run = 0, eta = 0, percent = 1 tpels = 100000, npels = 1440
Traceback (most recent call last):
  File "./signal.py", line 29, in <module>
    image.copy_memory()
  File "/home/john/GIT/pyvips/pyvips/vimage.py", line 490, in copy_memory
    raise Error('unable to copy to memory')
pyvips.error.Error: unable to copy to memory
  VipsImage: killed for image "temp-0"
VipsImage: killed for image "temp-0"
VipsImage: killed for image "temp-0"

Comment out the kill line and it'll run all the way through. You should be able to see the various fields in progress ticking up.

This will be in 2.1.10, due pretty soon now, hopefully.

jcupitt commented 4 years ago

I should have said, this feature is in pyvips 2.1.9, which should be out soon. Thanks for suggesting ti!

You can see the API here:

https://github.com/libvips/pyvips/blob/master/tests/test_progress.py

I'll add some more docs before release.

lipanchine commented 4 years ago

Hi John,

Is there any progress on this issue? I'm using pyvips with Flask to tile image, but _image. signal_connect seems not thread safe, preeval and posteval_ thread can't be released correctly after a request is finished.

Many thanks in advance for any tips you can provide.

jcupitt commented 4 years ago

Hello, as far as I know this is all released and working. If you have an example that fails, please open a new issue.