tell buffer and target savers the file format #2499

jcupitt · 2021-10-26T16:59:02Z

Currently, buffer and target savers are not told the format they should
write.

This is usually OK (the JPEG saver already knows it should write JPEG),
but some savers can write several formats, and these currently need an
extra parameter.

For example:

buf = x.write_to_buffer ".bmp", format: "bmp"

The first ".bmp" gets libvips to pick magicksave, the second
format: param is necessary to tell magicksave to write BMP.

This patch makes vips_image_write_to_buffer and
vips_image_write_to_target add an extra image metadata item called
format_string which contains the string used to pick the write format.

Savers can use this extra field (if present) to pick a default save
format. In this case, the magick saver will extract the "bmp".

See: libvips/ruby-vips#319

Currently, buffer and target savers are not told the format they should write. This is usually OK (the JPEG saver already knows it should write JPEG), but some savers can write several formats, and these currently need an extra parameter. For example: ```ruby buf = x.write_to_buffer ".bmp", format: "bmp" ``` The first ".bmp" gets libvips to pick magicksave, the second `format:` param is necessary to tell magicksave to write BMP. This patch makes `vips_image_write_to_buffer` and `vips_image_write_to_target` add an extra image metadata item called `format_string` which contains the string used to pick the write format. Savers can use this extra field (if present) to pick a default save format. In this case, the magick saver will extract the "bmp". See: libvips/ruby-vips#319

jcupitt · 2021-10-26T17:01:51Z

ruby-vips needs a small change:

diff --git a/lib/vips/image.rb b/lib/vips/image.rb
index 1b800d1..6e0be6b 100644
--- a/lib/vips/image.rb
+++ b/lib/vips/image.rb
@@ -626,7 +626,12 @@ module Vips
       saver = Vips.vips_foreign_find_save_buffer filename
       raise Vips::Error if saver.nil?
 
-      buffer = Vips::Operation.call saver, [self], opts, option_string
+      # note the format_string for savers
+      image = self.mutate do |x|
+        x.set_type! Vips::REFSTR_TYPE, "format_string", format_string
+      end
+
+      buffer = Vips::Operation.call saver, [image], opts, option_string
       raise Vips::Error if buffer.nil?
 
       write_gc

All language bindings (eg. net-vips, pyvips) will need something similar.

jcupitt · 2021-10-26T17:13:04Z

Test program to show the issue:

require 'vips'

x = Vips::Image.new_from_file ARGV[0]

%w(dz hdr jpc jpt jp2 j2c j2k webp).each do |format|
  begin
    buf = x.write_to_buffer ".#{format}"
    puts "#{format}: written #{buf.length} bytes"
  rescue Vips::Error
    puts "#{format}: buffer write not supported"
  end
end

# imagemagick-based savers need to be told the format to write twice :(
%w(bmp gif).each do |format|
  begin
    buf = x.write_to_buffer ".#{format}", format: format
    puts "#{format}: written #{buf.length} bytes"
  rescue Vips::Error
    puts "#{format}: buffer write not supported"
  end
end

# libheif buffer savers need to be told the format too
buf = x.write_to_buffer ".heif", compression: "hevc"
puts "heic: written #{buf.length} bytes"

buf = x.write_to_buffer ".heif", compression: "av1"
puts "avif: written #{buf.length} bytes"

Rationale

I tried a couple of other fixes, hoping to find some way to pass the format down to the saver without needing changes to bindings, but failed :( The closest was getting vips_filename_get_options() to paste in an extra argument called perhaps format containing the suffix of the filename part, but it seemed very fragile and ugly.

This needs changes to bindings, but the changes are simple and safe, and will be forwards and backwards compatible. And there's no need for extra secret args to the save operations.

TODO

heifsave needs this adding
ppmsave needs this too (eg. write to ".pfm" to save as float)
jp2ksave might need it, I'm unsure
pyvips, ruby-vips, the C++ binding and I guess netvips need this change

jcupitt · 2021-10-26T17:14:29Z

Any thoughts, @kleisauke? This would affect net-vips.

and a version bump see libvips/libvips#2499

see libvips/libvips#2499

and remove gif from the list of things that magick supports (since we have our own saver now)

jcupitt · 2021-10-27T14:22:18Z

Revised test prog:

#!/usr/bin/ruby

require 'vips'

x = Vips::Image.new_from_file ARGV[0]

# don't try bmp ... we don't have magicksave_target
%w(heif avif ppm pfm pgm).each do |format|
  target = Vips::Target.new_to_memory
  x.write_to_target target, ".#{format}"
  puts "#{format}: written #{target.get("blob").length} bytes"
end

%w(bmp heif avif ppm pfm pgm).each do |format|
  buf = x.write_to_buffer ".#{format}"
  puts "#{format}: written #{buf.length} bytes"
end

With all the changes I see:

$ ./anita.rb ~/pics/k2.jpg
heif: written 209233 bytes
avif: written 254465 bytes
ppm: written 8908859 bytes
pfm: written 35635258 bytes
pgm: written 2969659 bytes
bmp: written 8913034 bytes
heif: written 209233 bytes
avif: written 254465 bytes
ppm: written 8908859 bytes
pfm: written 35635258 bytes
pgm: written 2969659 bytes

So it's correctly picking the target format from the suffix.

jcupitt · 2021-10-27T14:28:59Z

Same for python:

#!/usr/bin/python3

import sys
import pyvips

x = pyvips.Image.new_from_file(sys.argv[1])

for format in ["heif", "avif", "ppm", "pfm", "pgm"]: 
    target = pyvips.Target.new_to_memory()
    x.write_to_target(target, f".{format}")
    print(f"{format}: written {len(target.get('blob'))} bytes")

for format in ["bmp", "heif", "avif", "ppm", "pfm", "pgm"]: 
    buf = x.write_to_buffer(f".{format}")
    print(f"{format}: written {len(buf)} bytes")

Giving:

$ ./anita.py ~/pics/k2.jpg
heif: written 209233 bytes
avif: written 254465 bytes
ppm: written 8908859 bytes
pfm: written 35635258 bytes
pgm: written 2969659 bytes
bmp: written 8913034 bytes
heif: written 209233 bytes
avif: written 254465 bytes
ppm: written 8908859 bytes
pfm: written 35635258 bytes
pgm: written 2969659 bytes

since we use kebab-case elsewhere for image metadata

jcupitt · 2021-10-27T14:43:42Z

OK, all done!

I switched it to format-string for the metadata item, since we use hyphen rather than underscore elsewhere for image metadata.

Seems to work.

kleisauke · 2021-10-27T20:48:47Z

The closest was getting vips_filename_get_options() to paste in an extra argument called perhaps format containing the suffix of the filename part, but it seemed very fragile and ugly.

FWIW, I managed to get this working with commit kleisauke@921922b. This would result in fewer checks within savers, and you can still write (for example):

$ vips magicksave x.jpg x.bmp --format gif
$ head -c 6 x.bmp
GIF89a

But indeed, it's a bit ugly since this option would also be present in all savers and loaders:

$ vips gifload | grep format
   format       - Format hint, input gchararray

I also thought about splitting these deviating savers into aliased GObject classes, for example:

vips_foreign_find_save( "x.avif" ) -> VipsForeignSaveAvifFile -> VipsForeignSaveHeifFile (@compression av1)
vips_foreign_find_save( "x.bmp" ) -> VipsForeignSaveBmpFile -> VipsForeignSaveMagickFile (@format bmp)
vips_foreign_find_save( "x.pgm" ) -> VipsForeignSavePgmFile -> VipsForeignSavePpmFile (@format pgm)
vips_foreign_find_save( "x.pbm" ) -> same as line above
vips_foreign_find_save( "x.pfm" ) -> VipsForeignSavePfmFile -> VipsForeignSavePpmFile (@format pfm)

But I'm not sure if this can be done without too much hassle.

See: #2499.

kleisauke · 2021-10-28T08:57:34Z

But I'm not sure if this can be done without too much hassle.

This can be done easily, see for example commit kleisauke@d415a9b. With that I see:

$ vips copy x.jpg x.avif
$ vips copy x.jpg x.heic
$ vipsheader x.avif -f heif-compression
av1
$ vipsheader x.heic -f heif-compression
hevc

I marked that subclass as "deprecated" to hide it from vips -l foreign | grep -i save | awk '{ print $1; }'.

jcupitt · 2021-10-28T09:08:10Z

I also thought about splitting these deviating savers into aliased GObject classes, for example:

Oh, that's a good idea. Though it would be very wordy (as you say).

I also thought of hiding the extra info inside the VipsTarget, but that would need changes to bindings as well, I think.

Something else I tried was changing vips_object_set_from_string() to take the full format spec, not just a list of options. It could check the object and see if it supported a format argument, then set that from the suffix. This would avoid the fragile trick of appending the format to the option list.

It would still need changes to all the bindings though :( And the binding changes wouldn't work with older libvipses, so bindings would need to see which libvips version they had and alter behaviour.

jcupitt · 2021-10-28T09:39:23Z

This can be done easily, see for example commit kleisauke/libvips@d415a9b. With that I see:

Oh nice! We'd need to make these I guess:

VipsForeignSaveAvifBuffer
VipsForeignSaveAvifTarget
VipsForeignSaveMagickBmpBuffer
VipsForeignSavePgmBuffer
VipsForeignSavePgmTarget
VipsForeignSavePfmBuffer
VipsForeignSavePfmTarget

So 7 new subclasses.

We could possibly skip the Buffer versions (though not for magick), so we'd only need four new subclasses. The C vips_image_write_to_buffer() tries the target API first, and only checks the buffer API if there's no target writer.

pyvips and ruby-vips don't yet have this logic, but they probably should.

See: #2499.

kleisauke · 2021-10-28T11:10:34Z

Commit kleisauke@9b33fb9 adds the remaining subclasses. There are now a total of new 5 saver subclasses:

VipsForeignSaveAvifTarget
VipsForeignSaveMagickBmpFile
VipsForeignSaveMagickBmpBuffer
VipsForeignSavePgmTarget
VipsForeignSavePfmTarget

vips_image_write_to_file() will also try the target API first, so we don't have to subclass the file versions (I think). Though, the bindings should probably also add this logic (as you say).

I've tested this with:

$ vips copy x.jpg x.avif
$ vips copy x.jpg x.heic
$ vips copy x.jpg x.ppm
$ vips copy x.jpg x.pgm
$ vips copy x.jpg x.pbm
$ vips copy x.jpg x.pfm
$ vips copy x.jpg x.bmp
$ vipsheader x.avif -f heif-compression
av1
$ vipsheader x.heic -f heif-compression
hevc
$ head -c 2 x.ppm
P6
$ head -c 2 x.pgm
P5
$ head -c 2 x.pbm
P5
$ head -c 2 x.pfm
PF
$ head -c 2 x.bmp
BM

kleisauke · 2021-10-28T11:17:48Z

Oh, CI is currently failing due to:

pyvips.error.Error: unable to make temp file
  VipsForeignSave: "/tmp/vips-33-1024954283.avif" is not a known file format

To ensure that these subclasses are also skipped in `cplusplus/gen-operators.py`.

let's get pyvips etc. to use the target API instead

kleisauke · 2021-10-28T12:37:58Z

Great! You might also be interested in these 2 commits:
kleisauke@5f6b13f
kleisauke@8508761

…uffer-save3

jcupitt · 2021-10-28T12:42:29Z

Sure, they look good.

See: libvips/libvips#2499.

kleisauke · 2021-10-28T13:43:27Z

Commit kleisauke/net-vips@f0b7ab9 implements the "try to save with the target API first" approach in net-vips, this seems to work for me.

While doing that, I noticed that sometimes the vips_error_freeze() / vips_error_thaw() combination is used and sometimes not, see for example:

libvips/libvips/iofuncs/image.c

Lines 2201 to 2203 in 1f321d3

    
           vips_error_freeze(); 
        
           operation_name = vips_foreign_find_load_source( source ); 
        
           vips_error_thaw();

libvips/libvips/iofuncs/image.c

Lines 2687 to 2691 in 1f321d3

    
           vips_error_freeze(); 
        
           operation_name = vips_foreign_find_save_target( filename ); 
        
           vips_error_thaw(); 
        
           if( operation_name ) {

libvips/libvips/iofuncs/image.c

Line 2753 in 1f321d3

if( (operation_name = vips_foreign_find_save_target( filename )) ) {

Perhaps this mechanism for hiding errors can be removed?

and fix up heif a bit too

in vips_image_write_to_buffer()

jcupitt · 2021-10-28T14:00:15Z

You're right that third one needs a freeze/thaw as well.

Freeze/thaw is ugly and stupid :( It would be better if vips_foreign_find_save_target() didn't log an error and worked more like a tester. But that would break API compat :(

A cleaner solution would be to make eg. vips_foreign_lookup_save_target(), make vips_foreign_find_save_target() just call that and set the error, and also tag it as deprecated.

write_to_buffer will now attempt to use the new Target API before falling back to the old buffer system. More savers support target than buffer now. See libvips/libvips#2499

jcupitt · 2021-10-28T14:31:48Z

https://github.com/libvips/ruby-vips/tree/revise-buffer-save4 adds target-first to ruby-vips. I'll have a look at py as well.

kleisauke · 2021-10-28T15:14:45Z

Nice! I mirrored some of that ruby-vips code with commit kleisauke/net-vips@b7d1fd6 in net-vips.

I think we are almost done, the only missing piece is that ppmsave currently misses this logic in vips_foreign_save_ppm_file_build:

	if( vips_iscasepostfix( file->filename, ".pbm" ) ||
		vips_iscasepostfix( file->filename, ".pgm" ) )
		VIPS_SETSTR( ppm->format, "pgm" );
	else if( vips_iscasepostfix( file->filename, ".pfm" ) )
		VIPS_SETSTR( ppm->format, "pfm" );

(Similar to the logic in heifsave)

see libvips/libvips#2499

jcupitt · 2021-10-28T15:17:24Z

OK, py done as well!

jcupitt · 2021-10-28T15:29:11Z

Oh you're right. Let's add an enum, all these string suffixes are a bit clunky.

@Format

and add a @Format param to ppmsave

jcupitt · 2021-10-28T16:18:54Z

It now supports PBM (bitmap format) too!

We'll need to regenerate the static parts of the bindings, and I guess eg. php-vips needs some work too, but that can all wait.

This is a nice API cleanup! And good job on the stub-subclass idea, it's much neater.

OK to merge?

kleisauke

I left a small comment regarding the .pbm file extension, but otherwise this looks good to me!

libvips/foreign/ppmsave.c

kleisauke

Oh, I just saw some potentially redundant code too.

libvips/foreign/vips2magick.c

Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>

Needed after PR libvips#2499.

Needed after PR #2499.

jcupitt added this to the 8.12 milestone Oct 26, 2021

jcupitt changed the title ~~tell buffer and target savers the file format~~ WIP: tell buffer and target savers the file format Oct 26, 2021

jcupitt marked this pull request as draft October 26, 2021 17:14

jcupitt added 2 commits October 27, 2021 13:49

heifsave tests for format_string

db7d320

ppm looks for the file suffix

b6823db

jcupitt added a commit to libvips/ruby-vips that referenced this pull request Oct 27, 2021

add support for format_string

b3ad0d6

and a version bump see libvips/libvips#2499

jcupitt added a commit to libvips/pyvips that referenced this pull request Oct 27, 2021

linked issue

4446e9b

see libvips/libvips#2499

jcupitt added 2 commits October 27, 2021 15:07

add ppmsave_buffer

00a4da6

fix up pfm save

7723652

and remove gif from the list of things that magick supports (since we have our own saver now)

jcupitt added 2 commits October 27, 2021 15:33

add support for format_string to the C++ binding

faf5405

switch to format-string

33e07ac

since we use kebab-case elsewhere for image metadata

jcupitt changed the title ~~WIP: tell buffer and target savers the file format~~ tell buffer and target savers the file format Oct 27, 2021

Set format hint in vips__filename_split8

921922b

Seems to work.

[WiP] Subclass savers that can store in multiple formats

d415a9b

See: #2499.

Finish implementation

9b33fb9

See: #2499.

kleisauke added 2 commits October 28, 2021 14:08

Set ->flags instead

5f6b13f

To ensure that these subclasses are also skipped in `cplusplus/gen-operators.py`.

Still check for pbm format

8508761

remove ppmsave_buffer

49a7709

let's get pyvips etc. to use the target API instead

jcupitt added 2 commits October 28, 2021 13:40

Merge commit '5f6b13f296a27a46aa6c92af2748ab47bf1fabf7' into revise-b…

7abfc86

…uffer-save3

Merge commit '850876147f6c05b2908f5bb4de1e8d92fcd4907e' into revise-b…

b7c51be

…uffer-save3

kleisauke added a commit to kleisauke/net-vips that referenced this pull request Oct 28, 2021

[WiP] Try to save with the target API first

f0b7ab9

See: libvips/libvips#2499.

heifsave to file supports all suffixes

19c6d25

jcupitt added 2 commits October 28, 2021 14:45

ppm/pfm/pgm target save sets the suffs

18be2e8

and fix up heif a bit too

hide a spurious error

ef8d8a2

in vips_image_write_to_buffer()

jcupitt added a commit to libvips/pyvips that referenced this pull request Oct 28, 2021

buffer save tries with the target API first

ac24957

see libvips/libvips#2499

add an enum for ppm format

9b182a0

and add a @Format param to ppmsave

fix compiler warning

7273232

kleisauke approved these changes Oct 28, 2021

View reviewed changes

libvips/foreign/ppmsave.c Outdated Show resolved Hide resolved

kleisauke reviewed Oct 28, 2021

View reviewed changes

libvips/foreign/vips2magick.c Outdated Show resolved Hide resolved

libvips/foreign/vips2magick.c Outdated Show resolved Hide resolved

libvips/foreign/vips2magick.c Outdated Show resolved Hide resolved

libvips/foreign/vips2magick.c Outdated Show resolved Hide resolved

jcupitt and others added 2 commits October 28, 2021 18:33

Update libvips/foreign/ppmsave.c

94ed22e

Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>

remove some unneeded code in magick

7c33d20

jcupitt marked this pull request as ready for review October 28, 2021 17:55

jcupitt merged commit 47383b5 into master Oct 28, 2021

jcupitt deleted the revise-buffer-save3 branch October 28, 2021 17:57

kleisauke added a commit to kleisauke/libvips that referenced this pull request Nov 2, 2021

Try the new target API first in VImage::write_to_buffer

d1ab1ec

Needed after PR libvips#2499.

kleisauke mentioned this pull request Nov 2, 2021

Try the new target API first in VImage::write_to_buffer #2511

Merged

jcupitt pushed a commit that referenced this pull request Nov 2, 2021

Try the new target API first in VImage::write_to_buffer (#2511)

fc92290

Needed after PR #2499.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tell buffer and target savers the file format #2499

tell buffer and target savers the file format #2499

jcupitt commented Oct 26, 2021

jcupitt commented Oct 26, 2021 •

edited

Loading

jcupitt commented Oct 26, 2021 •

edited

Loading

jcupitt commented Oct 26, 2021

jcupitt commented Oct 27, 2021

jcupitt commented Oct 27, 2021

jcupitt commented Oct 27, 2021

kleisauke commented Oct 27, 2021

kleisauke commented Oct 28, 2021

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021 •

edited

Loading

kleisauke commented Oct 28, 2021 •

edited

Loading

kleisauke commented Oct 28, 2021

kleisauke commented Oct 28, 2021

jcupitt commented Oct 28, 2021

kleisauke commented Oct 28, 2021 •

edited

Loading

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021

kleisauke commented Oct 28, 2021

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021 •

edited

Loading

kleisauke left a comment

kleisauke left a comment

tell buffer and target savers the file format #2499

tell buffer and target savers the file format #2499

Conversation

jcupitt commented Oct 26, 2021

jcupitt commented Oct 26, 2021 • edited Loading

jcupitt commented Oct 26, 2021 • edited Loading

Rationale

TODO

jcupitt commented Oct 26, 2021

jcupitt commented Oct 27, 2021

jcupitt commented Oct 27, 2021

jcupitt commented Oct 27, 2021

kleisauke commented Oct 27, 2021

kleisauke commented Oct 28, 2021

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021 • edited Loading

kleisauke commented Oct 28, 2021 • edited Loading

kleisauke commented Oct 28, 2021

kleisauke commented Oct 28, 2021

jcupitt commented Oct 28, 2021

kleisauke commented Oct 28, 2021 • edited Loading

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021

kleisauke commented Oct 28, 2021

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021

jcupitt commented Oct 28, 2021 • edited Loading

kleisauke left a comment

Choose a reason for hiding this comment

kleisauke left a comment

Choose a reason for hiding this comment

jcupitt commented Oct 26, 2021 •

edited

Loading

jcupitt commented Oct 26, 2021 •

edited

Loading

jcupitt commented Oct 28, 2021 •

edited

Loading

kleisauke commented Oct 28, 2021 •

edited

Loading

kleisauke commented Oct 28, 2021 •

edited

Loading

jcupitt commented Oct 28, 2021 •

edited

Loading