Skip to content
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

Importing miniaudio.h library results in dependency loop error #19392

Open
kavika13 opened this issue Mar 22, 2024 · 5 comments
Open

Importing miniaudio.h library results in dependency loop error #19392

kavika13 opened this issue Mar 22, 2024 · 5 comments
Labels
bug Observed behavior contradicts documented or intended behavior translate-c C to Zig source translation feature (@cImport)
Milestone

Comments

@kavika13
Copy link
Contributor

Zig Version

0.12.0-dev.3403+b5cef9e8b

Steps to Reproduce and Observed Behavior

  1. zig init

  2. git clone https://github.com/mackron/miniaudio

  3. edit build.zig, and add these lines just after the const exe = ... declaration:

    exe.addIncludePath(.{ .path = "miniaudio" });
    exe.linkLibC();
  1. edit src/main.zig, and delete all the code but the const std = @import("std"); line.

  2. add the following additional code code:

const miniaudio = @cImport({
    @cDefine("MINIAUDIO_IMPLEMENTATION", {});
    @cInclude("miniaudio.h");
});

fn data_callback(_: *ma_device, _: *void, _: *const void, _: u32) void {}

pub fn main() !void {
    var config: miniaudio.ma_device_config = miniaudio.ma_device_config_init(miniaudio.ma_device_type_playback);
    config.playback.format = miniaudio.ma_format_f32;
    config.playback.channels = 2;
    config.sampleRate = 48000;
    config.dataCallback = data_callback;
}
  1. Run zig build

Behavior:

<srcroot>\zig-cache\o\3ce1300da0f0bde9e958a78552702100\cimport.zig:118:5: error: dependency loop detected
pub const ma_device_data_proc = ?*const fn ([*c]ma_device, ?*anyopaque, ?*const anyopaque, ma_uint32) callconv(.C) void;

Expected Behavior

I expected the program to compile successfully, or at least get further.
There may be other errors to fix in the zig code, but the error in the C import would go away.

@kavika13 kavika13 added the bug Observed behavior contradicts documented or intended behavior label Mar 22, 2024
@kavika13
Copy link
Contributor Author

kavika13 commented Mar 22, 2024

I stripped the miniaudio.h header down as much as I could, and here's zig and C code that replicate the problem about as minimally as I could manage:

const std = @import("std");
const miniaudio = @cImport({
    @cInclude("repro.h");
});

pub fn main() !void {
    miniaudio.ma_device_config_init();
}
typedef struct ma_device ma_device;

typedef void (* ma_device_data_proc)(struct ma_device* pDevice);

struct ma_device_config
{
    ma_device_data_proc dataCallback;
};

struct ma_device
{
    ma_device_data_proc onData;
};

void ma_device_config_init(void) {
    struct ma_device_config val;
}

The generated zig code from cimport.zig looks likes this:

pub const ma_device_data_proc = ?*const fn ([*c]struct_ma_device) callconv(.C) void;
pub const struct_ma_device = extern struct {
    onData: ma_device_data_proc = @import("std").mem.zeroes(ma_device_data_proc),
};
pub const ma_device = struct_ma_device;
pub const struct_ma_device_config = extern struct {
    dataCallback: ma_device_data_proc = @import("std").mem.zeroes(ma_device_data_proc),
};
pub export fn ma_device_config_init() void {
    var val: struct_ma_device_config = undefined;
    _ = &val;
}

@xdBronch
Copy link
Contributor

i think this looks like #12325

@odecaux
Copy link

odecaux commented Mar 22, 2024

I've had a similar issue using miniaudio in the past, but it got fixed by #17692, according to @kcbanner

@andrewrk andrewrk added the translate-c C to Zig source translation feature (@cImport) label Mar 28, 2024
@andrewrk andrewrk added this to the 0.15.0 milestone Mar 28, 2024
sphaerophoria added a commit to sphaerophoria/video-editor that referenced this issue May 13, 2024
Implement audio playback, hopefully motivation is self explanatory

The audio landscape is vast in linux, we have alsa, pulse, jack,
pipewire and alsa, pulse, jack interfaces into pipewire, alsa interfaces
into pulse, etc. Instead of trying to deal with this ourselves, pull in
an audio lib to deal with it for us

Just skimming online, miniaudio fits well. PortAudio and libsoundio also
seem to both be good candidates. No strong reasoning went into this
choice, the single header impl of miniaudio felt easy to work with.

We were tricked though, with the default miniaudio impl we hit a zig
compiler bug that triggers circular dependencies. Something about
using a struct pointer in a function pointer stored by the struct, see
ziglang/zig#18247 (comment)

Patch miniaudio to just use void pointers instead, this doesn't seem to
have any negative effects

Add an audio player abstraction that just injects a sin wave for now,
add test binary to try it

Potentially related issues...
ziglang/zig#12325
ziglang/zig#16419
ziglang/zig#19392
@sphaerophoria
Copy link

If it's helpful, I've got this to work by just patching miniaudio to replace ma_device* function arguments with void* function arguments.

--- miniaudio.h 2024-05-09 15:09:33.966673423 -0700
+++ miniaudio.h_        2024-05-09 15:03:58.688300510 -0700
@@ -6785,7 +6785,7 @@
 due to things like an incoming phone call. Currently this is only implemented on iOS. None of the
 Android backends will report this notification.
 */
-typedef void (* ma_device_notification_proc)(const ma_device_notification* pNotification);
+typedef void (* ma_device_notification_proc)(const void* pNotification);


 /*
@@ -6828,7 +6828,7 @@

 The proper way to stop the device is to call `ma_device_stop()` from a different thread, normally the main application thread.
 */
-typedef void (* ma_device_data_proc)(ma_device* pDevice, void* pOutput, const void* pInput, ma_uint32 frameCount);
+typedef void (* ma_device_data_proc)(void* pDevice, void* pOutput, const void* pInput, ma_uint32 frameCount);



@@ -6852,7 +6852,7 @@
 -------
 Do not restart or uninitialize the device from the callback.
 */
-typedef void (* ma_stop_proc)(ma_device* pDevice);  /* DEPRECATED. Use ma_device_notification_proc instead. */
+typedef void (* ma_stop_proc)(void* pDevice);  /* DEPRECATED. Use ma_device_notification_proc instead. */

 typedef enum
 {
@@ -16187,7 +16187,7 @@
 static ma_result ma_mutex_init__posix(ma_mutex* pMutex)
 {
     int result;
-
+
     if (pMutex == NULL) {
         return MA_INVALID_ARGS;
     }

sphaerophoria added a commit to sphaerophoria/video-editor that referenced this issue May 14, 2024
Implement audio playback, hopefully motivation is self explanatory

The audio landscape is vast in linux, we have alsa, pulse, jack,
pipewire and alsa, pulse, jack interfaces into pipewire, alsa interfaces
into pulse, etc. Instead of trying to deal with this ourselves, pull in
an audio lib to deal with it for us

Just skimming online, miniaudio fits well. PortAudio and libsoundio also
seem to both be good candidates. No strong reasoning went into this
choice, the single header impl of miniaudio felt easy to work with.

We were tricked though, with the default miniaudio impl we hit a zig
compiler bug that triggers circular dependencies. Something about
using a struct pointer in a function pointer stored by the struct, see
ziglang/zig#18247 (comment)

Patch miniaudio to just use void pointers instead, this doesn't seem to
have any negative effects

Extract audio frames from ffmepg, and feed them into our audio
subsystem. Not a ton to say here, everything is just using the APIs
provided by miniaudio and ffmpeg

Replace test video with a 20s segment of big buck bunny

Potentially related issues...
ziglang/zig#12325
ziglang/zig#16419
ziglang/zig#19392
@kavika13
Copy link
Contributor Author

kavika13 commented Nov 19, 2024

I've dug in a bit more.
I triggered this bug with code generated by TranslateC, but it appears to me the TranslateC step itself is succeeding, and the code in the cache looks just fine. I don't think this bug should necessarily be tagged TranslateC.

This is definitely just a special case of #12325

I've had a similar issue using miniaudio in the past, but it got fixed by #17692, according to @kcbanner

I've verified, and this bug still persists. It's not fixed by that other fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

No branches or pull requests

5 participants