Re: [igt-dev] [PATCH i-g-t v2 5/9] lib/igt_audio: introduce audio_convert_to

Martin Peres
2019-05-27T12:27:24Z

On 24/05/2019 18:03, Simon Ser wrote:
> This function converts normalized doubles into an ALSA PCM format.
>
> Instead of having per-format audio_signal_fill_* functions, we can only have
> audio_signal_fill that outputs normalized doubles. Then in ALSA's playback
> callback, we can simply use the new audio_convert_to function to fill the
> buffer.
>
> This makes the test code simpler and prevents code duplication when another
> ALSA playback callback is implemented.
>
> This adds a dependency of igt_audio over ALSA for the PCM format enum, but I
> don't think this is a concern, since I don't see the point of using igt_audio
> without igt_alsa. If this is an issue, it would always be possible to replace
> ALSA's enum with our own in the future.
>
> Signed-off-by: Simon Ser <simon.ser@intel.com>

Yeah, this is a better design!

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>

> ---
> lib/igt_audio.c | 87 +++++++++++++++++++++----------------------
> lib/igt_audio.h | 12 +++---
> tests/kms_chamelium.c | 22 ++++-------
> 3 files changed, 55 insertions(+), 66 deletions(-)
>
> diff --git a/lib/igt_audio.c b/lib/igt_audio.c
> index 376e04ba6ed6..f2aac0e23a37 100644
> --- a/lib/igt_audio.c
> +++ b/lib/igt_audio.c
> @@ -304,51 +304,6 @@ void audio_signal_fill(struct audio_signal *signal, double *buffer,
> audio_sanity_check(buffer, signal->channels * samples);
> }
>
> -void audio_signal_fill_s16_le(struct audio_signal *signal, int16_t *buffer,
> - size_t samples)
> -{
> - double *tmp;
> - size_t i;
> -
> - tmp = malloc(sizeof(double) * signal->channels * samples);
> - audio_signal_fill(signal, tmp, samples);
> -
> - for (i = 0; i < signal->channels * samples; ++i)
> - buffer[i] = INT16_MAX * tmp[i];
> -
> - free(tmp);
> -}
> -
> -void audio_signal_fill_s24_le(struct audio_signal *signal, int32_t *buffer,
> - size_t samples)
> -{
> - double *tmp;
> - size_t i;
> -
> - tmp = malloc(sizeof(double) * signal->channels * samples);
> - audio_signal_fill(signal, tmp, samples);
> -
> - for (i = 0; i < signal->channels * samples; ++i)
> - buffer[i] = 0x7FFFFF * tmp[i];
> -
> - free(tmp);
> -}
> -
> -void audio_signal_fill_s32_le(struct audio_signal *signal, int32_t *buffer,
> - size_t samples)
> -{
> - double *tmp;
> - size_t i;
> -
> - tmp = malloc(sizeof(double) * signal->channels * samples);
> - audio_signal_fill(signal, tmp, samples);
> -
> - for (i = 0; i < signal->channels * samples; ++i)
> - buffer[i] = INT32_MAX * tmp[i];
> -
> - free(tmp);
> -}
> -
> /**
> * Checks that frequencies specified in signal, and only those, are included
> * in the input data.
> @@ -508,6 +463,48 @@ size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
> return dst_len;
> }
>
> +static void audio_convert_to_s16_le(int16_t *dst, double *src, size_t len)
> +{
> + size_t i;
> +
> + for (i = 0; i < len; ++i)
> + dst[i] = INT16_MAX * src[i];
> +}
> +
> +static void audio_convert_to_s24_le(int32_t *dst, double *src, size_t len)
> +{
> + size_t i;
> +
> + for (i = 0; i < len; ++i)
> + dst[i] = 0x7FFFFF * src[i];
> +}
> +
> +static void audio_convert_to_s32_le(int32_t *dst, double *src, size_t len)
> +{
> + size_t i;
> +
> + for (i = 0; i < len; ++i)
> + dst[i] = INT32_MAX * src[i];
> +}
> +
> +void audio_convert_to(void *dst, double *src, size_t len,
> + snd_pcm_format_t format)
> +{
> + switch (format) {
> + case SND_PCM_FORMAT_S16_LE:
> + audio_convert_to_s16_le(dst, src, len);
> + break;
> + case SND_PCM_FORMAT_S24_LE:
> + audio_convert_to_s24_le(dst, src, len);
> + break;
> + case SND_PCM_FORMAT_S32_LE:
> + audio_convert_to_s32_le(dst, src, len);
> + break;
> + default:
> + assert(false); /* unreachable */
> + }
> +}
> +
> #define RIFF_TAG "RIFF"
> #define WAVE_TAG "WAVE"
> #define FMT_TAG "fmt "
> diff --git a/lib/igt_audio.h b/lib/igt_audio.h
> index c8de70871faa..5c910c27304d 100644
> --- a/lib/igt_audio.h
> +++ b/lib/igt_audio.h
> @@ -32,6 +32,8 @@
> #include <stdbool.h>
> #include <stdint.h>
>
> +#include <alsa/asoundlib.h>
> +
> struct audio_signal;
>
> struct audio_signal *audio_signal_init(int channels, int sampling_rate);
> @@ -41,18 +43,14 @@ int audio_signal_add_frequency(struct audio_signal *signal, int frequency,
> void audio_signal_synthesize(struct audio_signal *signal);
> void audio_signal_reset(struct audio_signal *signal);
> void audio_signal_fill(struct audio_signal *signal, double *buffer,
> - size_t buffer_len);
> -void audio_signal_fill_s16_le(struct audio_signal *signal, int16_t *buffer,
> - size_t buffer_len);
> -void audio_signal_fill_s24_le(struct audio_signal *signal, int32_t *buffer,
> - size_t buffer_len);
> -void audio_signal_fill_s32_le(struct audio_signal *signal, int32_t *buffer,
> - size_t buffer_len);
> + size_t samples);
> bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> int channel, const double *samples, size_t samples_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);
> +void audio_convert_to(void *dst, double *src, size_t len,
> + snd_pcm_format_t format);
> int audio_create_wav_file_s32_le(const char *qualifier, uint32_t sample_rate,
> uint16_t channels, char **path);
>
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 56918a3b43fc..063840721f46 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -1012,20 +1012,14 @@ static int
> audio_output_frequencies_callback(void *data, void *buffer, int samples)
> {
> struct audio_state *state = data;
> -
> - switch (state->playback.format) {
> - case SND_PCM_FORMAT_S16_LE:
> - audio_signal_fill_s16_le(state->signal, buffer, samples);
> - break;
> - case SND_PCM_FORMAT_S24_LE:
> - audio_signal_fill_s24_le(state->signal, buffer, samples);
> - break;
> - case SND_PCM_FORMAT_S32_LE:
> - audio_signal_fill_s32_le(state->signal, buffer, samples);
> - break;
> - default:
> - assert(false); /* unreachable */
> - }
> + double *tmp;
> + size_t len;
> +
> + len = samples * state->playback.channels;
> + tmp = malloc(len * sizeof(double));
> + audio_signal_fill(state->signal, tmp, samples);
> + audio_convert_to(buffer, tmp, len, state->playback.format);
> + free(tmp);
>
> return state->run ? 0 : -1;
> }
>