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 700748 - rtpjpegpay/depay: Add framerate and optional framesize SDP attribute to payloaded caps
rtpjpegpay/depay: Add framerate and optional framesize SDP attribute to paylo...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-20 21:26 UTC by Sebastian Rasmussen
Modified: 2013-05-31 13:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch adding framerate SDP attribute value to caps. (2.82 KB, patch)
2013-05-20 21:30 UTC, Sebastian Rasmussen
committed Details | Review
Proposed patch adding framesize SDP attribute value to caps. (4.88 KB, patch)
2013-05-20 21:30 UTC, Sebastian Rasmussen
committed Details | Review
Proposed patch replacing framesize with width/height in caps (7.28 KB, patch)
2013-05-22 01:46 UTC, Sebastian Rasmussen
none Details | Review
Proposed patch replacing framerate caps field with fraction instead of string (7.49 KB, patch)
2013-05-22 01:47 UTC, Sebastian Rasmussen
none Details | Review
Proposed patch replacing framesize with width/height in caps (7.54 KB, patch)
2013-05-22 09:17 UTC, Sebastian Rasmussen
none Details | Review
Proposed patch replacing framerate caps field with fraction instead of string (7.00 KB, patch)
2013-05-22 09:18 UTC, Sebastian Rasmussen
none Details | Review
Proposed patch replacing framesize with width/height in caps (8.13 KB, patch)
2013-05-23 12:22 UTC, Sebastian Rasmussen
committed Details | Review
Proposed patch replacing framerate caps field with fraction instead of string (7.84 KB, patch)
2013-05-23 12:22 UTC, Sebastian Rasmussen
committed Details | Review

Description Sebastian Rasmussen 2013-05-20 21:26:58 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.
Comment 1 Sebastian Rasmussen 2013-05-20 21:30:34 UTC
Created attachment 244870 [details] [review]
Proposed patch adding framerate SDP attribute value to caps.
Comment 2 Sebastian Rasmussen 2013-05-20 21:30:48 UTC
Created attachment 244871 [details] [review]
Proposed patch adding framesize SDP attribute value to caps.
Comment 3 Sebastian Dröge (slomo) 2013-05-21 07:09:29 UTC
commit 2361567bae3f93e725a478af2557c24470cd535d
Author: Sebastian Rasmussen <sebrn@axis.com>
Date:   Mon May 20 21:44:13 2013 +0200

    rtpjpegpay/depay: Add framesize caps for use in SDP
    
    The format of the value adheres to RFC6064 and it is meant to be parsed
    and included in the SDP sent by gst-rtsp-server to its clients.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700748

commit 919eed07871b456cf814d8a805d37c473a4e0535
Author: Sebastian Rasmussen <sebrn@axis.com>
Date:   Mon May 20 21:34:13 2013 +0200

    rtpjpegpay: Add optional framerate caps for use in SDP
    
    The format of the value adheres to RFC4566 and it is meant to be parsed
    and included in the SDP sent by gst-rtsp-server to its clients.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700748
Comment 4 Sebastian Dröge (slomo) 2013-05-21 07:21:28 UTC
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
Comment 5 Sebastian Rasmussen 2013-05-22 01:46:42 UTC
Created attachment 245010 [details] [review]
Proposed patch replacing framesize with width/height in caps
Comment 6 Sebastian Rasmussen 2013-05-22 01:47:17 UTC
Created attachment 245011 [details] [review]
Proposed patch replacing framerate caps field with fraction instead of string
Comment 7 Tim-Philipp Müller 2013-05-22 06:49:49 UTC
I'm a bit confused. why add width/height to the payloader src template caps and not sink template? if you make it mandatory which is fine imho since input needs to be parsed then add it to the input template caps too.
Comment 8 Sebastian Rasmussen 2013-05-22 09:17:04 UTC
> I'm a bit confused. why add width/height to the payloader src template caps and
> not sink template? if you make it mandatory which is fine imho since input
> needs to be parsed then add it to the input template caps too.

yeah, that makes no sense. I also accidentally used "int" instead of "(int)" in the first patch which was changed in the second patch.

I'm uploading corrected patches (again). Hoefully thi is the last round...
Comment 9 Sebastian Rasmussen 2013-05-22 09:17:41 UTC
Created attachment 245021 [details] [review]
Proposed patch replacing framesize with width/height in caps
Comment 10 Sebastian Rasmussen 2013-05-22 09:18:21 UTC
Created attachment 245022 [details] [review]
Proposed patch replacing framerate caps field with fraction instead of string
Comment 11 Olivier Crête 2013-05-22 09:31:22 UTC
You probably want to fallback to reading the x-dimensions in the depayloader at least so it stays compatible with older GStreamer versions.
Comment 12 Sebastian Dröge (slomo) 2013-05-22 09:33:47 UTC
As width/height are optional, I don't think they should be in the depayloader template caps. In the payloader they should though as you require them (in both template caps).

Otherwise looks good
Comment 13 Sebastian Rasmussen 2013-05-22 10:15:01 UTC
(In reply to comment #11)
> You probably want to fallback to reading the x-dimensions in the depayloader at
> least so it stays compatible with older GStreamer versions.

The x-dimensions never really ended up as a=x-dimensions: 1234,1234 but rather as a=fmtp ...; x-dimensions=1234,1234; ... So I view it as x-dimensions never really worked as intended. I guess you could argue that the negotiation between payloader/depayloader might have worked though.
Comment 14 Sebastian Rasmussen 2013-05-22 10:22:23 UTC
(In reply to comment #12)
> As width/height are optional, I don't think they should be in the depayloader
> template caps. In the payloader they should though as you require them (in both
> template caps).

Wait, what..? Are width/height optional to the depayloader..? Why? Would this apply to rtph264pay as well in https://bugzilla.gnome.org/show_bug.cgi?id=700749?
Comment 15 Sebastian Rasmussen 2013-05-22 14:05:45 UTC
Ok, so the details were hashed out on #gstreamer instead of in bugzilla, quoted here should anyone try to figure out why the code looks like this at some point.

12:01 < sebras> slomo: when I spoke with wtay the other day I read him as that it was ok to always set the framesize SDP attribute. this is why I see width/height as required.
12:02 < slomo> sure, it's always ok to *generate* that
12:02 < slomo> but can we always require to *get* that from anything that generates rtp streams?
12:02 < slomo> from what you say i don't think so
12:02 < slomo> so it should be optional in the depayloader, but mandatory in the payloader
12:02 < sebras> slomo: ok. so then there shouldn't even be width/height on the srcpad of the depayloader...?
12:03 < sebras> slomo: however if we happen to get those width/height caps then we might propagate them.
12:03 < slomo> yes, the depayloader shouldn't have that in the template caps (on both pads). just add it on srcpad caps if the input contains it
12:04 < sebras> slomo: ok. back to the drawing board. thanks for hashing this out. :)
12:04 < slomo> sebras: that's the same behaviour you have for the framerate, right?
12:06 < sebras> slomo: mostly. framerate is not required even in the payloader as it might not be present in caps (not required for parsed jpeg/h264).
12:08 < slomo> sebras: and in the depayloader it's optional too (i.e. not in the template caps, but used when present)?
12:08 < sebras> slomo: yes, exactly.
12:09 < sebras> slomo: and in gst-rtsp-server the a=framerate SDP attribute is generated if the framerate caps is present.
12:10 < slomo> ok
12:10 < slomo> so just do the same for the width/height in the depayloader ;) but in the payloader it should be mandatory (so in both template caps)
Comment 16 Sebastian Rasmussen 2013-05-23 12:22:23 UTC
Created attachment 245121 [details] [review]
Proposed patch replacing framesize with width/height in caps
Comment 17 Sebastian Rasmussen 2013-05-23 12:22:38 UTC
Created attachment 245122 [details] [review]
Proposed patch replacing framerate caps field with fraction instead of string
Comment 18 Sebastian Dröge (slomo) 2013-05-23 13:56:02 UTC
Review of attachment 245122 [details] [review]:

::: gst/rtp/gstrtpjpegdepay.c
@@ +495,3 @@
+  if (gst_structure_get_fraction (structure, "framerate", &num, &denom) &&
+      (num < 0 || denom <= 0)) {
+    goto invalid_framerate;

Should work without framerate too as before
Comment 19 Sebastian Dröge (slomo) 2013-05-23 13:56:49 UTC
Review of attachment 245121 [details] [review]:

::: gst/rtp/gstrtpjpegdepay.c
@@ +470,3 @@
+  }
+  if (gst_structure_get_int (structure, "height", &height) && height <= 0) {
+    goto invalid_dimension;

Should work without width/height as before too
Comment 20 Olivier Crête 2013-05-23 14:35:46 UTC
Vs comment #19, I think all that does is reject "height is there but is 0".. That said, framerate 0/N is valid and should be ignored, not rejected.
Comment 21 Sebastian Rasmussen 2013-05-23 19:02:52 UTC
(In reply to comment #20)
> Vs comment #19, I think all that does is reject "height is there but is 0"..

yes, exactly.

> That said, framerate 0/N is valid and should be ignored, not rejected.

but my framerate checks are on the form (num < 0 || denom <= 0) which means that if num == 0 and denom == 1 the expression evaluates to FALSE which means that the framerate will not be considered invalid (the jump is not taken).

so any negative numerator or denominator would be invalid, as would a denominator which is equal to zero. I believe this to be correct.
Comment 22 Sebastian Dröge (slomo) 2013-05-23 19:10:03 UTC
Yes, we're just all a bit confused :) It's all good!

commit 9fd25a810b859e0ec205176578735100d83de4af
Author: Sebastian Rasmussen <sebrn@axis.com>
Date:   Wed May 22 02:40:52 2013 +0200

    rtpjpegpay/depay: Replace framerate caps field with fraction
    
    The previous implementation had the formatting of SDP attributes happen
    in each RTP payloader, now instead the constituent values are propagated
    as caps fields. This allows for applications to do SDP offer/answer
    based on caps negotiation.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700748

commit 0075d111b475ca27895ee9476154260b6902940b
Author: Sebastian Rasmussen <sebrn@axis.com>
Date:   Wed May 22 01:58:57 2013 +0200

    rtpjpegpay/depay: Replace framesize caps with width/height
    
    The previous implementation had the formatting of SDP attributes happen
    in each RTP payloader, now instead the constituent values are propagated
    as caps fields. This allows for applications to do SDP offer/answer
    based on caps negotiation.
    
    Keep parsing a-framerate, x-framerate and x-dimensions in rtpjpegdepay
    to be backwards compatible with previous payloaders.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=700748
Comment 23 Wim Taymans 2013-05-31 13:47:24 UTC
reverted now.