GNOME Bugzilla – Bug 564437
rtpjpegdepay was unable to handle frame dimensions greater than 2040
Last modified: 2009-08-03 16:15:07 UTC
Please describe the problem: The frame dimensions are coded on 8 bits in the rtp payload, so the maximum frame dimension was 255*8=2040. To overcome this, the frame dimensions, when > 2040, are passed as SDP media arguments, "x-dimensions" or "x-width" and "x-height", like for live555 protocol, and the rtp payload dimension is set to 0. rtspsrc must put in the caps those attributes and the jpeg depayloader must use them when the rtp payload dimension is set to 0. Steps to reproduce: 1. set your camera to 2048x1536 resolution or any dimension > 2040 2. gst-launch rtspsrc location=rtsp://cam ! rtpjpegdepay ! jpegdec ! autovideosink 3. Actual results: jpegdec complains about NULL frame size Expected results: display frames up to maximum jpeg frame dimensions (has to be fixed in gstjpegdec.c where it is told MAX_WIDTH=4096, which is false) Does this happen every time? yes Other information: other sdp media or session attributes beginning with "x-", or registered attributes from http://www.iana.org/assignments/sdp-parameters could also be copied in caps, maybe with a prefix like 'a-' for registered ones to avoid collision with caps attributes names
i need to finalize my patches in order to comply with sdp RFC, but they are already working if there is no sdp attribute name collision. actually the session level attributes are discarded, only the media level attributes are copied to caps without filtering except for "width" and "height" that i will remove it is working yet, but i'll post the final patch in a few days or tomorrow..
Created attachment 124617 [details] [review] cvs diff -c gst/rtsp/gstrtspsrc.c to be finalized, already working
Created attachment 124618 [details] [review] cvs diff -c gst/rtp/gstrtpjpegdepay.h to be finalized, already working
Created attachment 124619 [details] [review] cvs diff -c gst/rtp/gstrtpjpegdepay.c to be finalized, already working
It's okay for me ! Now session level SDP attributes are mapped to caps first, before media level ones. SDP attributes that are prefixed with 'x-' (ie: not IANA registered, see http://www.iana.org/assignments/sdp-parameters), are copied 'as is' in the caps. SDP attributes that are IANA registered (not prefixed with 'x-') are now prefixed with 'a-' (could be changed), to avoid collision with existing gstreamer caps properties names. RFC recommends to discard unrecognized attributes, but who knows who need what, when attributes are not mandatory IANA registered, and when registered attributes list is big and subject to change ? Mapping all incoming attributes according to the previous paragraph rules seems to be the better solution. here are the 'final' patches:
Created attachment 124640 [details] [review] cvs diff -c gst/rtsp/gstrtspsrc.c
Created attachment 124641 [details] [review] cvs diff -c gst/rtp/gstrtpjpegdepay.h
Created attachment 124642 [details] [review] cvs diff -c gst/rtp/gstrtpjpegdepay.c
Created attachment 124643 [details] [review] cvs diff -c gst/rtp/gstrtpjpegdepay.c comment update
Created attachment 124706 [details] [review] cvs diff -c gst/rtp/gstrtpjpegdepay.c removed x-width and x-height since nobody sends them except my camera ($hit goes down..), which is also sending x-dimensions (which is used by live555 and quicktime)... changed from x-framerate to a-framerate, since "framerate" is a registered IANA sdp media attribute, and nobody sends x-framerate except my camera (...) which is also sending "framerate" ... in two words: i removed some useless crap and corrected a little bug (changed x-framerate to a-framerate), and added support for optional gstreamer caps "framerate"
damn :) theres a "framesize" attribute used to specify the maximum frame size used during a session :) but it is not yet IANA registered.. more about it here: http://www.ietf.org/internet-drafts/draft-westerlund-mmusic-3gpp-sdp-rtsp-06.txt or here: http://lists.live555.com/pipermail/live-devel/2007-December.txt or in any search engine: sdp "a=framesize" wtdo ? maybe i'll add it tomorrow... :/
Lets take a look on the old and OBSOLETE RFC2327 and see whats told in there : " Extension Mechanism: Tool writers can define experimental bandwidth modifiers by prefixing their modifier with "X-". For example: b=X-YZ:128 SDP parsers should ignore bandwidth fields with unknown modifiers. Modifiers should be alpha-numeric and, although no length limit is given, they are recommended to be short. " *** in the above paragrah from OBSOLETE RFC2327 they talk about bandwidth fields with unknown modifiers, not about media or session attributes, " Attributes that will be commonly used can be registered with IANA (see Appendix B). Unregistered attributes should begin with "X-" to prevent inadvertent collision with registered attributes. In either case, if an attribute is received that is not understood, it should simply be ignored by the receiver. " (...) *** however the OBSOLETE RFC2327 say above it is ok to use "x-" prefix for unregistered attributes... but is now obsoleted by RFC4566, which tell now : " Most uses of the "x-" prefix notation for experimental parameters are disallowed and the other uses are deprecated. " (...) *** so they say in the paragraph above of RFC4566 that it is disallowed or deprecated to use "x-" in general ( they do not say anything about the context), so it would also apply to session and media attributes... " Use of the "X-" prefix is NOT RECOMMENDED: instead new modifiers SHOULD be registered with IANA in the standard namespace. SDP parsers MUST ignore bandwidth fields with unknown modifiers. Modifiers MUST be alphanumeric and, although no length limit is given, it is recommended that they be short. " (...) *** And the paragraph above from RFC4566 is not talking about session or media attributes but about modifiers for bandwidth fields... Hopefully i have much too much hairs (here and now) :D
Could you use 'cvs diff -up' for generating your patches ?
ok, gonna regenerate my patches with "cvs diff -up", sorry I was thinking about writing to all those people to say they should register their attributes with IANA before documenting them (...) , or prepend X- in front of them in the meanwhile, as "here and now" they can only be called "application specific" or "experimental" and, to follow the RFC2327 convention, they should be prefixed with "x-" to avoid mind confusion and leave me (sleep) alone :) But since RFC4566 say it is now disallowed or deprecated to use "x-" and "a=dimensions" seems not used for mjpeg, so i can ignore it until its uses spread into some mjpeg streamer implemenation. I can now just rest in peace and forget it :) bug 564437 is now fixed (isn't it ?)
Created attachment 124727 [details] [review] same patches, but done with cvs diff -up
Created attachment 124728 [details] just a resend, bugzilla doesnt like .tar.gz tagged as "patch"
Created attachment 124749 [details] [review] cvs diff -up gst/rtp gst/rtsp sorry im quite unexperienced with bugzilla :) hope it is the right way to send a patch this time :P
The "x-dimensions" sdp attribute is coming with quicktime, And we know that proprietary softwares like Apple or Microsoft ones are not aiming inter-operability at all, and that rather the inverse is true... So we are facing the fact that, to allow inter-operability with those softwares, we must cope with unilateral conventions that are not following the free software community standards or conventions: a good example is javascript. So in the crazy world it is not totally heretic to be aware of and to support some sdp session or media attributes not registered with iana.org or said to be 'disallowed' or 'deprecated' by RFCs... but it should be avoided as much as possible... (...) :) LOL
I dont understand why there's no trace of my patches in the latest git or stable releases :(((
It seems that while working on bugs 580880 or 578257, somebody (Wim Taymans ?) did erase fixes for this one
I dont know what happened exactly. That's weird: the bug is marked fixed in the release notes for version 0.10.14 http://gstreamer.freedesktop.org/releases/gst-plugins-good/0.10.14.html But there no trace of the fixes in the sourcecode (0.10.14 and later), And nothing in git history: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/log/?qt=grep&q=rtp&ofs=50 :) Thanks in advance for including this fix in the next release ! Best regards, L:üc:
> I dont know what happened exactly. > That's weird: the bug is marked fixed in the release notes for version 0.10.14 It looks like (see 'View Bug Activity' at the top) you marked it as fixed and no one noticed that that wasn't actually the case.
Created attachment 138905 [details] [review] git diff (previous patch could no more be applied) not yet tested, i'll be back home in 2 days
I did some changes and cleanups here and there. commit 784b95ddbf8c3f3babe50c641d2ef8fe64442d15 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Aug 3 18:13:46 2009 +0200 rtspsrc: don't add non-utf8 chars to structures commit 654ca56d854471fcceb9e406c68a0e2f6f48e116 Author: Luc Deschenaux <luc.deschenaux at freesurf.ch> Date: Mon Aug 3 18:02:31 2009 +0200 jpegdepay: use attributes for extra properties Use some of the SDP attributes when they are present to specify the output dimension and framerate. This allows us to receive jpeg frames larger than 2040 width/height. Fixes #564437 commit efb9c1797553d88c073c8f8ceccfd9e638894fd2 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Aug 3 18:01:27 2009 +0200 RTP docs: update with attributes in caps commit f96e900a649436db630f41c9e0edf779ba599169 Author: Luc Deschenaux <luc.deschenaux at freesurf.ch> Date: Mon Aug 3 17:21:44 2009 +0200 rtspsrc: put all SDP attributes on caps Put the SDP attributes on the caps too so that they can be used by depayloaders. See #564437