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 564437 - rtpjpegdepay was unable to handle frame dimensions greater than 2040
rtpjpegdepay was unable to handle frame dimensions greater than 2040
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 0.10.16
Assigned To: Wim Taymans
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-12-14 02:59 UTC by Luc
Modified: 2009-08-03 16:15 UTC
See Also:
GNOME target: ---
GNOME version: 2.23/2.24


Attachments
cvs diff -c gst/rtsp/gstrtspsrc.c (1.04 KB, patch)
2008-12-14 03:14 UTC, Luc
none Details | Review
cvs diff -c gst/rtp/gstrtpjpegdepay.h (609 bytes, patch)
2008-12-14 03:15 UTC, Luc
none Details | Review
cvs diff -c gst/rtp/gstrtpjpegdepay.c (5.38 KB, patch)
2008-12-14 03:16 UTC, Luc
none Details | Review
cvs diff -c gst/rtsp/gstrtspsrc.c (2.67 KB, patch)
2008-12-14 12:08 UTC, Luc
none Details | Review
cvs diff -c gst/rtp/gstrtpjpegdepay.h (609 bytes, patch)
2008-12-14 12:09 UTC, Luc
none Details | Review
cvs diff -c gst/rtp/gstrtpjpegdepay.c (5.40 KB, patch)
2008-12-14 12:09 UTC, Luc
none Details | Review
cvs diff -c gst/rtp/gstrtpjpegdepay.c (5.42 KB, patch)
2008-12-14 12:20 UTC, Luc
none Details | Review
cvs diff -c gst/rtp/gstrtpjpegdepay.c (5.14 KB, patch)
2008-12-15 06:48 UTC, Luc
none Details | Review
same patches, but done with cvs diff -up (2.62 KB, patch)
2008-12-15 14:17 UTC, Luc
none Details | Review
just a resend, bugzilla doesnt like .tar.gz tagged as "patch" (2.62 KB, application/x-gzip)
2008-12-15 14:20 UTC, Luc
  Details
cvs diff -up gst/rtp gst/rtsp (7.35 KB, patch)
2008-12-15 18:59 UTC, Luc
none Details | Review
git diff (previous patch could no more be applied) (7.77 KB, patch)
2009-07-21 13:40 UTC, Luc
committed Details | Review

Description Luc 2008-12-14 02:59:38 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
Comment 1 Luc 2008-12-14 03:03:09 UTC
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..
Comment 2 Luc 2008-12-14 03:14:01 UTC
Created attachment 124617 [details] [review]
cvs diff -c  gst/rtsp/gstrtspsrc.c

to be finalized, already working
Comment 3 Luc 2008-12-14 03:15:28 UTC
Created attachment 124618 [details] [review]
cvs diff -c  gst/rtp/gstrtpjpegdepay.h

to be finalized, already working
Comment 4 Luc 2008-12-14 03:16:27 UTC
Created attachment 124619 [details] [review]
cvs diff -c  gst/rtp/gstrtpjpegdepay.c

to be finalized, already working
Comment 5 Luc 2008-12-14 12:07:13 UTC
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:
Comment 6 Luc 2008-12-14 12:08:24 UTC
Created attachment 124640 [details] [review]
cvs diff -c gst/rtsp/gstrtspsrc.c
Comment 7 Luc 2008-12-14 12:09:01 UTC
Created attachment 124641 [details] [review]
cvs diff -c gst/rtp/gstrtpjpegdepay.h
Comment 8 Luc 2008-12-14 12:09:36 UTC
Created attachment 124642 [details] [review]
cvs diff -c gst/rtp/gstrtpjpegdepay.c
Comment 9 Luc 2008-12-14 12:20:03 UTC
Created attachment 124643 [details] [review]
cvs diff -c gst/rtp/gstrtpjpegdepay.c

comment update
Comment 10 Luc 2008-12-15 06:48:16 UTC
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"
Comment 11 Luc 2008-12-15 07:31:33 UTC
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... :/
Comment 12 Luc 2008-12-15 13:44:47 UTC
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 


Comment 13 Edward Hervey 2008-12-15 14:00:46 UTC
Could you use 'cvs diff -up' for generating your patches ?
Comment 14 Luc 2008-12-15 14:07:08 UTC
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 ?)

Comment 15 Luc 2008-12-15 14:17:52 UTC
Created attachment 124727 [details] [review]
same patches, but done with cvs diff -up
Comment 16 Luc 2008-12-15 14:20:19 UTC
Created attachment 124728 [details]
just a resend, bugzilla doesnt like .tar.gz tagged as "patch"
Comment 17 Luc 2008-12-15 18:59:28 UTC
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
Comment 18 Luc 2008-12-15 22:30:45 UTC
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
Comment 19 Luc 2009-07-03 12:33:12 UTC
I dont understand why there's no trace of my patches in the latest git or stable releases

:(((


Comment 20 Luc 2009-07-03 14:02:10 UTC
It seems that while working on bugs 580880 or 578257, somebody (Wim Taymans ?) did erase fixes for this one
Comment 21 Luc 2009-07-03 14:46:02 UTC
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:
Comment 22 Tim-Philipp Müller 2009-07-03 14:56:45 UTC
> 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.
Comment 23 Luc 2009-07-21 13:40:35 UTC
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
Comment 24 Wim Taymans 2009-08-03 16:15:07 UTC
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