Created attachment 311518 [details] [review]
typefindelement: fix buffer leak

Do you have a testcase for this, both code paths? gst_push_push() should always take ownership of the buffer, even on failure. And gst_pad_pull_range() should never return a buffer if it fails.

I've tested it with gst-validate with valgrind option. The test file was smi(subtitle), and it failed to be played and I got mem leak log too.
Do I need to attach the log and valgrind output file?

It would be good if you could attach a simple testcase that reproduces this behaviour, not something as complicated as gst-validate.

This hints at a bug elsewhere than in typefind though.

And, actually not both path.

According to the valgrind log, gst_pad_push (typefind->src, outbuf) was failed and there was a memory leakage.

I will attach the simple test case as your comments. 

And I have a question about the test command.
The test command was a kind of  [gst-launch playbin uri=file:/// ... / *.smi ].
As you know, application should use suburi property of playbin for the smi file but gst-validate just put the file path in a media directory at 'uri' property of playbin.

Can I use this command to check the memory leakage even though the usage is wrong?

A simple C application that shows the problem would be best, but if you can reproduce it with gst-launch that it also ok.

I will add test application. Thank you.

Created attachment 311574 [details] [review]
typefindelement: fix buffer leak

I attached the test command below and I modified the patch/description.
In my test case, the gst_pad_pull_range() return without error but the following gst_pad_push return error so the buffer which was allocated in gst_pad_pull_range() is not being freed.


This is the test command with gst-launch. (instead of test app)

G_SLICE=always-malloc valgrind --trace-children=yes --tool=memcheck --leak-check=full --leak-resolution=high --num-callers=20 gst-launch-1.0 playbin uri=file:///home/eunhae/workspace/05_sample/issue/Sherlock.1x02.The.Blind.Banker.HDTV.XviD-FoV.smi audio-sink='fakesink sync=true' video-sink='fakesink sync=true'

Created attachment 311575 [details]
output of valgrind with gst-launch cmd

Ok, but then the bug is in whatever code is called by gst_pad_push(). Probably one of the downstream elements is leaking the buffer in an error code path.

Created attachment 311578 [details]
test smi file and output of valgrind test

Created attachment 311729 [details] [review]
inputselector: fix buffer leak

When I checked the downstream path, I got leak point in inputselector so I uploaded new patch.

After gst_pad_push return with error, it try to keep the buffer in cache but the buffer is not cached because of the segment format is not TIME. so it can not be released.

Created attachment 311731 [details] [review]
inputselector: fix buffer leak

(In reply to Eunhae Choi from comment #13)
> Created attachment 311731 [details] [review] [review]
> inputselector: fix buffer leak

This patch doesn't look correct. If input-selector is keeping an extra ref on a buffer and then leaking it, I don't think it's here.

(In reply to Jan Schmidt from comment #14)
> (In reply to Eunhae Choi from comment #13)
> > Created attachment 311731 [details] [review] [review] [review]
> > inputselector: fix buffer leak
> 
> This patch doesn't look correct. If input-selector is keeping an extra ref
> on a buffer and then leaking it, I don't think it's here.

Actually it would. This function is taking ownership of the buffer reference in the other code path.

commit ebd2748cd0d6b2e0b35c1d87b7170dfe673ec917
Author: Eunhae Choi <eunhae1.choi@samsung.com>
Date:   Mon Sep 21 14:58:46 2015 +0900

    inputselector: Fix buffer leak in sync_streams & cache_buffers mode
    
    After doing gst_pad_push() in case of sync_streams and cache_buffers,
    if the buffer can not be kept in cache, it should be unreffed to avoid
    memory leackage.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=755141

Sorry, my review was for the old patch. The new one looks correct :)