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

Martin Peres
2019-04-23T07:51:09Z

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>