diff mbox

[FFmpeg-devel] avutil/avstring: improve av_strreplace implement

Message ID 20170402144909.51374-1-lq@chinaffmpeg.org
State Superseded
Headers show

Commit Message

Steven Liu April 2, 2017, 2:49 p.m. UTC
Use AVBprint to implementing av_strreplace

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavutil/avstring.c | 74 ++++++----------------------------------------------
 1 file changed, 8 insertions(+), 66 deletions(-)

Comments

Steven Liu April 2, 2017, 2:50 p.m. UTC | #1
2017-04-02 22:49 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>:

> Use AVBprint to implementing av_strreplace
>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavutil/avstring.c | 74 ++++++------------------------
> ----------------------
>  1 file changed, 8 insertions(+), 66 deletions(-)
>
> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> index 52e6e6cd13..55f3bee94c 100644
> --- a/libavutil/avstring.c
> +++ b/libavutil/avstring.c
> @@ -233,78 +233,20 @@ int av_strncasecmp(const char *a, const char *b,
> size_t n)
>
>  char *av_strreplace(const char *str, const char *from, const char *to)
>  {
> -    /* Adjust each of the below values to suit your needs. */
> -    /* Increment positions cache size initially by this number. */
> -    size_t cache_sz_inc = 16;
> -    /* Thereafter, each time capacity needs to be increased,
> -     * multiply the increment by this factor. */
> -    const size_t cache_sz_inc_factor = 3;
> -    /* But never increment capacity by more than this number. */
> -    const size_t cache_sz_inc_max = 1048576;
> -
> -    char *pret, *ret = NULL;
> +    char *ret = NULL;
>      const char *pstr2, *pstr = str;
> -    size_t i, count = 0;
> -    uintptr_t *pos_cache_tmp, *pos_cache = NULL;
> -    size_t cache_sz = 0;
> -    size_t cpylen, orglen, retlen, tolen, fromlen = strlen(from);
> +    size_t tolen = strlen(to), fromlen = strlen(from);
> +    AVBPrint pbuf;
>
> -    /* Find all matches and cache their positions. */
> +    av_bprint_init(&pbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
>      while ((pstr2 = av_stristr(pstr, from))) {
> -        count++;
> -        /* Increase the cache size when necessary. */
> -        if (cache_sz < count) {
> -            cache_sz += cache_sz_inc;
> -            pos_cache_tmp = av_realloc(pos_cache, sizeof(*pos_cache) *
> cache_sz);
> -            if (!pos_cache_tmp) {
> -                goto end_strreplace;
> -            } else pos_cache = pos_cache_tmp;
> -            cache_sz_inc *= cache_sz_inc_factor;
> -            if (cache_sz_inc > cache_sz_inc_max) {
> -                cache_sz_inc = cache_sz_inc_max;
> -            }
> -        }
> -
> -        pos_cache[count-1] = pstr2 - str;
> +        av_bprint_append_data(&pbuf, pstr, pstr2 - pstr);
>          pstr = pstr2 + fromlen;
> +        av_bprint_append_data(&pbuf, to, tolen);
>      }
> -    orglen = pstr - str + strlen(pstr);
> -    /* Allocate memory for the post-replacement string. */
> -    if (count > 0) {
> -        tolen = strlen(to);
> -        retlen = orglen + (tolen - fromlen) * count;
> -    } else {
> -        retlen = orglen;
> -    }
> -    ret = av_malloc(retlen + 1);
> -    if (!ret) {
> -        goto end_strreplace;
> -    }
> -
> -    if (!count) {
> -        /* If no matches, then just duplicate the string. */
> -        av_strlcpy(ret, str, retlen + 1);
> -    } else {
> -        /* Otherwise, duplicate the string whilst performing
> -         * the replacements using the position cache. */
> -        pret = ret;
> -        memcpy(pret, str, pos_cache[0]);
> -        pret += pos_cache[0];
> -        for (i = 0; i < count; i++) {
> -            memcpy(pret, to, tolen);
> -            pret += tolen;
> -            pstr = str + pos_cache[i] + fromlen;
> -            cpylen = (i == count-1 ? orglen : pos_cache[i+1]) -
> pos_cache[i] - fromlen;
> -            memcpy(pret, pstr, cpylen);
> -            pret += cpylen;
> -        }
> -        ret[retlen] = '\0';
> -    }
> +    av_bprint_append_data(&pbuf, pstr, strlen(pstr));
> +    av_bprint_finalize(&pbuf, &ret);
>
> -end_strreplace:
> -    /* Free the cache and return the post-replacement string,
> -     * which will be NULL in the event of an error. */
> -    av_free(pos_cache);
>      return ret;
>  }
>
> --
> 2.11.0 (Apple Git-81)
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>



I have implementation the av_strreplace use AVBprint, the API context now
is:

char *av_strreplace(const char *str, const char *from, const char *to)
{
    char *ret = NULL;
    const char *pstr2, *pstr = str;
    size_t tolen = strlen(to), fromlen = strlen(from);
    AVBPrint pbuf;

    av_bprint_init(&pbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
    while ((pstr2 = av_stristr(pstr, from))) {
        av_bprint_append_data(&pbuf, pstr, pstr2 - pstr);
        pstr = pstr2 + fromlen;
        av_bprint_append_data(&pbuf, to, tolen);
    }
    av_bprint_append_data(&pbuf, pstr, strlen(pstr));
    av_bprint_finalize(&pbuf, &ret);

    return ret;
}
Nicolas George April 2, 2017, 3:18 p.m. UTC | #2
Le tridi 13 germinal, an CCXXV, Steven Liu a écrit :
> I have implementation the av_strreplace use AVBprint, the API context now
> is:

No need to Cc mo. On the other hand, sending the function itself instead
of the diff makes it easier in this case, thanks.

Nice. Just one issue:

> char *av_strreplace(const char *str, const char *from, const char *to)
> {
>     char *ret = NULL;
>     const char *pstr2, *pstr = str;
>     size_t tolen = strlen(to), fromlen = strlen(from);
>     AVBPrint pbuf;
> 
>     av_bprint_init(&pbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
>     while ((pstr2 = av_stristr(pstr, from))) {
>         av_bprint_append_data(&pbuf, pstr, pstr2 - pstr);
>         pstr = pstr2 + fromlen;
>         av_bprint_append_data(&pbuf, to, tolen);
>     }
>     av_bprint_append_data(&pbuf, pstr, strlen(pstr));

>     av_bprint_finalize(&pbuf, &ret);

Just before that, a check with av_bprint_is_complete() is needed because
some malloc may have failed: if it is not complete, av_bprint_finalize()
to NULL to free the buffer and return NULL.

> 
>     return ret;
> }

Also, did you consider extending libavutil/tests/avstring.c to cover
this new function?

Regards,
Steven Liu April 2, 2017, 4:06 p.m. UTC | #3
2017-04-02 23:18 GMT+08:00 Nicolas George <george@nsup.org>:

> Le tridi 13 germinal, an CCXXV, Steven Liu a écrit :
> > I have implementation the av_strreplace use AVBprint, the API context now
> > is:
>
> No need to Cc mo. On the other hand, sending the function itself instead
> of the diff makes it easier in this case, thanks.
>
> Nice. Just one issue:
>
> > char *av_strreplace(const char *str, const char *from, const char *to)
> > {
> >     char *ret = NULL;
> >     const char *pstr2, *pstr = str;
> >     size_t tolen = strlen(to), fromlen = strlen(from);
> >     AVBPrint pbuf;
> >
> >     av_bprint_init(&pbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
> >     while ((pstr2 = av_stristr(pstr, from))) {
> >         av_bprint_append_data(&pbuf, pstr, pstr2 - pstr);
> >         pstr = pstr2 + fromlen;
> >         av_bprint_append_data(&pbuf, to, tolen);
> >     }
> >     av_bprint_append_data(&pbuf, pstr, strlen(pstr));
>
> >     av_bprint_finalize(&pbuf, &ret);
>
> Just before that, a check with av_bprint_is_complete() is needed because
> some malloc may have failed: if it is not complete, av_bprint_finalize()
> to NULL to free the buffer and return NULL.
>
> >
> >     return ret;
> > }
>
> Also, did you consider extending libavutil/tests/avstring.c to cover
> this new function?
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
What about this one:


char *av_strreplace(const char *str, const char *from, const char *to)
{
    char *ret = NULL;
    const char *pstr2, *pstr = str;
    size_t tolen = strlen(to), fromlen = strlen(from);
    AVBPrint pbuf;

    av_bprint_init(&pbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
    while ((pstr2 = av_stristr(pstr, from))) {
        av_bprint_append_data(&pbuf, pstr, pstr2 - pstr);
        pstr = pstr2 + fromlen;
        av_bprint_append_data(&pbuf, to, tolen);
    }
    av_bprint_append_data(&pbuf, pstr, strlen(pstr));
    if (!av_bprint_is_complete(&pbuf)) {
        av_bprint_finalize(&pbuf, NULL);
    } else {
        av_bprint_finalize(&pbuf, &ret);
    }

    return ret;
}





and i will update this patch, add TEST case.
and update patch for patchwork.
diff mbox

Patch

diff --git a/libavutil/avstring.c b/libavutil/avstring.c
index 52e6e6cd13..55f3bee94c 100644
--- a/libavutil/avstring.c
+++ b/libavutil/avstring.c
@@ -233,78 +233,20 @@  int av_strncasecmp(const char *a, const char *b, size_t n)
 
 char *av_strreplace(const char *str, const char *from, const char *to)
 {
-    /* Adjust each of the below values to suit your needs. */
-    /* Increment positions cache size initially by this number. */
-    size_t cache_sz_inc = 16;
-    /* Thereafter, each time capacity needs to be increased,
-     * multiply the increment by this factor. */
-    const size_t cache_sz_inc_factor = 3;
-    /* But never increment capacity by more than this number. */
-    const size_t cache_sz_inc_max = 1048576;
-
-    char *pret, *ret = NULL;
+    char *ret = NULL;
     const char *pstr2, *pstr = str;
-    size_t i, count = 0;
-    uintptr_t *pos_cache_tmp, *pos_cache = NULL;
-    size_t cache_sz = 0;
-    size_t cpylen, orglen, retlen, tolen, fromlen = strlen(from);
+    size_t tolen = strlen(to), fromlen = strlen(from);
+    AVBPrint pbuf;
 
-    /* Find all matches and cache their positions. */
+    av_bprint_init(&pbuf, 1, AV_BPRINT_SIZE_UNLIMITED);
     while ((pstr2 = av_stristr(pstr, from))) {
-        count++;
-        /* Increase the cache size when necessary. */
-        if (cache_sz < count) {
-            cache_sz += cache_sz_inc;
-            pos_cache_tmp = av_realloc(pos_cache, sizeof(*pos_cache) * cache_sz);
-            if (!pos_cache_tmp) {
-                goto end_strreplace;
-            } else pos_cache = pos_cache_tmp;
-            cache_sz_inc *= cache_sz_inc_factor;
-            if (cache_sz_inc > cache_sz_inc_max) {
-                cache_sz_inc = cache_sz_inc_max;
-            }
-        }
-
-        pos_cache[count-1] = pstr2 - str;
+        av_bprint_append_data(&pbuf, pstr, pstr2 - pstr);
         pstr = pstr2 + fromlen;
+        av_bprint_append_data(&pbuf, to, tolen);
     }
-    orglen = pstr - str + strlen(pstr);
-    /* Allocate memory for the post-replacement string. */
-    if (count > 0) {
-        tolen = strlen(to);
-        retlen = orglen + (tolen - fromlen) * count;
-    } else {
-        retlen = orglen;
-    }
-    ret = av_malloc(retlen + 1);
-    if (!ret) {
-        goto end_strreplace;
-    }
-
-    if (!count) {
-        /* If no matches, then just duplicate the string. */
-        av_strlcpy(ret, str, retlen + 1);
-    } else {
-        /* Otherwise, duplicate the string whilst performing
-         * the replacements using the position cache. */
-        pret = ret;
-        memcpy(pret, str, pos_cache[0]);
-        pret += pos_cache[0];
-        for (i = 0; i < count; i++) {
-            memcpy(pret, to, tolen);
-            pret += tolen;
-            pstr = str + pos_cache[i] + fromlen;
-            cpylen = (i == count-1 ? orglen : pos_cache[i+1]) - pos_cache[i] - fromlen;
-            memcpy(pret, pstr, cpylen);
-            pret += cpylen;
-        }
-        ret[retlen] = '\0';
-    }
+    av_bprint_append_data(&pbuf, pstr, strlen(pstr));
+    av_bprint_finalize(&pbuf, &ret);
 
-end_strreplace:
-    /* Free the cache and return the post-replacement string,
-     * which will be NULL in the event of an error. */
-    av_free(pos_cache);
     return ret;
 }