<!DOCTYPE html><html><head><title></title><style type="text/css">p.MsoNormal,p.MsoNoSpacing{margin:0}</style></head><body><div><br></div><div><br></div><div>On Fri, Jan 24, 2020, at 18:16, Farid HAMMANE wrote:<br></div><blockquote type="cite" id="qt"><div dir="ltr"><div dir="ltr"><div>Hello Thomas,<br></div><div><br></div><div>Thank you for your review<br></div></div><div><br></div><div class="qt-gmail_quote"><div class="qt-gmail_attr" dir="ltr">On Tue, Jan 21, 2020 at 8:43 AM Thomas Guillem <<a class="" href="mailto:thomas@gllm.fr">thomas@gllm.fr</a>> wrote:<br></div><blockquote style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1ex;" class="qt-gmail_quote"><div><br></div><div><br></div><div>On Mon, Jan 20, 2020, at 18:10, Farid Hammane wrote:<br></div><div> > amem now supports three formats: S16N, S32N and FL32.<br></div><div> > <br></div><div> > Signed-off-by: Farid Hammane <<a href="mailto:farid.hammane@gmail.com">farid.hammane@gmail.com</a>><br></div><div> > ---<br></div><div> >  modules/audio_output/amem.c | 57 +++++++++++++++++++++++++++++++++++--<br></div><div> >  1 file changed, 55 insertions(+), 2 deletions(-)<br></div><div> > <br></div><div> > diff --git a/modules/audio_output/amem.c b/modules/audio_output/amem.c<br></div><div> > index 7f144ac334..d16f7e2022 100644<br></div><div> > --- a/modules/audio_output/amem.c<br></div><div> > +++ b/modules/audio_output/amem.c<br></div><div> > @@ -32,6 +32,10 @@ static void Close (vlc_object_t *);<br></div><div> >  <br></div><div> >  #define AMEM_SAMPLE_RATE_MAX 384000<br></div><div> >  #define AMEM_CHAN_MAX 8<br></div><div> > +#define AMEM_NB_FORMATS 3<br></div><div> > +<br></div><div> > +/* Forward declaration */<br></div><div> > +static const char *const format_list[AMEM_NB_FORMATS];<br></div><div> >  <br></div><div> >  vlc_module_begin ()<br></div><div> >      set_shortname (N_("Audio memory"))<br></div><div> > @@ -43,6 +47,7 @@ vlc_module_begin ()<br></div><div> >  <br></div><div> >      add_string ("amem-format", "S16N",<br></div><div> >                  N_("Sample format"), N_("Sample format"), false)<br></div><div> > +        change_string_list( format_list, format_list )<br></div><div> >          change_private()<br></div><div> >      add_integer ("amem-rate", 44100,<br></div><div> >                   N_("Sample rate"), N_("Sample rate"), false)<br></div><div> > @@ -55,6 +60,18 @@ vlc_module_begin ()<br></div><div> >  <br></div><div> >  vlc_module_end ()<br></div><div> >  <br></div><div> > +static const char *const format_list[AMEM_NB_FORMATS] = {<br></div><div> > +    "S16N",<br></div><div> > +    "S32N",<br></div><div> > +    "FL32",<br></div><div> > +};<br></div><div> > +<br></div><div> > +static const vlc_fourcc_t format_list_fourcc[AMEM_NB_FORMATS] = {<br></div><div> > +    VLC_CODEC_S16N,<br></div><div> > +    VLC_CODEC_S32N,<br></div><div> > +    VLC_CODEC_FL32,<br></div><div> > +};<br></div><div> > +<br></div><div> >  typedef struct<br></div><div> >  {<br></div><div> >      void *opaque;<br></div><div> > @@ -200,6 +217,7 @@ static int Start (audio_output_t *aout, <br></div><div> > audio_sample_format_t *fmt)<br></div><div> >      aout_sys_t *sys = aout->sys;<br></div><div> >      char format[5] = "S16N";<br></div><div> >      unsigned channels;<br></div><div> > +    int i_idx;<br></div><div> >  <br></div><div> >      if (aout_FormatNbChannels(fmt) == 0)<br></div><div> >          return VLC_EGENERIC;<br></div><div> > @@ -218,6 +236,29 @@ static int Start (audio_output_t *aout, <br></div><div> > audio_sample_format_t *fmt)<br></div><div> >      }<br></div><div> >      else<br></div><div> >      {<br></div><div> > +        char *psz_format;<br></div><div> > +<br></div><div> > +        psz_format = var_InheritString (aout, "amem-format");<br></div><div> > +        if (psz_format == NULL)<br></div><div> > +        {<br></div><div> > +            msg_Err(aout, "Not enough memory to get amem-format");<br></div><div> <br></div><div> 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></div><div> <br></div></blockquote><div><br></div><div>Ok, I'll remove it<br></div><div><br></div><div> <br></div><blockquote style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1ex;" class="qt-gmail_quote"><div>> +            vlc_mutex_unlock(&sys->lock);<br></div><div> > +<br></div><div> > +            return VLC_ENOMEM;<br></div><div> > +        }<br></div><div> > +<br></div><div> > +        if (strlen(psz_format) != 4) /* fourcc string length */<br></div><div> > +        {<br></div><div> > +            msg_Err (aout, "Invalid paramameter for amem-format: '%s'",<br></div><div> > +                     psz_format);<br></div><div> > +            free(psz_format);<br></div><div> > +            vlc_mutex_unlock(&sys->lock);<br></div><div> > +<br></div><div> > +            return VLC_EGENERIC;<br></div><div> > +        }<br></div><div> > +<br></div><div> > +        strcpy(format, psz_format);<br></div><div> > +        free(psz_format);<br></div><div> >          fmt->i_rate = sys->rate;<br></div><div> >          channels = sys->channels;<br></div><div> >      }<br></div><div> > @@ -228,10 +269,23 @@ static int Start (audio_output_t *aout, <br></div><div> > audio_sample_format_t *fmt)<br></div><div> >          sys->set_volume(sys->opaque, sys->volume, sys->mute);<br></div><div> >      vlc_mutex_unlock(&sys->lock);<br></div><div> >  <br></div><div> > +    /* amem-format: string to fourcc */<br></div><div> > +    for (i_idx = 0; i_idx < AMEM_NB_FORMATS; i_idx++)<br></div><div> > +    {<br></div><div> > +        if (strncmp(format,<br></div><div> > +                    format_list[i_idx],<br></div><div> > +                    strlen(format_list[i_idx])) == 0)<br></div><div> > +        {<br></div><div> <br></div><div> I would rather do<br></div><div> <br></div><div> assert(strlen(format_list[i_idx] == 4);<br></div><div> if (strcmp(format, format_list[i_idx]) == 0)<br></div><div> <br></div><div> Since format_list[i_idx] size has to be 4.<br></div><div> <br></div></blockquote><div><br></div><div><br></div><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></div><div>So:<br></div></div><div><div><br></div><div>    assert(strlen(format_list[i_idx] == 4);<br></div><div><br></div><div>is not really needed, and I agree that we can just do an<br></div><div><br></div><div>    if (strcmp(format, format_list[i_idx]) == 0)<br></div><div><br></div><div>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.<br></div></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><br></div><div><br></div><div><div>Is it ok for you to keep only 
strcmp() ?<br></div></div></div></div></blockquote><div><br></div><div>Yes, ok for me, I forgot that \0 could be missing.<br></div><div>I will merge it on Monday (afk). </div><div><br></div><blockquote type="cite" id="qt"><div dir="ltr"><div class="qt-gmail_quote"><div> <br></div><blockquote style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1ex;" class="qt-gmail_quote"><div>> +            fmt->i_format = format_list_fourcc[i_idx];<br></div><div> > +<br></div><div> > +            break;<br></div><div> > +        }<br></div><div> > +    }<br></div><div> > +<br></div><div> >      /* Ensure that format is supported */<br></div><div> >      if (fmt->i_rate == 0 || fmt->i_rate > AMEM_SAMPLE_RATE_MAX<br></div><div> >       || channels == 0 || channels > AMEM_CHAN_MAX<br></div><div> > -     || strcmp(format, "S16N") /* TODO: amem-format */)<br></div><div> > +     || i_idx == AMEM_NB_FORMATS)<br></div><div> >      {<br></div><div> >          msg_Err (aout, "format not supported: %s, %u channel(s), %u <br></div><div> > Hz",<br></div><div> >                   format, channels, fmt->i_rate);<br></div><div> > @@ -273,7 +327,6 @@ static int Start (audio_output_t *aout, <br></div><div> > audio_sample_format_t *fmt)<br></div><div> >              vlc_assert_unreachable();<br></div><div> >      }<br></div><div> >  <br></div><div> > -    fmt->i_format = VLC_CODEC_S16N;<br></div><div> >      fmt->channel_type = AUDIO_CHANNEL_TYPE_BITMAP;<br></div><div> >      return VLC_SUCCESS;<br></div><div> <br></div><div> Otherwise, fin with me, thanks for this patch<br></div><div> <br></div></blockquote><div><br></div><div>Thank you<br></div><div><br></div><div>Best regards<br></div><div><br></div><div>Farid<br></div><div><br></div><blockquote style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1ex;" class="qt-gmail_quote"><div>>  }<br></div><div> > -- <br></div><div> > 2.20.1<br></div><div> > <br></div><div> > _______________________________________________<br></div><div> > vlc-devel mailing list<br></div><div> > To unsubscribe or modify your subscription options:<br></div><div> > <a rel="noreferrer" href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></div><div> _______________________________________________<br></div><div> vlc-devel mailing list<br></div><div> To unsubscribe or modify your subscription options:<br></div><div> <a rel="noreferrer" href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></div></blockquote></div></div><div>_______________________________________________<br></div><div>vlc-devel mailing list<br></div><div>To unsubscribe or modify your subscription options:<br></div><div>https://mailman.videolan.org/listinfo/vlc-devel<br></div></blockquote><div><br></div></body></html>