[igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data

[Arkadiusz Hiler]

On Tue, May 14, 2019 at 04:51:29PM +0300, Ville Syrjälä wrote:
> On Tue, May 14, 2019 at 04:13:24PM +0300, Simon Ser wrote:
> > audio_signal_detect uses gsl_fft_real_radix2_transform which mutates the data
> > array. This can be surprising when calling audio_signal_detect and then read
> > again the data (e.g. for another check).
> > 
> > Instead of mutating the array, make audio_signal_detect less error-prone by
> > taking a const parameter. Do an internal copy before calling the gsl function.
> > 
> > Signed-off-by: Simon Ser <simon.ser at intel.com>
> > ---
> >  lib/igt_audio.c | 18 +++++++++++++++---
> >  lib/igt_audio.h |  2 +-
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/igt_audio.c b/lib/igt_audio.c
> > index fd8cf07c0de3..d0e34b275fa2 100644
> > --- a/lib/igt_audio.c
> > +++ b/lib/igt_audio.c
> > @@ -251,11 +251,14 @@ void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
> >   * Checks that frequencies specified in signal, and only those, are included
> >   * in the input data.
> >   *
> > - * sampling_rate is given in Hz. data_len is the number of elements in data.
> > + * sampling_rate is given in Hz. samples_len is the number of elements in
> > + * samples.
> >   */
> >  bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > -			 int channel, double *data, size_t data_len)
> > +			 int channel, const double *samples, size_t samples_len)
> >  {
> > +	double *data;
> > +	size_t data_len = samples_len;
> >  	size_t bin_power_len = data_len / 2 + 1;
> >  	double bin_power[bin_power_len];
> >  	bool detected[FREQS_MAX];
> > @@ -264,12 +267,19 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> >  	size_t i, j;
> >  	bool above, success;
> > 
> > +	/* gsl will mutate the array in-place, so make a copy */
> > +	data = malloc(samples_len * sizeof(double));
> > +	memcpy(data, samples, samples_len * sizeof(double));
> 
> Maybe some kind of memdup() might be useful. Not sure how many instances
> of this pattern we have in igt.
> 
> > +
> >  	/* Allowed error in Hz due to FFT step */
> >  	freq_accuracy = sampling_rate / data_len;
> >  	igt_debug("Allowed freq. error: %d Hz\n", freq_accuracy);
> > 
> >  	ret = gsl_fft_real_radix2_transform(data, 1, data_len);
> > -	igt_assert(ret == 0);
> > +	if (ret != 0) {
> > +		free(data);
> > +		igt_assert(0);
> > +	}
> 
> It's an assert. Do we really care about cleanup?

igt_assert, so basically we jump out of test and continue executing.

we wouldn't leak much and matters a little when running a whole binary
but it makes some code analysis tools less angry

> Patch is
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >  	/* Compute the power received by every bin of the FFT, and record the
> >  	 * maximum power received as a way to normalize all the others.
> > @@ -372,6 +382,8 @@ bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> >  		}
> >  	}
> > 
> > +	free(data);
> > +
> >  	return success;
> >  }
> > 
> > diff --git a/lib/igt_audio.h b/lib/igt_audio.h
> > index 466e772a75a4..d5ba1caaca63 100644
> > --- a/lib/igt_audio.h
> > +++ b/lib/igt_audio.h
> > @@ -43,7 +43,7 @@ void audio_signal_reset(struct audio_signal *signal);
> >  void audio_signal_fill(struct audio_signal *signal, int16_t *buffer,
> >  		       size_t buffer_len);
> >  bool audio_signal_detect(struct audio_signal *signal, int sampling_rate,
> > -			 int channel, double *data, size_t data_len);
> > +			 int channel, const double *samples, size_t samples_len);
> >  size_t audio_extract_channel_s32_le(double *dst, size_t dst_cap,
> >  				    int32_t *src, size_t src_len,
> >  				    int n_channels, int channel);
> > --
> > 2.21.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev