[FFmpeg-devel] [PATCH] avcodec/bsf: switch to av_get_token to parse bsf list string

[James Almer]

On 7/3/2021 8:42 AM, Gyan Doshi wrote:
> The recently added setts bsf makes use of the eval API whose
> expressions can contain commas. The existing parsing in
> av_bsf_list_parse_str() uses av_strtok to naively split
> the string at commas, thus preventing the use of setts filter
> with expressions containing commas.
> ---
>   libavcodec/bsf.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 9d67ea5395..726911785d 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -520,7 +520,8 @@ static int bsf_parse_single(char *str, AVBSFList *bsf_lst)
>   int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
>   {
>       AVBSFList *lst;
> -    char *bsf_str, *buf, *dup, *saveptr;
> +    char *bsf_str, *dup;
> +    const char *buf;
>       int ret;
>   
>       if (!str)
> @@ -530,18 +531,18 @@ int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
>       if (!lst)
>           return AVERROR(ENOMEM);
>   
> -    if (!(dup = buf = av_strdup(str))) {
> +    if (!(buf = dup = av_strdup(str))) {

Is this av_strdup() even necessary? You could let av_get_token() update 
str just fine and remove both buf and dup. Or maybe just use a copy of 
the pointer in buf.

>           ret = AVERROR(ENOMEM);
>           goto end;
>       }
>   
> -    while (bsf_str = av_strtok(buf, ",", &saveptr)) {
> +    do {
> +        bsf_str = av_get_token(&buf, ",");

You can reduce the scope of bsf_str now, so declare it here.

>           ret = bsf_parse_single(bsf_str, lst);
> +        av_free(bsf_str);
>           if (ret < 0)
>               goto end;
> -
> -        buf = NULL;
> -    }
> +    } while (*buf == ',' && buf++);

You can simplify this into while (*++buf) or (*++str) if you remove the 
av_strdup(). It will also preserve the existing behavior of discarding 
the last comma in the string if it's the last character, instead of 
aborting with EINVAL.

>   
>       ret = av_bsf_list_finalize(&lst, bsf_lst);
>   end:
>