[FFmpeg-devel] [PATCH 1/2] lavu: add av_bprintf and related.

[Nicolas George]

Le decadi 30 pluviôse, an CCXX, Clément Bœsch a écrit :
> I agree with moving this to a dedicated place.

Done: bprint.c and bprint.h were the obvious choice.

> I think "av_bprint_is_allocated()" is better here; the current name may
> suggest it returns the current internally allocated size.

Ok.

> > +    if (!av_bprint_complete(buf))
> > +        return -1;
> AVERROR_BUG?

This is not a bug: the buffer may already be truncated because a previous
memory allocation has already failed. In that case, it will likely fail
again, but even if it could succeed, some data has already been lost.

There was a comment until I used a named function for the test; I put it
back.

I also moved there the check for size vs. size_max, instead of having it
duplicated.

> > +    new_str = av_realloc(old_str, new_size);
> > +    if (!new_str)
> > +        return -1;
> AVERROR(ENOMEM)?

The error is never forwarded, so it is a bit moot, but you are right, I
ought to keep good practices and set the example.

> Adding tests might be welcome; not only for testing, but as an API
> demonstration purpose.

I will think about it. I also need to learn how to add stuff to fate.

> What happens in case of error?

If this is a memory allocation failure, the buffer contents is truncated to
the available size, and only the length is updated. I added a length
explanation at the top of the header.

Other printf possible errors are silently ignored, as they are almost
exclusively the result of programming bugs and not environmental conditions.

> Any reason this function is inline while av_bprint_room() and
> av_bprint_allocated() are (unexported) macros?

Macros are tricky, for example they give strange error messages: I like to
avoid them in public headers, especially when their name looks like a
function. For internal code, I trust the programmer more.

> > + * Define a structure with extra padding to a fixed size.
> Maybe you should mention it is mainly for ABI compat purpose?

Done, and moved to bprint.h, as it is the only place it is of use for now.

> I still like this very much, and will be happy to make use of it in a few
> places.

Thanks.

I send a new patch shortly; the graphdump part is unchanged apart from the
#include.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120219/46b881bb/attachment.asc>