After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730914 - mpegts lib : Ensure all functions/fields are introspectable and are not leaked
mpegts lib : Ensure all functions/fields are introspectable and are not leaked
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.3.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-29 06:20 UTC by Edward Hervey
Modified: 2014-07-11 08:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
example gchar[] to gchar* (2.85 KB, patch)
2014-05-30 15:52 UTC, Stefan Ringel
none Details | Review
change from gchar[] to gchar* (10.86 KB, patch)
2014-05-31 08:18 UTC, Stefan Ringel
none Details | Review
example boxed type (5.70 KB, patch)
2014-05-31 11:32 UTC, Stefan Ringel
none Details | Review
change to GType and from gchar[] to gchar* (66.76 KB, patch)
2014-05-31 19:58 UTC, Stefan Ringel
none Details | Review
change to GType and from gchar[] to gchar* (50.95 KB, patch)
2014-06-01 08:45 UTC, Stefan Ringel
none Details | Review
fix annotation (934 bytes, patch)
2014-06-24 17:20 UTC, Stefan Ringel
committed Details | Review
mpegts: add getter (7.76 KB, patch)
2014-07-04 10:59 UTC, Stefan Ringel
none Details | Review
mpegts: add getter (7.85 KB, patch)
2014-07-04 11:29 UTC, Stefan Ringel
none Details | Review
mpegts: add getter (7.85 KB, patch)
2014-07-04 11:37 UTC, Stefan Ringel
none Details | Review
mpegts: docs: add missed *_free methods (1.99 KB, patch)
2014-07-04 12:05 UTC, Stefan Ringel
committed Details | Review
mpegts: use getter for edge linkage descriptor type (6.69 KB, patch)
2014-07-04 12:06 UTC, Stefan Ringel
committed Details | Review

Description Edward Hervey 2014-05-29 06:20:21 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
Comment 1 Edward Hervey 2014-05-29 06:21:18 UTC
And I also forgot the FIXME:

For every function/fields:
Switch all language codes from gchar[] to gchar * (NULL-terminated)
Comment 2 Stefan Ringel 2014-05-30 15:52:01 UTC
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.?
Comment 3 Stefan Ringel 2014-05-31 08:18:56 UTC
Created attachment 277607 [details] [review]
change from gchar[] to gchar*
Comment 4 Edward Hervey 2014-05-31 08:37:54 UTC
(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
Comment 5 Stefan Ringel 2014-05-31 11:26:04 UTC
(In reply to comment #4)
> 
> Looks good, but:
> * all annotations need to be updated
to what (transfer full) ?
Comment 6 Stefan Ringel 2014-05-31 11:32:40 UTC
Created attachment 277617 [details] [review]
example boxed type

Is that correct? should I change all other structs to GBoxed?
Comment 7 Stefan Ringel 2014-05-31 19:58:10 UTC
Created attachment 277636 [details] [review]
change to GType and from gchar[] to gchar*
Comment 8 Edward Hervey 2014-06-01 07:30:06 UTC
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)
Comment 9 Stefan Ringel 2014-06-01 07:41:21 UTC
(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);
}

?
Comment 10 Edward Hervey 2014-06-01 07:43:45 UTC
Or just simply make the free method you've already created... public :)
Comment 11 Stefan Ringel 2014-06-01 08:45:37 UTC
Created attachment 277663 [details] [review]
change to GType and from gchar[] to gchar*
Comment 12 Sebastian Dröge (slomo) 2014-06-23 18:48:57 UTC
Edward, what's left to be done here?
Comment 13 Edward Hervey 2014-06-24 05:55:28 UTC
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...
Comment 14 Edward Hervey 2014-06-24 05:56:30 UTC
Also make sure that the exported g-i symbols are not fugly.
Comment 15 Stefan Ringel 2014-06-24 17:20:01 UTC
Created attachment 279132 [details] [review]
fix annotation
Comment 16 Sebastian Dröge (slomo) 2014-06-28 14:36:16 UTC
Ping? :)
Comment 17 Sebastian Dröge (slomo) 2014-06-29 19:22:18 UTC
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 :)
Comment 18 Sebastian Dröge (slomo) 2014-06-29 19:34:34 UTC
GstMPEGTSDescriptor needs a public unref or free function, and then the g_boxed_free() inside the unit test have to be replaced.
Comment 19 Edward Hervey 2014-07-04 07:58:59 UTC
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).
Comment 20 Stefan Ringel 2014-07-04 08:10:24 UTC
(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?
Comment 21 Edward Hervey 2014-07-04 09:32:49 UTC
That would indeed be better :)
Comment 22 Stefan Ringel 2014-07-04 10:59:04 UTC
Created attachment 279894 [details] [review]
mpegts: add getter
Comment 23 Stefan Ringel 2014-07-04 11:29:29 UTC
Created attachment 279896 [details] [review]
mpegts: add getter
Comment 24 Stefan Ringel 2014-07-04 11:37:15 UTC
Created attachment 279897 [details] [review]
mpegts: add getter
Comment 25 Edward Hervey 2014-07-04 11:59:11 UTC
Can you provide a separate patch for non-related doc fixes

Can you provide a proper commit message also ?
Comment 26 Stefan Ringel 2014-07-04 12:05:57 UTC
Created attachment 279901 [details] [review]
mpegts: docs: add missed *_free methods
Comment 27 Stefan Ringel 2014-07-04 12:06:40 UTC
Created attachment 279902 [details] [review]
mpegts: use getter for edge linkage descriptor type
Comment 28 Stefan Ringel 2014-07-06 10:12:23 UTC
Ping? :)
Comment 29 Thiago Sousa Santos 2014-07-08 17:59:12 UTC
We also might want to add some padding to the basic structures as we already foresee needing to store some more stuff in those.
Comment 30 Edward Hervey 2014-07-09 05:53:29 UTC
Ok, I think we're done now. Added padding, pushed remaining commits and fixed a leak.