[Sumover-dev] Re: Possibly memory leak in UCL common/compiling VPMedia against UCL common

[Andrew Ford]

2011/1/10 Douglas Kosovic <doug at uq.edu.au>

> Hi Andrew,
>
>
>
> Regarding why the headers now get installed into ‘uclmmbase’ instead of
> ‘common’, I decided to mimic what the numerous Linux and BSD distributions
> that currently ship an old uclmmbase package do. I think it also makes sense
> for consistency as the corresponding library was renamed to uclmmbase over
> 10 years ago. I was hoping to submit package update requests and patches for
> Debian/Ubuntu, FreeBSD and NetBSD once the new tarballs for common, vic and
> rat were released.
>

That's fine - certainly better than calling the whole thing just 'common' :)
- I just wanted to point out that any previous testing of VPMedia against
UCL common would have been incomplete without accounting for that.


>  Regarding the static libraries, there were a number of applications and
> utilities looking for the static libraries in common/src, so didn’t want to
> break them, plus the Linux and BSD distributions didn’t ship static
> libraries with their existing uclmmbase packages.
>
>
>
> In Sumover SVN, config_unix.h and config_win32.h have:
>
>     #ifdef HAVE_CONFIG_H
>
>     #include "uclconf.h"
>
>     #endif
>
>
>
> For config_win32.h, I had to add the HAVE_CONFIG_H pre-processor
> conditional for VisualStudio which doesn’t use uclconf.h after Andrew Rowley
> added mingw support. I honestly can’t remember why I added it to
> config_unix.h, but HAVE_CONFIG_H is automatically defined by the configure
> script and adds -DHAVE_CONFIG_H  to the @DEFS variable in the Makefile.in. I
> guess the pre-processor conditional could be removed from config_unix.h.
>

That seems fine for the lib itself, but -DHAVE_CONFIG_H doesn't get
propagated to client apps/libs like VPMedia (should it be added manually in
certain cases?), hence the necessity to include uclconf.h manually in
VPMedia as it stands. I'm not sure what the best practices are in this case.
Are there any cases in which uclconf.h definitely should not be included? (A
Visual Studio build on Windows?)


>  Sorry I’m not sure about the memory leak issue.
>

I think I fixed this - properly doing xfree on the packet data in the RX_RTP
handling in VPMedia's RTP callback function seems to fix the problem (though
valgrind complains about the memory freed being a different size than the
allocated block? I couldn't quite make sense of that, I'll have to look into
that more); I'm just not sure whether it's necessary for the other cases
(RX_SDES, RX_BYE, etc.). Can anyone confirm that?

--Andrew


>
>
>
> Cheers,
>
> Doug
>
>
>
> *From:* adhesionmusic at gmail.com [mailto:adhesionmusic at gmail.com] *On
> Behalf Of *Andrew Ford
> *Sent:* Tuesday, 11 January 2011 6:44 AM
> *To:* sumover-dev
> *Cc:* Piers O'Hanlon; Douglas Kosovic; Rhys Hawkins
> *Subject:* Re: Possibly memory leak in UCL common/compiling VPMedia
> against UCL common
>
>
>
> On a deeper look (re:the memory leak), it just looks like VPMedia isn't
> freeing the packet in the RTP callback. I'm not sure why this didn't happen
> with the if-media version of the common library - it doesn't look like
> anything related (like RTP_OPT_REUSE_PACKET_BUFS) was changed.
> Is it safe to say that the RTP callback should always free that packet
> regardless of what type it is?
>
> --Andrew
>
> 2011/1/10 Andrew Ford <andrew.ford at rit.edu>
>
> Hi,
>
> I've been having some issues (on Linux, Ubuntu 10.10) with my video app
> that relies on VPMedia & the common library. There's a massive memory leak
> when I tried to switch from using the if-media library to the UCL library -
> based on some quick valgrind runs it seems to be in rtp_recv. I'm not
> totally sure yet whether this is actually a problem in the UCL library or if
> something in VPMedia needs to be changed, I'll be investigating this a bit
> deeper soon.
>
> I thought I saw some reference in the VPMedia SVN log to compiling against
> UCL common, but I'm not sure if that was fully tested, since on a normal
> install, the if-media include files (assuming prefix is /usr/local) actually
> installs to /usr/local/include/common whereas the UCL version goes to
> /usr/local/include/uclmmbase, and VPMedia includes files from common/. I
> changed the includes to point to the right spot before confirming my tests
> above, so that's not the problem.
>
> Side note - should client apps now directly include uclconf.h? It seems
> like the if-media version had an #ifdef to include it in certain cases (via
> config_unix or config_win32) but the most recent UCL code doesn't, so I had
> to add that manually to VPMedia or else it would complain about things like
> a #def for endianness not being set.
>
> Also, it looks like the make install target in UCL common doesn't actually
> copy the static version of the lib (.a), even if static is enabled and it
> has been built. I haven't checked the specifics in the makefile yet.
>
> --Andrew
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oakham.cs.ucl.ac.uk/pipermail/sumover-dev/attachments/20110111/eac4b86a/attachment.html