[igt-dev] [PATCH i-g-t v6 3/7] tests/kms_chamelium: test we receive a signal from both audio channels
Ser, Simon
simon.ser at intel.com
Tue Apr 23 12:23:12 UTC 2019
On Tue, 2019-04-23 at 10:41 +0300, Martin Peres wrote:
> On 17/04/2019 15:43, Simon Ser wrote:
> > This commit updates the audio test to make sure we receive a signal from both
> > audio channels. However this commit doesn't check that left and right channels
> > are not swapped. Such a check requires some more work (because the Chamelium
> > device does swap left and right channels) and will be implemented in a future
> > commit.
> >
> > This commit adds a new channel argument to audio_signal_add_frequency, to add
> > a frequency to a single channel only.
> >
> > Some light refactoring has been performed (a proper audio_signal_deinit
> > function has been introduced) and logging has been improved.
> >
> > Signed-off-by: Simon Ser <simon.ser at intel.com>
> > ---
> > lib/igt_alsa.c | 26 +++++++--
> > lib/igt_audio.c | 119 +++++++++++++++++++++++++-----------------
> > lib/igt_audio.h | 11 ++--
> > tests/kms_chamelium.c | 56 ++++++++++++--------
> > 4 files changed, 137 insertions(+), 75 deletions(-)
> >
> > diff --git a/lib/igt_alsa.c b/lib/igt_alsa.c
> > index fc6d336b..a478686a 100644
> > --- a/lib/igt_alsa.c
> > +++ b/lib/igt_alsa.c
> > @@ -182,6 +182,8 @@ static char *alsa_resolve_indentifier(const char *device_name, int skip)
> > continue;
> > }
> >
> > + igt_debug("Matched device \"%s\"\n", pcm_name);
> > +
>
> Should have been a separate commit.
Split into a separate commit.
> > snprintf(name, sizeof(name), "hw:%d,%d", card,
> > dev);
> >
> > @@ -329,6 +331,9 @@ static bool alsa_test_configuration(snd_pcm_t *handle, int channels,
> > {
> > snd_pcm_hw_params_t *params;
> > int ret;
> > + unsigned int min_channels, max_channels;
> > + unsigned int min_rate, max_rate;
> > + int min_rate_dir, max_rate_dir;
> >
> > snd_pcm_hw_params_alloca(¶ms);
> >
> > @@ -337,12 +342,24 @@ static bool alsa_test_configuration(snd_pcm_t *handle, int channels,
> > return false;
> >
> > ret = snd_pcm_hw_params_test_rate(handle, params, sampling_rate, 0);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + snd_pcm_hw_params_get_rate_min(params, &min_rate, &min_rate_dir);
> > + snd_pcm_hw_params_get_rate_max(params, &max_rate, &max_rate_dir);
> > + igt_debug("Output device supports rates between %u and %u, "
> > + "requested %d\n",
> > + min_rate, max_rate, sampling_rate);
> > return false;
> > + }
>
> This likely should be in patch 5/7.
Split into a separate commit instead (with other logging-related
changes).
> >
> > ret = snd_pcm_hw_params_test_channels(handle, params, channels);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + snd_pcm_hw_params_get_channels_min(params, &min_channels);
> > + snd_pcm_hw_params_get_channels_max(params, &max_channels);
> > + igt_debug("Output device supports between %u and "
> > + "%u channels, requested %d\n",
> > + min_channels, max_channels, channels);
> > return false;
> > + }
> >
> > return true;
> > }
> > @@ -409,13 +426,16 @@ void alsa_configure_output(struct alsa *alsa, int channels,
> > snd_pcm_t *handle;
> > int ret;
> > int i;
> > + int soft_resample = 0; /* Don't allow ALSA to resample */
> > + unsigned int latency = 0;
> >
> > for (i = 0; i < alsa->output_handles_count; i++) {
> > handle = alsa->output_handles[i];
> >
> > ret = snd_pcm_set_params(handle, SND_PCM_FORMAT_S16_LE,
> > SND_PCM_ACCESS_RW_INTERLEAVED,
> > - channels, sampling_rate, 0, 0);
> > + channels, sampling_rate,
> > + soft_resample, latency);
>
> Probably should have been a separate commit again ;)
Split into a separate commit.
> > igt_assert(ret >= 0);
> > }
> >
> > diff --git a/lib/igt_audio.c b/lib/igt_audio.c
> > index 7624f565..a2a5c594 100644
> > --- a/lib/igt_audio.c
> > +++ b/lib/igt_audio.c
> > @@ -35,7 +35,7 @@
> > #include "igt_audio.h"
> > #include "igt_core.h"
> >
> > -#define FREQS_MAX 8
> > +#define FREQS_MAX 64
> >
> > /**
> > * SECTION:igt_audio
> > @@ -49,9 +49,10 @@
> >
> > struct audio_signal_freq {
> > int freq;
> > + int channel;
> >
> > - short *period;
> > - int frames;
> > + int16_t *period;
> > + size_t period_len;
> > int offset;
> > };
> >
> > @@ -60,7 +61,7 @@ struct audio_signal {
> > int sampling_rate;
> >
> > struct audio_signal_freq freqs[FREQS_MAX];
> > - int freqs_count;
> > + size_t freqs_count;
> > };
> >
> > /**
> > @@ -89,21 +90,28 @@ struct audio_signal *audio_signal_init(int channels, int sampling_rate)
> > * audio_signal_add_frequency:
> > * @signal: The target signal structure
> > * @frequency: The frequency to add to the signal
> > + * @channel: The channel to add this frequency to, or -1 to add it to all
> > + * channels
> > *
> > * Add a frequency to the signal.
> > *
> > * Returns: An integer equal to zero for success and negative for failure
> > */
> > -int audio_signal_add_frequency(struct audio_signal *signal, int frequency)
> > +int audio_signal_add_frequency(struct audio_signal *signal, int frequency,
> > + int channel)
>
> Is the alignment correct here? Doesn't look like it!
Not sure what's going on with vim. It looks fine in vim, but looks bad
in git. Let's trust git.
> > {
> > - int index = signal->freqs_count;
> > + size_t index = signal->freqs_count;
> > + struct audio_signal_freq *freq;
> >
> > - if (index == FREQS_MAX)
> > - return -1;
> > + igt_assert(index < FREQS_MAX);
> > + igt_assert(channel < signal->channels);
> >
> > /* Stay within the Nyquist–Shannon sampling theorem. */
> > - if (frequency > signal->sampling_rate / 2)
> > + if (frequency > signal->sampling_rate / 2) {
> > + igt_debug("Skipping frequency %d: too high for a %d Hz "
> > + "sampling rate\n", frequency, signal->sampling_rate);
> > return -1;
> > + }
>
> Should have been a separate commit.
Split into a separate commit (along with other logging-related
changes).
> >
> > /* Clip the frequency to an integer multiple of the sampling rate.
> > * This to be able to store a full period of it and use that for
> > @@ -111,11 +119,14 @@ int audio_signal_add_frequency(struct audio_signal *signal, int frequency)
> > */
> > frequency = signal->sampling_rate / (signal->sampling_rate / frequency);
> >
> > - igt_debug("Adding test frequency %d\n", frequency);
> > + igt_debug("Adding test frequency %d to channel %d\n",
> > + frequency, channel);
> > +
> > + freq = &signal->freqs[index];
> > + memset(freq, 0, sizeof(*freq));
> > + freq->freq = frequency;
> > + freq->channel = channel;
> >
> > - signal->freqs[index].freq = frequency;
> > - signal->freqs[index].frames = 0;
> > - signal->freqs[index].offset = 0;
> > signal->freqs_count++;
> >
> > return 0;
> > @@ -133,20 +144,17 @@ void audio_signal_synthesize(struct audio_signal *signal)
> > {
> > int16_t *period;
> > double value;
> > - int frames;
> > + size_t period_len;
> > int freq;
> > int i, j;
> >
> > - if (signal->freqs_count == 0)
> > - return;
> > -
> > for (i = 0; i < signal->freqs_count; i++) {
> > freq = signal->freqs[i].freq;
> > - frames = signal->sampling_rate / freq;
> > + period_len = signal->sampling_rate / freq;
> >
> > - period = calloc(1, frames * sizeof(short));
> > + period = calloc(1, period_len * sizeof(int16_t));
> >
> > - for (j = 0; j < frames; j++) {
> > + for (j = 0; j < period_len; j++) {
> > value = 2.0 * M_PI * freq / signal->sampling_rate * j;
> > value = sin(value) * INT16_MAX / signal->freqs_count;
> >
> > @@ -154,26 +162,34 @@ void audio_signal_synthesize(struct audio_signal *signal)
> > }
> >
> > signal->freqs[i].period = period;
> > - signal->freqs[i].frames = frames;
> > + signal->freqs[i].period_len = period_len;
> > }
> > }
> >
> > /**
> > - * audio_signal_synthesize:
> > + * audio_signal_deinit:
> > + *
> > + * Release the signal.
> > + */
> > +void audio_signal_deinit(struct audio_signal *signal)
>
> Is the coding style to use deinit or fini?
I've used deinit as the code I've seen (igt_alsa, igt_chamelium) uses
it. grep says _fini is used a lot more often though, so let's switch to
that convention.
> > +{
> > + audio_signal_reset(signal);
> > + free(signal);
> > +}
> > +
> > +/**
> > + * audio_signal_reset:
> > * @signal: The target signal structure
> > *
> > * Free the resources allocated by audio_signal_synthesize and remove
> > * the previously-added frequencies.
> > */
> > -void audio_signal_clean(struct audio_signal *signal)
> > +void audio_signal_reset(struct audio_signal *signal)
> > {
> > - int i;
> > + size_t i;
> >
> > for (i = 0; i < signal->freqs_count; i++) {
> > - if (signal->freqs[i].period)
> > - free(signal->freqs[i].period);
> > -
> > - memset(&signal->freqs[i], 0, sizeof(struct audio_signal_freq));
> > + free(signal->freqs[i].period);
> > }
> >
> > signal->freqs_count = 0;
> > @@ -183,44 +199,45 @@ void audio_signal_clean(struct audio_signal *signal)
> > * audio_signal_fill:
> > * @signal: The target signal structure
> > * @buffer: The target buffer to fill
> > - * @frames: The number of frames to fill
> > + * @samples: The number of samples to fill
> > *
> > - * Fill the requested number of frames to the target buffer with the audio
> > + * Fill the requested number of samples to the target buffer with the audio
> > * signal data (in interleaved S16_LE format), at the requested sampling rate
> > * and number of channels.
> > */
> > -void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames)
> > +void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
> > + size_t buffer_len)
> > {
> > int16_t *destination, *source;
> > + struct audio_signal_freq *freq;
> > int total;
> > - int freq_frames;
> > - int freq_offset;
> > int count;
> > int i, j, k;
> >
> > - memset(buffer, 0, sizeof(int16_t) * signal->channels * frames);
> > + memset(buffer, 0, sizeof(int16_t) * signal->channels * buffer_len);
> >
> > for (i = 0; i < signal->freqs_count; i++) {
> > + freq = &signal->freqs[i];
> > total = 0;
> >
> > - while (total < frames) {
> > - freq_frames = signal->freqs[i].frames;
> > - freq_offset = signal->freqs[i].offset;
> > + igt_assert(freq->period);
> >
> > - source = signal->freqs[i].period + freq_offset;
> > + while (total < buffer_len) {
> > + source = freq->period + freq->offset;
> > destination = buffer + total * signal->channels;
> >
> > - count = freq_frames - freq_offset;
> > - if (count > (frames - total))
> > - count = frames - total;
> > + count = freq->period_len - freq->offset;
> > + if (count > buffer_len - total)
> > + count = buffer_len - total;
> >
> > - freq_offset += count;
> > - freq_offset %= freq_frames;
> > -
> > - signal->freqs[i].offset = freq_offset;
> > + freq->offset += count;
> > + freq->offset %= freq->period_len;
> >
> > for (j = 0; j < count; j++) {
> > for (k = 0; k < signal->channels; k++) {
> > + if (freq->channel >= 0 &&
> > + freq->channel != k)
> > + continue;
> > destination[j * signal->channels + k] += source[j];
> > }
> > }
> > @@ -237,11 +254,11 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames)
> > * sampling_rate is given in Hz. data_len is the number of elements in data.
> > */
> > bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > - double *data, size_t data_len)
> > + int channel, double *data, size_t data_len)
> > {
> > size_t bin_power_len = data_len / 2 + 1;
> > double bin_power[bin_power_len];
> > - bool detected[signal->freqs_count];
> > + bool detected[FREQS_MAX];
> > int ret, freq_accuracy, freq, local_max_freq;
> > double max, local_max, threshold;
> > size_t i, j;
> > @@ -308,6 +325,10 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > * invalid. */
> > if (bin_power[i] < threshold) {
> > for (j = 0; j < signal->freqs_count; j++) {
> > + if (signal->freqs[j].channel >= 0 &&
> > + signal->freqs[j].channel != channel)
> > + continue;
> > +
> > if (signal->freqs[j].freq >
> > local_max_freq - freq_accuracy &&
> > signal->freqs[j].freq <
> > @@ -340,6 +361,10 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> >
> > /* Check that all frequencies we generated have been detected. */
> > for (i = 0; i < signal->freqs_count; i++) {
> > + if (signal->freqs[i].channel >= 0 &&
> > + signal->freqs[i].channel != channel)
> > + continue;
> > +
> > if (!detected[i]) {
> > igt_debug("Missing frequency: %d\n",
> > signal->freqs[i].freq);
> > diff --git a/lib/igt_audio.h b/lib/igt_audio.h
> > index 4aa43e69..fe26bb57 100644
> > --- a/lib/igt_audio.h
> > +++ b/lib/igt_audio.h
> > @@ -35,12 +35,15 @@
> > struct audio_signal;
> >
> > struct audio_signal *audio_signal_init(int channels, int sampling_rate);
> > -int audio_signal_add_frequency(struct audio_signal *signal, int frequency);
> > +void audio_signal_deinit(struct audio_signal *signal);
> > +int audio_signal_add_frequency(struct audio_signal *signal, int frequency,
> > + int channel);
> > void audio_signal_synthesize(struct audio_signal *signal);
> > -void audio_signal_clean(struct audio_signal *signal);
> > -void audio_signal_fill(struct audio_signal *signal, int16_t *buffer, int frames);
> > +void audio_signal_reset(struct audio_signal *signal);
> > +void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
> > + size_t buffer_len);
> > bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > - double *data, size_t data_len);
> > + int channel, double *data, size_t data_len);
> > size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
> > int32_t *src, size_t src_len,
> > int n_channels, int channel);
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 014a22b3..d336612f 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -777,16 +777,16 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
> > struct alsa *alsa, int playback_channels,
> > int playback_rate)
> > {
> > - int ret, capture_rate, capture_channels, msec;
> > + int ret, capture_rate, capture_channels, msec, freq;
> > struct chamelium_audio_file *audio_file;
> > struct chamelium_stream *stream;
> > enum chamelium_stream_realtime_mode stream_mode;
> > struct audio_signal *signal;
> > int32_t *recv, *buf;
> > double *channel;
> > - size_t i, streak, page_count;
> > + size_t i, j, streak, page_count;
> > size_t recv_len, buf_len, buf_cap, buf_size, channel_len;
> > - bool ok;
> > + bool ok, success;
> > char dump_suffix[64];
> > char *dump_path = NULL;
> > int dump_fd = -1;
> > @@ -794,10 +794,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
> > struct audio_state state = {};
> >
> > if (!alsa_test_output_configuration(alsa, playback_channels,
> > - playback_rate))
> > + playback_rate)) {
> > + igt_debug("Skipping test with sample rate %d and %d channels "
> > + "because selected output devices don't support this "
> > + "configuration\n", playback_rate, playback_channels);
> > return false;
> > + }
> >
> > - igt_debug("Testing with playback sampling rate %d\n", playback_rate);
> > + igt_debug("Testing with playback sampling rate %d and %d channels\n",
> > + playback_rate, playback_channels);
> > alsa_configure_output(alsa, playback_channels, playback_rate);
> >
> > chamelium_start_capturing_audio(data->chamelium, port, false);
> > @@ -825,8 +830,12 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
> > signal = audio_signal_init(playback_channels, playback_rate);
> > igt_assert(signal);
> >
> > - for (i = 0; i < test_frequencies_count; i++)
> > - audio_signal_add_frequency(signal, test_frequencies[i]);
> > + for (i = 0; i < test_frequencies_count; i++) {
> > + for (j = 0; j < playback_channels; j++) {
> > + freq = test_frequencies[i];
> > + audio_signal_add_frequency(signal, freq, j);
> > + }
> > + }
> > audio_signal_synthesize(signal);
> >
> > state.signal = signal;
> > @@ -851,10 +860,11 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
> > recv = NULL;
> > recv_len = 0;
> >
> > + success = false;
> > streak = 0;
> > msec = 0;
> > i = 0;
> > - while (streak < MIN_STREAK && msec < AUDIO_TIMEOUT) {
> > + while (!success && msec < AUDIO_TIMEOUT) {
> > ok = chamelium_stream_receive_realtime_audio(stream,
> > &page_count,
> > &recv, &recv_len);
> > @@ -872,21 +882,27 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
> > igt_assert(write(dump_fd, buf, buf_size) == buf_size);
> > }
> >
> > - /* TODO: check other channels too, not just the first one */
> > - audio_extract_channel_s32_le(channel, channel_len, buf, buf_len,
> > - capture_channels, 0);
> > -
> > msec = i * channel_len / (double) capture_rate * 1000;
> > igt_debug("Detecting audio signal, t=%d msec\n", msec);
> >
> > - if (audio_signal_detect(signal, capture_rate, channel,
> > - channel_len))
> > - streak++;
> > - else
> > - streak = 0;
> > + for (j = 0; j < playback_channels; j++) {
> > + igt_debug("Processing channel %zu\n", j);
> > +
> > + audio_extract_channel_s32_le(channel, channel_len,
> > + buf, buf_len,
> > + capture_channels, j);
> > +
> > + if (audio_signal_detect(signal, capture_rate, j,
> > + channel, channel_len))
> > + streak++;
> > + else
> > + streak = 0;
> > + }
> >
> > buf_len = 0;
> > i++;
> > +
> > + success = streak == MIN_STREAK * playback_channels;
> > }
> >
> > igt_debug("Stopping audio playback\n");
> > @@ -921,12 +937,10 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
> > chamelium_destroy_audio_file(audio_file);
> > }
> >
> > - audio_signal_clean(signal);
> > - free(signal);
> > -
> > + audio_signal_deinit(signal);
> > chamelium_stream_deinit(stream);
> >
> > - igt_assert(streak == MIN_STREAK);
> > + igt_assert(success);
> > return true;
> > }
> >
>
> With the coding style issues fixed:
>
> Reviewed-by: Martin Peres <martin.peres at linux.intel.com>
Thanks for the review!
More information about the igt-dev
mailing list