[pulseaudio-discuss] [PATCH 1/3] alsa-util: Add a function to read ELD info

[David Henningsson]

On 02/15/2013 11:08 PM, Tanu Kaskinen wrote:
> On Fri, 2013-02-15 at 13:42 +0100, David Henningsson wrote:
>> Currently, this function only reads the monitor name, but could
>> be extended to read e g supported formats as well.
>>
>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>> ---
>>   src/modules/alsa/alsa-util.c |   55 ++++++++++++++++++++++++++++++++++++++++++
>>   src/modules/alsa/alsa-util.h |    7 ++++++
>>   2 files changed, 62 insertions(+)
>>
>> diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
>> index 114ab27..7cc9492 100644
>> --- a/src/modules/alsa/alsa-util.c
>> +++ b/src/modules/alsa/alsa-util.c
>> @@ -1574,3 +1574,58 @@ snd_mixer_t *pa_alsa_open_mixer_for_pcm(snd_pcm_t *pcm, char **ctl_device, snd_h
>>       snd_mixer_close(m);
>>       return NULL;
>>   }
>> +
>> +bool pa_alsa_get_hdmi_eld(snd_hctl_t *hctl, int device, pa_hdmi_eld_t *eld) {
>> +
>
> The function could start with a comment containing a link to a
> specification of the ELD data format. I didn't manage to find such
> document (it doesn't help that I couldn't even figure out what the
> acronym stands for), so I can't check whether the parsing code is
> correct.

Ok

http://www.intel.com/content/www/us/en/standards/high-definition-audio-specification.html

(Yes, this is HDA specific.)

>> +    int err;
>> +    snd_ctl_elem_id_t *id;
>> +    snd_hctl_elem_t *elem;
>> +    snd_ctl_elem_info_t *info;
>> +    snd_ctl_elem_value_t *value;
>> +    unsigned char *elddata;
>
> Usually uint8_t is used as the type for binary data.

Ok

>
>> +    unsigned int eldsize, mnl;
>> +
>> +    pa_assert(eld != NULL);
>> +
>> +    /* See if we can find the ELD control */
>> +    snd_ctl_elem_id_alloca(&id);
>
> This allocates memory, right? It's never freed.

alloca = allocate on the stack, as such it is auto-freed when function 
exits.

>
>> +    snd_ctl_elem_id_clear(id);
>> +    snd_ctl_elem_id_set_interface(id, SND_CTL_ELEM_IFACE_PCM);
>> +    snd_ctl_elem_id_set_name(id, "ELD");
>> +    snd_ctl_elem_id_set_device(id, device);
>> +
>> +    elem = snd_hctl_find_elem(hctl, id);
>> +    if (elem == NULL) {
>> +        pa_log_debug("No ELD info control found (for device=%d)", device);
>> +        return false;
>> +    }
>> +
>> +    /* Does it have any contents? */
>> +    snd_ctl_elem_info_alloca(&info);
>> +    snd_ctl_elem_value_alloca(&value);
>
> These are not freed either.
>
>> +    if ((err = snd_hctl_elem_info(elem, info)) < 0 ||
>> +       (err = snd_hctl_elem_read(elem, value)) < 0) {
>> +        pa_log_warn("Accessing ELD control failed with error %s", snd_strerror(err));
>> +        return false;
>> +    }
>> +
>> +    eldsize = snd_ctl_elem_info_get_count(info);
>> +    elddata = (unsigned char *) snd_ctl_elem_value_get_bytes(value);
>> +    if (eldsize < 20 || eldsize > 256 || elddata == NULL) {
>> +        pa_log_debug("ELD info empty (for device=%d)", device);
>
> The error message is not very good if eldsize > 0.

Ok

>
>> +        return false;
>> +    }
>> +
>> +    /* Try to fetch monitor name */
>> +    mnl = elddata[4] & 0x1f;
>> +    if (mnl == 0 || mnl > 16 || 20 + mnl > eldsize) {
>> +        pa_log_debug("No monitor name in ELD info (for device=%d)", device);
>> +        mnl = 0;
>> +    }
>> +    memcpy(eld->monitor_name, &elddata[20], mnl);
>> +    eld->monitor_name[mnl] = '\0';
>> +    if (mnl)
>> +        pa_log_debug("Monitor name in ELD info is '%s' (for device=%d)", eld->monitor_name, device);
>> +
>> +    return true;
>> +}
>> diff --git a/src/modules/alsa/alsa-util.h b/src/modules/alsa/alsa-util.h
>> index 1326e64..db6f638 100644
>> --- a/src/modules/alsa/alsa-util.h
>> +++ b/src/modules/alsa/alsa-util.h
>> @@ -146,4 +146,11 @@ snd_hctl_elem_t* pa_alsa_find_jack(snd_hctl_t *hctl, const char* jack_name);
>>
>>   snd_mixer_t *pa_alsa_open_mixer(int alsa_card_index, char **ctl_device, snd_hctl_t **hctl);
>>
>> +typedef struct pa_hdmi_eld_t pa_hdmi_eld_t;
>
> Structs shouldn't have the _t suffix.

Ok

>
>> +struct pa_hdmi_eld_t {
>> +    char monitor_name[17];
>
> Is the parsed ELD field only set for monitors, and not for any other
> type of HDMI hardware?

The name in the specification is "monitor_name", hence the name here 
too. My HDMI receiver (which is not a monitor) supplies the name of 
itself here.



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic