GNOME Bugzilla – Bug 745880
sdp: SDP <-> GstCaps helper functions
Last modified: 2016-01-04 00:47:04 UTC
Currently we have code for the above in gst-rtsp-server and rtspsrc and sdpdemux. This should be moved into the library instead of copying the code around further.
Yes, please!
I've been thinking of this for a while. IMHO, base apis for this are something like below. GstCaps* gst_sdp_media_get_caps_from_media (const GstSDPMedia *media, gint pt); GstSDPResult gst_sdp_media_set_media_from_caps (const GstCaps* caps, GstSDPMedia *media); const gchar * gst_sdp_make_keymgmt (const GstCaps *caps, guint32 ssrc); GstSDPResult gst_sdp_message_attributes_to_caps (GstSDPMessage *msg, GstCaps *caps); GstSDPResult gst_sdp_media_attributes_to_caps (GstSDPMedia *media, GstCaps *caps); There's a couple of things making me confusing. 1. These kind of apis should be included to gstsdpmessage.h or gstsdp.h? 2. There's logic to use gstrtp lib to get clock-rate. Can I use this in this base library?
I think the SDP->caps code should be taken from rtspsrc, the caps->SDP code from gst-rtsp-server. The proposed API sounds reasonable, but I'd have to see some example usage. Maybe just implement it that way, write some unit tests with it and then let's see :)
Created attachment 308450 [details] [review] sdp: add helper fuctions from/to sdp from/to caps This patch can't be built. Because it uses gst_rtp_payload_info_for_name / gst_rtp_payload_info_for_pt in gstrtp library. Can I link this lib to this base library? If so, I'll modify Makefile.am
There are two test cases. I think it needs more codes to verify.
Yes, you can link to libgstrtp
Created attachment 308467 [details] [review] sdp: add helper fuctions from/to sdp from/to caps Makefile.am patched
Review of attachment 308467 [details] [review]: Just one build issue so far, will review properly later :) ::: gst-libs/gst/sdp/Makefile.am @@ +12,2 @@ libgstsdp_@GST_API_VERSION@_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(GST_BASE_CFLAGS) $(GST_CFLAGS) $(GIO_CFLAGS) +libgstsdp_@GST_API_VERSION@_la_LIBADD = $(GST_LIBS) $(GIO_LIBS) -lgstrtp-@GST_API_VERSION@ This has to be $(top_builddir)/gst-libs/gst/rtp/libgstrtp-@GST_API_VERSION@.la
Created attachment 308472 [details] [review] sdp: add helper fuctions from/to sdp from/to caps Oh.. I learned one thing. Thanks :)
Created attachment 308644 [details] [review] sdp: add helper fuctions from/to sdp from/to caps - Add api descriptions to doc. - Remove gst_sdp_media_set_port_info call in gst_sdp_media_set_media_from_caps
Created attachment 308645 [details] [review] rtspsrc: replace duplicated codes to calling base sdp apis To test with completed pipeline.
Created attachment 308646 [details] [review] rtsp-server: sdp: replace duplicated codes to calling base sdp api To test with completed pipeline.
Created attachment 308654 [details] [review] rtspsrc: replace duplicated codes to calling base sdp apis Add code to check if return value is NULL.
Review of attachment 308646 [details] [review]: Looks like a big improvement :) Just some minor comments. Also note that sdpdemux has the same code ::: gst/rtsp-server/rtsp-media.c @@ +3143,3 @@ /* do some tweaks */ GST_DEBUG ("mapping sdp session level attributes to caps"); + gst_sdp_message_attributes_to_caps ((const GstSDPMessage *) sdp, caps); Why the cast? ::: gst/rtsp-server/rtsp-sdp.c @@ +153,3 @@ /* check for srtp */ + gst_rtsp_stream_get_ssrc (stream, &ssrc); + base64 = (gchar *) gst_sdp_make_keymgmt (caps, ssrc); Isn't this way of keymgmt specific to mikey? Maybe the API should be named like that then
Review of attachment 308654 [details] [review]: ::: gst/rtsp/gstrtspsrc.c @@ +5818,3 @@ + result = + g_strdup_printf ("prot=mikey;uri=\"%s\";data=\"%s\"", + stream->conninfo.location, base64); Maybe this g_strdup_printf() should be part of the library API?
Review of attachment 308644 [details] [review]: ::: gst-libs/gst/sdp/gstsdpmessage.h @@ +506,3 @@ +const gchar * gst_sdp_make_keymgmt (const GstCaps *caps, guint32 ssrc); +GstSDPResult gst_sdp_message_attributes_to_caps (const GstSDPMessage *msg, GstCaps *caps); +GstSDPResult gst_sdp_media_attributes_to_caps (const GstSDPMedia *media, GstCaps *caps); The last two are only _to_caps(). Shouldn't there be an inverse one for parsing caps and putting attributes into the SDP?
> + gst_sdp_message_attributes_to_caps ((const GstSDPMessage *) sdp, > caps); > > Why the cast? > I'm going to remove the unnecessary cast :) > + base64 = (gchar *) gst_sdp_make_keymgmt (caps, ssrc); > > Isn't this way of keymgmt specific to mikey? Maybe the API should be named > like that then Yes. IMHO, 2 new apis in gst-mikey could be implemented like below. GstMIKEYMessage* gst_mikey_message_from_caps (GstCaps *) const gchar* gst_mikey_message_base64_encode (GstMIKEYMessage*) What do you think?
Sounds good
> + result = > + g_strdup_printf ("prot=mikey;uri=\"%s\";data=\"%s\"", > + stream->conninfo.location, base64); > > Maybe this g_strdup_printf() should be part of the library API? That'll be good. This is really like below.:-) const gchar * gst_sdp_make_keymgmt (const gchar *uri, const ghcar *base64)
> +GstSDPResult gst_sdp_message_attributes_to_caps (const > GstSDPMessage *msg, GstCaps *caps); > +GstSDPResult gst_sdp_media_attributes_to_caps (const > GstSDPMedia *media, GstCaps *caps); > > The last two are only _to_caps(). Shouldn't there be an inverse one for > parsing caps and putting attributes into the SDP? I think I don't understand perfectly. This inverse api can provide attributes except for default attributes?
(In reply to Hyunjun from comment #20) > > +GstSDPResult gst_sdp_message_attributes_to_caps (const > > GstSDPMessage *msg, GstCaps *caps); > > +GstSDPResult gst_sdp_media_attributes_to_caps (const > > GstSDPMedia *media, GstCaps *caps); > > > > The last two are only _to_caps(). Shouldn't there be an inverse one for > > parsing caps and putting attributes into the SDP? > > I think I don't understand perfectly. > This inverse api can provide attributes except for default attributes? Just wondering because you now have GstSDP* -> GstCaps, but not the other way around
Created attachment 309347 [details] [review] sdp: add helper fuctions from/to sdp from/to caps New 2 apis added to gstmikey GstMIKEYMessage* gst_mikey_message_new_from_caps (GstCaps *) gchar* gst_mikey_message_base64_encode (GstMIKEYMessage*)
Created attachment 309348 [details] [review] sdp: replace duplicated codes to call new base sdp apis
Created attachment 309349 [details] [review] rtspsrc: replace duplicated codes to call new base sdp apis I think I'd better work on sdpdemux after review is almost done. :-)
I think API-wise this is good now, will have to check the implementation of the functions more detailed later :)
> > Just wondering because you now have GstSDP* -> GstCaps, but not the other > way around Probably, the inverse apis would be useful. Currently, it's not needed, but I can implement this when it's needed in detail. :)
Caps to SDP is more-or-less what gst-rtsp-server has in the rtsp-sdp.[ch] helper
Created attachment 312806 [details] [review] sdp: add helper fuctions from/to sdp from/to caps Rebased on the latest git. And adjust commit http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/rtsp?id=fe4ed1d1dfbbc858865a59a470331264c6015f4c http://cgit.freedesktop.org/gstreamer/gst-rtsp-server/commit/gst/rtsp-server/rtsp-media.c?id=315c2f93bb3bd17514dd20fb269684dc2c23ee4d
Created attachment 312807 [details] [review] rtspsrc: replace duplicated codes to call new base sdp apis Rebased on the latest git.
Created attachment 312808 [details] [review] rtsp-server: sdp: replace duplicated codes to call new base sdp apis Rebased on the latest git.
Created attachment 312809 [details] [review] sdpdemux: replace duplicated codes to call new base sdp apis
(In reply to Hyunjun from comment #28) > Created attachment 312806 [details] [review] [review] > sdp: add helper fuctions from/to sdp from/to caps Fixed some minor problems and memory leaks in this one.
All merged now, thanks for preparing these patches :)
I should have rebased these patches on the latest. Thanks for fixing and merge :)