| Message ID | 20200417141315.17560-1-lance.lmwang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v1] avcodec/bsf: simplify the code | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
lance.lmwang@gmail.com: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavcodec/bsf.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > index 7b96183e64..c4c939c205 100644 > --- a/libavcodec/bsf.c > +++ b/libavcodec/bsf.c > @@ -533,7 +533,7 @@ end: > int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) > { > AVBSFList *lst; > - char *bsf_str, *buf, *dup, *saveptr; > + char *bsf_str, *buf, *dup; > int ret; > > if (!str) > @@ -548,16 +548,10 @@ int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) > goto end; > } > > - while (1) { > - bsf_str = av_strtok(buf, ",", &saveptr); > - if (!bsf_str) > - break; > - > + while (bsf_str = av_strtok(buf, ",", &buf)) { > ret = bsf_parse_single(bsf_str, lst); > if (ret < 0) > goto end; > - > - buf = NULL; > } > > ret = av_bsf_list_finalize(&lst, bsf_lst); > This is against the documentation of av_strtok() which states: * On the first call to av_strtok(), s should point to the string to * parse, and the value of saveptr is ignored. In subsequent calls, s * should be NULL, and saveptr should be unchanged since the previous * call. It works now, but it is not guaranteed to work. - Andreas
On Fri, Apr 17, 2020 at 04:41:44PM +0200, Andreas Rheinhardt wrote: > lance.lmwang@gmail.com: > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavcodec/bsf.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > > index 7b96183e64..c4c939c205 100644 > > --- a/libavcodec/bsf.c > > +++ b/libavcodec/bsf.c > > @@ -533,7 +533,7 @@ end: > > int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) > > { > > AVBSFList *lst; > > - char *bsf_str, *buf, *dup, *saveptr; > > + char *bsf_str, *buf, *dup; > > int ret; > > > > if (!str) > > @@ -548,16 +548,10 @@ int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) > > goto end; > > } > > > > - while (1) { > > - bsf_str = av_strtok(buf, ",", &saveptr); > > - if (!bsf_str) > > - break; > > - > > + while (bsf_str = av_strtok(buf, ",", &buf)) { > > ret = bsf_parse_single(bsf_str, lst); > > if (ret < 0) > > goto end; > > - > > - buf = NULL; > > } > > > > ret = av_bsf_list_finalize(&lst, bsf_lst); > > > This is against the documentation of av_strtok() which states: > * On the first call to av_strtok(), s should point to the string to > * parse, and the value of saveptr is ignored. In subsequent calls, s > * should be NULL, and saveptr should be unchanged since the previous > * call. > > It works now, but it is not guaranteed to work. I don't know why the subsequent calls, s should be NULL. I think it's willing, not must. If we're clear, why not to make the buf point to the next token, it looks more simple and easy to read. Also, a lot of such case have used in existing code. [lmwang@vpn ffmpeg]$ grep av_strtok libavformat/*.c |grep while libavformat/dashdec.c: while (mpdName = av_strtok(tmp, "/", &tmp)) { libavformat/ftp.c: while(fact = av_strtok(mlsd, ";", &mlsd)) { libavformat/http.c: while ((param = av_strtok(next_param, ";", &next_param))) { libavformat/http.c: while ((cookie = av_strtok(next, "\n", &next)) && !ret) { > > - Andreas > _______________________________________________ > 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 4/17/20, Limin Wang <lance.lmwang@gmail.com> wrote: > On Fri, Apr 17, 2020 at 04:41:44PM +0200, Andreas Rheinhardt wrote: >> lance.lmwang@gmail.com: >> > From: Limin Wang <lance.lmwang@gmail.com> >> > >> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >> > --- >> > libavcodec/bsf.c | 10 ++-------- >> > 1 file changed, 2 insertions(+), 8 deletions(-) >> > >> > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c >> > index 7b96183e64..c4c939c205 100644 >> > --- a/libavcodec/bsf.c >> > +++ b/libavcodec/bsf.c >> > @@ -533,7 +533,7 @@ end: >> > int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) >> > { >> > AVBSFList *lst; >> > - char *bsf_str, *buf, *dup, *saveptr; >> > + char *bsf_str, *buf, *dup; >> > int ret; >> > >> > if (!str) >> > @@ -548,16 +548,10 @@ int av_bsf_list_parse_str(const char *str, >> > AVBSFContext **bsf_lst) >> > goto end; >> > } >> > >> > - while (1) { >> > - bsf_str = av_strtok(buf, ",", &saveptr); >> > - if (!bsf_str) >> > - break; >> > - >> > + while (bsf_str = av_strtok(buf, ",", &buf)) { >> > ret = bsf_parse_single(bsf_str, lst); >> > if (ret < 0) >> > goto end; >> > - >> > - buf = NULL; >> > } >> > >> > ret = av_bsf_list_finalize(&lst, bsf_lst); >> > >> This is against the documentation of av_strtok() which states: >> * On the first call to av_strtok(), s should point to the string to >> * parse, and the value of saveptr is ignored. In subsequent calls, s >> * should be NULL, and saveptr should be unchanged since the previous >> * call. >> >> It works now, but it is not guaranteed to work. > > I don't know why the subsequent calls, s should be NULL. I think it's > willing, > not must. If we're clear, why not to make the buf point to the next token, > it > looks more simple and easy to read. > > Also, a lot of such case have used in existing code. > > [lmwang@vpn ffmpeg]$ grep av_strtok libavformat/*.c |grep while > libavformat/dashdec.c: while (mpdName = av_strtok(tmp, "/", &tmp)) { > libavformat/ftp.c: while(fact = av_strtok(mlsd, ";", &mlsd)) { > libavformat/http.c: while ((param = av_strtok(next_param, ";", > &next_param))) { > libavformat/http.c: while ((cookie = av_strtok(next, "\n", &next)) && > !ret) { > Thanks for listing invalid code lines. > >> >> - Andreas >> _______________________________________________ >> 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". > > -- > Thanks, > Limin Wang > _______________________________________________ > 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 Fri, Apr 17, 2020 at 05:24:32PM +0200, Paul B Mahol wrote: > On 4/17/20, Limin Wang <lance.lmwang@gmail.com> wrote: > > On Fri, Apr 17, 2020 at 04:41:44PM +0200, Andreas Rheinhardt wrote: > >> lance.lmwang@gmail.com: > >> > From: Limin Wang <lance.lmwang@gmail.com> > >> > > >> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > >> > --- > >> > libavcodec/bsf.c | 10 ++-------- > >> > 1 file changed, 2 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > >> > index 7b96183e64..c4c939c205 100644 > >> > --- a/libavcodec/bsf.c > >> > +++ b/libavcodec/bsf.c > >> > @@ -533,7 +533,7 @@ end: > >> > int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) > >> > { > >> > AVBSFList *lst; > >> > - char *bsf_str, *buf, *dup, *saveptr; > >> > + char *bsf_str, *buf, *dup; > >> > int ret; > >> > > >> > if (!str) > >> > @@ -548,16 +548,10 @@ int av_bsf_list_parse_str(const char *str, > >> > AVBSFContext **bsf_lst) > >> > goto end; > >> > } > >> > > >> > - while (1) { > >> > - bsf_str = av_strtok(buf, ",", &saveptr); > >> > - if (!bsf_str) > >> > - break; > >> > - > >> > + while (bsf_str = av_strtok(buf, ",", &buf)) { > >> > ret = bsf_parse_single(bsf_str, lst); > >> > if (ret < 0) > >> > goto end; > >> > - > >> > - buf = NULL; > >> > } > >> > > >> > ret = av_bsf_list_finalize(&lst, bsf_lst); > >> > > >> This is against the documentation of av_strtok() which states: > >> * On the first call to av_strtok(), s should point to the string to > >> * parse, and the value of saveptr is ignored. In subsequent calls, s > >> * should be NULL, and saveptr should be unchanged since the previous > >> * call. > >> > >> It works now, but it is not guaranteed to work. > > > > I don't know why the subsequent calls, s should be NULL. I think it's > > willing, > > not must. If we're clear, why not to make the buf point to the next token, > > it > > looks more simple and easy to read. > > > > Also, a lot of such case have used in existing code. > > > > [lmwang@vpn ffmpeg]$ grep av_strtok libavformat/*.c |grep while > > libavformat/dashdec.c: while (mpdName = av_strtok(tmp, "/", &tmp)) { > > libavformat/ftp.c: while(fact = av_strtok(mlsd, ";", &mlsd)) { > > libavformat/http.c: while ((param = av_strtok(next_param, ";", > > &next_param))) { > > libavformat/http.c: while ((cookie = av_strtok(next, "\n", &next)) && > > !ret) { > > > > Thanks for listing invalid code lines. Sorry, have checked with man of strtok_r, it has the same requirement, so it's better to follow its rule: On the first call to strtok_r(), str should point to the string to be parsed, and the value of saveptr is ignored. In subsequent calls, str should be NULL, and saveptr should be unchanged since the previous call. I'll try to submit patches to fix these invalid case if you're OK. > > > > >> > >> - Andreas > >> _______________________________________________ > >> 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". > > > > -- > > Thanks, > > Limin Wang > > _______________________________________________ > > 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 7b96183e64..c4c939c205 100644 --- a/libavcodec/bsf.c +++ b/libavcodec/bsf.c @@ -533,7 +533,7 @@ end: int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) { AVBSFList *lst; - char *bsf_str, *buf, *dup, *saveptr; + char *bsf_str, *buf, *dup; int ret; if (!str) @@ -548,16 +548,10 @@ int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst) goto end; } - while (1) { - bsf_str = av_strtok(buf, ",", &saveptr); - if (!bsf_str) - break; - + while (bsf_str = av_strtok(buf, ",", &buf)) { ret = bsf_parse_single(bsf_str, lst); if (ret < 0) goto end; - - buf = NULL; } ret = av_bsf_list_finalize(&lst, bsf_lst);