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 745880 - sdp: SDP <-> GstCaps helper functions
sdp: SDP <-> GstCaps helper functions
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-09 08:22 UTC by Sebastian Dröge (slomo)
Modified: 2016-01-04 00:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
sdp: add helper fuctions from/to sdp from/to caps (27.79 KB, patch)
2015-07-30 12:08 UTC, Hyunjun Ko
none Details | Review
sdp: add helper fuctions from/to sdp from/to caps (28.53 KB, patch)
2015-07-30 15:03 UTC, Hyunjun Ko
none Details | Review
sdp: add helper fuctions from/to sdp from/to caps (28.88 KB, patch)
2015-07-30 15:28 UTC, Hyunjun Ko
none Details | Review
sdp: add helper fuctions from/to sdp from/to caps (30.95 KB, patch)
2015-08-03 05:28 UTC, Hyunjun Ko
none Details | Review
rtspsrc: replace duplicated codes to calling base sdp apis (20.13 KB, patch)
2015-08-03 05:29 UTC, Hyunjun Ko
none Details | Review
rtsp-server: sdp: replace duplicated codes to calling base sdp api (25.66 KB, patch)
2015-08-03 05:30 UTC, Hyunjun Ko
none Details | Review
rtspsrc: replace duplicated codes to calling base sdp apis (20.27 KB, patch)
2015-08-03 08:42 UTC, Hyunjun Ko
none Details | Review
sdp: add helper fuctions from/to sdp from/to caps (33.54 KB, patch)
2015-08-16 09:09 UTC, Hyunjun Ko
none Details | Review
sdp: replace duplicated codes to call new base sdp apis (25.98 KB, patch)
2015-08-16 09:10 UTC, Hyunjun Ko
none Details | Review
rtspsrc: replace duplicated codes to call new base sdp apis (20.45 KB, patch)
2015-08-16 09:13 UTC, Hyunjun Ko
none Details | Review
sdp: add helper fuctions from/to sdp from/to caps (33.65 KB, patch)
2015-10-07 09:56 UTC, Hyunjun Ko
committed Details | Review
rtspsrc: replace duplicated codes to call new base sdp apis (20.94 KB, patch)
2015-10-07 09:56 UTC, Hyunjun Ko
committed Details | Review
rtsp-server: sdp: replace duplicated codes to call new base sdp apis (26.26 KB, patch)
2015-10-07 09:57 UTC, Hyunjun Ko
committed Details | Review
sdpdemux: replace duplicated codes to call new base sdp apis (8.56 KB, patch)
2015-10-07 09:57 UTC, Hyunjun Ko
committed Details | Review

Description Sebastian Dröge (slomo) 2015-03-09 08:22:58 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.
Comment 1 Tim-Philipp Müller 2015-03-09 08:35:42 UTC
Yes, please!
Comment 2 Hyunjun Ko 2015-07-20 08:46:30 UTC
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?
Comment 3 Sebastian Dröge (slomo) 2015-07-29 10:33:35 UTC
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 :)
Comment 4 Hyunjun Ko 2015-07-30 12:08:34 UTC
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
Comment 5 Hyunjun Ko 2015-07-30 12:10:07 UTC
There are two test cases.
I think it needs more codes to verify.
Comment 6 Sebastian Dröge (slomo) 2015-07-30 12:48:26 UTC
Yes, you can link to libgstrtp
Comment 7 Hyunjun Ko 2015-07-30 15:03:30 UTC
Created attachment 308467 [details] [review]
sdp: add helper fuctions from/to sdp from/to caps

Makefile.am patched
Comment 8 Sebastian Dröge (slomo) 2015-07-30 15:07:21 UTC
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
Comment 9 Hyunjun Ko 2015-07-30 15:28:37 UTC
Created attachment 308472 [details] [review]
sdp: add helper fuctions from/to sdp from/to caps

Oh.. I learned one thing.
Thanks :)
Comment 10 Hyunjun Ko 2015-08-03 05:28:15 UTC
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
Comment 11 Hyunjun Ko 2015-08-03 05:29:13 UTC
Created attachment 308645 [details] [review]
rtspsrc: replace duplicated codes to calling base sdp apis

To test with completed pipeline.
Comment 12 Hyunjun Ko 2015-08-03 05:30:07 UTC
Created attachment 308646 [details] [review]
rtsp-server: sdp: replace duplicated codes to calling base sdp api

To test with completed pipeline.
Comment 13 Hyunjun Ko 2015-08-03 08:42:36 UTC
Created attachment 308654 [details] [review]
rtspsrc: replace duplicated codes to calling base sdp apis

Add code to check if return value is NULL.
Comment 14 Sebastian Dröge (slomo) 2015-08-13 11:19:43 UTC
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
Comment 15 Sebastian Dröge (slomo) 2015-08-13 11:23:19 UTC
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?
Comment 16 Sebastian Dröge (slomo) 2015-08-13 11:25:15 UTC
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?
Comment 17 Hyunjun Ko 2015-08-14 08:36:06 UTC
> +      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?
Comment 18 Sebastian Dröge (slomo) 2015-08-14 08:38:33 UTC
Sounds good
Comment 19 Hyunjun Ko 2015-08-14 08:43:47 UTC
> +    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)
Comment 20 Hyunjun Ko 2015-08-14 08:47:01 UTC
> +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?
Comment 21 Sebastian Dröge (slomo) 2015-08-14 08:56:31 UTC
(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
Comment 22 Hyunjun Ko 2015-08-16 09:09:33 UTC
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*)
Comment 23 Hyunjun Ko 2015-08-16 09:10:28 UTC
Created attachment 309348 [details] [review]
sdp: replace duplicated codes to call new base sdp apis
Comment 24 Hyunjun Ko 2015-08-16 09:13:46 UTC
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. :-)
Comment 25 Sebastian Dröge (slomo) 2015-08-16 09:15:29 UTC
I think API-wise this is good now, will have to check the implementation of the functions more detailed later :)
Comment 26 Hyunjun Ko 2015-08-16 09:29:37 UTC
> 
> 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. :)
Comment 27 Jan Schmidt 2015-09-09 04:42:32 UTC
Caps to SDP is more-or-less what gst-rtsp-server has in the rtsp-sdp.[ch] helper
Comment 29 Hyunjun Ko 2015-10-07 09:56:48 UTC
Created attachment 312807 [details] [review]
rtspsrc: replace duplicated codes to call new base sdp apis

Rebased on the latest git.
Comment 30 Hyunjun Ko 2015-10-07 09:57:29 UTC
Created attachment 312808 [details] [review]
rtsp-server: sdp: replace duplicated codes to call new base sdp apis

Rebased on the latest git.
Comment 31 Hyunjun Ko 2015-10-07 09:57:57 UTC
Created attachment 312809 [details] [review]
sdpdemux: replace duplicated codes to call new base sdp apis
Comment 32 Sebastian Dröge (slomo) 2015-12-31 15:12:23 UTC
(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.
Comment 33 Sebastian Dröge (slomo) 2015-12-31 15:19:03 UTC
All merged now, thanks for preparing these patches :)
Comment 34 Hyunjun Ko 2016-01-04 00:47:04 UTC
I should have rebased these patches on the latest.
Thanks for fixing and merge :)