[pulseaudio-discuss] [RFC] [PATCH] alsa: Extract supported formats from HDMI ELD
Arun Raghavan
arun.raghavan at collabora.co.uk
Fri Jun 14 05:55:40 PDT 2013
On Fri, 2013-06-14 at 14:41 +0200, David Henningsson wrote:
> 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).
That would be good, yes. Simplest test would be to plug it in, make sure
you see AC3/DTS formats in 'pactl list sinks'.
Next, find some files with AC3/DTS samples, and do a:
gst-launch-1.0 playbin uri=file:///path/to/your/file
You need to make sure no other sink inputs are connected to the HDMI
sink. If things start playing correctly, your receiver should indicate
what format it is receiving, and the sink input format reported by
'pactl list sink-inputs' should be the compressed format you're playing
back.
> 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 \
> >0- 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.
Possibly. I'm wondering if we should be leaving that to changes in the
channel map kctl (is ALSA going to be parsing the ELD and notifying us
of channel map changes there?).
> > + } else {
> > + int i = 0, rates[8] = { 0, };
>
> Nitpick: Do you need the comma? { 0 } should do, I think.
Hum, I'm not sure why I believe the comma was part of the syntax for
zero initialising structures.
> > +
> > + /* 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.
Okay, so I should be going from ctl -> port -> sink.
> > + 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?
I expect this file to grow in the future as we add pa_format_info
manipulation that we don't want to expose in libpulse.
Thanks for the review,
Arun
> > --- /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
> >
>
>
>
More information about the pulseaudio-discuss
mailing list