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

[Farid HAMMANE]

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() ?



> > +            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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200124/df30172d/attachment.html>