Opened 6 years ago

Closed 5 years ago

#9945 closed bug (fixed)

BMediaFile leaks ca. 100 KB

Reported by: ttcoder Owned by: nobody
Priority: normal Milestone: R1
Component: Audio & Video/Codecs Version: R1/Development
Keywords: ffmpeg Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

Calling new / SetTo() / delete (passing a valid audio file to SetTo()) results in circa 100 kilobytes heap memory leaked each time. Ten audio files loaded -> 1 MB memory leak and so on.

See simple test case attached.

Attachments (1)

BMediaFile-leak.cpp (1.0 KB ) - added by ttcoder 6 years ago.
Change filepath and compile: the leak is reproduced in a dozen lines or so

Download all attachments as: .zip

Change History (17)

by ttcoder, 6 years ago

Attachment: BMediaFile-leak.cpp added

Change filepath and compile: the leak is reproduced in a dozen lines or so

comment:1 by ttcoder, 6 years ago

Might be ffmpeg that's leaking memory: inspecting the allocated 260 bytes blocks in Debugger shows "pmff" tags at the beginning of these blocks; and I sometime get the error message here going like

PluginManager: Error, unloading PlugIn ffmpeg with usecount 1

Tentatively setting priority to High. This is something that would need be adressed sooner rather than later since in my testing a radio station can run out of virtual memory in as little as 4~5 days with such a bad-ass leak :-o ; and this leak might not be hard to track down (unless it's deep in ffmpeg eh..?). Or at least I'd need a work-around to keep me going until it's fixed.

comment:2 by korli, 6 years ago

Component: Kits/Media KitAudio & Video/Codecs
Owner: changed from axeld to nobody

comment:3 by phoudoin, 6 years ago

The "pmff" tag seems to be the misc's ffmpeg media format we set by default in the global codec media formats table, gAVCodecFormats:

http://haiku.it.su.se:8180/source/xref/src/add-ons/media/plugins/ffmpeg/CodecTable.cpp#30

It's a 1024 entries table of media_format, which could be related to those ~100kb heap memory leak. Maybe somewhere we leak it somehow, in particular when ffmpeg plugin is unloaded while still somewhat used.

comment:4 by ttcoder, 6 years ago

Philippe indeed, if the entries from that table are somehow copied to somewhere allocated with new or malloc() then that could be it. I see the 'ffmp' constant set at line 149.

Note to myself: if feeling like fishing in the dark later on, I could take a look at https://trac.ffmpeg.org/ticket/2937

I wonder if there is a way I could set a "watchpoint" in Debugger on a given heap area (rather than watch a given variable).. If yes then, knowing in advance one of the addresses of leaked memory blocks (from a previous run with printf() debug output ..etc) I could invoke debugger() at the beginning of main(), set the watchpoint on that address, click "Run" and get immediately notified (with an explicit backtrace) when that memory block gets malloc()ed. If I know who malloc()s it I'll have a good lead on who should be free()ing it yet fails to, probably.

EDIT: I mean to say that if somebody calls malloc(260) but does not write any contents at that address, then watching the address of that block won't trigger a debugger notification at malloc() time but rather at a later time when somebody (else) writes into that block, presumably. What I would need is to "watch" the malloc chain or something.

Last edited 6 years ago by ttcoder (previous) (diff)

in reply to:  4 comment:5 by anevilyak, 6 years ago

Replying to ttcoder:

I wonder if there is a way I could set a "watchpoint" in Debugger on a given heap area (rather than watch a given variable).. If yes then, knowing in advance one of the addresses of leaked memory blocks (from a previous run with printf() debug output ..etc) I could invoke debugger() at the beginning of main(), set the watchpoint on that address, click "Run" and get immediately notified (with an explicit backtrace) when that memory block gets malloc()ed. If I know who malloc()s it I'll have a good lead on who should be free()ing it yet fails to, probably.

You can set such a watchpoint ; when you right click -> watch, the resulting dialog just prepopulates the address of the variable, but you're free to change that to an arbitrary address. However, that won't do you any good here, Haiku uses address space layout randomization, so the addresses will not in any way be predictable between runs.

comment:6 by korli, 6 years ago

Please check with hrev46028 or newer.

comment:7 by ttcoder, 6 years ago

Priority: highnormal

@korli: Roxors! kudos to you. The leak went down dramatically from 100KB (in multiple allocs) to 2072 bytes (in one alloc):

src_plugins> LD_PRELOAD=libroot_debug.so ./BMediaFile-leak
(....)
total allocations: 824; total bytes: 196008
total allocations: 826; total bytes: 200152
total allocations: 827; total bytes: 202224
total allocations: 828; total bytes: 204296
total allocations: 829; total bytes: 206368
total allocations: 830; total bytes: 208440
total allocations: 831; total bytes: 210512
total allocations: 832; total bytes: 212584
total allocations: 833; total bytes: 214656

Leaking 50 times more slowly than before, a station could go on for close to a year before even needing an app restart. Bug's 99% fixed and no longer urgent in my mind at all.

@anevilyak: no matter how many times I run the above .cpp from Terminal, its new BMediaFile constructor call always allocates the object at the same address, as is the embedded mp3 codec (see below); I probably misunderstand what ASLR is about though, that would be odd if my simple test case had uncovered a kernel problem of any sort. This is with hrev46033 retrieved just now.

Run 1:

src_plugins> LD_PRELOAD=libroot_debug.so ./BMediaFile-leak
[mp3 @ 0x18092800] max_analyze_duration 5000000 reached at 5015510
...

Run 2:

src_plugins> LD_PRELOAD=libroot_debug.so ./BMediaFile-leak
[mp3 @ 0x18092800] max_analyze_duration 5000000 reached at 5015510
...

comment:8 by korli, 6 years ago

Resolution: fixed
Status: newclosed

Fixed in hrev46107.

comment:9 by ttcoder, 5 years ago

Resolution: fixed
Status: closedreopened

The ctor leak is back, though much smaller now. After noticing ticket:9458#comment:2 I thought of running the test file here (including an augmented version that also calls TrackAt()) and we do get one leak when calling only the ctor, and an additional second leak when calling TrackAt() (so that's reading tracks, whereas #9458 is apparently about CreateTrack()):

~/Desktop/New folder> uname -a
Haiku shredder 1 hrev48619 Jan  6 2015 08:42:39 BePC x86 Haiku
~/Desktop/New folder> gcc -g BMediaFile-leak.cpp -lbe -lmedia -lroot_debug
~/Desktop/New folder> LD_PRELOAD=libroot_debug.so ./BMediaFile-leak
(..)
total allocations: 1631; total bytes: 388946
total allocations: 1632; total bytes: 389142
total allocations: 1633; total bytes: 389338
total allocations: 1634; total bytes: 389534
total allocations: 1635; total bytes: 389730
~/Desktop/New folder> # ctor only: leaks 1 alloc, 196 bytes ###################################################
~/Desktop/New folder> 
~/Desktop/New folder> gcc -g BMediaFile-leak.cpp -lbe -lmedia -lroot_debug
~/Desktop/New folder> LD_PRELOAD=libroot_debug.so ./BMediaFile-leak
(..)
total allocations: 1637; total bytes: 390122
total allocations: 1639; total bytes: 390514
total allocations: 1641; total bytes: 390906
total allocations: 1643; total bytes: 391298
total allocations: 1645; total bytes: 391690
~/Desktop/New folder> # ctor+TrackAt(): leaks (1+)1 alloc, (196+)196 bytes ##############################

Looking at the history of thread.c I see one recent change that (going on a limb here...) could explain the regression: the introduction of TLS. Is there an easy way to check the code ? Otherwise I'll try to download the closest revs I can find that are before and after hrev47198 and do a before-and-after test to confirm.

Last edited 5 years ago by ttcoder (previous) (diff)

comment:10 by ttcoder, 5 years ago

Can I volunteer this ticket for R1/beta1 ?

comment:11 by waddlesplash, 5 years ago

Sounds good to me, but Adrien will have to OK it.

comment:12 by ttcoder, 5 years ago

Reviewed my owned/Cc'ed list of tickets yet again, and this stands out as the only important issue that is not yet reviewed and is in R1 rather than R1/b1, maybe i should start clean (with the new editing privileges the way to set a milestone is with New Ticket, and more importantly, it seems justified to start clean for this ticket since the new 196 bytes leak only might be related to the 2072 bytes leak last seen here). Anyone wants to objects to my adding a new ticket, give a shout here today or tomorrow :-)

in reply to:  12 comment:13 by anevilyak, 5 years ago

Replying to ttcoder:

(with the new editing privileges the way to set a milestone is with New Ticket)

No, if you think an existing ticket deserves to be in a different milestone, then you can ask for it to be changed, and that request will be suitably evaluated. The only thing that's different is that it isn't simply a free-for-all where every random person with an account gets to decide priorities.

comment:14 by pulkomandy, 5 years ago

However, your new issue may as well be a completely different one, and having to read the 10 comments to understand that several things were fixed, and then the issue is back, isn't very useful. I would prefer a new ticket for the new problem (mentionning this one as it may be related). There are a lot of other places where BMediaTrack could be leaking memory.

comment:15 by ttcoder, 5 years ago

Thanks much, created #11752 , so this one can be closed for good I believe.

comment:16 by pulkomandy, 5 years ago

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.