Return devices #58

johns10 · 2024-06-08T03:02:18Z

I want to ship this in a desktop app. So, I'd like to call Membrane.PortAudio.Devices.list() and get back a list of devices so the user can select one.

I implemented it by adding a device spec and iterating over the DeviceInfo structs to create devices. I then return those in a list.

This is my first crack at NIF's so be gentle.

thbar · 2024-06-09T09:13:21Z

I was about to start a PR on exactly this, since I have the exact same need (I'm capturing sound cards audio, which can get plugged in / out, not on the same port etc).

Thanks for your work!

For history context:

c_src/membrane_portaudio_plugin/pa_devices.c

mat-hek · 2024-06-10T11:43:20Z

c_src/membrane_portaudio_plugin/pa_devices.spec.exs

@@ -1,3 +1,12 @@
 module Membrane.PortAudio.Devices

-spec list() :: :ok
+type(
+  device :: %Device{


Suggested change

device :: %Device{

device :: %Membrane.PortAudio.Device{

Also, let's create a corresponding struct in a Membrane.PortAudio.Device module.

mat-hek · 2024-06-10T11:52:50Z

c_src/membrane_portaudio_plugin/pa_devices.spec.exs

+    max_output_channels: int
+  }
+)
+


Suggested change

max_output_channels: int

}

)

max_output_channels: int,

default_device: default_device

}

)

type(default_device :: false | :input | :output)

Then you can use that like device.device_default = DEVICE_DEFAULT_INPUT; in C

I had added is_default, will move to this instead.

mat-hek · 2024-06-10T11:53:27Z

c_src/membrane_portaudio_plugin/pa_devices.spec.exs

+  }
+)
+
+spec list() :: {:ok :: label, devices :: [device]}


Suggested change

spec list() :: {:ok :: label, devices :: [device]}

spec list() :: (devices :: [device])

johns10 · 2024-06-11T13:32:11Z

I'll hit these changes shortly.

johns10 · 2024-06-15T22:01:14Z

Thanks for considering my changes, and your valuable feedback.

I implemented everything you've recommended.

I do question the way we are handling returns.
There are some error cases here.
I don't know Unifex enough to understand how and when to return the ok and error tuples.

Since I removed the :ok from the spec, I can no longer use return_list_ok.
Of course, we can just return empty lists in our error case.
Would it be better for us to return {:ok, devices} or {:error, "Pa_CountDevices returned #{numDevices"} to retain the semantics of the module?

Given my limited experience with Unifex, it's not obvious how to do this, so I just accepted your proposal.
If you think this would be beneficial, I can attempt a refactor.
Your input on syntax would be very helpful.

Additionally, I was seeing intermittent segmentation faults in dev, so I changed the way I was assigning names to dynamically allocate memory on the device, and then copy the value from deviceInfo. I hope that resolves the segmentation fault, but for the safety of your users I should probably kick this around on my local a bit before you merge this PR.

Maybe give it a closer eye and make sure I haven't done anything grotesque with memory management? I'm a bit green with C.

mat-hek · 2024-06-17T08:14:15Z

Hi @johns10, basically, the label in .spec.exs maps to the result function name. Have you seen https://hexdocs.pm/unifex/Unifex.Specs.DSL.html? There's also https://hexdocs.pm/unifex/creating_unifex_natives.html. Let me know if we can improve the docs somehow to make it clearer.

Regarding this particular error, you're right, we need to do something about it. If we're about to return an error, it should be somehow structured, for example {:error, {:init, error_no}} and then we should leave the :ok as well. However, I'd rather raise with unifex_raise(env, description).

mat-hek · 2024-06-17T08:56:06Z

c_src/membrane_portaudio_plugin/pa_devices.c

  Pa_Initialize();
  int numDevices = Pa_GetDeviceCount();
-  if (numDevices < 0) {
+  device devices[numDevices];


What if numDevices is negative? :P Also, I'd allocate this on the heap just in case numDevices is big for some reason

mat-hek · 2024-06-17T09:02:38Z

c_src/membrane_portaudio_plugin/pa_devices.c

+      devices[i].name = malloc(strlen(device_info->name) + 1);
+      if (device_info->name != NULL)
+      {
+        strcpy(devices[i].name, device_info->name);
+      }


If you allocate anything on the heap, you should free it. In this case, after the call to list_result. However, unifex does malloc and memcpy when converting to an erlang term, so it seems redundant.

mat-hek · 2024-06-17T09:05:12Z

lib/membrane_portaudio_plugin/device.ex

@@ -0,0 +1,10 @@
+defmodule Membrane.PortAudio.Device do


a piece of doc would be great

mat-hek · 2024-06-17T09:08:52Z

lib/membrane_portaudio.ex

  def print_devices() do
    Application.ensure_all_started(:membrane_portaudio_plugin)
+
    __MODULE__.SyncExecutor.apply(__MODULE__.Devices, :list, [])
+    |> IO.inspect()


Let's add list_devices/0 here too, the existing one is in a private module and not guarded with the sync executor

* Return devices * Fix typo * add default * PR comments * whoops, missed id

* Return devices (#58) * Return devices * Fix typo * add default * PR comments * whoops, missed id * format pa_devices.c * improvements & fixes for pa_devices * update deps * bump version to 0.19.3 * make credo happy, update deps --------- Co-authored-by: John Davenport <[email protected]>

Return devices

c209d1b

johns10 closed this Jun 8, 2024

johns10 reopened this Jun 8, 2024

johns10 added 2 commits June 7, 2024 23:06

Fix typo

3ceaf9d

add default

a68826b

mat-hek self-requested a review June 10, 2024 09:59

mat-hek self-assigned this Jun 10, 2024

mat-hek requested changes Jun 10, 2024

View reviewed changes

PR comments

4e0ffe8

mat-hek requested changes Jun 17, 2024

View reviewed changes

whoops, missed id

38716ed

mat-hek changed the base branch from master to list_devices July 10, 2024 16:36

mat-hek merged commit 83654d6 into membraneframework:list_devices Jul 10, 2024
1 of 3 checks passed

mat-hek mentioned this pull request Jul 10, 2024

List devices #60

Merged

mat-hek pushed a commit that referenced this pull request Jul 11, 2024

Return devices (#58)

da6745a

* Return devices * Fix typo * add default * PR comments * whoops, missed id

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return devices #58

Return devices #58

johns10 commented Jun 8, 2024

thbar commented Jun 9, 2024

mat-hek Jun 10, 2024 •

edited

Loading

mat-hek Jun 10, 2024

johns10 Jun 15, 2024

mat-hek Jun 10, 2024

johns10 commented Jun 11, 2024

johns10 commented Jun 15, 2024 •

edited

Loading

mat-hek commented Jun 17, 2024

mat-hek Jun 17, 2024

mat-hek Jun 17, 2024

mat-hek Jun 17, 2024

mat-hek Jun 17, 2024

-    max_output_channels: int
-  }
-)
+    max_output_channels: int,
+    default_device: default_device
+  }
+)
+type(default_device :: false | :input | :output)

	spec list() :: {:ok :: label, devices :: [device]}
	spec list() :: (devices :: [device])

Return devices #58

Return devices #58

Conversation

johns10 commented Jun 8, 2024

thbar commented Jun 9, 2024

mat-hek Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

mat-hek Jun 10, 2024

Choose a reason for hiding this comment

johns10 Jun 15, 2024

Choose a reason for hiding this comment

mat-hek Jun 10, 2024

Choose a reason for hiding this comment

johns10 commented Jun 11, 2024

johns10 commented Jun 15, 2024 • edited Loading

mat-hek commented Jun 17, 2024

mat-hek Jun 17, 2024

Choose a reason for hiding this comment

mat-hek Jun 17, 2024

Choose a reason for hiding this comment

mat-hek Jun 17, 2024

Choose a reason for hiding this comment

mat-hek Jun 17, 2024

Choose a reason for hiding this comment

mat-hek Jun 10, 2024 •

edited

Loading

johns10 commented Jun 15, 2024 •

edited

Loading