<div dir="ltr"><div dir="ltr"><div>Hello Thomas,</div><div><br></div><div>Thank you for your review<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 21, 2020 at 8:43 AM Thomas Guillem <<a href="mailto:thomas@gllm.fr">thomas@gllm.fr</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On Mon, Jan 20, 2020, at 18:10, Farid Hammane wrote:<br>
> amem now supports three formats: S16N, S32N and FL32.<br>
> <br>
> Signed-off-by: Farid Hammane <<a href="mailto:farid.hammane@gmail.com" target="_blank">farid.hammane@gmail.com</a>><br>
> ---<br>
> modules/audio_output/amem.c | 57 +++++++++++++++++++++++++++++++++++--<br>
> 1 file changed, 55 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/modules/audio_output/amem.c b/modules/audio_output/amem.c<br>
> index 7f144ac334..d16f7e2022 100644<br>
> --- a/modules/audio_output/amem.c<br>
> +++ b/modules/audio_output/amem.c<br>
> @@ -32,6 +32,10 @@ static void Close (vlc_object_t *);<br>
> <br>
> #define AMEM_SAMPLE_RATE_MAX 384000<br>
> #define AMEM_CHAN_MAX 8<br>
> +#define AMEM_NB_FORMATS 3<br>
> +<br>
> +/* Forward declaration */<br>
> +static const char *const format_list[AMEM_NB_FORMATS];<br>
> <br>
> vlc_module_begin ()<br>
> set_shortname (N_("Audio memory"))<br>
> @@ -43,6 +47,7 @@ vlc_module_begin ()<br>
> <br>
> add_string ("amem-format", "S16N",<br>
> N_("Sample format"), N_("Sample format"), false)<br>
> + change_string_list( format_list, format_list )<br>
> change_private()<br>
> add_integer ("amem-rate", 44100,<br>
> N_("Sample rate"), N_("Sample rate"), false)<br>
> @@ -55,6 +60,18 @@ vlc_module_begin ()<br>
> <br>
> vlc_module_end ()<br>
> <br>
> +static const char *const format_list[AMEM_NB_FORMATS] = {<br>
> + "S16N",<br>
> + "S32N",<br>
> + "FL32",<br>
> +};<br>
> +<br>
> +static const vlc_fourcc_t format_list_fourcc[AMEM_NB_FORMATS] = {<br>
> + VLC_CODEC_S16N,<br>
> + VLC_CODEC_S32N,<br>
> + VLC_CODEC_FL32,<br>
> +};<br>
> +<br>
> typedef struct<br>
> {<br>
> void *opaque;<br>
> @@ -200,6 +217,7 @@ static int Start (audio_output_t *aout, <br>
> audio_sample_format_t *fmt)<br>
> aout_sys_t *sys = aout->sys;<br>
> char format[5] = "S16N";<br>
> unsigned channels;<br>
> + int i_idx;<br>
> <br>
> if (aout_FormatNbChannels(fmt) == 0)<br>
> return VLC_EGENERIC;<br>
> @@ -218,6 +236,29 @@ static int Start (audio_output_t *aout, <br>
> audio_sample_format_t *fmt)<br>
> }<br>
> else<br>
> {<br>
> + char *psz_format;<br>
> +<br>
> + psz_format = var_InheritString (aout, "amem-format");<br>
> + if (psz_format == NULL)<br>
> + {<br>
> + msg_Err(aout, "Not enough memory to get amem-format");<br>
<br>
We don't generally log small allocation errors. You have to be really unlucky to get hit by one of those. Picture/block allocation errors is more likely for example.<br>
<br></blockquote><div><br></div><div>Ok, I'll remove it</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + vlc_mutex_unlock(&sys->lock);<br>
> +<br>
> + return VLC_ENOMEM;<br>
> + }<br>
> +<br>
> + if (strlen(psz_format) != 4) /* fourcc string length */<br>
> + {<br>
> + msg_Err (aout, "Invalid paramameter for amem-format: '%s'",<br>
> + psz_format);<br>
> + free(psz_format);<br>
> + vlc_mutex_unlock(&sys->lock);<br>
> +<br>
> + return VLC_EGENERIC;<br>
> + }<br>
> +<br>
> + strcpy(format, psz_format);<br>
> + free(psz_format);<br>
> fmt->i_rate = sys->rate;<br>
> channels = sys->channels;<br>
> }<br>
> @@ -228,10 +269,23 @@ static int Start (audio_output_t *aout, <br>
> audio_sample_format_t *fmt)<br>
> sys->set_volume(sys->opaque, sys->volume, sys->mute);<br>
> vlc_mutex_unlock(&sys->lock);<br>
> <br>
> + /* amem-format: string to fourcc */<br>
> + for (i_idx = 0; i_idx < AMEM_NB_FORMATS; i_idx++)<br>
> + {<br>
> + if (strncmp(format,<br>
> + format_list[i_idx],<br>
> + strlen(format_list[i_idx])) == 0)<br>
> + {<br>
<br>
I would rather do<br>
<br>
assert(strlen(format_list[i_idx] == 4);<br>
if (strcmp(format, format_list[i_idx]) == 0)<br>
<br>
Since format_list[i_idx] size has to be 4.<br>
<br></blockquote><div><br></div><div><br></div><div>In fact, I used strncmp to add a small defense, but since format_list is a "const char *const", it cannot be different than 4.<br>So:</div><div><br> assert(strlen(format_list[i_idx] == 4);<br><br>is not really needed, and I agree that we can just do an<br><br> if (strcmp(format, format_list[i_idx]) == 0)<br><br>On its side, the length of format might be different than 4. But since it has a size of 5 bytes (greater than format_list), and even if it has no ‘\0’, nothing wrong can happen.</div><div>Finally, I didn't want to test the lenght of the variable format because it can be modified by
sys->setup()
and may not contain any '\0'.<br></div><div></div><div><br></div><div>Is it ok for you to keep only
strcmp() ?<br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + fmt->i_format = format_list_fourcc[i_idx];<br>
> +<br>
> + break;<br>
> + }<br>
> + }<br>
> +<br>
> /* Ensure that format is supported */<br>
> if (fmt->i_rate == 0 || fmt->i_rate > AMEM_SAMPLE_RATE_MAX<br>
> || channels == 0 || channels > AMEM_CHAN_MAX<br>
> - || strcmp(format, "S16N") /* TODO: amem-format */)<br>
> + || i_idx == AMEM_NB_FORMATS)<br>
> {<br>
> msg_Err (aout, "format not supported: %s, %u channel(s), %u <br>
> Hz",<br>
> format, channels, fmt->i_rate);<br>
> @@ -273,7 +327,6 @@ static int Start (audio_output_t *aout, <br>
> audio_sample_format_t *fmt)<br>
> vlc_assert_unreachable();<br>
> }<br>
> <br>
> - fmt->i_format = VLC_CODEC_S16N;<br>
> fmt->channel_type = AUDIO_CHANNEL_TYPE_BITMAP;<br>
> return VLC_SUCCESS;<br>
<br>
Otherwise, fin with me, thanks for this patch<br>
<br></blockquote><div><br></div><div>Thank you</div><div><br></div><div>Best regards</div><div><br></div><div>Farid</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> }<br>
> -- <br>
> 2.20.1<br>
> <br>
> _______________________________________________<br>
> vlc-devel mailing list<br>
> To unsubscribe or modify your subscription options:<br>
> <a href="https://mailman.videolan.org/listinfo/vlc-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
_______________________________________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="https://mailman.videolan.org/listinfo/vlc-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/vlc-devel</a></blockquote></div></div>