Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Plug some memory leaks in mkv streaming #169

Closed
wants to merge 1 commit into from
Closed

Plug some memory leaks in mkv streaming #169

wants to merge 1 commit into from

Conversation

mikrohard
Copy link
Contributor

This commit closes two memory leaks in mkv streaming.

The first one was in avc_convert_pkt (pretty obvious if you take a closer look). Valgrind log below:

==18963== 1,853,928 bytes in 77,247 blocks are definitely lost in loss record 303 of 303
==18963==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18963==    by 0x428E18: avc_convert_pkt (avc.c:189)
==18963==    by 0x435BA7: globalheaders_input (globalheaders.c:170)
==18963==    by 0x435191: normalize_ts.isra.1 (tsfix.c:212)
==18963==    by 0x411C6D: streaming_pad_deliver (streaming.c:298)
==18963==    by 0x41AD9C: parser_deliver.isra.4 (parsers.c:1407)
==18963==    by 0x41B2D5: parse_h264 (parsers.c:1243)
==18963==    by 0x41AB50: parse_sc (parsers.c:339)
==18963==    by 0x41D82E: ts_recv_packet0 (tsdemux.c:126)
==18963==    by 0x41DA3F: ts_recv_packet1 (tsdemux.c:260)
==18963==    by 0x427B4C: iptv_thread (iptv_input.c:116)
==18963==    by 0x5C41E99: start_thread (pthread_create.c:308)

The second one was in the mkv muxer and occurred only when there was an error. Valgrind logs below:

==18754== 363,773 (200 direct, 363,573 indirect) bytes in 5 blocks are definitely lost in loss record 317 of 319
==18754==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18754==    by 0x425C39: htsbuf_append (htsbuf.c:110)
==18754==    by 0x43ACCF: ebml_append_id (ebml.c:39)
==18754==    by 0x43AE50: ebml_append_bin (ebml.c:71)
==18754==    by 0x43AF67: ebml_append_uint (ebml.c:90)
==18754==    by 0x43C78A: mk_mux_write_pkt (mkmux.c:732)
==18754==    by 0x4452BC: tvh_muxer_write_pkt (muxer_tvh.c:125)
==18754==    by 0x43D8C1: http_stream_run.isra.1 (webui.c:201)
==18754==    by 0x43DCFB: http_stream (webui.c:614)
==18754==    by 0x4086BC: http_exec (http.c:343)
==18754==    by 0x408A82: http_cmd_get (http.c:374)
==18754==    by 0x408C21: process_request (http.c:454)

==18754== 329,077 (160 direct, 328,917 indirect) bytes in 4 blocks are definitely lost in loss record 316 of 319
==18754==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18754==    by 0x425C39: htsbuf_append (htsbuf.c:110)
==18754==    by 0x43C6B9: mk_mux_write_pkt (mkmux.c:759)
==18754==    by 0x4452BC: tvh_muxer_write_pkt (muxer_tvh.c:125)
==18754==    by 0x43D8C1: http_stream_run.isra.1 (webui.c:201)
==18754==    by 0x43DCFB: http_stream (webui.c:614)
==18754==    by 0x4086BC: http_exec (http.c:343)
==18754==    by 0x408A82: http_cmd_get (http.c:374)
==18754==    by 0x408C21: process_request (http.c:454)
==18754==    by 0x408FFF: http_serve (http.c:748)
==18754==    by 0x4074A8: tcp_server_start (tcp.c:401)
==18754==    by 0x5C41E99: start_thread (pthread_create.c:308)

There are still some smaller memory leaks (in range of a few 10kB). I'll take a look at them when I have some more time.

john-tornblom
john-tornblom reviewed Oct 18, 2012
View reviewed changes
src/dvr/mkmux.c
@@ -901,6 +901,8 @@ mk_mux_t *mk_mux_create(void)

if(!mkm->error)
pkt_ref_dec(pkt);
else
mk_close_cluster(mkm);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to flush the cluster buffer in mk_mux_destroy() instead.

Something like this should do it:

if(mkm->cluster)
htsbuf_queue_flush(mkm->cluster);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried as you suggested...

diff --git a/src/dvr/mkmux.c b/src/dvr/mkmux.c
index 881cc47..b2b5d6f 100644
--- a/src/dvr/mkmux.c
+++ b/src/dvr/mkmux.c
@@ -901,8 +901,6 @@ mk_mux_write_pkt(mk_mux_t *mkm, struct th_pkt *pkt)

   if(!mkm->error)
     pkt_ref_dec(pkt);
-  else
-    mk_close_cluster(mkm);

   return mkm->error;
 }
@@ -977,6 +975,8 @@ mk_mux_close(mk_mux_t *mkm)
 void
 mk_mux_destroy(mk_mux_t *mkm)
 {
+  if(mkm->cluster)
+    htsbuf_queue_flush(mkm->cluster);
   free(mkm->filename);
   free(mkm->tracks);
   free(mkm->title);

It didn't help with these memory leaks (they must be happening during streaming and not at the end). Valgrind log below:

==23967== 73,855 (40 direct, 73,815 indirect) bytes in 1 blocks are definitely lost in loss record 316 of 322
==23967==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23967==    by 0x425949: htsbuf_append (htsbuf.c:110)
==23967==    by 0x43C2B9: mk_mux_write_pkt (mkmux.c:759)
==23967==    by 0x444C8C: tvh_muxer_write_pkt (muxer_tvh.c:125)
==23967==    by 0x43D4D1: http_stream_run.isra.1 (webui.c:201)
==23967==    by 0x43D90B: http_stream (webui.c:614)
==23967==    by 0x4086BC: http_exec (http.c:343)
==23967==    by 0x408A82: http_cmd_get (http.c:374)
==23967==    by 0x408C21: process_request (http.c:454)
==23967==    by 0x408FFF: http_serve (http.c:748)
==23967==    by 0x4074A8: tcp_server_start (tcp.c:401)
==23967==    by 0x5C41E99: start_thread (pthread_create.c:308)
==23967== 
==23967== 176,597 (80 direct, 176,517 indirect) bytes in 2 blocks are definitely lost in loss record 318 of 322
==23967==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23967==    by 0x425949: htsbuf_append (htsbuf.c:110)
==23967==    by 0x43A8CF: ebml_append_id (ebml.c:39)
==23967==    by 0x43AA50: ebml_append_bin (ebml.c:71)
==23967==    by 0x43AB67: ebml_append_uint (ebml.c:90)
==23967==    by 0x43C38A: mk_mux_write_pkt (mkmux.c:732)
==23967==    by 0x444C8C: tvh_muxer_write_pkt (muxer_tvh.c:125)
==23967==    by 0x43D4D1: http_stream_run.isra.1 (webui.c:201)
==23967==    by 0x43D90B: http_stream (webui.c:614)
==23967==    by 0x4086BC: http_exec (http.c:343)
==23967==    by 0x408A82: http_cmd_get (http.c:374)
==23967==    by 0x408C21: process_request (http.c:454)
==23967== 
==23967== 206,088 (240 direct, 205,848 indirect) bytes in 6 blocks are definitely lost in loss record 319 of 322
==23967==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23967==    by 0x43C3CF: mk_mux_write_pkt (mkmux.c:652)
==23967==    by 0x444C8C: tvh_muxer_write_pkt (muxer_tvh.c:125)
==23967==    by 0x43D4D1: http_stream_run.isra.1 (webui.c:201)
==23967==    by 0x43D90B: http_stream (webui.c:614)
==23967==    by 0x4086BC: http_exec (http.c:343)
==23967==    by 0x408A82: http_cmd_get (http.c:374)
==23967==    by 0x408C21: process_request (http.c:454)
==23967==    by 0x408FFF: http_serve (http.c:748)
==23967==    by 0x4074A8: tcp_server_start (tcp.c:401)
==23967==    by 0x5C41E99: start_thread (pthread_create.c:308)
==23967==    by 0x5F4ADBC: clone (clone.S:112)
==23967== 
==23967== 211,838 (72 direct, 211,766 indirect) bytes in 3 blocks are definitely lost in loss record 320 of 322
==23967==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23967==    by 0x42581C: htsbuf_queue_alloc (htsbuf.c:57)
==23967==    by 0x43C363: mk_mux_write_pkt (mkmux.c:727)
==23967==    by 0x444C8C: tvh_muxer_write_pkt (muxer_tvh.c:125)
==23967==    by 0x43D4D1: http_stream_run.isra.1 (webui.c:201)
==23967==    by 0x43D90B: http_stream (webui.c:614)
==23967==    by 0x4086BC: http_exec (http.c:343)
==23967==    by 0x408A82: http_cmd_get (http.c:374)
==23967==    by 0x408C21: process_request (http.c:454)
==23967==    by 0x408FFF: http_serve (http.c:748)
==23967==    by 0x4074A8: tcp_server_start (tcp.c:401)
==23967==    by 0x5C41E99: start_thread (pthread_create.c:308)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that your commit fixes this memory leak?

I've made various improvements at https://github.com/john-tornblom/tvheadend/commits/http_stream_testing

Do you think you could give that one a go? It seems to do the trick for me. Oh, and be sure to test it on a stream with mpeg2 video packets, I don't think h264 is affected but the leakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My commit fixes the memory leak by 0x425949: htsbuf_append (htsbuf.c:110)... not sure about the later ones... I was testing with two connected clients and with switching between h264 and mpeg2 chennels for about 5 minutes. I'll make another test tomorrow asap to se if your streaming improvements fix the same leak as my commit.

@john-tornblom
Copy link
Contributor

Hmm, ok. When there is a write error, the streaming will end and no more packets are sent to the muxer. I guess I have overlooked something. I'll see if I can reproduce this.

@mikrohard
Copy link
Contributor Author

@john-tornblom I can confirm that your improvements fix all memory leaks during mkv streaming (except the obvious one in avc_convert_pkt). Together with the one left in this pull request the remaining memory leaks are minimal.

@john-tornblom
Copy link
Contributor

all is merged now, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mikrohard @john-tornblom