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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

Patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev