GNOME Bugzilla – Bug 733119
utils: Unref/release pads in error cases when linking pads
Last modified: 2014-08-06 08:02:24 UTC
I accidentally discovered that gst_element_link_pads_full() had several instances where if it failed to link it was leaking reference to pads or forgot to release request pads. The attached patch attempts to fix these leaks as well as add a unit test to verify that no leaks persist. I ran this successfully in valgrind. Now, this slightly changes the behaviour of gst_element_link_pads_full() as it previously would in some cases not release any request pads on the elements you tried to link together. I don't know if there is any code out there that used to take this into account. Also please review the changes carefully. While I think this patch improves the situation, the code in this helper(!)-function is sufficiently hairy to have made me suspicious of my own changes.
Created attachment 280579 [details] [review] Proposed patch 1/2 that plugs the leaks.
Created attachment 280580 [details] [review] Proposed patch 2/2 that adds a unit test to verify the changes in 1/2.
I'm not sure if aids in reviewing this, but I ran this in lcov and it seems like almost all of gst_element_link_pads_full() is covered by the testcases. The parts that are not handle the scenario where a specific srcpad is not named so gst_element_link_pads_full() chooses a random srcpad which fails to link with a compatible destpad so gst_element_link_pads_full() advances to the next srcpad. And the reverse case where the destpad is not named and gst_element_link_pads_full() is looking for a compatible srcpad. I don't think it is worthwhile to add tests for these cases, at least not right now.
This fix broke some tests in core: collectpads, ghostpad and bin.
Created attachment 282614 [details] [review] Proposed patch 1/2 that plugs the leaks.
Created attachment 282615 [details] [review] Proposed patch 2/2 that adds a unit test to verify the changes in 1/2.
Ok, I have now updated my patches to not cause any issues for collectpads, ghostpad and bin tests.
commit 2e4ce5caf6aeab383d79006cb4fb7714eda13174 Author: Sebastian Rasmussen <sebras@hotmail.com> Date: Sun Jul 13 15:31:08 2014 +0200 tests: Add test verifying gst_element_link_pads_full() Fixes https://bugzilla.gnome.org/show_bug.cgi?id=733119 commit a041e65503dec792e904a803e82f4f263d92ec22 Author: Sebastian Rasmussen <sebras@hotmail.com> Date: Sun Jul 13 15:28:32 2014 +0200 utils: Unref/release pads in error cases when linking pads Previously gst_element_link_pads_full() forgot to unreference or release request pads in several error cases. Also comments were added mentioning why releasing is not necessary in some places. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=733119