Re: [igt-dev] [PATCH i-g-t 1/5] lib/igt_alsa: support all alsa formats

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

On 20/05/2019 13:38, Ser, Simon wrote:
> On Mon, 2019-05-20 at 13:11 +0300, Martin Peres wrote:
>> On 17/05/2019 19:02, Simon Ser wrote:
>>> This commit adds a new format argument to alsa_configure_output. This allows
>>> the caller to decide the format used for the audio signal.
>>>
>>> This removes the assumption that samples are S16_LE items in igt_alsa. The
>>> alsa function snd_pcm_format_physical_width is used instead of sizeof(short).
>>> This also removes the theorically incorrect assumption that sizeof(short) ==
>>> sizeof(int16_t).
>>>
>>> Right now S16_LE is hardcoded in the test. Tests with other formats will be
>>> added in future commits.
>>>
>>> Signed-off-by: Simon Ser <simon.ser@intel.com>
>>> ---
>>>  lib/igt_alsa.c        | 23 +++++++++++++----------
>>>  lib/igt_alsa.h        |  7 ++++---
>>>  tests/kms_chamelium.c |  7 ++++---
>>>  3 files changed, 21 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/lib/igt_alsa.c b/lib/igt_alsa.c
>>> index 28a866e0ee74..2dfb8ccd3ad9 100644
>>> --- a/lib/igt_alsa.c
>>> +++ b/lib/igt_alsa.c
>>> @@ -27,7 +27,6 @@
>>>  #include "config.h"
>>>  
>>>  #include <limits.h>
>>> -#include <alsa/asoundlib.h>
>>>  
>>>  #include "igt_alsa.h"
>>>  #include "igt_aux.h"
>>> @@ -47,10 +46,11 @@
>>>  struct alsa {
>>>  	snd_pcm_t *output_handles[HANDLES_MAX];
>>>  	int output_handles_count;
>>> +	snd_pcm_format_t output_format;
>>>  	int output_sampling_rate;
>>>  	int output_channels;
>>>  
>>> -	int (*output_callback)(void *data, short *buffer, int samples);
>>> +	int (*output_callback)(void *data, void *buffer, int samples);
>>>  	void *output_callback_data;
>>>  	int output_samples_trigger;
>>>  };
>>> @@ -342,8 +342,8 @@ bool alsa_test_output_configuration(struct alsa *alsa, int channels,
>>>   * Configure the output devices with the configuration specified by @channels
>>>   * and @sampling_rate.
>>>   */
>>> -void alsa_configure_output(struct alsa *alsa, int channels,
>>> -			   int sampling_rate)
>>> +void alsa_configure_output(struct alsa *alsa, snd_pcm_format_t fmt,
>>> +			   int channels, int sampling_rate)
>>>  {
>>>  	snd_pcm_t *handle;
>>>  	int ret;
>>> @@ -354,13 +354,14 @@ void alsa_configure_output(struct alsa *alsa, int channels,
>>>  	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,
>>> +		ret = snd_pcm_set_params(handle, fmt,
>>>  					 SND_PCM_ACCESS_RW_INTERLEAVED,
>>>  					 channels, sampling_rate,
>>>  					 soft_resample, latency);
>>>  		igt_assert(ret >= 0);
>>>  	}
>>>  
>>> +	alsa->output_format = fmt;
>>>  	alsa->output_channels = channels;
>>>  	alsa->output_sampling_rate = sampling_rate;
>>>  }
>>> @@ -379,7 +380,7 @@ void alsa_configure_output(struct alsa *alsa, int channels,
>>>   * for failure.
>>>   */
>>>  void alsa_register_output_callback(struct alsa *alsa,
>>> -				   int (*callback)(void *data, short *buffer, int samples),
>>> +				   int (*callback)(void *data, void *buffer, int samples),
>>>  				   void *callback_data, int samples_trigger)
>>>  {
>>>  	alsa->output_callback = callback;
>>> @@ -402,12 +403,13 @@ void alsa_register_output_callback(struct alsa *alsa,
>>>  int alsa_run(struct alsa *alsa, int duration_ms)
>>>  {
>>>  	snd_pcm_t *handle;
>>> -	short *output_buffer = NULL;
>>> +	char *output_buffer = NULL;
>>
>> Why not void*?
> 
> Below in the snd_pcm_writei call, we need to add an offset to this
> address. Since it's not possible to use the array syntax on void * I
> used char *. That way output_buffer[i] is the i-th byte of
> output_buffer.
> 
> (Note that doing pointer arithmetic on void * is undefined behaviour.)

Fair-enough. Keep char* or use uint8_t*. Both work for me.

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

> 
>>>  	int output_limit;
>>>  	int output_total = 0;
>>>  	int output_counts[alsa->output_handles_count];
>>>  	bool output_ready = false;
>>>  	int output_channels;
>>> +	int bytes_per_sample;
>>>  	int output_trigger;
>>>  	bool reached;
>>>  	int index;
>>> @@ -418,9 +420,10 @@ int alsa_run(struct alsa *alsa, int duration_ms)
>>>  
>>>  	output_limit = alsa->output_sampling_rate * duration_ms / 1000;
>>>  	output_channels = alsa->output_channels;
>>> +	bytes_per_sample = snd_pcm_format_physical_width(alsa->output_format) / 8;
>>>  	output_trigger = alsa->output_samples_trigger;
>>> -	output_buffer = malloc(sizeof(short) * output_channels *
>>> -			       output_trigger);
>>> +	output_buffer = malloc(output_channels * output_trigger *
>>> +			       bytes_per_sample);
>>>  
>>>  	do {
>>>  		reached = true;
>>> @@ -454,7 +457,7 @@ int alsa_run(struct alsa *alsa, int duration_ms)
>>>  					count = avail < count ? avail : count;
>>>  
>>>  					ret = snd_pcm_writei(handle,
>>> -							     &output_buffer[index],
>>> +							     &output_buffer[index * bytes_per_sample],
>>>  							     count);
>>>  					if (ret < 0) {
>>>  						ret = snd_pcm_recover(handle,
>>> diff --git a/lib/igt_alsa.h b/lib/igt_alsa.h
>>> index a10985ff777f..46b3568d26fd 100644
>>> --- a/lib/igt_alsa.h
>>> +++ b/lib/igt_alsa.h
>>> @@ -29,6 +29,7 @@
>>>  
>>>  #include "config.h"
>>>  
>>> +#include <alsa/asoundlib.h>
>>>  #include <stdbool.h>
>>>  
>>>  struct alsa;
>>> @@ -39,10 +40,10 @@ int alsa_open_output(struct alsa *alsa, const char *device_name);
>>>  void alsa_close_output(struct alsa *alsa);
>>>  bool alsa_test_output_configuration(struct alsa *alsa, int channels,
>>>  				    int sampling_rate);
>>> -void alsa_configure_output(struct alsa *alsa, int channels,
>>> -			   int sampling_rate);
>>> +void alsa_configure_output(struct alsa *alsa, snd_pcm_format_t fmt,
>>> +			   int channels, int sampling_rate);
>>>  void alsa_register_output_callback(struct alsa *alsa,
>>> -				   int (*callback)(void *data, short *buffer, int samples),
>>> +				   int (*callback)(void *data, void *buffer, int samples),
>>>  				   void *callback_data, int samples_trigger);
>>>  int alsa_run(struct alsa *alsa, int duration_ms);
>>>  
>>> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
>>> index c8b6b22d7b4a..2b465565418d 100644
>>> --- a/tests/kms_chamelium.c
>>> +++ b/tests/kms_chamelium.c
>>> @@ -801,11 +801,11 @@ struct audio_state {
>>>  };
>>>  
>>>  static int
>>> -audio_output_callback(void *data, short *buffer, int frames)
>>> +audio_output_callback(void *data, void *buffer, int samples)
>>>  {
>>>  	struct audio_state *state = data;
>>>  
>>> -	audio_signal_fill_s16_le(state->signal, buffer, frames);
>>> +	audio_signal_fill_s16_le(state->signal, buffer, samples);
>>>  
>>>  	return state->run ? 0 : -1;
>>>  }
>>> @@ -843,7 +843,8 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>>>  
>>>  	igt_debug("Testing with playback sampling rate %d Hz and %d channels\n",
>>>  		  playback_rate, playback_channels);
>>> -	alsa_configure_output(alsa, playback_channels, playback_rate);
>>> +	alsa_configure_output(alsa, SND_PCM_FORMAT_S16_LE,
>>> +			      playback_channels, playback_rate);
>>>  
>>>  	chamelium_start_capturing_audio(data->chamelium, port, false);
>>>  
>>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev