magickload should have a format hint parameter about pyvips HOT 21 CLOSED

Hello @Flips01 ,

Huh that's weird. It seems to be just .ico files -- with this test program:

#!/usr/bin/env python

import sys
import pyvips

with open(sys.argv[1], "rb") as f:
    buf = f.read()

img = pyvips.Image.magickload_buffer(buf)

print "width = ", img.width;
print "height = ", img.height;

(you can call the ImageMagick load-from-buffer operation directly. Don't give new_from_buffer a * for the second argument, just leave it empty)

I see:

john@kiwi:~/try$ ./buffer.py ~/pics/k2.jpg
width =  1450
height =  2048
john@kiwi:~/try$ ./buffer.py ~/pics/k2.tif 
width =  1450
height =  2048
john@kiwi:~/try$ ./buffer.py ~/pics/k2.gif 
width =  1450
height =  2048
john@kiwi:~/try$ ./buffer.py ~/pics/k2.pdf 
width =  1450
height =  2048
john@kiwi:~/try$ ./buffer.py ~/pics/favicon.ico 
Traceback (most recent call last):
  File "./buffer.py", line 9, in <module>
    img = pyvips.Image.new_from_buffer(buf, '')
  File "/home/john/.local/lib/python2.7/site-packages/pyvips/vimage.py", line 247, in new_from_buffer
    raise Error('unable to load from buffer')
pyvips.error.Error: unable to load from buffer
  VipsForeignLoad: buffer is not in a known format

So I guess it's probably an imagemagick bug.

from pyvips.

I'll see if I can find any other formats it fails for.

from pyvips.

I tried this:

#!/usr/bin/env python

import os
import sys
import pyvips

for filename in sys.argv[1:]:
    if not os.path.isfile(filename):
        continue
    if os.path.getsize(filename) > 10000000:
        continue

    try:
        with open(filename, "rb") as f:
            buf = f.read()

        img = pyvips.Image.magickload_buffer(buf)
    except:
        # now try loading directly from the file
        try:
            img = pyvips.Image.new_from_file(filename)
            print "failed for buffer, succeeded for file:", filename
        except:
            pass

And on my image collection I got (removing format duplicates):

john@kiwi:~/try$ ./buffer.py ~/pics/*
failed for buffer, succeeded for file: /home/john/pics/1.webp
failed for buffer, succeeded for file: /home/john/pics/3x3.v
failed for buffer, succeeded for file: /home/john/pics/4way3.mor
failed for buffer, succeeded for file: /home/john/pics/avg.mat
failed for buffer, succeeded for file: /home/john/pics/blur3x3.con
failed for buffer, succeeded for file: /home/john/pics/cielab-dagams.tiff
failed for buffer, succeeded for file: /home/john/pics/DSCN3778.mov
failed for buffer, succeeded for file: /home/john/pics/favicon.ico
failed for buffer, succeeded for file: /home/john/pics/hixxxcs.mp4
failed for buffer, succeeded for file: /home/john/pics/johnski.avi
failed for buffer, succeeded for file: /home/john/pics/noheader.svg
failed for buffer, succeeded for file: /home/john/pics/redsquare.svgz
failed for buffer, succeeded for file: /home/john/pics/sRGB_IEC61966-2-1_black_scaled.icc
failed for buffer, succeeded for file: /home/john/pics/wagon.csv

So it looks like there are quite a few minor formats where imagemagick is unable to load from a buffer but can load from a file.

from pyvips.

... I had a dig inside ImageMagick and the icon.c coder does not have a format sniffer. Things like JPEG, for example, have a function called IsJPEG which can spot JPEG objects:

https://github.com/ImageMagick/ImageMagick/blob/master/coders/jpeg.c#L1584

But icon.c just has load and save methods:

https://github.com/ImageMagick/ImageMagick/blob/master/coders/icon.c#L784

So to get the .ico loader to trigger you need to signal the format explicitly, I guess by setting the format hint to ICO.

from pyvips.

Could you provide an example for using the format hint? At least the following attempt doesn‘t work:

pyvips.Image.new_from_buffer(buf, ‘‘, format=‘ICO‘)

from pyvips.

Sorry, format hints are not in the libvips API, they would need to be added. Let's tag this as an enhancement.

from pyvips.

I had a moment so I hacked a format option in. I now see:

$ cp favicon.ico x
$ vipsheader x
vipsheader: VipsForeignLoad: "x" is not a known file format
$ vips magickload x x.png
magick2vips: unable to read file "x"
libMagick error: no decode delegate for this image format `' @ error/constitute.c/ReadImage/504 (null)
$ vips magickload x x.png --format=ICO
$ eog x.png

And I can view the .ico file. You should be able to give format="ICO" to new_from_buffer too.

Would you be able to test it? You'd need to build git master libvips, unfortunately.

from pyvips.

It doesn't seem to work. I've build libvips from master:

philip@philip-VirtualBox:~$ cp favicon.ico x
philip@philip-VirtualBox:~$ vips magickload x x.png
magickload: Magick: no decode delegate for this image format `' @ error/constitute.c/ReadImage/509 (null)
philip@philip-VirtualBox:~$ vips magickload x x.png --format=ICO
philip@philip-VirtualBox:~$

Using the format parameter in pyvips doesn't make any difference:

Traceback (most recent call last):
  File "test.py", line 10, in <module>
    buffer_image = pyvips.Image.new_from_buffer(buf, "", format="ICO")
  File "/home/philip/.local/lib/python2.7/site-packages/pyvips/vimage.py", line 247, in new_from_buffer
    raise Error('unable to load from buffer')
pyvips.error.Error: unable to load from buffer
  VipsForeignLoad: buffer is not in a known format

Also I've noticed that I'll get an exception when I use the format parameter for an JPEG image:

pyvips.error.Error: VipsForeignLoadJpegBuffer does not support argument format

I would assume that a hint is more of a fallback option: When I supply an JPEG image and give an ICO-hint it should still be capable to handle it correctly. It already seems to work this way since it selects the correct loader but it should gracefully ignore the format parameter shouldn't it?

from pyvips.

Sorry, you're right, you'll have to call the imagemagick buffer loader directly. This seems to work:

$ python
Python 2.7.14 (default, Sep 23 2017, 22:06:14) 
[GCC 7.2.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyvips
>>> with open("favicon.ico", "rb") as f:
...     buf = f.read()
... 
>>> x = pyvips.Image.magickload_buffer(buf, format="ICO")
>>> x.width
16

I should have tested it.

from pyvips.

# tmp file content = https://www.welt.de/favicon.ico
tmp_file = '/dev/shm/imo_3tP2LK'
image = Image.magickload(tmp_file, format='ico')

Seems to work with jcupitt/libvips@b7bac0d.

But hinting the thumbnail operator seems not to work (with the embedded string option: [format=ico]).

I've prepared a example python test here.
Expected output:

.ico file is readable
try to load it with thumbnail
Successfully load .ico with thumbnail

Current output:

.ico file is readable
try to load it with thumbnail
Failed to load .ico with thumbnail

from pyvips.

It seems to work for me. With your sample program I see:

john@yingna ~/pics $ python3 ~/try/ico.py 
.ico file is readable
try to load it with thumbnail
Successfully load .ico with thumbnail

with git master libvips.

from pyvips.

@jcupitt Try to rename the .ico file to a file without extension.

from pyvips.

Ah, of course, sorry. Yes, I see a failure here too, I'll investigate.

from pyvips.

You can see the problem here:

https://github.com/jcupitt/libvips/blob/master/libvips/foreign/foreign.c#L520

When searching for a loader, it strips off any filename options, then calls the is_a member for all load classes in priority order. So the is_a() member expects a plain filename.

The format option is odd because it can change whether a file is recognised or not. Most options just change the way the file is loaded.

A simple workaround would be to use magickload to load the ICO file, then to pass the image on to thumbnail_image. It's not very satisfactory to have a special path though :-(

Maybe a better fix would be to add our own is_ICO() test and call that as part of magickload_is_a()?

It's a shame IM does not have a complete set of file format sniffers.

from pyvips.

Thinking about this a bit more, adding our own sniffers is a much better idea than the format parameter. format is messy, inconsistent with the rest of the API, and unnecessary with a little extra sniffing ourselves.

Let's remove format and do this instead!

from pyvips.

OK, there's a branch which adds a ICO sniffer. It should be simple to add any more we need. It's just this function:

https://github.com/jcupitt/libvips/blob/remove-format/libvips/foreign/magick.c#L244

I now see:

john@yingna ~/pics $ file x
x: MS Windows icon resource - 3 icons, 16x16, 8 bits/pixel, 32x32, 8 bits/pixel
john@yingna ~/pics $ vipsthumbnail x
john@yingna ~/pics $ 

Much nicer.

from pyvips.

Indeed much nicer. Just tested it and it seems to work! I now see:

.ico file has loader
try to load it with new_from_file
Successfully load .ico with new_from_file. Loader: magickload
try to load it with thumbnail
Successfully load .ico with thumbnail. Loader: magickload

(with the edited readable.py script)

I'll let you know when more image formats needs to be sniffed. On libvips 8.6.4 we only had problems with .svg and .ico. See here for our hotfix.

The .svg issue was due to missing is_a support for the SVG file class loader (fixed with: https://github.com/jcupitt/libvips/pull/905) and the .ico issue seems to be fixed with the remove-format branch.

Can't wait for libvips 8.7. Let me know if anything needs to be tested prior to the release.

from pyvips.

That's great, thanks for testing, I'll merge to master and add a test for .ico.

Yes, 8.7 RSN, hopefully. It's very late already :(

from pyvips.

I put 8.7.0-rc1 up. I'll give it a week for testing, then let's make 8.7.0.

from pyvips.

Wow nice! Issue 703 (I'm not sure if this already has been resolved in master) and 879 on the libvips issue tracker can safely be moved to another milestone (just minor issues that I've encountered).

We would like to experiment with resizing animated GIF's some day (has no priority). For this thumbnail needs to support page_height rather than image height for many-page images (issue 839).

In my opinion you can release 8.7 as it is now.

from pyvips.

Ooop, forgot to bump the milestones. Thanks!

I'll push that lua-vips version too. I was waiting for something, but I forget what.

from pyvips.