> -----Original Message-----
> From: Stefano Sabatini <stefa...@gmail.com>
> Sent: Montag, 28. April 2025 21:56
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Cc: softworkz <softwo...@hotmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH v7 02/13] fftools/textformat: Apply quality
> improvements
>
> On date Friday 2025-04-25 23:30:57 +0000, softworkz wrote:
> > From: softworkz <softwo...@hotmail.com>
> >
> > Perform multiple improvements to increase code robustness.
> > In particular:
> > - favor unsigned counters for loops
> > - add missing checks
> > - avoid possible leaks
> > - move variable declarations to inner scopes when feasible
> > - provide explicit type-casting when needed
> >
> > Signed-off-by: softworkz <softwo...@hotmail.com>
> > ---
> > fftools/textformat/avtextformat.c | 85 ++++++++++++++++++++-----------
> > fftools/textformat/avtextformat.h | 6 +--
> > fftools/textformat/tf_default.c | 8 ++-
> > fftools/textformat/tf_ini.c | 2 +-
> > fftools/textformat/tf_json.c | 17 ++++---
> > fftools/textformat/tf_xml.c | 3 --
> > fftools/textformat/tw_avio.c | 11 +++-
> > 7 files changed, 83 insertions(+), 49 deletions(-)
> >
> > diff --git a/fftools/textformat/avtextformat.c
> b/fftools/textformat/avtextformat.c
> > index 74d179c516..1939a1f739 100644
> > --- a/fftools/textformat/avtextformat.c
> > +++ b/fftools/textformat/avtextformat.c
> > @@ -93,9 +93,8 @@ static const AVClass textcontext_class = {
> >
> > static void bprint_bytes(AVBPrint *bp, const uint8_t *ubuf, size_t
> ubuf_size)
> > {
> > - int i;
> > av_bprintf(bp, "0X");
> > - for (i = 0; i < ubuf_size; i++)
> > + for (unsigned i = 0; i < ubuf_size; i++)
> > av_bprintf(bp, "%02X", ubuf[i]);
> > }
> >
> > @@ -137,6 +136,9 @@ int avtext_context_open(AVTextFormatContext **ptctx,
> const AVTextFormatter *form
> > AVTextFormatContext *tctx;
> > int i, ret = 0;
> >
>
> > + if (!ptctx || !formatter)
> > + return AVERROR(EINVAL);
>
> assert0?
OK.
>
> > +
> > if (!(tctx = av_mallocz(sizeof(AVTextFormatContext)))) {
> > ret = AVERROR(ENOMEM);
> > goto fail;
> > @@ -209,25 +211,26 @@ int avtext_context_open(AVTextFormatContext **ptctx,
> const AVTextFormatter *form
> > av_log(NULL, AV_LOG_ERROR, " %s", n);
> > av_log(NULL, AV_LOG_ERROR, "\n");
> > }
> > - return ret;
> > + goto fail;
> > }
> >
> > /* validate replace string */
> > {
> > - const uint8_t *p = tctx->string_validation_replacement;
> > - const uint8_t *endp = p + strlen(p);
> > + const uint8_t *p = (uint8_t *)tctx->string_validation_replacement;
> > + const uint8_t *endp = p + strlen((const char *)p);
> > while (*p) {
> > const uint8_t *p0 = p;
> > int32_t code;
> > ret = av_utf8_decode(&code, &p, endp, tctx-
> >string_validation_utf8_flags);
> > if (ret < 0) {
>
> > AVBPrint bp;
> > - av_bprint_init(&bp, 0, AV_BPRINT_SIZE_AUTOMATIC);
> > + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> > bprint_bytes(&bp, p0, p - p0),
> > av_log(tctx, AV_LOG_ERROR,
> > "Invalid UTF8 sequence %s found in string
> validation replace '%s'\n",
> > bp.str, tctx->string_validation_replacement);
> > - return ret;
> > + av_bprint_finalize(&bp, NULL);
>
> I see no advantage in this, since it's adding dynamic allocation and
> the need to free which was previously not needed
The dynamic allocation does not happen until the buffer content size
does not fit anymore into the stack-alloced memory.
>
> > + goto fail;
> > }
> > }
> > }
> > @@ -255,6 +258,9 @@ static const char unit_bit_per_second_str[] = "bit/s";
> >
>
> > void avtext_print_section_header(AVTextFormatContext *tctx, const void
> *data, int section_id)
> > {
> > + if (section_id < 0 || section_id >= tctx->nb_sections)
> > + return;
>
> Given this is a check on the input data, we should probably make it
> fail with a log message, meaning we should also check the return value
> from the callee.
>
> Given this is not a public API I'm not against the assert though.
Let's discuss this in the other thread (Shaping...).
Did you see my recent reply?
Thanks
sw
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
