[FFmpeg-devel] Memory leak using bitstream filters with shared libs

[Michael Niedermayer]

On Thu, Feb 19, 2009 at 07:51:38PM +0100, Reimar D?ffinger wrote:
> On Wed, Apr 02, 2008 at 01:37:58PM +0200, Michael Niedermayer wrote:
> > On Wed, Apr 02, 2008 at 10:50:20AM +0200, Baptiste Coudurier wrote:
> > > Hi guys,
> > > 
> > > M?ns Rullg?rd wrote:
> > > > Baptiste discovered a rather nasty memory leak when using a bitstream
> > > > filter with shared libraries.
> > > > 
> > > > In ffmpeg.c:write_frame() the output of bitstream filters is stored in
> > > > an AVPacket, and its destruct function is set like this (line 417):
> > > > 
> > > >             new_pkt.destruct= av_destruct_packet;
> > > > 
> > > > Later on, in av_interleave_packet_per_dts(), AVPacket.destruct is
> > > > compared against av_destruct_packet, like this (utils.c:2439):
> > > > 
> > > >         if(pkt->destruct == av_destruct_packet)
> > > > 
> > > > The trouble here is that with shared libraries, the address of
> > > > av_destruct_packet when used in the main executable or libs other than
> > > > the one defining it resolves to a trampoline that calls the real
> > > > function using a GOT entry.  As a result, this comparison is always
> > > > false when the destruct pointer was assigned outside libavformat.
> > > > 
> > > > Does anyone have an idea for fixing this?
> > > > 
> > > 
> > > Sorry, but could we try to find an acceptable solution for this issue ?
> > 
> > as said elsewhere in this thread:
> >  static inline void av_free_packet(AVPacket *pkt)
> >  {
> > -    if (pkt && pkt->destruct) {
> > -        pkt->destruct(pkt);
> > +    if(pkt){
> > +        if(pkt->destruct)
> > +            pkt->destruct(pkt);
> > +        pkt->data = NULL; pkt->size = 0;
> >      }
> >  }
> > ----
> >  void av_destruct_packet(AVPacket *pkt)
> >  {
> >      av_free(pkt->data);
> > -    pkt->data = NULL; pkt->size = 0;
> >  }
> > ----
> > -void av_destruct_packet_nofree(AVPacket *pkt)
> > -{
> > -    pkt->data = NULL; pkt->size = 0;
> > -}
> > 
> > and use NULL instead of av_destruct_packet_nofree
> > -----------------
> 
> The reason I do not like this much is because it means you can't e.g.
> set destruct to something that does only logging, the destruct function
> implicitly defines how copies are made.

noone wants to do that, and if someone wants she can put the loging code
in utils.c and recompile


> Anyway, while trying to implement this I came about something I don't
> really understand, in ff_interleave_add_packet:
> >     if(pkt->destruct == av_destruct_packet)
> >         pkt->destruct= NULL; // not shared -> must keep original from being freed
> >     else
> >        av_dup_packet(&this_pktl->pkt);  //shared -> must dup
> 
> is this just a complete brainfart, or is that comparison against a

the check possibly should be changed to != NULL && != no_free


> static function declared in a header on purpose?
> And are the semantics really supposed to be that when when a
> user-defined destruct it used it will be called multiple times on the
> same pkt (more or less)?

> And are there any reasons why it is assumed that the original AVPacket
> passed into ff_interleave_add_packet will not be used longer than the
> copy?

it can be used longer depending in pkt->destruct behavior
if you have a realistic example where the current system is insufficient
then a patch that implements something better is welcome.
Just keep in mind that packets can be created and freed in different threads
so dont forget mutexes and all that ...
its just replacing destruct by your reference counting locking thread safe
free ...


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090219/adbb6852/attachment.pgp>