GNOME Bugzilla – Bug 758169
rtph264depay should translate "a-framerate (string)" attribute to "framerate (fraction)" on certain cameras
Last modified: 2018-11-03 15:06:01 UTC
When playing h264 streams from Sony SNC-VB600 and FLIR FC-690-PAL cameras, the rtph264depay will receive caps containing "a-framerate" attribute, which contains string value, for example "12.0", but rtph264depay does not recognize it and reports "framerate=(fraction)0/1" on src pad, so the whole pipeline downstream has "0/1" framerate, which means "variable bitrate" according to documentation. I believe that rtph264depay could translate the nonstandard "a-framerate" to "framerate", so the caps downstream would report correct framerate when it is in fact known, instead of reporting a variable framerate (0/1), which is not true. If someone has access to cameras mentioned above (and I think it's safe to assume that probably all contemporary Sony and FLIR cameras will exhibit the same behaviour), you can check using simple pipeline: gst-launch-1.0 rtspsrc location=rtsp://<addr> ! decodebin ! xvimagesink This is the log record where "a-framerate" is first mentioned: DEBUG rtspsrc gstrtspsrc.c:1892:gst_rtspsrc_sdp_attributes_to_caps: adding caps: a-framerate=12.0 and from there, every other element has "framerate=0/1" in caps. For additional info, see bug 758023
It really should be "max-framerate", as RFC 4566 defines it as "This gives the maximum video frame rate in frames/sec." And we should probably do that in the rtpbasedepayload base class for all video/ output formats.
Created attachment 315730 [details] [review] rtpbasedepayload: set max-framerate if a-framerate is passed via caps IMHO, this patch might be a solution.
Created attachment 315731 [details] [review] rtpbasedepayload: set max-framerate if a-framerate is passed via caps Sorry there was a mistake
Review of attachment 315731 [details] [review]: Generally looks good, just a few little things. ::: gst-libs/gst/rtp/gstrtpbasedepayload.c @@ +279,3 @@ + return; + } else + return; if (media == NULL || g_strcmp0 (media, "video")) return; @@ +282,3 @@ + + src_caps = gst_pad_get_current_caps (GST_RTP_BASE_DEPAYLOAD_SRCPAD (filter)); + if (!src_caps) if (!src_caps || !gst_caps_is_fixed (src_caps)) return; Should probably also check if framerate or max-framerate is already there in the src caps. @@ +285,3 @@ + return; + + a_framerate = gst_structure_get_string (caps_struct, "a-framerate"); Can you also check for x-framerate, I've heard some people mistakenly use that. @@ +301,3 @@ + + fps_num = gst_value_get_fraction_numerator (&dest); + fps_denom = gst_value_get_fraction_denominator (&dest); You can avoid the annoying GValue using gst_util_double_to_fraction()
Created attachment 315796 [details] [review] rtpbasedepayload: set max-framerate if a-framerate is passed via caps Following Olivier's review, thanks to Olivier :) And fix minor leaks.
Created attachment 315797 [details] [review] rtpjpegdepay: Parse a-framerate using gst_util_double_to_fraction For consistency and simplification, we can adjust this patch.
Review of attachment 315796 [details] [review]: Nit: there are 4 instances of gst_caps_unref (src_caps); return; You could make that a goto at the end of the function.
Created attachment 316133 [details] [review] rtpbasedepayload: set max-framerate if a-framerate is passed via caps Following Luis's review. Thanks Luis :)
Any other comment about these patches? Can I push them?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/238.