[FFmpeg-devel] [PATCH v1] avcodec/bsf: simplify the code

[Limin Wang]

On Fri, Apr 17, 2020 at 05:24:32PM +0200, Paul B Mahol wrote:
> On 4/17/20, Limin Wang <lance.lmwang at gmail.com> wrote:
> > On Fri, Apr 17, 2020 at 04:41:44PM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang at gmail.com:
> >> > From: Limin Wang <lance.lmwang at gmail.com>
> >> >
> >> > Signed-off-by: Limin Wang <lance.lmwang at 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 at 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 at ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >
> > --
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang