* [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data
@ 2019-05-14 13:13 Simon Ser
2019-05-14 13:51 ` Ville Syrjälä
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Simon Ser @ 2019-05-14 13:13 UTC (permalink / raw)
To: igt-dev
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));
+
/* 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);
+ }
/* 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data
2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
@ 2019-05-14 13:51 ` Ville Syrjälä
2019-05-15 7:50 ` Arkadiusz Hiler
2019-05-14 14:21 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2) Patchwork
2019-05-14 21:10 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2019-05-14 13:51 UTC (permalink / raw)
To: Simon Ser; +Cc: igt-dev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2)
2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
2019-05-14 13:51 ` Ville Syrjälä
@ 2019-05-14 14:21 ` Patchwork
2019-05-14 21:10 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2019-05-14 14:21 UTC (permalink / raw)
To: Simon Ser; +Cc: igt-dev
== Series Details ==
Series: lib/igt_audio: make audio_signal_detect take const data (rev2)
URL : https://patchwork.freedesktop.org/series/60585/
State : success
== Summary ==
CI Bug Log - changes from IGT_4988 -> IGTPW_2982
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/60585/revisions/2/mbox/
Known issues
------------
Here are the changes found in IGTPW_2982 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_contexts:
- fi-bdw-gvtdvm: [PASS][1] -> [DMESG-FAIL][2] ([fdo#110235])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
#### Possible fixes ####
* igt@i915_selftest@live_hangcheck:
- fi-kbl-8809g: [DMESG-FAIL][3] -> [PASS][4]
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/fi-kbl-8809g/igt@i915_selftest@live_hangcheck.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/fi-kbl-8809g/igt@i915_selftest@live_hangcheck.html
[fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
Participating hosts (51 -> 46)
------------------------------
Additional (2): fi-icl-u2 fi-pnv-d510
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus
Build changes
-------------
* IGT: IGT_4988 -> IGTPW_2982
CI_DRM_6080: 6c6b621677cc0d3616de3dab025d967aa2c43877 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2982: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
IGT_4988: 2f6303d13e09b2457762540383c7f36cecd02bbf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 5+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for lib/igt_audio: make audio_signal_detect take const data (rev2)
2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
2019-05-14 13:51 ` Ville Syrjälä
2019-05-14 14:21 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2) Patchwork
@ 2019-05-14 21:10 ` Patchwork
2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2019-05-14 21:10 UTC (permalink / raw)
To: Simon Ser; +Cc: igt-dev
== Series Details ==
Series: lib/igt_audio: make audio_signal_detect take const data (rev2)
URL : https://patchwork.freedesktop.org/series/60585/
State : success
== Summary ==
CI Bug Log - changes from IGT_4988_full -> IGTPW_2982_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/60585/revisions/2/mbox/
Known issues
------------
Here are the changes found in IGTPW_2982_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_pm_rpm@i2c:
- shard-iclb: [PASS][1] -> [DMESG-WARN][2] ([fdo#109982])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb8/igt@i915_pm_rpm@i2c.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@i915_pm_rpm@i2c.html
* igt@kms_flip@2x-flip-vs-suspend-interruptible:
- shard-hsw: [PASS][3] -> [INCOMPLETE][4] ([fdo#103540])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-hsw6/igt@kms_flip@2x-flip-vs-suspend-interruptible.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-hsw2/igt@kms_flip@2x-flip-vs-suspend-interruptible.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render:
- shard-iclb: [PASS][5] -> [FAIL][6] ([fdo#103167]) +2 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render.html
* igt@kms_psr@psr2_cursor_plane_onoff:
- shard-iclb: [PASS][7] -> [SKIP][8] ([fdo#109441]) +4 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb7/igt@kms_psr@psr2_cursor_plane_onoff.html
* igt@kms_vblank@pipe-c-ts-continuation-suspend:
- shard-apl: [PASS][9] -> [DMESG-WARN][10] ([fdo#108566]) +2 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl1/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl3/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
#### Possible fixes ####
* igt@gem_wait@basic-wait-all:
- shard-iclb: [INCOMPLETE][11] ([fdo#107713]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb8/igt@gem_wait@basic-wait-all.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@gem_wait@basic-wait-all.html
* igt@gem_workarounds@suspend-resume:
- shard-apl: [DMESG-WARN][13] ([fdo#108566]) -> [PASS][14] +3 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl8/igt@gem_workarounds@suspend-resume.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl4/igt@gem_workarounds@suspend-resume.html
* igt@kms_color@pipe-b-ctm-negative:
- shard-iclb: [DMESG-WARN][15] -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_color@pipe-b-ctm-negative.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb7/igt@kms_color@pipe-b-ctm-negative.html
* igt@kms_dp_dsc@basic-dsc-enable-edp:
- shard-iclb: [SKIP][17] ([fdo#109349]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb5/igt@kms_dp_dsc@basic-dsc-enable-edp.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt:
- shard-iclb: [FAIL][19] ([fdo#103167]) -> [PASS][20] +4 similar issues
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
* igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt:
- shard-iclb: [FAIL][21] ([fdo#109247]) -> [PASS][22] +1 similar issue
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt.html
* igt@kms_plane_lowres@pipe-a-tiling-x:
- shard-iclb: [FAIL][23] ([fdo#103166]) -> [PASS][24]
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb1/igt@kms_plane_lowres@pipe-a-tiling-x.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html
* igt@kms_psr@primary_mmap_gtt:
- shard-iclb: [FAIL][25] ([fdo#107383] / [fdo#110215]) -> [PASS][26]
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb2/igt@kms_psr@primary_mmap_gtt.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb7/igt@kms_psr@primary_mmap_gtt.html
* igt@kms_setmode@basic:
- shard-apl: [FAIL][27] ([fdo#99912]) -> [PASS][28]
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl2/igt@kms_setmode@basic.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl4/igt@kms_setmode@basic.html
- shard-kbl: [FAIL][29] ([fdo#99912]) -> [PASS][30]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-kbl6/igt@kms_setmode@basic.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-kbl4/igt@kms_setmode@basic.html
* igt@kms_universal_plane@universal-plane-pipe-b-functional:
- shard-apl: [FAIL][31] ([fdo#110037]) -> [PASS][32]
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-apl7/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-apl3/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
- shard-kbl: [FAIL][33] ([fdo#110037]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-kbl4/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-kbl2/igt@kms_universal_plane@universal-plane-pipe-b-functional.html
#### Warnings ####
* igt@gem_mmap_gtt@forked-big-copy-odd:
- shard-iclb: [TIMEOUT][35] ([fdo#109673]) -> [INCOMPLETE][36] ([fdo#107713] / [fdo#109100])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4988/shard-iclb8/igt@gem_mmap_gtt@forked-big-copy-odd.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/shard-iclb5/igt@gem_mmap_gtt@forked-big-copy-odd.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
[fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
[fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
[fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
[fdo#110037]: https://bugs.freedesktop.org/show_bug.cgi?id=110037
[fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (7 -> 6)
------------------------------
Missing (1): shard-skl
Build changes
-------------
* IGT: IGT_4988 -> IGTPW_2982
CI_DRM_6080: 6c6b621677cc0d3616de3dab025d967aa2c43877 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2982: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
IGT_4988: 2f6303d13e09b2457762540383c7f36cecd02bbf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2982/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data
2019-05-14 13:51 ` Ville Syrjälä
@ 2019-05-15 7:50 ` Arkadiusz Hiler
0 siblings, 0 replies; 5+ messages in thread
From: Arkadiusz Hiler @ 2019-05-15 7:50 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: igt-dev
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@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@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-15 7:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14 13:13 [igt-dev] [PATCH i-g-t v2] lib/igt_audio: make audio_signal_detect take const data Simon Ser
2019-05-14 13:51 ` Ville Syrjälä
2019-05-15 7:50 ` Arkadiusz Hiler
2019-05-14 14:21 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_audio: make audio_signal_detect take const data (rev2) Patchwork
2019-05-14 21:10 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.