Skip to content

Add data_ptr method to Vips::Image #418

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Apr 11, 2025

Hi, this PR adds support for vips_image_get_data to Ruby with a new data_ptr method that returns a sized FFI::Pointer. This provides similar functionality as write_to_memory but without additional copying.

With the current implementation, it would be the user's responsibility to ensure the pointer has not been invalidated (but we could add a reference to the pointer to ensure the image is not garbage collected while the pointer is active - I'm not sure if there are other operations that invalidate the pointer).

Ref: FFI enum docs

@jcupitt
Copy link
Member

jcupitt commented Apr 12, 2025

Hi @ankane, thanks for this!

Unfortunately, vips_image_get_data() is not thread-safe -- it transforms the argument into a memory image, and if some other thread is using the same image for something else (images can become shared between threads by the operation cache), you'll see a crash.

https://www.libvips.org/API/current/libvips-header.html#vips-image-get-data

The threadsafe equivalent is vips_image_copy_memory(). This makes a new memory image from an existing image, and, if possible, does it with just a pointer-copy. This is exposed in ruby-vips as Image#copy_memory:

https://libvips.github.io/ruby-vips/Vips/Image.html#copy_memory-instance_method

https://www.libvips.org/API/current/VipsImage.html#vips-image-copy-memory

@ankane
Copy link
Contributor Author

ankane commented Apr 12, 2025

Thanks @jcupitt. It sounds like the thread-safe way to get a read-only pointer with minimal copying is to combine the two.

copy = image.copy_memory
read_ptr = copy.data_ptr

What do you think about having a read_ptr method for this (see updated code)?

@jcupitt
Copy link
Member

jcupitt commented Apr 12, 2025

Yes, that's better. Having unsafe_data_ptr in the public API makes me very uneasy -- it's likely to cause mysterious random crashes. Maybe just have read_ptr and hide the unsafe code in there?

What happens if the caller slices the result from read_ptr? Would the hidden reference to the underlying image be lost? It still sounds unsafe, though perhaps everyone who wants a FFI pointer will know that! Maybe a note in the docs stressing the danger would be useful too.

@ankane
Copy link
Contributor Author

ankane commented Apr 12, 2025

Sounds good. Calling ptr.slice seems to retain the reference (otherwise, adding GC.stress = true to the test case should fail).

@jcupitt
Copy link
Member

jcupitt commented Apr 13, 2025

Great! Please add a note to the changelog (and credit yourself!), and a note to the docs saying this is potentially a very expensive operation.

Thank you for doing this work!

@ankane
Copy link
Contributor Author

ankane commented Apr 13, 2025

Thanks @jcupitt. Re expensive: I feel like I'm missing something about how libvips works that may make this PR not very useful. Both write_to_memory and read_ptr produce the same result.

image = Vips::Image.new_from_file("image.png")
ptr = image.read_ptr
p image.write_to_memory == ptr.read_string(ptr.size) # => true

However, write_to_memory is cheaper (both with and without a previous operation).

require "benchmark/ips"
require "ruby-vips"

Benchmark.ips do |x|
  x.report("write_to_memory") do
    image = Vips::Image.new_from_file("image.png")
    image.write_to_memory
  end
  x.report("read_ptr") do
    image = Vips::Image.new_from_file("image.png")
    image.read_ptr
  end
end

Output

ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +PRISM [arm64-darwin24]
Warming up --------------------------------------
     write_to_memory    52.000 i/100ms
            read_ptr     7.000 i/100ms
Calculating -------------------------------------
     write_to_memory    521.467 (± 4.4%) i/s    (1.92 ms/i) -      2.652k in   5.096587s
            read_ptr     75.914 (±26.3%) i/s   (13.17 ms/i) -    371.000 in   5.046573s

I'd expect them to be roughly the same when the image pixels aren't already loaded (since they produce the same result), and I'd expect read_ptr to be faster when the pixels are loaded (since it could return the pointer instead of copying). Do you know why this isn't the case?

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2025

You're right, I'd expect copy_memory and write_to_memory to be similar, but copy_memory is a lot slower. I'll have a look.

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2025

I made a small C prog for testing:

// compile with
//   gcc -g -Wall copywrite.c `pkg-config vips --cflags --libs`

#include <vips/vips.h>

int
main(int argc, char **argv)
{   
    VIPS_INIT(argv[0]);

    // 2.33s, 75mb
    for (int i = 0; i < 1000; i++) {
        VipsImage *image = vips_image_new_from_file(argv[1], NULL);
        size_t size = VIPS_IMAGE_SIZEOF_IMAGE(image);
        void *buf = g_try_malloc(size);
        VipsImage *x = vips_image_new_from_memory(buf, size,
            image->Xsize, image->Ysize, image->Bands, image->BandFmt);
        vips_image_write(image, x);
        g_object_unref(x);
        g_free(buf);
        g_object_unref(image);
    }

    // 2.93s, 75mb
    for (int i = 0; i < 1000; i++) {
        VipsImage *image = vips_image_new_from_file(argv[1], NULL);
        
        VipsImage *x = vips_image_new_memory();
        vips_image_write(image, x);
        g_object_unref(x);
        g_object_unref(image);
    }

    // 2.33s, 75mb
    for (int i = 0; i < 1000; i++) {
        VipsImage *image = vips_image_new_from_file(argv[1], NULL);
        void *buf = vips_image_write_to_memory(image, NULL);

        g_free(buf);
        g_object_unref(image);
    }
    
    return 0;
}

The middle one is what copy_memory does and is 2.93s (on this PC and with a 1450 x 2048 pixel JPG). The top one is the same process but doing the malloc ourselves, and runs 20% faster, which puzzles me.

Anyway, the difference is relatively small, and not the huge difference you are seeing, which means it must be an inefficiency in ruby-vips.

The same program in ruby-vips is:

#!/usr/bin/env ruby

require "ruby-vips"

1000.times do |i|
  puts "loop #{i} ..."

  image = Vips::Image.new_from_file(ARGV[0])
  # 4.0s, 200mb
  #image.write_to_memory

  # 18.9s, 2.1gb
  image.copy_memory
end

So copy_memory is incredibly slow and needs 10x the memory.

Looking at the code, it's just:

    def copy_memory
      new_image = Vips.vips_image_copy_memory self
      Vips::Image.new new_image
    end

Which (I think?) should be OK. I'll keep digging.

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2025

Changing copy_memory to be:

    def copy_memory
      new_image = Vips.vips_image_copy_memory self
      ::GObject.g_object_unref new_image
    end

Obviously won't work, haha, but it gets the time down to 3.2s.

If I make it:

    def copy_memory
      Vips::Image.new_from_memory self.write_to_memory, 
          self.width, self.height, self.bands, self.format
    end

I see 4.0s, 240mb, so the same as write_to_memory.

I'm not sure why Vips::Image.new is so slow, it's just wrapping a pointer. I don't suppose you have any thoughts?

@ankane
Copy link
Contributor Author

ankane commented Apr 14, 2025

Sampling the following program on Mac arm64:

require "ruby-vips"

loop do
  image = Vips::Image.new_from_file("image.png")
  image.copy_memory
end

shows it's spending most of the time (~95%) on vips_sink_memory in libvips. Out of ~2150 samples:

2049 ffi_call_SYSV  (in ffi_c.bundle) + 80
  2041 vips_image_copy_memory  (in libvips.42.dylib) + 164
  ! 2041 vips_image_write  (in libvips.42.dylib) + 100
  !   2015 vips_image_generate  (in libvips.42.dylib) + 420
  !   : 2015 vips_sink_memory  (in libvips.42.dylib) + 248
  !   :   1750 vips_threadpool_run  (in libvips.42.dylib) + 328

@jcupitt
Copy link
Member

jcupitt commented Apr 15, 2025

That's very strange. I'll try sysprof here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants