GNOME Bugzilla – Bug 787714
gstomxh265enc:Fix Memory leak in error case
Last modified: 2017-09-22 00:58:00 UTC
Hi, It is observed that there is memory leak in omxmodule. File Name : omx/gstomxh265enc.c Function Name : gst_omx_h265_enc_get_caps In gstomxh265enc.c at gst_omx_h265_enc_get_caps : 450 memory is allocated for caps ,But in case of fail it has not been unref. code: caps = gst_caps_new_simple ("video/x-h265", "stream-format", G_TYPE_STRING, "byte-stream", "alignment", G_TYPE_STRING, "au", NULL); GST_OMX_INIT_STRUCT (¶m); param.nPortIndex = GST_OMX_VIDEO_ENC (self)->enc_out_port->index; err = gst_omx_component_get_parameter (GST_OMX_VIDEO_ENC (self)->enc, OMX_IndexParamVideoProfileLevelCurrent, ¶m); if (err != OMX_ErrorNone && err != OMX_ErrorUnsupportedIndex) return NULL; Sol : gst_caps_unref (caps); Please provide your feedback.
Please attach a patch in "git format-patch" format here. See https://gstreamer.freedesktop.org/documentation/contribute/#how-to-submit-patches
sure. I am creating the Patch. I will a attach soon
sorry for delay. definitely will attach the patch today
Created attachment 360019 [details] [review] patch file attached
Created attachment 360039 [details] [review] attached the path attached the path. Please review it
Comment on attachment 360039 [details] [review] attached the path Thanks for the patch! >--- a/omx/gstomxh265enc.c >+++ b/omx/gstomxh265enc.c >@@ -457,8 +457,10 @@ gst_omx_h265_enc_get_caps (GstOMXVideoEnc * enc, GstOMXPort * port, > err = > gst_omx_component_get_parameter (GST_OMX_VIDEO_ENC (self)->enc, > OMX_IndexParamVideoProfileLevelCurrent, ¶m); >- if (err != OMX_ErrorNone && err != OMX_ErrorUnsupportedIndex) >- return NULL; >+ if (err != OMX_ErrorNone && err != OMX_ErrorUnsupportedIndex) { >+ gst_caps_unref (caps); >+ return NULL; >+ } > > if (err == OMX_ErrorNone) { > switch (param.eProfile) { I think a nicer fix would be to move the caps = gst_caps_new_simple(...) after the _get_parameter(), i.e. jst before the if (err == OMX_ErrorNone) {...
Created attachment 360098 [details] [review] patch file attached I moved caps as per your suggestion. patch attached . Please review it
Review of attachment 360098 [details] [review]: ::: omx/gstomxh265enc.c @@ +459,3 @@ + caps = gst_caps_new_simple ("video/x-h265", + "stream-format", G_TYPE_STRING, "byte-stream", + "alignment", G_TYPE_STRING, "au", NULL); You can move this below the whole switch blocks to the very bottom, then no unreffing is ever needed
> You can move this below the whole switch blocks to the very bottom, then no > unreffing is ever needed I think in the OMX_ErrorUnsupportedIndex case we just return the caps without profile/level info?
yes , in the OMX_ErrorUnsupportedIndex case just return the caps Please check again .
Indeed. Then only the indentation has to be fixed (use gst-indent), and please fix the commit message format: omxh265enc: Some short explanation [empty line] longer explanation in multiple lines if necessary [empty line] https://bugzilla.gnome.org/show_bug.cgi?id=787714
Created attachment 360167 [details] [review] patch file attached correct the indentation. Please review it . attach patch file.
Review of attachment 360167 [details] [review]: You are still missing the bug URL on the commit message. Please add it. Additionally, A pair of gotos to a single label after "return caps" followed by unref (caps) & return NULL would suffice but if no one else minds, then leave it as it is.
Thanks. I fine without goto. For you interest Reynaldo, when you do "git bz pull ###", the bug link automatically get added.
commit 27cd61a21a7f1a9d8a7734a3b302df5d8ef98fd1 (HEAD -> master, origin/master, origin/HEAD) Author: Ponnam Srinivas <p.srinivas@samsung.com> Date: Thu Sep 21 11:36:46 2017 +0530 omxh265enc: fix caps leak