r18406 - in trunk/libavcodec: h264.c h264.h h264_parser.c
Author: reimar Date: Fri Apr 10 09:01:55 2009 New Revision: 18406 Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters. Modified: trunk/libavcodec/h264.c trunk/libavcodec/h264.h trunk/libavcodec/h264_parser.c Modified: trunk/libavcodec/h264.c ============================================================================== --- trunk/libavcodec/h264.c Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264.c Fri Apr 10 09:01:55 2009 (r18406) @@ -8083,10 +8083,8 @@ int main(void){ #endif /* TEST */ -static av_cold int decode_end(AVCodecContext *avctx) +av_cold ff_h264_free_context(H264Context *h) { - H264Context *h = avctx->priv_data; - MpegEncContext *s = &h->s; int i; av_freep(&h->rbsp_buffer[0]); @@ -8098,6 +8096,14 @@ static av_cold int decode_end(AVCodecCon for(i = 0; i < MAX_PPS_COUNT; i++) av_freep(h->pps_buffers + i); +} + +static av_cold int decode_end(AVCodecContext *avctx) +{ + H264Context *h = avctx->priv_data; + MpegEncContext *s = &h->s; + + ff_h264_free_context(h); MPV_common_end(s); Modified: trunk/libavcodec/h264.h ============================================================================== --- trunk/libavcodec/h264.h Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264.h Fri Apr 10 09:01:55 2009 (r18406) @@ -569,4 +569,9 @@ const uint8_t *ff_h264_decode_nal(H264Co */ int ff_h264_decode_rbsp_trailing(H264Context *h, const uint8_t *src); +/** + * frees any data that may have been allocated in the H264 context like SPS, PPS etc. + */ +av_cold ff_h264_free_context(H264Context *h); + #endif /* AVCODEC_H264_H */ Modified: trunk/libavcodec/h264_parser.c ============================================================================== --- trunk/libavcodec/h264_parser.c Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264_parser.c Fri Apr 10 09:01:55 2009 (r18406) @@ -306,6 +306,7 @@ static void close(AVCodecParserContext * ParseContext *pc = &h->s.parse_context; av_free(pc->buffer); + ff_h264_free_context(h); }
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Author: reimar Date: Fri Apr 10 09:01:55 2009 New Revision: 18406
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
Hmm.. there is a problem I missed... ff_h264_free_context calls free_tables - I think that would not be necessary to fix the memleak, but it does seem safer. The problem is though, that free_tables contains this:
for(i = 0; i < h->s.avctx->thread_count; i++) { and avctx is not set in the parser. Does adding a if (!h->s.avctx) return; before that seem a reasonable solution?
On Fri, Apr 10, 2009 at 12:32:10PM +0200, Reimar D?ffinger wrote:
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Author: reimar Date: Fri Apr 10 09:01:55 2009 New Revision: 18406
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
Hmm.. there is a problem I missed... ff_h264_free_context calls free_tables - I think that would not be necessary to fix the memleak, but it does seem safer. The problem is though, that free_tables contains this:
for(i = 0; i < h->s.avctx->thread_count; i++) { and avctx is not set in the parser. Does adding a if (!h->s.avctx) return; before that seem a reasonable solution?
I was quite confused as to how I could miss this... It seems neither FATE nor make test cover the H.264 parser, at least not if it crashes upon closing, Mike could you consider maybe adding one of the samples from issue 706 or 944 (with -an)? I guess the full 6MB of those are probably a bit big... I did confirm the leak to be gone with valgrind but in all the output I probably missed the crash message (lesson learned: always test without valgrind, too)...
Resurecting an old thread... On 04/10/2009 12:54 PM, Reimar D?ffinger wrote:
On Fri, Apr 10, 2009 at 12:32:10PM +0200, Reimar D?ffinger wrote:
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Author: reimar Date: Fri Apr 10 09:01:55 2009 New Revision: 18406
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
Hmm.. there is a problem I missed... ff_h264_free_context calls free_tables - I think that would not be necessary to fix the memleak, but it does seem safer. The problem is though, that free_tables contains this:
for(i = 0; i< h->s.avctx->thread_count; i++) { and avctx is not set in the parser. Does adding a if (!h->s.avctx) return; before that seem a reasonable solution?
I was quite confused as to how I could miss this... It seems neither FATE nor make test cover the H.264 parser, at least not if it crashes upon closing, Mike could you consider maybe adding one of the samples from issue 706 or 944 (with -an)? I guess the full 6MB of those are probably a bit big...
Can you suggest any other files? The one from issue 944 does not seem to use the parser and the one from issue 706 decodes as junk (and always did, I tried it with r18407 too). -Vitor
On Fri, Apr 10, 2009 at 12:32:10PM +0200, Reimar D?ffinger wrote:
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Author: reimar Date: Fri Apr 10 09:01:55 2009 New Revision: 18406
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
Hmm.. there is a problem I missed... ff_h264_free_context calls free_tables - I think that would not be necessary to fix the memleak, but it does seem safer. The problem is though, that free_tables contains this:
for(i = 0; i < h->s.avctx->thread_count; i++) { and avctx is not set in the parser. Does adding a if (!h->s.avctx) return; before that seem a reasonable solution?
Actually, this seems like a good solution to me: Index: libavcodec/h264.c =================================================================== --- libavcodec/h264.c (revision 18412) +++ libavcodec/h264.c (working copy) @@ -1989,7 +1989,7 @@ av_freep(&h->mb2b_xy); av_freep(&h->mb2b8_xy); - for(i = 0; i < h->s.avctx->thread_count; i++) { + for(i = 0; i < MAX_THREADS; i++) { hx = h->thread_context[i]; if(!hx) continue; av_freep(&hx->top_borders[1]);
On Fri, Apr 10, 2009 at 01:37:04PM +0200, Reimar D?ffinger wrote:
On Fri, Apr 10, 2009 at 12:32:10PM +0200, Reimar D?ffinger wrote:
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Author: reimar Date: Fri Apr 10 09:01:55 2009 New Revision: 18406
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
Hmm.. there is a problem I missed... ff_h264_free_context calls free_tables - I think that would not be necessary to fix the memleak, but it does seem safer. The problem is though, that free_tables contains this:
for(i = 0; i < h->s.avctx->thread_count; i++) { and avctx is not set in the parser. Does adding a if (!h->s.avctx) return; before that seem a reasonable solution?
Actually, this seems like a good solution to me: Index: libavcodec/h264.c =================================================================== --- libavcodec/h264.c (revision 18412) +++ libavcodec/h264.c (working copy) @@ -1989,7 +1989,7 @@ av_freep(&h->mb2b_xy); av_freep(&h->mb2b8_xy);
- for(i = 0; i < h->s.avctx->thread_count; i++) { + for(i = 0; i < MAX_THREADS; i++) { hx = h->thread_context[i]; if(!hx) continue; av_freep(&hx->top_borders[1]);
Well, I applied it before more people waste their time creating bug reports for it and because I like that you do not have to be careful not to free the avctx or manipulate thread_count before freeing the H.264 context after this change.
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
--- trunk/libavcodec/h264.c Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264.c Fri Apr 10 09:01:55 2009 (r18406) @@ -8083,10 +8083,8 @@ int main(void){
-static av_cold int decode_end(AVCodecContext *avctx) +av_cold ff_h264_free_context(H264Context *h)
--- trunk/libavcodec/h264.h Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264.h Fri Apr 10 09:01:55 2009 (r18406) @@ -569,4 +569,9 @@ const uint8_t *ff_h264_decode_nal(H264Co
+/** + * frees any data that may have been allocated in the H264 context like SPS, PPS etc. + */ +av_cold ff_h264_free_context(H264Context *h);
Ahem... libavcodec/h264.h:575: warning: data definition has no type or storage class libavcodec/h264.h:575: warning: type defaults to ?int? in declaration of ?ff_h264_free_context? Diego
On Fri, Apr 10, 2009 at 01:16:33PM +0200, Diego Biurrun wrote:
On Fri, Apr 10, 2009 at 09:01:56AM +0200, reimar wrote:
Log: Add a ff_h264_free_context function and call it from the H.264 parser. This ensures that the parser will no longer leak memory for all SPS/PPS it encounters.
--- trunk/libavcodec/h264.c Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264.c Fri Apr 10 09:01:55 2009 (r18406) @@ -8083,10 +8083,8 @@ int main(void){
-static av_cold int decode_end(AVCodecContext *avctx) +av_cold ff_h264_free_context(H264Context *h)
--- trunk/libavcodec/h264.h Fri Apr 10 02:09:07 2009 (r18405) +++ trunk/libavcodec/h264.h Fri Apr 10 09:01:55 2009 (r18406) @@ -569,4 +569,9 @@ const uint8_t *ff_h264_decode_nal(H264Co
+/** + * frees any data that may have been allocated in the H264 context like SPS, PPS etc. + */ +av_cold ff_h264_free_context(H264Context *h);
Ahem...
libavcodec/h264.h:575: warning: data definition has no type or storage class libavcodec/h264.h:575: warning: type defaults to ?int? in declaration of ?ff_h264_free_context?
Crap. Is there a compiler flag we could add that makes this an error?
participants (4)
-
diego＠biurrun.de -
Reimar.Doeffinger＠gmx.de -
subversion＠mplayerhq.hu -
vitor1001＠gmail.com