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

[Martin Peres <martin.peres@linux.intel.com>]

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;
>  }
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev