[FFmpeg-devel] [PATCH 2/3] ffprobe: replace fmt callback with str callback.

[Clément Bœsch]

On Fri, Sep 09, 2011 at 12:59:58AM +0200, Stefano Sabatini wrote:
> On date Thursday 2011-09-08 23:13:28 +0200, Clément Bœsch encoded:
> > On Tue, Sep 06, 2011 at 03:31:36PM +0200, Stefano Sabatini wrote:
> > [...]
> > > >  static void default_print_int(const char *key, int value)
> > > > @@ -167,11 +164,15 @@ static void default_print_footer(const char *section)
> > > >  
> > > >  /* Print helpers */
> > > >  
> > > > -#define print_fmt0(k, f, a...) w->print_fmt_f(k, f, ##a)
> > > > -#define print_fmt( k, f, a...) do {   \
> > > > +#define print_fmt0(k, f, a...) do {   \
> > > > +    char *strv = av_asprintf(f, ##a); \
> > > 
> > > > +    w->print_str_f(k, strv);          \
> > > > +    av_free(strv);                    \
> > > > +} while (0)
> > > > +#define print_fmt(k, f, a...) do {    \
> > > >      if (w->item_sep)                  \
> > > >          printf("%s", w->item_sep);    \
> > > > -    w->print_fmt_f(k, f, ##a);        \
> > > > +    print_fmt0(k, f, ##a);            \
> > > >  } while (0)
> > > 
> > > Is print_fmt/print_fmt0 still required? (I can't see any use in the
> > > code right now.)
> > > 
> > 
> > What I changed is the callback, not the helpers; and yes
> > print_fmt/print_fmt0 are still in use since a few options need to be
> > printed in a "special" way (such as fraction or 64 bits integers).
> > 
> > > Also I noticed that ## is a GNU cpp extension, so it may fail with
> > > other pre-processors.
> > > 
> > 
> > Oups, fixed. Since the ## is already upstream, I'd like to push this new
> > patch ASAP (I'll push it tomorrow if no one object).
> > 
> > > Also I'm afraid that all this malloc/free may slow down the overall
> > > performance.
> > > 
> > 
> 
> > This is a concern; though, I'm not sure adding a fixed buffer length
> > limitation is appropriate for the ffprobe tool. While I think there is a
> > real problem with tools like ffmpeg, I'm not sure it really is an issue
> > here. Do you have something better to propose for variable-length
> > printing?
> 
> Could you benchmark it? allocing/freeing a buffer for every single
> print looks quite expensive.
> 

With a 250M media input:

  av_asprintf/av_free: ~1.5sec
  4096B stack buffer:  ~1.1sec

> Alternatively: you pass a buffer which is reallocated if and only when
> needed, this would need some custom code.
> 

The code will start to be pretty complicated; I can think of a
av_fast_asprintf, but that might be quite overkill. I can still commit the
stack buffer version and maybe remove the size limitation later.

> As for the ## fix please fix it separately (no need for patch, just
> commit it).
> 

Done a few days ago.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110911/ba7c77fa/attachment.asc>