diff mbox series

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

Message ID 20200417141315.17560-1-lance.lmwang@gmail.com
State Superseded
Headers
Series [FFmpeg-devel,v1] avcodec/bsf: simplify the code |

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lance Wang April 17, 2020, 2:13 p.m. UTC 
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(-)

Comments

Andreas Rheinhardt April 17, 2020, 2:41 p.m. UTC | #1 
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
Lance Wang April 17, 2020, 3:05 p.m. UTC | #2 
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".
Paul B Mahol April 17, 2020, 3:24 p.m. UTC | #3 
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".
Lance Wang April 17, 2020, 11:50 p.m. UTC | #4 
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 mbox series

Patch

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);