lib/igt_audio: make audio_signal_detect take const data

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 <[email protected]>

---
 lib/igt_audio.c | 18 +++++++++++++++---
 lib/igt_audio.h |  2 +-
 2 files changed, 16 insertions(+), 4 deletions(-)

lib/igt_audio.c +5 -2

index fd8cf07c0de3..d0e34b275fa2 100644
@@ -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];

lib/igt_audio.c +2 -0

 	/* 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;
 }
 

lib/igt_audio.h +1 -2

index 466e772a75a4..d5ba1caaca63 100644
@@ -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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Quoted 35 lines · Simon Ser

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 <[email protected]>

---
 lib/igt_audio.c | 18 +++++++++++++++---
 lib/igt_audio.h |  2 +-
 2 files changed, 16 insertions(+), 4 deletions(-)

lib/igt_audio.c +5 -2

index fd8cf07c0de3..d0e34b275fa2 100644
@@ -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];

lib/igt_audio.c bool audio_signal_detect(struct audio_signal *signal, int sampling_rate, · line 272

Quoted 6 lines

lib/igt_audio.c +3 -0

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

Quoted 5 lines

lib/igt_audio.c +1 -0

+
 	/* Allowed error in Hz due to FFT step */
 	freq_accuracy = sampling_rate / data_len;
 	igt_debug("Allowed freq. error: %d Hz\n", freq_accuracy);
 

lib/igt_audio.c bool audio_signal_detect(struct audio_signal *signal, int sampling_rate, · line 282

Quoted 6 lines

lib/igt_audio.c +4 -1

 	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ä <[email protected]>

Quoted 27 lines

lib/igt_audio.c +2 -0

 	/* 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;
 }
 

lib/igt_audio.h +1 -2

index 466e772a75a4..d5ba1caaca63 100644
@@ -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

Quoted 4 lines

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Signature · 6 lines

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