GNOME Bugzilla – Bug 730914
mpegts lib : Ensure all functions/fields are introspectable and are not leaked
Last modified: 2014-07-11 08:01:31 UTC
Yes, this will need an API/ABI break We have an issue with quite a few of the descriptor parsing functions that allocate memory. One (simple) example ======================================== struct _GstMpegTsComponentDescriptor { guint8 stream_content; guint8 component_type; guint8 component_tag; /* FIXME : Make it a separate (allocated, null-terminated) return value */ gchar language_code[4]; gchar *text; }; gboolean gst_mpegts_descriptor_parse_dvb_component (const GstMpegTsDescriptor *descriptor, GstMpegTsComponentDescriptor *res); ======================================== Problem: 1) the text field is allocated but we have no simple way to free it (with gobject-introspection in mind) 2) there's no registered free/copy/Gtype for the return structure, meaning g-i will leak it 3) a lot of the annotations aren't correct (ex: the list argument of gst_mpegts_descriptor_parse_dvb_ca_identifier) Since 1.4. is coming up RSN, we should not do this break in the middle of it. Yes, this helper library is in -bad and is an unstable API, but breaking it halfway through will suck ... Proposal: For every parsed structure (such as GstMpegTsComponentDescriptor) that will have allocated memory: 1) The parsing function will take that argument as a pointer of pointer (ex: (..., GstMpegTsComponentDescriptor **res)) 2) The annotation will indicate (transfer full) 3) There will be a registered GBoxed GType will registered copy/free functions For every function: 1) make sure the transfer annotations are correct
And I also forgot the FIXME: For every function/fields: Switch all language codes from gchar[] to gchar * (NULL-terminated)
Created attachment 277569 [details] [review] example gchar[] to gchar* (In reply to comment #1) > And I also forgot the FIXME: > > For every function/fields: > Switch all language codes from gchar[] to gchar * (NULL-terminated) Edward, I have a example for switch to gchar*. Is that o.k.?
Created attachment 277607 [details] [review] change from gchar[] to gchar*
(In reply to comment #3) > Created an attachment (id=277607) [details] [review] > change from gchar[] to gchar* Looks good, but: * all annotations need to be updated * values returned directly (such as for the subtitling descriptor) need to be freed now
(In reply to comment #4) > > Looks good, but: > * all annotations need to be updated to what (transfer full) ?
Created attachment 277617 [details] [review] example boxed type Is that correct? should I change all other structs to GBoxed?
Created attachment 277636 [details] [review] change to GType and from gchar[] to gchar*
There was no need to convert the structures for which we don't allocate any memory (such as the delivery descriptors). And you need to expose the _free function for all the relevant descriptors instead of requiring people to use g_boxed_free(LET_ME_GUESS_THE_TYPE)
(In reply to comment #8) > > And you need to expose the _free function for all the relevant descriptors > instead of requiring people to use g_boxed_free(LET_ME_GUESS_THE_TYPE) as macro, like this: #define GST_MPEGTS_COMPONENT_DESCRIPTOR_FREE (res) gst_boxed_free (GST_TYPE_MPEGTS_COMPONENT_DESCRIPTOR, res) or as methode like this gst_mpegts_component_descriptor_free (GstMpegTsComponentDescriptor * res) { gst_boxed_free (GST_TYPE_MPEGTS_COMPONENT_DESCRIPTOR, res); } ?
Or just simply make the free method you've already created... public :)
Created attachment 277663 [details] [review] change to GType and from gchar[] to gchar*
Edward, what's left to be done here?
I need to review all the patches, makes sure it makes sense, makes sure it works with g-i, makes sure we have no leak, etc...
Also make sure that the exported g-i symbols are not fugly.
Created attachment 279132 [details] [review] fix annotation
Ping? :)
gst_mpegts_section_get_pmt() returns no new ref, gst_mpegts_section_get_pat() returns a new ref. Would be good to make these things consistent :)
GstMPEGTSDescriptor needs a public unref or free function, and then the g_boxed_free() inside the unit test have to be replaced.
Most issues are fixed now. The only one remaining is the linkage descriptor which is exposing a pointer (the nature of that data depends on some other field).
(In reply to comment #19) > Most issues are fixed now. The only one remaining is the linkage descriptor > which is exposing a pointer (the nature of that data depends on some other > field). the pointer as private field of linkage descriptor and for edge different linkage a getter methode?
That would indeed be better :)
Created attachment 279894 [details] [review] mpegts: add getter
Created attachment 279896 [details] [review] mpegts: add getter
Created attachment 279897 [details] [review] mpegts: add getter
Can you provide a separate patch for non-related doc fixes Can you provide a proper commit message also ?
Created attachment 279901 [details] [review] mpegts: docs: add missed *_free methods
Created attachment 279902 [details] [review] mpegts: use getter for edge linkage descriptor type
We also might want to add some padding to the basic structures as we already foresee needing to store some more stuff in those.
Ok, I think we're done now. Added padding, pushed remaining commits and fixed a leak.