Re: [igt-dev] [PATCH i-g-t v6 4/7] tests/kms_chamelium: test audio channels are not mixed up

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

On 17/04/2019 15:43, Simon Ser wrote:
> Send a different signal to each channel and make sure captured audio channels
> are not swapped or mixed up.
> 
> The Chamelium device has a bug and already swaps the left and right channels.
> For this reason, clients need to retrieve the Chamelium channel mapping and
> accomodate for this. See https://crbug.com/950922 for a discussion about this.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  lib/igt_chamelium.c   | 34 ++++++++++++++++++++++++++++++++++
>  lib/igt_chamelium.h   |  3 +++
>  tests/kms_chamelium.c | 37 +++++++++++++++++++++++++++++++------
>  3 files changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 7c9030d1..6ac7b722 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -930,6 +930,40 @@ int chamelium_get_captured_frame_count(struct chamelium *chamelium)
>  	return ret;
>  }
>  
> +/**
> + * chamelium_get_audio_channel_mapping:
> + * @chamelium: the Chamelium instance
> + * @port: the audio port
> + * @mapping: will be filled with the channel mapping
> + *
> + * Obtains the channel mapping for an audio port.
> + *
> + * Audio channels are not guaranteed not to be swapped. Users can use the
> + * channel mapping to match an input channel to a capture channel.
> + *
> + * The mapping contains one element per capture channel. Each element indicates
> + * which input channel the capture channel is mapped to. As a special case, -1
> + * means that the channel isn't mapped.
> + */
> +void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
> +					 struct chamelium_port *port,
> +					 int mapping[static 8])
> +{
> +	xmlrpc_value *res, *res_channel;
> +	int res_len, i;
> +
> +	res = chamelium_rpc(chamelium, port, "GetAudioChannelMapping", "(i)",
> +			    port->id);
> +	res_len = xmlrpc_array_size(&chamelium->env, res);
> +	igt_assert(res_len == 8);
> +	for (i = 0; i < res_len; i++) {
> +		xmlrpc_array_read_item(&chamelium->env, res, i, &res_channel);
> +		xmlrpc_read_int(&chamelium->env, res_channel, &mapping[i]);
> +		xmlrpc_DECREF(res_channel);
> +	}
> +	xmlrpc_DECREF(res);
> +}
> +
>  /**
>   * chamelium_start_capturing_audio:
>   * @chamelium: the Chamelium instance
> diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> index 047f8c5d..728d16ea 100644
> --- a/lib/igt_chamelium.h
> +++ b/lib/igt_chamelium.h
> @@ -106,6 +106,9 @@ void chamelium_start_capture(struct chamelium *chamelium,
>  void chamelium_stop_capture(struct chamelium *chamelium, int frame_count);
>  void chamelium_capture(struct chamelium *chamelium, struct chamelium_port *port,
>  		       int x, int y, int w, int h, int frame_count);
> +void chamelium_get_audio_channel_mapping(struct chamelium *chamelium,
> +					 struct chamelium_port *port,
> +					 int mapping[static 8]);
>  void chamelium_start_capturing_audio(struct chamelium *chamelium,
>  				    struct chamelium_port *port, bool save_to_file);
>  struct chamelium_audio_file *chamelium_stop_capturing_audio(struct chamelium *chamelium,
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index d336612f..8d6cc092 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -777,7 +777,7 @@ 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, freq;
> +	int ret, capture_rate, capture_channels, msec, freq, step;
>  	struct chamelium_audio_file *audio_file;
>  	struct chamelium_stream *stream;
>  	enum chamelium_stream_realtime_mode stream_mode;
> @@ -792,16 +792,17 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>  	int dump_fd = -1;
>  	pthread_t thread;
>  	struct audio_state state = {};
> +	int channel_mapping[8], capture_chan;
>  
>  	if (!alsa_test_output_configuration(alsa, playback_channels,
>  					    playback_rate)) {
> -		igt_debug("Skipping test with sample rate %d and %d channels "
> +		igt_debug("Skipping test with sample rate %d Hz and %d channels "
>  			  "because selected output devices don't support this "

How about "because at least some output devices do not support this"?

>  			  "configuration\n", playback_rate, playback_channels);
>  		return false;
>  	}
>  
> -	igt_debug("Testing with playback sampling rate %d and %d channels\n",
> +	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);
>  
> @@ -830,9 +831,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>  	signal = audio_signal_init(playback_channels, playback_rate);
>  	igt_assert(signal);
>  
> +	/* We'll choose different frequencies per channel to make sure they are
> +	 * independent from each other. To do so, we'll add a different offset
> +	 * to the base frequencies for each channel. We need to choose a big
> +	 * enough offset so that we're sure to detect mixed up channels.
> +	 */
> +	step = 2 * capture_rate / CAPTURE_SAMPLES;

Please say that this formula adds an offset of 2 bins in the final FFT,
thus enforcing a clear difference (please make sure of that).

>  	for (i = 0; i < test_frequencies_count; i++) {
>  		for (j = 0; j < playback_channels; j++) {
> -			freq = test_frequencies[i];
> +			freq = test_frequencies[i] + j * step;
>  			audio_signal_add_frequency(signal, freq, j);
>  		}
>  	}
> @@ -847,6 +854,20 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>  	ret = pthread_create(&thread, NULL, run_audio_thread, alsa);
>  	igt_assert(ret == 0);
>  
> +	chamelium_get_audio_channel_mapping(data->chamelium, port,
> +					    channel_mapping);
> +	/* Make sure we can capture all channels we send. */
> +	for (i = 0; i < playback_channels; i++) {
> +		ok = false;
> +		for (j = 0; j < capture_channels; j++) {
> +			if (channel_mapping[j] == i) {
> +				ok = true;
> +				break;
> +			}
> +		}
> +		igt_assert(ok);
> +	}
> +
>  	/* Needs to be a multiple of 128, because that's the number of samples
>  	 * we get per channel each time we receive an audio page from the
>  	 * Chamelium device. */
> @@ -886,11 +907,15 @@ do_test_display_audio(data_t *data, struct chamelium_port *port,
>  		igt_debug("Detecting audio signal, t=%d msec\n", msec);
>  
>  		for (j = 0; j < playback_channels; j++) {
> -			igt_debug("Processing channel %zu\n", j);
> +			capture_chan = channel_mapping[j];
> +			igt_assert(capture_chan >= 0);
> +			igt_debug("Processing channel %zu (captured as "
> +				  "channel %d)\n", j, capture_chan);
>  
>  			audio_extract_channel_s32_le(channel, channel_len,
>  						     buf, buf_len,
> -						     capture_channels, j);
> +						     capture_channels,
> +						     capture_chan);
>  
>  			if (audio_signal_detect(signal, capture_rate, j,
>  						channel, channel_len))
> 

Looks great!

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev