Event loop + audio callback = segfault

[psqu]

Try to move mouse cursor over window. Always leads to crash (segfault 11, sometimes Bus error 10).
When you stop audio or remove event loop, everything works well.
When you switch to another app or do not emit any event, it works.
Tested on OSX 10.9, opam instalation of Ocaml 4.01.0, Tsdl 8.1, libsdl2.0.3 an libffi from macports.
Tested on MacBook Pro Retina and MacPro 2008 with usb audio device.

Compiled with: ocamlbuild -pkg tsdl audio.native

audio.ml:

open Tsdl
open Bigarray

let audio_freq    = 44100
let audio_samples = 4096
let time = ref 0

let audio_setup () =
  let audio_callback (output : (int32, int32_elt) Sdl.bigarray) =
    for i = 0 to ((Array1.dim output / 2) - 1) do
      let phase = ((float_of_int !time) /. (66100.0 +. 1000.0 *. sin (0.0001 *. (float_of_int !time)))) *. 3000.0 in
        let sample = Int32.of_float ((sin phase) *. 1073741823.0) in
          begin
            output.{ 2 * i     } <- sample;
            output.{ 2 * i + 1 } <- sample;
            time := !time + 1
          end
    done
  in
    let desired_audiospec = {
      Sdl.as_freq = audio_freq;
      Sdl.as_format = Sdl.Audio.s32;
      Sdl.as_channels = 2;
      Sdl.as_callback = Some (audio_callback);
      Sdl.as_samples = audio_samples;
      Sdl.as_silence = 0;
      Sdl.as_size = Int32.of_int (audio_samples * 4 (* bajty na próbkę *) * 2 (* kanały *));
      Sdl.as_ba_kind = int32; }
    in
    match Sdl.open_audio_device None false desired_audiospec 0 with
    | `Error _ -> Sdl.log "Can't open audio device"; exit 1
    | `Ok (device_id, _) ->
      device_id

let video_setup () =
  match Sdl.create_window ~w:640 ~h:480 "SDL Audio Test" Sdl.Window.opengl with
  | `Error e -> Sdl.log "Create window error: %s" e; exit 1
  | `Ok w -> w

let main () = match Sdl.init Sdl.Init.(audio + video) with
| `Error e -> Sdl.log "Init error: %s" e; exit 1
| `Ok () ->
  let window = video_setup () in
  let device_id = audio_setup () in
  let () = Sdl.pause_audio_device device_id false in
  let e = Sdl.Event.create () in
  let rec loop () = match Sdl.wait_event (Some e) with
  | `Error err -> Sdl.log "Could not wait event: %s" err; ()
  | `Ok () ->
      match Sdl.Event.(enum (get e typ)) with
      | `Quit ->
        Sdl.pause_audio_device device_id true;
        Sdl.destroy_window window;
        Sdl.quit()

      | _ ->
        loop ()
  in
    loop ()

let () = main ()

[dbuenzli]

Yes reproducible. I suspect the problem mentioned in this comment is happening. Sorry it seems I forgot to lift it up in the documentation.

@yallop You recently added changes to release the runtime lock in bindings. Is there some support for callbacks aswell ?

[yallop]

@dbuenzli: there's some support for (re-)acquiring the runtime lock in callbacks in yallop/ocaml-ctypes#171 which should be merged soon.

[dbuenzli]

7de867c tries to do the obvious by simply acquiring the runtime lock however the program still crashes (Sdl.wait_event should be changed to release the lock aswell, but this is not done yet).

Unclear whether this is ctypes fault or mine, but I don't have time to investigate this further at the moment. If anyone's interested at having a closer look the test case is now included in tsdl (I hope it's ok for you @psqu) and can be built and run with ./build test_audio.native && ./test_audio.native.

[tokenrove]

Is it possible that this is due to caml_c_thread_register not being called in the threads SDL creates for audio handling? I notice that ocaml-libuv tries to get around this by calling caml_c_thread_register every time in its callback.

[dbuenzli]

@tokenrove thanks for looking into that; that seems like a very plausible cause. OTOH it seems that SDL 2.0.4 which just got into RC1 introduces a way to queue audio buffers rather than use the callback so we may just aswell scrap the binding to the audiocallback API. What do you think ?

[tokenrove]

@dbuenzli I think that would be significantly less hassle. (This is also one of the reasons I tend to use SDL2_mixer when I can get away with it if I'm using SDL through an FFI; I see that the old OCamlSDL package had the same approach and doesn't even bother to provide the lower-level audio functionality.)

Of course if an elegant solution to yallop/ocaml-ctypes#269 comes up, that would solve it. I wonder if the SDL developers could be convinced to provide an extra hook into the SDL_Thread code that would allow us to call caml_c_thread_register once.

(FWIW, this morning I did try using the same approach as ocaml-libuv and started getting segfaults in caml_execute_signal instead; I haven't gone any deeper with it yet.)

[DemiMarie]

If this is a low-latency callback, then running OCaml code is a bad idea anyway – one is not allowed to allocate memory in such a callback, much less grab a global lock. Low latency audio will still require code in a systems language.

[dbuenzli]

If this is a low-latency callback, then running OCaml code is a bad idea anyway – one is not allowed to allocate memory in such a callback, much less grab a global lock.

It's still interesting to actually test these boundaries and challenge pre-conceptions, even if you end up failing. For one thing it is very easy to guarantee that OCaml code will not allocate in a given function, especially if you are only filling a bigarray buffer with samples.

[DemiMarie]

There is still the global lock, and the fact that one can't actually do much useful without allocating. OCaml is not designed for this job. Use a lower-level language.

[dbuenzli]

Use a lower-level language.

Yeah you are right JavaScript would actually be a better fit.

[dbuenzli]

and the fact that one can't actually do much useful without allocating

Btw. it's never a good idea to bound your views at the limits of your own creativity and knowledge.

[Armael]

BTW, on my computer, the test_audio.native test fails with:

test_audio.native: /auto/homes/apvg2/.opam/4.02.3/build/ctypes.0.4.1/src/ctypes-foreign-base/ffi_call_stubs.c:331: ctypes_call: Assertion `(((arg_ptr) & 1) == 0) && (((unsigned char *) (arg_ptr)) [-sizeof(value)]) == 252' failed.

[dbuenzli]

@yallop Do you have any idea about this assertion failure:

ffi_call_stubs.c:331: ctypes_call: Assertion `(((arg_ptr) & 1) == 0) && (((unsigned char *) (arg_ptr)) [-sizeof(value)]) == 252'

[tokenrove]

BTW, here is the change I was using that results in a segfault in caml_execute_signal instead, in case it gives anyone any ideas:

diff --git a/src/tsdl.ml b/src/tsdl.ml
index 7d5a6e8..fe6e7ec 100644
--- a/src/tsdl.ml
+++ b/src/tsdl.ml
@@ -4573,6 +4573,9 @@ let audio_spec_of_c c as_ba_kind =
   { as_freq; as_format; as_channels; as_silence; as_samples; as_size;
     as_ba_kind; as_callback; }

+external caml_c_thread_register : unit -> int = "caml_c_thread_register"
+external caml_c_thread_unregister : unit -> int = "caml_c_thread_unregister"
+
 let audio_spec_to_c a =
   let wrap_cb = match a.as_callback with
   | None -> None
@@ -4580,9 +4583,11 @@ let audio_spec_to_c a =
       let kind_bytes = ba_kind_byte_size a.as_ba_kind in
       let ba_ptr_typ = access_ptr_typ_of_ba_kind a.as_ba_kind in
       Some begin fun _ p len ->
+        ignore (caml_c_thread_register ());
         let p = coerce (ptr uint8_t) ba_ptr_typ p in
         let len = len / kind_bytes in
-        cb (bigarray_of_ptr array1 len a.as_ba_kind p)
+        cb (bigarray_of_ptr array1 len a.as_ba_kind p);
+        ignore (caml_c_thread_unregister ())
       end
   in
   let c = make audio_spec in

[dbuenzli]

So just talking to @yallop he mentions that the problem is certainly the fact that the OCaml runtime system is not aware of the audio thread.

So @tokenrove's intuition is the good one but doing this in the callback is unfortunately already too late.

Since we don't have a way of registering that thread with the runtime system using SDL's API. I'm simply going to remove that bit from the API until we can get something safe.

[dbuenzli]

Closing this, support for the audio callback was removed in 96143eb. We'll bind to audio buffers once SDL 2.0.4 is out.

[dbuenzli]

Even though audio buffers are certainly the way to go it would still be interesting to revert 96143eb and try with yallop/ocaml-ctypes#420

[yallop]

Let me know how it goes, if you do try it!

[dbuenzli]

@yallop it works !

[yallop]

Fantastic! Thanks for the update.