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

Ser, Simon
2019-04-23T12:57:26Z

On Tue, 2019-04-23 at 10:51 +0300, Martin Peres wrote:
> 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).

Indeed, better to be clear about this. Yeah, made sure this is correct,
the formula is the same as the one in audio_signal_detect:

freq = sampling_rate * i / data_len;

i is the bin index, data_len the number of samples.

> > 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>