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

Ser, Simon simon.ser at intel.com
Tue Apr 23 12:57:23 UTC 2019


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 at 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 at linux.intel.com>


More information about the igt-dev mailing list