<!DOCTYPE html><html><head><title></title><style type="text/css">p.MsoNormal,p.MsoNoSpacing{margin:0}</style></head><body><div>Hello Farid,<br></div><div><br></div><div>I just pushed your patches, after removing the extra line of log.<br></div><div><br></div><div>Thanks a lot !<br></div><div><br></div><div>On Mon, Jan 27, 2020, at 14:17, Farid HAMMANE wrote:<br></div><blockquote type="cite" id="qt"><div dir="ltr"><div dir="ltr"><br></div><div><br></div><div class="qt-gmail_quote"><div class="qt-gmail_attr" dir="ltr">On Fri, Jan 24, 2020 at 7:58 PM Thomas Guillem <<a 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-color:rgb(204, 204, 204);border-left-style:solid;border-left-width:1px;padding-left:1ex;" class="qt-gmail_quote"><div><u></u><br></div><div><div><br></div><div><br></div><div>On Fri, Jan 24, 2020, at 18:16, Farid HAMMANE wrote:<br></div><blockquote id="qt-m_-6090620838302463619m_-3363864379586036636gmail-m_8185022122068979617qt" type="cite"><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><div dir="ltr">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 style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-color:rgb(204, 204, 204);border-left-style:solid;border-left-width:1px;padding-left:1ex;"><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-color:rgb(204, 204, 204);border-left-style:solid;border-left-width:1px;padding-left:1ex;"><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). <br></div><div><br></div></div></blockquote><div><br></div><div>Hello Thomas,<br></div><div><br></div><div>To be sure that I understood your answer correctly, 
are you expecting a new patch from me ?<br></div><div><br></div><div>Best regards<br></div><div>Farid<br></div><blockquote style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-color:rgb(204, 204, 204);border-left-style:solid;border-left-width:1px;padding-left:1ex;" class="qt-gmail_quote"><div><blockquote id="qt-m_-6090620838302463619m_-3363864379586036636gmail-m_8185022122068979617qt" type="cite"><div dir="ltr"><div><div> <br></div><blockquote style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-color:rgb(204, 204, 204);border-left-style:solid;border-left-width:1px;padding-left:1ex;"><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-color:rgb(204, 204, 204);border-left-style:solid;border-left-width:1px;padding-left:1ex;"><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 href="https://mailman.videolan.org/listinfo/vlc-devel" rel="noreferrer">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 href="https://mailman.videolan.org/listinfo/vlc-devel" rel="noreferrer">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><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></div></blockquote><div><br></div></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>