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

mal_enumerate_devices alsa returns only null and pulse #2

Closed
VelvetyWhite opened this issue Nov 5, 2017 · 8 comments
Closed

mal_enumerate_devices alsa returns only null and pulse #2

VelvetyWhite opened this issue Nov 5, 2017 · 8 comments
Labels

Comments

@VelvetyWhite
Copy link

On Manjaro mal_enumerate_devices returns only null and pulse, I looked over your code and I understand what you want to achieve in mal_enumerate_devices__alsa the problem is that on my system this check if (colonPos == -1 || (colonPos == 2 && (NAME[0]=='h' && NAME[1]=='w'))) can only be true for null or pulse because these don't contain : and there's no entry starting with hw:something.
I wrote this function to check my entries:

int enumerateAlsaDevices()
{
    char **hints;
    /* Enumerate sound devices */
    int err = snd_device_name_hint(-1, "pcm", (void***)&hints);
    if (err != 0) return -1;//Error! Just return
    
    char** n = hints;
    while (*n != NULL)
    {
        char *name = snd_device_name_get_hint(*n, "NAME");
        char *desc = snd_device_name_get_hint(*n, "DESC");
        char *ioid = snd_device_name_get_hint(*n, "IOID");
        //printf("name len %lu, desc len %lu ioid len %lu\n", strlen(name), strlen(desc), strlen(ioid)); //There's no terminator...
    
        if (name != NULL && 0 != strcmp("null", name))
        {
            //Copy name to another buffer and then free it
            char hwID[16] = {'\0'};
            if(strcmp(name, "pulse") == 0)
            {
                sprintf(hwID, name);
                printf("Name: %s\nDescription: %s\nIOID: %s\nhwID: %s\n\n", name, desc, ioid, hwID);
            }
            else
            {
                char* aux = strstr(name, ":");
                if(aux != NULL)
                {
                    char* prop = aux + 1;
                    aux = strtok(prop, ",");
                    int cardIndex = snd_card_get_index(aux + 5);
                    aux = strtok(NULL, ",");
                    if(aux != NULL)
                    {
                        sprintf(hwID, "hw:%d,%s", cardIndex, aux + 4);
                    }
                    else
                    {
                        sprintf(hwID, "hw:%d,%d", cardIndex, 0);
                    }
                }
                printf("Name: %s\nDescription: %s\nIOID: %s\nhwID: %s\n\n", name, desc, ioid, hwID);
            }

            free(name);
            free(desc);
            free(ioid);
        }
        n++;
    }
    snd_device_name_free_hint((void**)hints);
    return 0;
}

and they look like this:

...
Name: sysdefault:CARD=U7
Description: Xonar U7, USB Audio
Default Audio Device
IOID: (null)
hwIDhw:1,0

Name: front:CARD=U7
Description: Xonar U7, USB Audio
Front speakers
IOID: (null)
hwIDhw:1,0

Name: surround21:CARD=U7
Description: Xonar U7, USB Audio
2.1 Surround output to Front and Subwoofer speakers
IOID: (null)
hwIDhw:1,0
...

Now I agree that it prints every kind of thing every card can do but there must be a middle ground so that you can enumerate and choose at run time what card you want to push sound through.
I am using it with your dr_flac.h and it's pretty solid other than this problem.

Versions

cat /proc/asound/version 
Advanced Linux Sound Architecture Driver Version k4.9.60-1-MANJARO.

aplay --version
aplay: version 1.1.4 by Jaroslav Kysela <[email protected]>

latest mini_al.h and dr_flac.h
@mackron
Copy link
Owner

mackron commented Nov 6, 2017

Thanks for your feedback on this. I'll get onto this later tonight when I get a chance.

@mackron
Copy link
Owner

mackron commented Nov 6, 2017

I've taken a look at this and I didn't realize how bad I was handling device enumeration! Take a look at branch "issue_2" for work on improving this. Still work in progress.

So far what I've done is basically just simplified the whole thing and return every device without trying to strip anything. The problem with this is that it's so unfriendly for the end user...

What do you think of the idea of giving applications the option to choose whether or not they want verbose device enumeration or compact enumeration? What I'm thinking of doing is adding a mal_context_config structure which is passed into mal_context_init() and would look something like this:

typedef struct
{
    mal_log_proc onLog;

    struct
    {
        mal_bool32 useVerbsoseDeviceEnumeration; // <-- Defaults to false.
    } alsa;
} mal_context_config;

When useVerboseDeviceEnumeration is set to true it will return every device that ALSA returns. When set to false it returns only unique devices based on card/dev pairs and uses "hw:%d,%d" when opening the device. Do you think that API design seems reasonable?

@VelvetyWhite
Copy link
Author

Now it's listing them all but there are still some problems.

int colonPos;
    src = mal_find_char(src, ':', &colonPos);
    if (src == NULL) {
        return -1;  // Couldn't find a colon
    }
    src++; //advance so we are on 'C' and not on colon

    char card[256];

    int commaPos;
    const char* dev = mal_find_char(src, ',', &commaPos);
    if (dev == NULL) {
        dev = "0";
        mal_strncpy_s(card, sizeof(card), src+5, (size_t)-1);
    } else {
        dev = dev + 5;
        mal_strncpy_s(card, sizeof(card), src+5, commaPos-5);
    }

    int cardIndex = ((mal_snd_card_get_index_proc)pContext->alsa.snd_card_get_index)(card);
    if (cardIndex < 0) {
        return -2;  // Failed to retrieve the card index.
    }

Here I had to advance the src pointer because it was on the colon and when it copied the card name mal_strncpy_s(card, sizeof(card), src+5, (size_t)-1); , src+5 was on = and it was also getting the = sign and sending it like this to snd_card_get_index the returning cardIndex was an error (For me the value of card was "=U7").
In mal_device_init__alsa

if (pDeviceID == NULL) {
        mal_strncpy_s(deviceName, sizeof(deviceName), "default", (size_t)-1);
    } else {
        if (!pConfig->alsa.preferPlugHW) {
            mal_strncpy_s(deviceName, sizeof(deviceName), pDeviceID->alsa, (size_t)-1);
        } else {
            // The client is preferencing a "plug" device, so we need to convert the device name to "plughw:%d,%d" format.
            deviceName[0] = 'p';
            deviceName[1] = 'l';
            deviceName[2] = 'u';
            deviceName[3] = 'g';
            if (mal_convert_device_name_to_hw_format__alsa(pContext, deviceName+4, sizeof(deviceName)-4, pDeviceID->alsa) != 0) {
                // Failed to convert to "hw:%d,%d" format. It could be set to "default", "pulse", "null", etc. This is not a critical error - just keep using the original name.
                mal_strncpy_s(deviceName, sizeof(deviceName), pDeviceID->alsa, (size_t)-1);
            }
        }
    }

if pDeviceID is not NULL and pConfig->alsa.preferPlugHW is 0 it will only copy that name from snd_device_name_get_hint(*n, "NAME"); which is in pDeviceID->alsa, never calling mal_convert_device_name_to_hw_format__alsa but it will call it if pConfig->alsa.preferPlugHW is true and then it proceeds to

snd_pcm_stream_t stream = (type == mal_device_type_playback) ? SND_PCM_STREAM_PLAYBACK : SND_PCM_STREAM_CAPTURE;
    if (((mal_snd_pcm_open_proc)pContext->alsa.snd_pcm_open)((snd_pcm_t**)&pDevice->alsa.pPCM, deviceName, stream, 0) < 0) {
        printf("Failed 1\n");
...

with that name which seems to work because I don't see that "Failed 1" printed but then I get this

flac: audio/test/mini_al/mini_al.h:2306: mal_device__read_frames_from_client: Assertion `frameCount > 0' failed.
Aborted (core dumped)

which seems related to issue 1 maybe? Because if I do config.alsa.preferPlugHW = 0;
it works.
However if I do this

char deviceName[256];
    if (pDeviceID == NULL) {
        mal_strncpy_s(deviceName, sizeof(deviceName), "default", (size_t)-1);
    } else {
        if (!pConfig->alsa.preferPlugHW) {
            printf("device id: %s\n", pDeviceID->alsa);
            mal_convert_device_name_to_hw_format__alsa(pContext, deviceName, sizeof(deviceName), pDeviceID->alsa);
            printf("device name: %s\n", deviceName);
            //mal_strncpy_s(deviceName, sizeof(deviceName), pDeviceID->alsa, (size_t)-1);
        } else {

when pDeviceID is not NULL and pConfig->alsa.preferPlugHW is 0 it will initialize directly with the id generated by mal_convert_device_name_to_hw_format__alsa and it will work even with config.alsa.preferPlugHW = 1;, but then pulse or default won't work for obvious reasons...
So, I think this initialization should somehow be reordered, try to use by default the hwid and treat pDeviceID ==NULL default and pulse as special cases.
That API design looks good and extensible to me but you know best, its your thing.

@mackron
Copy link
Owner

mackron commented Nov 7, 2017

Thanks for your feedback! Indeed, I didn't have the time to properly test that stuff last night. I've implemented bug fixes in the issue_2 branch.

The prioritization logic works in my head... Take the devices below as an example:

NAME: front:CARD=I82801AAICH,DEV=0 (hw:0,0)
NAME: surround21:CARD=I82801AAICH,DEV=0 (hw:0,0)

Note how these have the same "hw:0,0" ID.

If, for example, the end-user specifically wants to use the "front" device, wouldn't you want to try opening that specific device first, as the highest priority? If I were to change the priority so that the "hw:0,0" device is attempted first, wouldn't the user lose control of the specific device that's opened? How would it distinguish between the "front" device and the "surround21" device? My idea was to try the specific device selection first, and then fall back to the more general "hw:0,0" selection if that fails.

if pDeviceID is not NULL and pConfig->alsa.preferPlugHW is 0 it will only copy that name from snd_device_name_get_hint(*n, "NAME"); which is in pDeviceID->alsa, never calling mal_convert_device_name_to_hw_format__alsa

That's intended behaviour. My idea was to have it so the NAME property is used verbatim as the device identifier in snd_pcm_open(), then falling back to "hw:%d,%d" upon failure. Is using the NAME property verbatim as the device ID for snd_pcm_open() valid, do you know?

The preferPlugHW config is just a suggestion. It's not going to be used for the "default" nor "pulse" devices and I'm not considering it a critical error if a device cannot be converted to "plughw" format.

It's interesting that you are getting the same error as that from issue #1. I've got a potential fix in that branch so hopefully that'll be cleaned up soon, after which I'll get that merged into the issue_2 branch as well.

I've also decided to commit to that API design - I think it'll be good for extensibility like you say, and it'll allow me to add backend-specific configs in the future to give applications more control. I've not yet had a chance to implement that, though.

Thanks for taking the time to help figure this out!

@VelvetyWhite
Copy link
Author

Yes, yes, you are right, I thought about that after I wrote the message and this way, I think, it should also read multichannel configs from alsa.conf. I saw this pcm naming conventions and they have these examples

hw
hw:0
hw:0,0
hw:supersonic,1
hw:soundwave,1,2
hw:DEV=1,CARD=soundwave,SUBDEV=2

and I thought that's how it has to be but, I guess not really.
So, now

const char* xonarID = "hw:CARD=U7";
config.alsa.noMMap = MAL_FALSE;

works alright,

const char* xonarID = "sysdefault:CARD=U7";
config.alsa.noMMap = MAL_FALSE;

gives mal_device__read_frames_from_client: Assertion `frameCount > 0' failed

const char* xonarID = "sysdefault:CARD=U7";
config.alsa.noMMap = MAL_TRUE;

works,

const char* xonarID = "front:CARD=U7";
config.alsa.noMMap = MAL_TRUE or MAL_FALSE;

works,

const char* xonarID = "surround21:CARD=U7";
config.alsa.noMMap = MAL_FALSE;

gives mal_device_init__alsa: Assertion `pChmap->channels == pDevice->internalChannels' failed as expected.
With mini_al.h from branch issue_1

const char* xonarID = "hw:CARD=U7";
config.alsa.noMMap = MAL_FALSE;

works alright,

const char* xonarID = "sysdefault:CARD=U7";
config.alsa.noMMap = MAL_FALSE;

this just hangs, it's the same one that gives the issue_1 assertion,

const char* xonarID = "sysdefault:CARD=U7";
config.alsa.noMMap = MAL_TRUE;

works, and the front also works.
Now, it may be that there is something dubious with sysdefault in Manjaro.

Thank you for your work, I really appreciate it!

@mackron
Copy link
Owner

mackron commented Nov 7, 2017

Thanks for your work on testing this! I don't fully understand what's going on with that failed assertion. It's most likely that I'm just doing something wrong, but I can't reproduce it so it's been hard to figure it out.

gives mal_device_init__alsa: Assertion `pChmap->channels == pDevice->internalChannels' failed as expected.

I'll fix this one. mini_al should Just Work no matter how the device is configured - it should just do a channel conversion in this case.

mackron added a commit that referenced this issue Nov 8, 2017
 * By default, device enumeration will now only enumerate over unique
   card/device pairs. Applications can enable verbose device
   enumeration by setting the alsa.useVerboseDeviceInteration context
   config variable.
 * By default, the "null" device is excluded from enumeration. This can
   be changed by setting the alsa.includeNullDevice context config
   variable.

Relates to issue #2.
@mackron
Copy link
Owner

mackron commented Nov 8, 2017

I went ahead and pushed some work to improve device enumeration. See the commit message for details. I'll leave this issue open pending a fix for the channel map assertion, but I'll refer you to issue #1 for continued discussion about the mmap error.

Thanks for your help with this!

@mackron
Copy link
Owner

mackron commented Nov 11, 2017

A fix for the chmap assertion error is in the new version on the master branch. Thanks for your help testing all of that for me.

I ended up changing the logic for opening devices again. It's a bit simpler now and doesn't use the "plughw" fallback stuff any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants