[pulseaudio-discuss] [RFC] [PATCH] alsa: Extract supported formats from HDMI ELD
David Henningsson
david.henningsson at canonical.com
Fri Jun 14 05:41:33 PDT 2013
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
More information about the pulseaudio-discuss
mailing list