[vlc-devel] [PATCH] amem: Add support for S32N and FL32

Mon Jan 27 14:20:08 CET 2020

Hello Farid,

I just pushed your patches, after removing the extra line of log.

Thanks a lot !

On Mon, Jan 27, 2020, at 14:17, Farid HAMMANE wrote:
> 
> 
> On Fri, Jan 24, 2020 at 7:58 PM Thomas Guillem <thomas at gllm.fr> wrote:
>> __
>> 
>> 
>> On Fri, Jan 24, 2020, at 18:16, Farid HAMMANE wrote:
>>> Hello Thomas,
>>> 
>>> Thank you for your review
>>> 
>>> On Tue, Jan 21, 2020 at 8:43 AM Thomas Guillem <thomas at gllm.fr> wrote:
>>>> 
>>>> 
>>>> On Mon, Jan 20, 2020, at 18:10, Farid Hammane wrote:
>>>> > amem now supports three formats: S16N, S32N and FL32.
>>>> > 
>>>> > Signed-off-by: Farid Hammane <farid.hammane at gmail.com>
>>>> > ---
>>>> > modules/audio_output/amem.c | 57 +++++++++++++++++++++++++++++++++++--
>>>> > 1 file changed, 55 insertions(+), 2 deletions(-)
>>>> > 
>>>> > diff --git a/modules/audio_output/amem.c b/modules/audio_output/amem.c
>>>> > index 7f144ac334..d16f7e2022 100644
>>>> > --- a/modules/audio_output/amem.c
>>>> > +++ b/modules/audio_output/amem.c
>>>> > @@ -32,6 +32,10 @@ static void Close (vlc_object_t *);
>>>> > 
>>>> > #define AMEM_SAMPLE_RATE_MAX 384000
>>>> > #define AMEM_CHAN_MAX 8
>>>> > +#define AMEM_NB_FORMATS 3
>>>> > +
>>>> > +/* Forward declaration */
>>>> > +static const char *const format_list[AMEM_NB_FORMATS];
>>>> > 
>>>> > vlc_module_begin ()
>>>> > set_shortname (N_("Audio memory"))
>>>> > @@ -43,6 +47,7 @@ vlc_module_begin ()
>>>> > 
>>>> > add_string ("amem-format", "S16N",
>>>> > N_("Sample format"), N_("Sample format"), false)
>>>> > + change_string_list( format_list, format_list )
>>>> > change_private()
>>>> > add_integer ("amem-rate", 44100,
>>>> > N_("Sample rate"), N_("Sample rate"), false)
>>>> > @@ -55,6 +60,18 @@ vlc_module_begin ()
>>>> > 
>>>> > vlc_module_end ()
>>>> > 
>>>> > +static const char *const format_list[AMEM_NB_FORMATS] = {
>>>> > + "S16N",
>>>> > + "S32N",
>>>> > + "FL32",
>>>> > +};
>>>> > +
>>>> > +static const vlc_fourcc_t format_list_fourcc[AMEM_NB_FORMATS] = {
>>>> > + VLC_CODEC_S16N,
>>>> > + VLC_CODEC_S32N,
>>>> > + VLC_CODEC_FL32,
>>>> > +};
>>>> > +
>>>> > typedef struct
>>>> > {
>>>> > void *opaque;
>>>> > @@ -200,6 +217,7 @@ static int Start (audio_output_t *aout, 
>>>> > audio_sample_format_t *fmt)
>>>> > aout_sys_t *sys = aout->sys;
>>>> > char format[5] = "S16N";
>>>> > unsigned channels;
>>>> > + int i_idx;
>>>> > 
>>>> > if (aout_FormatNbChannels(fmt) == 0)
>>>> > return VLC_EGENERIC;
>>>> > @@ -218,6 +236,29 @@ static int Start (audio_output_t *aout, 
>>>> > audio_sample_format_t *fmt)
>>>> > }
>>>> > else
>>>> > {
>>>> > + char *psz_format;
>>>> > +
>>>> > + psz_format = var_InheritString (aout, "amem-format");
>>>> > + if (psz_format == NULL)
>>>> > + {
>>>> > + msg_Err(aout, "Not enough memory to get amem-format");
>>>> 
>>>> 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.
>>>> 
>>> 
>>> Ok, I'll remove it
>>> 
>>> 
>>>> > + vlc_mutex_unlock(&sys->lock);
>>>> > +
>>>> > + return VLC_ENOMEM;
>>>> > + }
>>>> > +
>>>> > + if (strlen(psz_format) != 4) /* fourcc string length */
>>>> > + {
>>>> > + msg_Err (aout, "Invalid paramameter for amem-format: '%s'",
>>>> > + psz_format);
>>>> > + free(psz_format);
>>>> > + vlc_mutex_unlock(&sys->lock);
>>>> > +
>>>> > + return VLC_EGENERIC;
>>>> > + }
>>>> > +
>>>> > + strcpy(format, psz_format);
>>>> > + free(psz_format);
>>>> > fmt->i_rate = sys->rate;
>>>> > channels = sys->channels;
>>>> > }
>>>> > @@ -228,10 +269,23 @@ static int Start (audio_output_t *aout, 
>>>> > audio_sample_format_t *fmt)
>>>> > sys->set_volume(sys->opaque, sys->volume, sys->mute);
>>>> > vlc_mutex_unlock(&sys->lock);
>>>> > 
>>>> > + /* amem-format: string to fourcc */
>>>> > + for (i_idx = 0; i_idx < AMEM_NB_FORMATS; i_idx++)
>>>> > + {
>>>> > + if (strncmp(format,
>>>> > + format_list[i_idx],
>>>> > + strlen(format_list[i_idx])) == 0)
>>>> > + {
>>>> 
>>>> I would rather do
>>>> 
>>>> assert(strlen(format_list[i_idx] == 4);
>>>> if (strcmp(format, format_list[i_idx]) == 0)
>>>> 
>>>> Since format_list[i_idx] size has to be 4.
>>>> 
>>> 
>>> 
>>> 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.
>>> So:
>>> 
>>>  assert(strlen(format_list[i_idx] == 4);
>>> 
>>> is not really needed, and I agree that we can just do an
>>> 
>>>  if (strcmp(format, format_list[i_idx]) == 0)
>>> 
>>> 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.
>>> 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'.
>>> 
>>> 
>>> Is it ok for you to keep only strcmp() ?
>> 
>> Yes, ok for me, I forgot that \0 could be missing.
>> I will merge it on Monday (afk). 
>> 
> 
> Hello Thomas,
> 
> To be sure that I understood your answer correctly, are you expecting a new patch from me ?
> 
> Best regards
> Farid
>>> 
>>>> > + fmt->i_format = format_list_fourcc[i_idx];
>>>> > +
>>>> > + break;
>>>> > + }
>>>> > + }
>>>> > +
>>>> > /* Ensure that format is supported */
>>>> > if (fmt->i_rate == 0 || fmt->i_rate > AMEM_SAMPLE_RATE_MAX
>>>> > || channels == 0 || channels > AMEM_CHAN_MAX
>>>> > - || strcmp(format, "S16N") /* TODO: amem-format */)
>>>> > + || i_idx == AMEM_NB_FORMATS)
>>>> > {
>>>> > msg_Err (aout, "format not supported: %s, %u channel(s), %u 
>>>> > Hz",
>>>> > format, channels, fmt->i_rate);
>>>> > @@ -273,7 +327,6 @@ static int Start (audio_output_t *aout, 
>>>> > audio_sample_format_t *fmt)
>>>> > vlc_assert_unreachable();
>>>> > }
>>>> > 
>>>> > - fmt->i_format = VLC_CODEC_S16N;
>>>> > fmt->channel_type = AUDIO_CHANNEL_TYPE_BITMAP;
>>>> > return VLC_SUCCESS;
>>>> 
>>>> Otherwise, fin with me, thanks for this patch
>>>> 
>>> 
>>> Thank you
>>> 
>>> Best regards
>>> 
>>> Farid
>>> 
>>>> > }
>>>> > -- 
>>>> > 2.20.1
>>>> > 
>>>> > _______________________________________________
>>>> > vlc-devel mailing list
>>>> > To unsubscribe or modify your subscription options:
>>>> > https://mailman.videolan.org/listinfo/vlc-devel
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> 
>> _______________________________________________
>>  vlc-devel mailing list
>>  To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200127/0c965ffb/attachment.html>