Re: [FFmpeg-devel] [PATCH v5 03/14] fftools/avtextformat: Re-use BPrint in loop

[Stefano Sabatini <stefasab@gmail.com>]

On date Tuesday 2025-04-22 21:55:32 +0000, softworkz wrote:
> From: softworkz <softworkz@hotmail.com>
> 
> Instead of initializing a new BPrint in each iteration of
> the loop, re-use the same BPrint struct and just clear it
> for each iteration.
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  fftools/textformat/avtextformat.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
> index 1939a1f739..0a221f4a9a 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -308,24 +308,25 @@ void avtext_print_integer(AVTextFormatContext *tctx, const char *key, int64_t va
>  
>  static inline int validate_string(AVTextFormatContext *tctx, char **dstp, const char *src)
>  {
> -    const uint8_t *p, *endp;
> +    const uint8_t *p, *endp, *srcp = (const uint8_t *)src;
>      AVBPrint dstbuf;
> +    AVBPrint bp;
>      int invalid_chars_nb = 0, ret = 0;
>  
> +    *dstp = NULL;
>      av_bprint_init(&dstbuf, 0, AV_BPRINT_SIZE_UNLIMITED);
> +    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
>  
> -    endp = src + strlen(src);
> -    for (p = src; *p;) {
> -        uint32_t code;
> +    endp = srcp + strlen(src);
> +    for (p = srcp; *p;) {
> +        int32_t code;
>          int invalid = 0;
>          const uint8_t *p0 = p;
>  
>          if (av_utf8_decode(&code, &p, endp, tctx->string_validation_utf8_flags) < 0) {
> -            AVBPrint bp;
> -            av_bprint_init(&bp, 0, AV_BPRINT_SIZE_AUTOMATIC);
> -            bprint_bytes(&bp, p0, p-p0);
> -            av_log(tctx, AV_LOG_DEBUG,
> -                   "Invalid UTF-8 sequence %s found in string '%s'\n", bp.str, src);

> +            av_bprint_clear(&bp);

> +            bprint_bytes(&bp, p0, p - p0);

cosmetical - let's keep it separated to avoid complicating the diff

> +            av_log(tctx, AV_LOG_DEBUG, "Invalid UTF-8 sequence %s found in string '%s'\n", bp.str, src);

I'm not convinced this is really needed, note that the bp is stored in
the stack and is only initialized in case of decode error. We can
theoretically have more than one error in a string, but still there
should be no need to use UNLIMITED and uninit.

Also if we want to use a function scope for bp, it's best to use a
more descriptive name (e.g. bp_invalid_sequence).

>              invalid = 1;
>          }
>  
> @@ -345,7 +346,7 @@ static inline int validate_string(AVTextFormatContext *tctx, char **dstp, const
>          }
>  
>          if (!invalid || tctx->string_validation == AV_TEXTFORMAT_STRING_VALIDATION_IGNORE)

> -            av_bprint_append_data(&dstbuf, p0, p-p0);
> +            av_bprint_append_data(&dstbuf, (const char *)p0, p - p0);

unrelated
_______________________________________________
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".