Chris00 / ocaml-cairo

Binding to Cairo, a 2D Vector Graphics Library.
GNU Lesser General Public License v3.0
54 stars 8 forks source link

Fatal error: exception Invalid_argument("Cairo.Image.get_data: not created from a bigarray") #28

Closed sanette closed 2 years ago

sanette commented 2 years ago

I get this error when the data was created by Tsdl.Sdl.lock_texture (which uses Ctypes.bigarray_of_ptr internally):

let (data : Cairo.Image.data8), stream_pitch =
      match Sdl.lock_texture stream None Bigarray.int8_unsigned with
      | Error (`Msg e) -> failwith e;
      | Ok r -> r in
let surface = Cairo.Image.(create_for_data8 data ARGB32) ~w ~h in
Cairo.Image.get_data8 surface

leads to

Fatal error: exception Invalid_argument("Cairo.Image.get_data: not created from a bigarray")

Maybe @dbuenzli has an idea?

dbuenzli commented 2 years ago

Maybe with a stack trace and an actual pointer to the code.

sanette commented 2 years ago

Hi @dbuenzli , many thanks for responding, here is a minimal code:

open Tsdl

let get_result = function
  | Error (`Msg e) -> failwith e
  | Ok r -> r

let bug () =
  let () = Sdl.init_sub_system Sdl.Init.video |> get_result in
  let w,h = 400,300 in
  let _win, renderer = Sdl.create_window_and_renderer ~w ~h Sdl.Window.windowed |> get_result in
  let format = Sdl.Pixel.format_argb8888 in
  let stream = Sdl.create_texture renderer format Sdl.Texture.access_streaming ~w ~h |> get_result in
  let data, _pitch = Sdl.lock_texture stream None Bigarray.int8_unsigned |> get_result in
  let surface = Cairo.Image.(create_for_data8 data ARGB32) ~w ~h in
  print_endline "Trying to get_data8:";
  Cairo.Image.get_data8 surface
  |> ignore

let () = bug ()

with dune file:

(executable
 (name bug)
 (libraries tsdl cairo2))

which I execute with dune exec ./bug.exe

dbuenzli commented 2 years ago

I meant where does it raises in Cairo ?

sanette commented 2 years ago

ah sorry, this is here https://github.com/Chris00/ocaml-cairo/blob/71a6dce91062f21d613f58ff4ef097d1fd3cff31/src/cairo_stubs.c#L1628

dbuenzli commented 2 years ago

So that proxy pointer is null. Could you check that the data value returned by Sdl.lock_texture is not bogus (e.g. dim 0) ? If not then the problem likely lies somewhere here (I don't have the whole bigarray proxy business in my head right now).

sanette commented 2 years ago

The data seems sound. I checked dim = 480000

sanette commented 2 years ago

mmm, I never wrote c stubs and the code there is quite hermetic to me :(

dbuenzli commented 2 years ago

The data seems sound. I checked dim = 480000

And if you access the array everything is fine ? (We just need to make sure the bigarray returned from tsdl is not the culprit)

sanette commented 2 years ago

yes, I tried

let x = ref 0 in
  for i = 0 to w*h*4 -1 do
    x := !x + Bigarray.Array1.get data i;
  done;
  Printf.printf "Sum = %i\n" !x;

and this runs fine and prints 0

sanette commented 2 years ago

on the other hand if instead of using Sdl.lock_texture I create a data with Bigarray.(Array1.create int8_unsigned c_layout (w*h*4)) then the bug disappears

sanette commented 2 years ago

mmm there is something fishy with bigarray_of_ptr. If I do the following stupid conversion:

  let data = Bigarray.(Array1.create int8_unsigned c_layout (w*h*4)) in
  let myptr = Ctypes.(bigarray_start array1 data) in
  let data = Ctypes.(bigarray_of_ptr array1 (w*h*4) Bigarray.int8_unsigned myptr) in

then the bug reappears!

sanette commented 2 years ago

So it seems that the problem has nothing to do with tsdl; here is a new minimal code that triggers the bug:

let bug2 () =
  let w, h = 300, 200 in
  let n = w*h*4 in
  let data = Bigarray.(Array1.create int8_unsigned c_layout n) in
  Bigarray.Array1.set data (n-1) 123;
  let myptr = Ctypes.(bigarray_start array1 data) in
  let data' = Ctypes.(bigarray_of_ptr array1 (w*h*4) Bigarray.int8_unsigned myptr) in
  assert (Bigarray.Array1.get data' (n-1) = 123);
  let surface = Cairo.Image.(create_for_data8 data' ARGB32) ~w ~h in
  print_endline "Trying to get_data8:";
  Cairo.Image.get_data8 surface
  |> ignore
sanette commented 2 years ago

my impression is that the new data' is sound and that ocaml-cairo detects a false positive.

Chris00 commented 2 years ago

I'll try to have a look ASAP but it will be Monday the earliest.

Chris00 commented 2 years ago

@sanette Isn't the above code unsafe (in a larger context)? Indeed, if data is reclaimed by the garbage collector, then data' will point to freed memory.

Chris00 commented 2 years ago

I've pushed a fix. May you try it and report? Thanks.

Chris00 commented 2 years ago

(I mean, on your application; on the test case above, it works.)

sanette commented 2 years ago

Thanks! in the meantime I finally stored the pointer to the data, as a workaround. I will try your fix when I find the time... (not very soon I suppose :-( )