[FFmpeg-devel] [PATCH v5 02/14] fftools/textformat: Apply quality improvements

[softworkz .]

Hi Stefano,


> 
> >              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),
> 
> Is this really needed? AUTOMATIC should be faster since it will avoid
> dynamic allocation and the need to finalize (although there is no
> practical need for such optimization, this still seems the simplest
> possible path). Besides, an UTF8 sequence cannot be longer than a few
> bytes, so possibly av_utf8_decode cannot decode more than a few bytes.

My understanding is as follows, please correct me when I'm wrong:

AV_BPRINT_SIZE_AUTOMATIC

Despite its name, there's automatism for anything. There's a certain 
(not exactly known) amount of (stack) memory and it doesn't ever grow.
That means in turn that after most av_brint_* invocations, you need to
check the return value and compare it to a length value to validate 
that the recent invocation didn't overflow - otherwise, things have 
gone wrong, the content of the BPrint is invalid (or at least not 
how you actually need it to be). So you need to error out.


AV_BPRINT_SIZE_UNLIMITED

The initialization like above does NOT cause any more mem allocation
than with AV_BPRINT_SIZE_AUTOMATIC. But when you approach the limit,
it will automatically allocate memory as needed, and your results 
are always as expected and you don't ever need to check and 
"chars written" return values.

The only thing that is required with AV_BPRINT_SIZE_AUTOMATICm
is to call av_bprint_finalize() to free mem that has possibly
been allocated (unless you are sure that the total length is 
always ever pretty short.


So, unless I'd misunderstand anything, I wonder in which cases, 
AV_BPRINT_SIZE_AUTOMATIC could even be a better choice than 
AV_BPRINT_SIZE_UNLIMITED ?



> In general, I see some confusion about how to deal with invalid
> parameters (this is probably an issue with the whole codebase), since
> it's not always clear what's the right thing to do - consider that as
> a programmer unrecoverable mistake and assert or let the user handle
> that or just silently perform a no-op. It's probably fine to go with
> the current patch and later refine in case we elaborate a set of
> guidelines to apply.

I do see the that confusion indeed. Did you notice my message from yesterday?

"[FFmpeg-devel] [RFC] Shaping the AVTextFormat API Surface"

It's meant as a base for discussion to get clear about all this 😉

Best,
sw