GNOME Bugzilla – Bug 697808
sdp: add boxed type for GstSDPMessage
Last modified: 2017-02-04 16:22:37 UTC
Created attachment 241262 [details] [review] sdp: Make GstSDPMessage a boxed type Attached patch converts GstSDPMessage type into a boxed type, this way it is easier to use it as an attribute or in signals. Patch can be applied over 1.0 and master branches. Patch includes tests for this new functionality.
Comment on attachment 241262 [details] [review] sdp: Make GstSDPMessage a boxed type It looks like you forgot to 'git add' the new tests/check/libs/sdp.c file? (while at it, make sure to add the binary to .gitignore).
Created attachment 241311 [details] [review] patch Sorry, here is the new patch
Comment on attachment 241311 [details] [review] patch >+++ b/gst-libs/gst/sdp/gstsdpmessage.h >@@ -251,6 +251,7 @@ typedef struct { > * The contents of the SDP message. > */ > typedef struct { >+ GType type; > gchar *version; > GstSDPOrigin origin; > gchar *session_name; This breaks ABI I'm afraid. I think we'll have to do without the GST_IS_SDP_MESSAGE() for now.
Created attachment 241321 [details] [review] sdp: add boxed type for GstSDPMessage Sorry I didn't think about the ABI. Additionally type attribute wasn't initialized, so there is no problem removing it.
These new public _copy() functions are a bit weird IMHO. I think either they should return the newly-allocated copied structure directly as return argument, or they should copy into an-already allocated struct. The former seems more straight-forward. Do we need to expose these functions at all? Maybe we should just keep them internal for now?
I also prefer to return the newly-allocated structure, but I did this way to be consistent with the _new functions, that I think that are a little bit weird too. I don't have any problem changing them. About keeping them public, I find them useful in some cases (ie: generating and answer for an offer) and I prefer to keep them public if it's OK for you.
Ok, let's remove them from public, I'll send a patch with this changes.
I don't mind - if you have a need for them it's fine. I didn't know if you did or not. Your consistency argument also makes sense. I'll just wait to see if Wim has an opinion.
I need for them, as well as a need for removing functions (ie, "gst_sdp_media_remove_connection (media, id)" and similars). I'm thinking about filing a new bug for this enhancements if this one is OK at the end and you think that they are useful. Otherwise I can implement them in my project.
commit 83a877daf6c2fb457b7364558fe55c3feb95cef3 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Apr 15 14:25:16 2013 +0200 sdp: add Since markers commit ba1e6938536b02b4af705ce45f2c89822a6abc62 Author: Jose Antonio Santos Cadenas <santoscadenas@gmail.com> Date: Fri Apr 12 09:35:34 2013 +0200 sdp: add boxed type for GstSDPMessage Also added some tests of this improvement. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=697808
Sorry Wim, but the Since markers shouldn't indicate 1.1 version? Or stable versions has always an even number?
We add Since markers for the upcoming stable version.
Jose, Miguel, this patch added a small amount of code that is GPLv3 to gst-plugins-base, taken from gst-plugins-kurento it seems. As your name is on the copyright header for that - would you be willing to relicense this to LGPLv2.1+ (as the rest of GStreamer)?
Hi Tim, I agree changing the license, actually it should be LGPL from the beginig. I can send a patch changing the license if you feel it's better. Cheers.
Of course, let's change the license ;).
If you could send a patch that'd be great, thanks!