Created attachment 244870 [details] [review]
Proposed patch adding framerate SDP attribute value to caps.

Created attachment 244871 [details] [review]
Proposed patch adding framesize SDP attribute value to caps.

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

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

Created attachment 245010 [details] [review]
Proposed patch replacing framesize with width/height in caps

Created attachment 245011 [details] [review]
Proposed patch replacing framerate caps field with fraction instead of string

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.

> 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...

Created attachment 245021 [details] [review]
Proposed patch replacing framesize with width/height in caps

Created attachment 245022 [details] [review]
Proposed patch replacing framerate caps field with fraction instead of string

You probably want to fallback to reading the x-dimensions in the depayloader at least so it stays compatible with older GStreamer versions.

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

(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.

(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?

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)

Created attachment 245121 [details] [review]
Proposed patch replacing framesize with width/height in caps

Created attachment 245122 [details] [review]
Proposed patch replacing framerate caps field with fraction instead of string

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

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

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.

(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.

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

reverted now.