GNOME Bugzilla – Bug 577784
Implement full H.263/+/++ caps negotiation according to RFC4629
Last modified: 2012-07-06 07:49:31 UTC
RFC 4629 specifies that the various annexes should be used when encoding only when recommended. It also specifies ways to set the maximum frame size and frame rates. The patches are in three series, against the rtp payloader, gst-ffmpeg and gst-openmax. The ffmpeg patches must be applied before the size/framerate restriction patch is added to the rtp payloader, otherwise there may be a caps negotiation failure. So what suggest is apply all the patches now except for the size/restriction patch on the payloader and only apply it after the next gst-ffmpeg release.
Created attachment 131946 [details] [review] good-0001: Set H263-2000 if thats what the other side wants The static caps states this element supports H263-2000, but setcaps never sets it, so it was lie.
Created attachment 131947 [details] [review] good-002: rtph263ppay: Implement getcaps following RFC 4629, picks the right annexes
Created attachment 131948 [details] [review] good-0003: rtph263ppay: Also implement size/framerate restrictions in getcaps This one should come after the ffmpeg/openmax patches have been merged and released.
Created attachment 131949 [details] [review] ffmpeg-0001: gstffmpegcfg: Expose loop-filter flag
Created attachment 131950 [details] [review] ffmpeg-0002: codecmap: Respect the various h263 options
Created attachment 131951 [details] [review] ffmpeg-0003: gstffmpegenc: Make getcaps proxy the downstream height/width/framerate requirements
Created attachment 131952 [details] [review] openmax-0001: Allow videoenc subclass to act on setcaps
Created attachment 131953 [details] [review] openmax-0002: omx_h263enc: Implement sink getcaps
Created attachment 131954 [details] [review] openmax-0003: omx_h263enc: Implement setcaps
Normally it's better if it continues to work with older versions of course, but I can't immediately see a way to avoid the problem here - you want the payloader to only accept restricted formats for the appropriate levels, so the encoders need to first be taught how to accept those. I don't think it'll be a problem for anyone in practice - presumably if they were configuring those levels in the past, they were already arranging for the encode to produce a compatible sized video.
Nice! For the omx part however, you are not checking the return value of the GetParameter for OMX_IndexParamVideoProfileLevelQuerySupported. If the omxil implementation doesn't support that query you'll get strange results.
Any chance to merge the ffmpeg part before the next freeze ?
Created attachment 135531 [details] [review] openmax-0003: omx_h263enc: Implement setcaps (updated) I updated the openmax patch to fix the problem pointed out by Felipe.
(In reply to comment #2) > Created an attachment (id=131947) [details] [review] > good-002: rtph263ppay: Implement getcaps following RFC 4629, picks the right > annexes >+static GstCaps * >+gst_rtp_h263p_pay_sink_getcaps (GstBaseRTPPayload * payload, GstPad * pad) >+{ >+ GstRtpH263PPay *rtph263ppay; >+ GstCaps *caps = gst_caps_new_empty (); >+ GstCaps *peercaps = NULL; >+ GstCaps *intersect = NULL; >+ guint i; >+ >+ rtph263ppay = GST_RTP_H263P_PAY (payload); >+ >+ peercaps = gst_pad_peer_get_caps (GST_BASE_RTP_PAYLOAD_SRCPAD (payload)); >+ if (!peercaps) >+ return NULL; If I read core right, it would go g_critical when getting NULL from a getcaps function, so this might have to be template caps instead (or whatever is tried to express here).
Created attachment 159091 [details] [review] rtph263ppay: Implement getcaps following RFC 4629, picks the right annexes
Comment on attachment 131947 [details] [review] good-002: rtph263ppay: Implement getcaps following RFC 4629, picks the right annexes Oops, not sure what I was thinking, updated patch attached
Btw, this seems like a good time to apply the gst-ffmpeg patches, and then the rtp -good patches following impending release ... ?
commit 21e855d167489a61aa2e8751d7b169ceccc6943e Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Thu Mar 5 19:12:18 2009 -0500 gstffmpegcfg: Expose loop-filter flag See #577784. commit 2b891aff61f30b4f12b597bb1ae325368903a22b Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Thu Mar 5 21:35:46 2009 -0500 codecmap: Respect the various h263 options See #577784. commit 83820511a8be7af645df8d2ed0289df6d44e0bdd Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Fri Mar 6 13:47:05 2009 -0500 gstffmpegenc: Make getcaps proxy the downstream height/width/framerate requirements See #577784.
Assuming these patches are still relevant and up-to-date (?), now seems like a good to apply those for the rtppayloader in -good ...
Created attachment 182419 [details] [review] good-0002: rtph263ppay: Implement getcaps following RFC 4629, picks the right annexes
Created attachment 182420 [details] [review] good-0003: rtph263ppay: Also implement size/framerate restrictions in getcaps Updated patches to build with newest git
So what's missing here now? Can the patches get committed?
I guess they should be
As good a time as any to (finally) push these, so: commit d4778dbe4341ddc13c1ce9214ae21a2e15cba25d Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Wed Mar 4 14:51:09 2009 -0500 rtph263ppay: Set H263-2000 if thats what the other side wants The static caps states this element supports H263-2000, but setcaps never sets it, so it was lie. See https://bugzilla.gnome.org/show_bug.cgi?id=577784 However, noticed something in the good-0002 patch: + peercaps = gst_pad_peer_get_caps (GST_BASE_RTP_PAYLOAD_SRCPAD (payload)); + if (!peercaps) + return + gst_caps_copy (gst_pad_get_pad_template_caps + (GST_BASE_RTP_PAYLOAD_SRCPAD (payload))); Should that not use the template caps of _SINKPAD (in a sink getcaps function) rather than of the SRCPAD ?
Setting to NEEDINFO for Mark's question in comment 24.
Created attachment 200526 [details] [review] rtph263ppay: Return the sink pad template as sink caps, not the src's
Mark: you're entirely right.. attached a fixed patch.
OK, so finally these ... commit e15c293f13429f3d4894f332db5026cbf1fd77e7 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Nov 2 12:58:12 2011 -0400 rtph263ppay: Return the sink pad template as sink caps, not the src's https://bugzilla.gnome.org/show_bug.cgi?id=577784 commit 4b28d9d44e1b332aa7dc9ceddb484ca11f4ab221 Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Sun Mar 15 19:26:48 2009 -0400 rtph263ppay: Also implement size/framerate restrictions in getcaps https://bugzilla.gnome.org/show_bug.cgi?id=577784 commit ff31090671a18900e5266d3d4a8a6995f0b4e208 Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Wed Mar 4 20:50:19 2009 -0500 rtph263ppay: Implement getcaps following RFC 4629, picks the right annexes https://bugzilla.gnome.org/show_bug.cgi?id=577784
I guess gst-openmax is not being maintained, closing this.
> I guess gst-openmax is not being maintained, closing this. You should probably file an issue against gst-openmax then, if you want Felipe to see the patches. Or just push them. Or better: port them to gst-omx ;)