Audio: initial interface #394

2022-07-07T20:18:57Z

alichraghi commented

2022-07-07 20:18:57 +00:00

(Migrated from github.com)

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

- [x] By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

iddev5 (Migrated from github.com) requested changes

2022-07-08 07:18:37 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

This is a bad idea. This should be strictly kept inside soundio.zig's Device struct

audio/src/soundio.zig

					
				@ -0,0 +1,159 @@

				const std = @import("std");

Naming is incorrect. These are not device but actually streams

Why are we storing so much information with id. DeviceDescriptor already has is_raw and mode fields.

Lets not store it, we should get the device only in requestDevice as mentioned above.

Lets not store it, we should get the device only in ``requestDevice`` as mentioned above.

iddev5 commented

2022-07-08 07:20:14 +00:00

(Migrated from github.com)

Lets not forget to mention that this PR depends on https://github.com/slimsag/libsoundio/pull/2 and also https://github.com/hexops/soundio needs to be updated accordingly.

iddev5 commented

2022-07-08 07:22:56 +00:00

(Migrated from github.com)

I think the purpose of Descriptor should be just to be able to store all the necessary information about a device which we can use to initialize it. It should not store the device handle itself. That would be a bad design. We already have soundio.get(*)DeviceFromID which just needs Descriptor.id and Descriptor.is_raw.

I think the purpose of Descriptor should be just to be able to store all the necessary information about a device which we can use to initialize it. It should not store the device handle itself. That would be a bad design. We already have ``soundio.get(*)DeviceFromID`` which just needs Descriptor.id and Descriptor.is_raw.

👍 1

alichraghi (Migrated from github.com) reviewed

2022-07-08 08:02:57 +00:00

audio/src/soundio.zig

					
				@ -0,0 +1,159 @@

				const std = @import("std");

If the same physical device supports both input and output, that makes
one SoundIoDevice for the input and one SoundIoDevice for the output.
In this case, the id of each SoundIoDevice will be the same, and
SoundIoDevice::aim will be different. Additionally, if the device
supports raw mode, there may be up to four devices with the same id:
one for each value of SoundIoDevice::is_raw and one for each value of
SoundIoDevice::aim.

if user save id somewhere and try to use it again there may be more devices.
also has cross-platform reasons. there might be no is_raw field in Web and Android backends so user get confused why he got error in desktop platform but not in other platforms

> If the same physical device supports both input and output, that makes one SoundIoDevice for the input and one SoundIoDevice for the output. In this case, the id of each SoundIoDevice will be the same, and SoundIoDevice::aim will be different. Additionally, if the device supports raw mode, there may be up to four devices with the same id: one for each value of SoundIoDevice::is_raw and one for each value of SoundIoDevice::aim. if user save `id` somewhere and try to use it again there may be more devices. also has cross-platform reasons. there might be no `is_raw` field in *Web* and *Android* backends so user get confused why he got error in desktop platform but not in other platforms

alichraghi (Migrated from github.com) reviewed

2022-07-08 08:37:54 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

in fact there's no need to move it into somewhere else
i was also doubt to keep it, just waited for reviews

in fact there's no need to move it into somewhere else i was also doubt to keep it, just waited for reviews

alichraghi (Migrated from github.com) reviewed

2022-07-08 08:44:18 +00:00

audio/src/soundio.zig

					
				@ -0,0 +1,159 @@

				const std = @import("std");

so if internal and parseID get removed, which is correct strategy?

InvalidParams error, if id exist but mode and is_raw was null
automatically connect to best device, if id exist but mode and is_raw was null

so if `internal` and `parseID` get removed, which is correct strategy? - `InvalidParams` error, if `id` exist but `mode` and `is_raw` was **null** - automatically connect to best device, if `id` exist but `mode` and `is_raw` was **null**

iddev5 (Migrated from github.com) reviewed

2022-07-08 09:58:04 +00:00

audio/src/soundio.zig

					
				@ -0,0 +1,159 @@

				const std = @import("std");

I think we had discussed that we may want to allow device creation just from id, and if I understand it correctly id doesn't have enough information to uniquely identify the devices. So that does justifies what you did above. But it has its problems.

The id held by soundio we actually get it from the internal audio server itself, so if we made id a custom format, it would be difficult to use the id in future non-soundio native backends.

We can in theory make the id format a standard which all backends must adhere to but it has the drawback that id would have to be allocated. Alternatively we can just not allow creating device solely from id but require the complete descriptor. cc: @slimsag

I think we had discussed that we may want to allow device creation just from id, and if I understand it correctly id doesn't have enough information to uniquely identify the devices. So that does justifies what you did above. But it has its problems. The id held by soundio we actually get it from the internal audio server itself, so if we made id a custom format, it would be difficult to use the id in future non-soundio native backends. We can in theory make the id format a standard which all backends must adhere to but it has the drawback that id would have to be allocated. Alternatively we can just not allow creating device solely from id but require the complete descriptor. cc: @slimsag

emidoots (Migrated from github.com) reviewed

2022-07-09 01:29:46 +00:00

audio/src/soundio.zig

					
				@ -0,0 +1,159 @@

				const std = @import("std");

The id held by soundio we actually get it from the internal audio server itself, so if we made id a custom format, it would be difficult to use the id in future non-soundio native backends.

I'm not worried about this. In my view, ID should be clearly marked as "a string of garbage only this backend understands" - nobody except that backend should try to interpret it, and there could be changes to the backend (like if we ever move away from soundio) that invalidate those IDs. It's safe to store it in a file and get the device from that ID, but only using the same backend - and not if the backend changes in the future.

> The id held by soundio we actually get it from the internal audio server itself, so if we made id a custom format, it would be difficult to use the id in future non-soundio native backends. I'm not worried about this. In my view, ID should be clearly marked as "a string of garbage only this backend understands" - nobody except that backend should try to interpret it, and there could be changes to the backend (like if we ever move away from soundio) that invalidate those IDs. It's safe to store it in a file and get the device from that ID, but only using the same backend - and not if the backend changes in the future.

emidoots (Migrated from github.com) reviewed

2022-07-09 01:38:32 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

It's a bummer we need to free each ID, though, I don't like that much

alichraghi (Migrated from github.com) reviewed

2022-07-09 07:27:24 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

relax. this happens once game load

iddev5 (Migrated from github.com) reviewed

2022-07-09 07:42:04 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

How do you know the user would do it only once? Well you cant say it surely.

alichraghi (Migrated from github.com) reviewed

2022-07-09 07:55:16 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

it's user fault and beyond of our control. user can save to an array and use it where needed
at least in mach/src/audio we surely do it once

it's user fault and beyond of our control. user can save to an array and use it where needed at least in `mach/src/audio` we surely do it once

iddev5 (Migrated from github.com) reviewed

2022-07-09 08:01:04 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

User cannot really store it since devices may change as the program is running, which is triggered by flushEvents() in my understanding

User cannot really store it since devices may change as the program is running, which is triggered by ``flushEvents()`` in my understanding

alichraghi (Migrated from github.com) reviewed

2022-07-09 08:53:50 +00:00

audio/src/main.zig

					
				@ -0,0 +1,140 @@

				const std = @import("std");

hmm right. but this seems the only way to keep id standalone. im almost ok with making it user job to save is_raw and mode by itself but it seems other backends provide a unique id.

hmm right. but this seems the only way to keep id standalone. im almost ok with making it user job to save `is_raw` and `mode` by itself but it seems other backends provide a unique id.

alichraghi commented

2022-07-09 16:30:30 +00:00

(Migrated from github.com)

will be ready once this merged: https://github.com/hexops/soundio/pull/1

iddev5 (Migrated from github.com) approved these changes

2022-07-09 17:56:05 +00:00

iddev5 (Migrated from github.com) left a comment

LGTM! Techincally Device and DeviceIterator should not be completely opaque but we can always fix that later along with that id allocation issue.

For now lets get something going atleast.

LGTM! Techincally Device and DeviceIterator should not be completely opaque but we can always fix that later along with that id allocation issue. For now lets get something going atleast.

emidoots (Migrated from github.com) reviewed

2022-07-09 18:13:59 +00:00

audio/src/main.zig

					
				@ -0,0 +90,4 @@

				    var iter = a.outputDeviceIterator();

				    var device_desc = (try iter.next()) orelse return error.NoDeviceFound;

				    const d = try a.requestDevice(device_desc);

My thinking (let's chat about this in Matrix):

We should create a new descriptor here, passing just .{.id = device_desc.id, .mode = .output} to requestDevice. Otherwise we're not sure if we are testing it looking up device by ID or by full descriptor info.

Additionally, we should create another test which verifies what happens when the ID doesn't match any known device. My thinking here is that if you pass .{.id = "does-not-match-any-existing-device-id", .mode = .output} for a reason like:

The user literally doesn't have that device anymore (they unplugged it)
We switched from soundio to a non-soundio backend and the ID format changed
...

Then requestDevice could see clearly that the device with that ID no longer exists, and instead give you one that you would get if you just passed requestDevice(.{.mode = .output}) without the ID field.

My thinking (let's chat about this in Matrix): We should create a new descriptor here, passing just `.{.id = device_desc.id, .mode = .output}` to `requestDevice`. Otherwise we're not sure if we are testing it looking up device by ID or by full descriptor info. Additionally, we should create another test which verifies what happens when the ID doesn't match any known device. My thinking here is that if you pass `.{.id = "does-not-match-any-existing-device-id", .mode = .output}` for a reason like: * The user literally doesn't have that device anymore (they unplugged it) * We switched from soundio to a non-soundio backend and the ID format changed * ... Then `requestDevice` could see clearly that the device with that ID no longer exists, and instead give you one that you would get if you just passed `requestDevice(.{.mode = .output})` without the ID field.