| Message ID | 20210703114222.5684-1-ffmpeg@gyani.pro |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/bsf: switch to av_get_token to parse bsf list string | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
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: >
On 2021-07-03 19:47, James Almer wrote: > 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(). Won't this go out of bounds at the end? > 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: >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 7/3/2021 12:48 PM, Gyan Doshi wrote: > > > On 2021-07-03 19:47, James Almer wrote: >> 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(). > > Won't this go out of bounds at the end? Yeah, good catch. Change it to while (*buf && *++buf) then, which will also preserve the existing behavior. > > >> 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: >>> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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))) { ret = AVERROR(ENOMEM); goto end; } - while (bsf_str = av_strtok(buf, ",", &saveptr)) { + do { + bsf_str = av_get_token(&buf, ","); ret = bsf_parse_single(bsf_str, lst); + av_free(bsf_str); if (ret < 0) goto end; - - buf = NULL; - } + } while (*buf == ',' && buf++); ret = av_bsf_list_finalize(&lst, bsf_lst); end: