[RFC] [PATCH] alsa: Extract supported formats from HDMI ELD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/14/2013 01:56 PM, Arun Raghavan wrote:
> This parses the CEA SAD field from the ELD data we get from HDMI
> receivers. The interesting bits are related to non-PCM formats, since
> this allows us to automaticall detect what receivers support and let
> applications then decide whether they need to perform decoding or not.
> ---
>
> (I haven't been able to test this one properly since my HDMI/DP port is
> refusing to output audio. The ELD parsing should be correct thanks to David
> sharing some ELD blobs from his hardware.

Let me know if you expect/want me to test this patch, if so please give 
me some test instructions (such as what client applications to use etc).

More review below.

>
> This isn't good to go yet -- there are some open issues such as conflict with
> manually set formats and the user impact of this change, especially when
> applications transparently choose what format to use.)
>
>   src/Makefile.am                     |   1 +
>   src/modules/alsa/alsa-util.c        | 172 +++++++++++++++++++++++++++++++++++-
>   src/modules/alsa/alsa-util.h        |  17 +++-
>   src/modules/alsa/module-alsa-card.c |  53 ++++++++++-
>   src/pulse/proplist.h                |   3 +
>   src/pulsecore/format-util.c         |  30 +++++++
>   src/pulsecore/format-util.h         |  36 ++++++++
>   7 files changed, 307 insertions(+), 5 deletions(-)
>   create mode 100644 src/pulsecore/format-util.c
>   create mode 100644 src/pulsecore/format-util.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2521670..ca37437 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -620,6 +620,7 @@ libpulsecommon_ at PA_MAJORMINOR@_la_SOURCES = \
>   		pulsecore/dynarray.c pulsecore/dynarray.h \
>   		pulsecore/endianmacros.h \
>   		pulsecore/flist.c pulsecore/flist.h \
> +		pulsecore/format-util.c pulsecore/format-util.h \
>   		pulsecore/g711.c pulsecore/g711.h \
>   		pulsecore/hashmap.c pulsecore/hashmap.h \
>   		pulsecore/i18n.c pulsecore/i18n.h \
> diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
> index b556349..235f2ad 100644
> --- a/src/modules/alsa/alsa-util.c
> +++ b/src/modules/alsa/alsa-util.c
> @@ -1598,7 +1598,7 @@ int pa_alsa_get_hdmi_eld(snd_hctl_t *hctl, int device, pa_hdmi_eld *eld) {
>       snd_ctl_elem_info_t *info;
>       snd_ctl_elem_value_t *value;
>       uint8_t *elddata;
> -    unsigned int eldsize, mnl;
> +    unsigned int eldsize, mnl, i;
>
>       pa_assert(eld != NULL);
>
> @@ -1640,5 +1640,175 @@ int pa_alsa_get_hdmi_eld(snd_hctl_t *hctl, int device, pa_hdmi_eld *eld) {
>       if (mnl)
>           pa_log_debug("Monitor name in ELD info is '%s' (for device=%d)", eld->monitor_name, device);
>
> +    eld->sad_count = elddata[5] >> 4;
> +    if (eld->sad_count > PA_CEA_SAD_MAX) {
> +        pa_log_debug("SAD_Count too high (device=%d, sad_count=%d", device, eld->sad_count);
> +        return -1;
> +    }
> +
> +    for (i = 0; i < eld->sad_count; i++) {
> +        unsigned int offset = 20 + mnl + (i * 3);
> +
> +        if (eldsize < offset + 3) {
> +            pa_log_debug("ELD smaller than required for SAD_Count (device=%d, sad_count=%d", device, eld->sad_count);
> +            return -1;
> +        }
> +
> +        eld->sad[i].format = (elddata[offset] & 0x7f) >> 3;
> +        eld->sad[i].channels = (elddata[offset] & 0x7) + 1;
> +        eld->sad[i].rates = elddata[offset + 1] & 0x7f;
> +        eld->sad[i].data = elddata[offset + 2];
> +    }
> +
>       return 0;
>   }
> +
> +/* Values taken from: https://en.wikipedia.org/wiki/Extended_display_identification_data
> + * and Linux kernel sources: sound/pci/hda/hda_eld.c */
> +#define CEA_SAD_FORMAT_LPCM     1
> +#define CEA_SAD_FORMAT_AC3      2
> +#define CEA_SAD_FORMAT_MPEG1    3
> +#define CEA_SAD_FORMAT_MP3      4
> +#define CEA_SAD_FORMAT_MPEG2    5
> +#define CEA_SAD_FORMAT_AAC      6
> +#define CEA_SAD_FORMAT_DTS      7
> +#define CEA_SAD_FORMAT_ATRAC    8
> +#define CEA_SAD_FORMAT_SACD     9
> +#define CEA_SAD_FORMAT_EAC3     10
> +#define CEA_SAD_FORMAT_DTSHD    11
> +#define CEA_SAD_FORMAT_DOLBY_TRUEHD 12
> +#define CEA_SAD_FORMAT_DST      13
> +#define CEA_SAD_FORMAT_WMAPRO   14
> +
> +/* Returns: a valid pa_format_info if everything went well, an invalid
> + * pa_format_info if the format was unknown to us, and NULL if there was an
> + * error in parsing the SAD */
> +static pa_format_info* cea_sad_to_format_info(pa_cea_sad *sad) {
> +    pa_format_info *format = pa_format_info_new();
> +
> +    switch (sad->format) {
> +        case CEA_SAD_FORMAT_LPCM:
> +            format->encoding = PA_ENCODING_PCM;
> +            break;
> +
> +        case CEA_SAD_FORMAT_AC3:
> +            format->encoding = PA_ENCODING_AC3_IEC61937;
> +            break;
> +
> +        case CEA_SAD_FORMAT_EAC3:
> +            format->encoding = PA_ENCODING_EAC3_IEC61937;
> +            break;
> +
> +        case CEA_SAD_FORMAT_DTS:
> +            format->encoding = PA_ENCODING_DTS_IEC61937;
> +            break;
> +
> +        case CEA_SAD_FORMAT_MPEG1:
> +        case CEA_SAD_FORMAT_MP3:
> +            format->encoding = PA_ENCODING_MPEG_IEC61937;
> +            break;
> +
> +        case CEA_SAD_FORMAT_MPEG2:
> +            format->encoding = PA_ENCODING_MPEG2_AAC_IEC61937;
> +            break;
> +
> +        default:
> +            /* Unsupported format */
> +            format->encoding = PA_ENCODING_INVALID;
> +            goto out;
> +    }
> +
> +    if (format->encoding == PA_ENCODING_PCM) {
> +        /* Currently, we do not export details of PCM rates, sample formats,
> +         * etc. supported in sink/source formats since the core will plug in
> +         * format conversion, resampling and remapping as needed. This may
> +         * change in the future if we decided to add more fine-grained format
> +         * negotiation. */

However, something to think about for later would be if we should 
reprobe the relevant profiles (currently stereo and 5.1 surround) based 
on this information.

> +    } else {
> +        int i = 0, rates[8] = { 0, };

Nitpick: Do you need the comma? { 0 } should do, I think.

> +
> +        /* The SAD channels is an "upto" number */
> +        pa_format_info_set_prop_int_range(format, PA_PROP_FORMAT_CHANNELS, 1, sad->channels);
> +
> +        if ((sad->rates & 0x7f) == 0) {
> +            pa_log_debug("No known rates set in SAD");
> +            goto error;
> +        }
> +
> +        if (sad->rates & 0x1)
> +            rates[i++] = 32000;
> +        if (sad->rates & 0x2)
> +            rates[i++] = 44100;
> +        if (sad->rates & 0x4)
> +            rates[i++] = 48000;
> +        if (sad->rates & 0x8)
> +            rates[i++] = 88200;
> +        if (sad->rates & 0xc)

Is this correct? 0x4 + 0x8 = 0xc, so anything supporting 96000 must also 
support 88200 and 48000 ?

> +            rates[i++] = 96000;
> +        if (sad->rates & 0x10)
> +            rates[i++] = 176000;
> +        if (sad->rates & 0x20)
> +            rates[i++] = 192000;
> +
> +        pa_format_info_set_prop_int_array(format, PA_PROP_FORMAT_RATE, rates, i);
> +
> +        /* For all the compressed formats we support, if the data field is the
> +         * maximum supported bitrate. */
> +        if (sad->data)
> +            pa_format_info_set_prop_int_range(format, PA_PROP_FORMAT_BITRATE, 0, sad->data * 8000);
> +    }
> +
> +out:
> +    return format;
> +
> +error:
> +    pa_format_info_free(format);
> +    format = NULL;
> +
> +    goto out;
> +}
> +
> +pa_idxset* pa_alsa_formats_from_eld(pa_hdmi_eld *eld) {
> +    pa_idxset *ret = pa_idxset_new(NULL, NULL);
> +    unsigned int i;
> +    bool have_pcm = false;
> +
> +    if (eld->sad_count == 0)
> +        goto out;

or just "return NULL"

> +
> +    for (i = 0; i < eld->sad_count; i++) {
> +        pa_format_info *format = cea_sad_to_format_info(&eld->sad[i]);
> +
> +        if (!format) {
> +            pa_log_debug("Error while parsing SAD");
> +            goto error;
> +        }
> +
> +        if (format->encoding == PA_ENCODING_INVALID) {
> +            /* Skip unknown format */
> +            pa_format_info_free(format);
> +            continue;
> +        }
> +
> +        if (format->encoding == PA_ENCODING_PCM) {
> +            /* Since we don't distinguish between different PCM SADs, only use the first */
> +            if (have_pcm) {
> +                pa_format_info_free(format);
> +                continue;
> +            } else
> +                have_pcm = true;
> +        }
> +        pa_idxset_put(ret, format, NULL);
> +    }
> +
> +out:
> +    return ret;
> +
> +error:
> +    if (ret) {
> +        pa_idxset_free(ret, (pa_free_cb_t) pa_format_info_free);
> +        ret = NULL;
> +    }
> +
> +    goto out;

Or just "return NULL"

> +}
> diff --git a/src/modules/alsa/alsa-util.h b/src/modules/alsa/alsa-util.h
> index 5b0a0bd..738ba19 100644
> --- a/src/modules/alsa/alsa-util.h
> +++ b/src/modules/alsa/alsa-util.h
> @@ -147,11 +147,22 @@ snd_hctl_elem_t* pa_alsa_find_eld_ctl(snd_hctl_t *hctl, int device);
>
>   snd_mixer_t *pa_alsa_open_mixer(int alsa_card_index, char **ctl_device, snd_hctl_t **hctl);
>
> -typedef struct pa_hdmi_eld pa_hdmi_eld;
> -struct pa_hdmi_eld {
> +#define PA_CEA_SAD_MAX 15
> +
> +typedef struct pa_cea_sad {
> +    uint8_t format;     /* format */
> +    uint8_t channels;   /* channel count (actual, not off-by-one as in the SAD itself) */
> +    uint8_t rates;      /* bitfield of supported sampling rates as in the SAD */
> +    uint8_t data;       /* bit depths for LPCM, profile for WMApro, maximum bitrate for others */
> +} pa_cea_sad;
> +
> +typedef struct pa_hdmi_eld {
>       char monitor_name[17];
> -};
> +    unsigned int sad_count;
> +    pa_cea_sad sad[PA_CEA_SAD_MAX];
> +} pa_hdmi_eld;
>
>   int pa_alsa_get_hdmi_eld(snd_hctl_t *hctl, int device, pa_hdmi_eld *eld);
> +pa_idxset* pa_alsa_formats_from_eld(pa_hdmi_eld *eld);
>
>   #endif
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index fe05e3d..acb925b 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -26,6 +26,7 @@
>   #include <pulse/xmalloc.h>
>
>   #include <pulsecore/core-util.h>
> +#include <pulsecore/format-util.h>
>   #include <pulsecore/i18n.h>
>   #include <pulsecore/modargs.h>
>   #include <pulsecore/queue.h>
> @@ -405,9 +406,13 @@ static int hdmi_eld_changed(snd_hctl_elem_t *elem, unsigned int mask) {
>       struct userdata *u = snd_hctl_elem_get_callback_private(elem);
>       int device = snd_hctl_elem_get_device(elem);
>       const char *old_monitor_name;
> +    pa_sink *sink;
>       pa_device_port *p;
>       pa_hdmi_eld eld;
> +    pa_idxset *formats, *old_formats;
> +    pa_format_info *format;
>       bool changed = false;
> +    uint32_t idx;
>
>       if (mask == SND_CTL_EVENT_MASK_REMOVE)
>           return 0;
> @@ -418,8 +423,9 @@ static int hdmi_eld_changed(snd_hctl_elem_t *elem, unsigned int mask) {
>           return 0;
>       }
>
> +    pa_zero(eld);
>       if (pa_alsa_get_hdmi_eld(u->hctl_handle, device, &eld) < 0)
> -        memset(&eld, 0, sizeof(eld));
> +        pa_zero(eld);
>
>       old_monitor_name = pa_proplist_gets(p->proplist, PA_PROP_DEVICE_PRODUCT_NAME);
>       if (eld.monitor_name[0] == '\0') {
> @@ -430,9 +436,54 @@ static int hdmi_eld_changed(snd_hctl_elem_t *elem, unsigned int mask) {
>           pa_proplist_sets(p->proplist, PA_PROP_DEVICE_PRODUCT_NAME, eld.monitor_name);
>       }
>
> +    formats = pa_alsa_formats_from_eld(&eld);
> +    if (!formats) {
> +        /* If we don't have any formats, assume PCM only */
> +        formats = pa_idxset_new(NULL, NULL);
> +        format = pa_format_info_new();
> +        format->encoding = PA_ENCODING_PCM;
> +        pa_idxset_put(formats, format, NULL);
> +    }
> +
> +    /* Note: The assumption here is that HDMI cards will only have one sink.
> +     * This is how ALSA currently works. */

HDMI *ports* will only have one sink, i e *cards* can have many sinks, 
each one with exactly one port.

> +    sink = pa_idxset_first(u->card->sinks, NULL);
> +    old_formats = pa_sink_get_formats(sink);
> +
> +    /* Now check if the format set has changed */
> +    if (pa_idxset_size(formats) != pa_idxset_size(old_formats)) {
> +        pa_sink_set_formats(sink, formats);
> +        changed = true;
> +    } else {
> +        pa_format_info *old_format;
> +        bool equal = true;
> +
> +        /* Quick and dirty format info comparison - good enough for checking if
> +         * there are any changes at all in formats from what we got last */
> +        PA_IDXSET_FOREACH(format, formats, idx) {
> +            PA_IDXSET_FOREACH(old_format, old_formats, idx) {
> +                if (!pa_format_info_identical(format, old_format)) {
> +                    equal = false;
> +                    break;
> +                }
> +            }
> +
> +            if (!equal)
> +                break;
> +        }
> +
> +        if (!equal) {
> +            pa_sink_set_formats(sink, formats);
> +            changed = true;
> +        }
> +    }
> +
>       if (changed && mask != 0)
>           pa_subscription_post(u->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, u->card->index);
>
> +    pa_idxset_free(formats, (pa_free_cb_t) pa_format_info_free);
> +    pa_idxset_free(old_formats, (pa_free_cb_t) pa_format_info_free);
> +
>       return 0;
>   }
>
> diff --git a/src/pulse/proplist.h b/src/pulse/proplist.h
> index dc3cddc..dbadb98 100644
> --- a/src/pulse/proplist.h
> +++ b/src/pulse/proplist.h
> @@ -269,6 +269,9 @@ PA_C_DECL_BEGIN
>   /** For PCM formats: the channel map of the stream as returned by pa_channel_map_snprint() \since 1.0 */
>   #define PA_PROP_FORMAT_CHANNEL_MAP             "format.channel_map"
>
> +/** For compressed formats: the bit rate (unsigned integer) \since 5.0 */
> +#define PA_PROP_FORMAT_BITRATE                 "format.bitrate"
> +
>   /** A property list object. Basically a dictionary with ASCII strings
>    * as keys and arbitrary data as values. \since 0.9.11 */
>   typedef struct pa_proplist pa_proplist;
> diff --git a/src/pulsecore/format-util.c b/src/pulsecore/format-util.c
> new file mode 100644
> index 0000000..1002a6c

It seems a little overkill to add two new files for just one tiny 
function, unless you're planning (in the short term) to add many more. 
Can't it fit into any existing file, e g the file defining the 
pa_format_info type?


> --- /dev/null
> +++ b/src/pulsecore/format-util.c
> @@ -0,0 +1,30 @@
> +/***
> +  This file is part of PulseAudio.
> +
> +  Copyright 2013 Collabora Ltd.
> +  Author: Arun Raghavan <arun.raghavan at collabora.co.uk>
> +
> +  PulseAudio is free software; you can redistribute it and/or modify
> +  it under the terms of the GNU Lesser General Public License as published
> +  by the Free Software Foundation; either version 2.1 of the License,
> +  or (at your option) any later version.
> +
> +  PulseAudio is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public License
> +  along with PulseAudio; if not, write to the Free Software
> +  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> +  USA.
> +***/
> +
> +#include <pulsecore/format-util.h>
> +#include <pulse/proplist.h>
> +
> +bool pa_format_info_identical(pa_format_info *f1, pa_format_info *f2) {
> +    /* We can do a string comparison on all the proplist values which is faster
> +     * than unpacking into json objects if order doesn't matter */
> +    return (f1->encoding == f2->encoding && !pa_proplist_equal(f1->plist, f2->plist));
> +}
> diff --git a/src/pulsecore/format-util.h b/src/pulsecore/format-util.h
> new file mode 100644
> index 0000000..46bf15c
> --- /dev/null
> +++ b/src/pulsecore/format-util.h
> @@ -0,0 +1,36 @@
> +#ifndef fooformatutilhfoo
> +#define fooformatutilhfoo
> +
> +/***
> +  This file is part of PulseAudio.
> +
> +  Copyright 2013 Collabora Ltd.
> +  Author: Arun Raghavan <arun.raghavan at collabora.co.uk>
> +
> +  PulseAudio is free software; you can redistribute it and/or modify
> +  it under the terms of the GNU Lesser General Public License as published
> +  by the Free Software Foundation; either version 2.1 of the License,
> +  or (at your option) any later version.
> +
> +  PulseAudio is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public License
> +  along with PulseAudio; if not, write to the Free Software
> +  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> +  USA.
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <pulsecore/macro.h>
> +#include <pulse/format.h>
> +
> +/* Check if two formats are equal, including array order */
> +bool pa_format_info_identical(pa_format_info *f1, pa_format_info *f2);
> +
> +#endif
>



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


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux