GNOME Bugzilla – Bug 700749
gstrtph264pay/depay: Add framerate and optional framesize SDP attribute to payloaded caps
Last modified: 2013-07-11 14:50:12 UTC
Stores the SDP attribute values for framerate and framesize in caps fields as expected by the patches in https://bugzilla.gnome.org/show_bug.cgi?id=700747. That bug contains links to the relevant RFCs.
Created attachment 244868 [details] [review] Proposed patch adding framerate SDP attribute value to caps.
Created attachment 244869 [details] [review] Proposed patch adding framesize SDP attribute value to caps.
Created attachment 244872 [details] [review] Proposed patch for code restructuring easing addition of optional SDP attribute value
Created attachment 244873 [details] [review] Proposed patch adding framerate SDP attribute value to caps.
Created attachment 244874 [details] [review] Proposed patch adding framesize SDP attribute value to caps.
Review of attachment 244872 [details] [review]: ::: gst/rtp/gstrtph264pay.c @@ +383,3 @@ + if (G_UNLIKELY (count)) { + return NULL; Isn't this leaking the GString sprops?
Review of attachment 244873 [details] [review]: ::: gst/rtp/gstrtph264depay.c @@ +317,3 @@ + rtph264depay->byte_stream ? "byte-stream" : "avc", + "alignment", G_TYPE_STRING, rtph264depay->merge ? "au" : "nal", NULL); + } Just create the caps once and if there's a framerate use gst_caps_set_simple() to set it on the already created caps @@ +643,3 @@ + + /* canonicalise floating point string so we can handle framerate strings + * in the form "24.930" or "24,930" irrespective of the current locale */ The RFC allows both?! @@ +651,3 @@ + g_value_set_double (&src, g_ascii_strtod (s, NULL)); + g_value_init (&dest, GST_TYPE_FRACTION); + g_value_transform (&src, &dest); There's also gst_util_double_to_fraction(), no need for this GValue dance :) ::: gst/rtp/gstrtph264pay.c @@ +567,3 @@ + gdouble framerate; + gst_util_fraction_to_double (num, denom, &framerate); + rate = g_strdup_printf("%f", framerate); This is always in the format 1.234 with a . as delimiter, right?
Just saw the comment in bug #700747 which makes sense... in the caps the framerate should probably be a GstFraction. And only converted to a string later. Same for the width and height
> Isn't this leaking the GString sprops? Yes, it does. I'll fix.
> gst_caps_set_simple() I was looking for something like this last night, but had a hard time finding it. Hence the over-complicated if-statements. I fix. > + /* canonicalise floating point string so we can handle framerate strings > + * in the form "24.930" or "24,930" irrespective of the current locale */ > The RFC allows both?! No, the RFC allows only a decimal point. However the following commit fron -good explains why the code looks this way. commit fafd0b7bc35f60d077fc3bc1287454778619d7ca Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Dec 29 14:40:05 2010 +0000 rtpjpegdepay: fix framerate parsing for locales that use a comma as floating point atof() converts strings according to the current locale, but the framerate string will likely always use a dot as floating point separator, so use g_ascii_strtod() instead (but also canonicalise the string before, so we can handle both formats as input). > @@ +651,3 @@ > + g_value_set_double (&src, g_ascii_strtod (s, NULL)); > + g_value_init (&dest, GST_TYPE_FRACTION); > + g_value_transform (&src, &dest); > > There's also gst_util_double_to_fraction(), no need for this GValue dance :) I blame all of this on __tim as motivated above. ;) But I'll fix it of course! > ::: gst/rtp/gstrtph264pay.c > @@ +567,3 @@ > + gdouble framerate; > + gst_util_fraction_to_double (num, denom, &framerate); > + rate = g_strdup_printf("%f", framerate); > > This is always in the format 1.234 with a . as delimiter, right? Probably not no, due to the locale-issues mentioned by __tim in his commit message. Maybe I ought to separate the integer part and the fractional part manually and stick a period in between. That way I could actually remove __tim's commit completely. How does that sound..?
(In reply to comment #10) > Probably not no, due to the locale-issues mentioned by __tim in his commit > message. Maybe I ought to separate the integer part and the fractional part > manually and stick a period in between. That way I could actually remove > __tim's commit completely. How does that sound..? No, use g_ascii_dtostr() instead.
Yes, ideally use non-locale-dependant functions everywhere instead of working around locales. (Same for your jpeg payloader patch btw :) )
Created attachment 245012 [details] [review] Proposed patch for easing adding optional caps fields
Created attachment 245013 [details] [review] Proposed patch adding width/height to caps.
Created attachment 245014 [details] [review] Proposed patch adding framerate to caps for later use in SDP.
(In reply to comment #11) > No, use g_ascii_dtostr() instead. Yes, that makes more sense. I found that function eventually, only to read your comment just now. :) (In reply to comment #12) > Yes, ideally use non-locale-dependant functions everywhere instead of working > around locales. (Same for your jpeg payloader patch btw :) ) Well, not really. I will actually do the formatting in gst-rtsp-server, so there will be just one instance of it and I opted to use g_ascii_formatd() instead because I needed an initial space for the SDP attribute value to be properly formatted. If you are not happy with me using g_ascii_formatd() instead of g_ascii_dtostr(), let me know in https://bugzilla.gnome.org/show_bug.cgi?id=700747
Looks good to me too, waiting for the SDP patches to get Olivier's or Wim's approval :) And g_ascii_formatd() is fine too of course.
You should add the width/height to the payloader template caps (both) though, if you require them.
Created attachment 245024 [details] [review] Proposed patch adding width/height to caps.
Created attachment 245025 [details] [review] Proposed patch adding framerate to caps for later use in SDP.
I would just like to remark that there are two possible locale-related problems with the float parsing/generating: (a) on the sender side and (b) on the receiver side. We may only control one part of the equation. My patch tried to also accommodate the case where someone created the SDP or whatever using printf using their own locale (which happened to use a comma as decimal separator). This is wrong of course, but then if we always errored out whenever something doesn't adhere 100% to the spec we could just shut down everything ;) True story btw IIRC, not just some "just in case" thing. When *creating* the string from the float, you should always use a decimal point irrespective of locale, so g_ascii_formatd() or g_ascii_dtostr() should be sufficient.
Created attachment 245123 [details] [review] Proposed patch adding width/height to caps.
Created attachment 245124 [details] [review] Proposed patch adding framerate to caps for later use in SDP
Review of attachment 245124 [details] [review]: ::: gst/rtp/gstrtph264depay.c @@ +656,3 @@ + if (gst_structure_get_fraction (structure, "framerate", &num, &denom) && + (num < 0 || denom <= 0)) { + goto invalid_framerate; This *requires* a framerate now. If there's none it will error out. It should only use a framerate if it is available in the caps, otherwise just work as before ::: gst/rtp/gstrtph264pay.c @@ +473,3 @@ + if (gst_structure_get_fraction (str, "framerate", &num, &denom) && + (num < 0 || denom <= 0)) { + goto invalid_framerate; Same here, it should work without a framerate too
Review of attachment 245123 [details] [review]: ::: gst/rtp/gstrtph264depay.c @@ +640,3 @@ + } + if (gst_structure_get_int (structure, "height", &height) && height <= 0) { + goto invalid_dimension; Same here, should work without width/heiight specified too because of compatibility reasons ::: gst/rtp/gstrtph264pay.c @@ +463,3 @@ + } + if (!gst_structure_get_int (str, "width", &width) || width <= 0) { + goto invalid_dimension; Here it's fine to require width/height to be present
Ignore that, I was just confused. Sorry :) commit d8825e2a5c0bfb883ff88e2c9da499c800ebca0a Author: Sebastian Rasmussen <sebrn@axis.com> Date: Wed May 22 03:18:07 2013 +0200 rtph264pay/depay: Add optional framerate caps for use in SDP This allows for applications to format SDP attributes and still do SDP offer/answer based on caps negotiation. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700749 commit 3dca756a5dba55266256f239e3e12a3d058e185a Author: Sebastian Rasmussen <sebrn@axis.com> Date: Wed May 22 03:09:44 2013 +0200 rtph264pay/depay: Add frame dimensions a payloaded caps This allows for applications to format SDP attributes and still do SDP offer/answer based on caps negotiation. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700749 commit 61666898cfe89a1b21d3e6850ab44f5b1633ed79 Author: Sebastian Rasmussen <sebrn@axis.com> Date: Mon May 20 22:14:44 2013 +0200 rtph264pay: Restructuring to allow for adding optional caps Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700749
Reverted now. These attributes are not allowed for h264.