GNOME Bugzilla – Bug 664817
Adding opus RTP payloader/depayloader module
Last modified: 2011-12-09 15:09:22 UTC
Created attachment 202137 [details] [review] opus_pay_depay_module.patch Adding a payloader/depayloader module for opus codec, based on the latest draft. http://tools.ietf.org/id/draft-spittka-payload-rtp-opus-00.txt
Review of attachment 202137 [details] [review]: I don't know much about RTP, so I may not have caught up any RTP semantics issue. You're using a gmail.com email for author info - maybe this needs to be the Collabora one, as well as a copyright + year line. Comments follow: ::: gst/rtpopus/gstrtpopus.c @@ +38,3 @@ + return FALSE; + + gst_tag_register_musicbrainz_tags (); I don't think you need that. There will be no Vorbis comment metadata through RTP, will there ? @@ +46,3 @@ + GST_VERSION_MINOR, + "rtpopus", + "RTP payloader/depayloader for OPUS encoder", "for Opus codec" ? ::: gst/rtpopus/gstrtpopusdepay.c @@ +45,3 @@ + GST_PAD_SRC, + GST_PAD_ALWAYS, + GST_STATIC_CAPS ("audio/x-opus, " "rate = (int) [ 8000, 48000 ]") Rates are constrained to 8000, 12000, 16000, 24000, 48000 only, but this is not inherent to the Opus stream. So caps should be just audio/x-opus here. @@ +98,3 @@ + ret = gst_pad_set_caps (GST_BASE_RTP_DEPAYLOAD_SRCPAD (depayload), srccaps); + + GST_DEBUG ("set caps on source: %" GST_PTR_FORMAT " (ret=%d)", srccaps, ret); Usually better to use the _OBJECT variant, it helps to dintinguish which object the log comes from in debug logs. @@ +119,3 @@ +{ + return gst_element_register (plugin, "rtpopusdepay", + GST_RANK_SECONDARY, GST_TYPE_RTP_OPUS_DEPAY); That registration already done in gstrtpopus.c ::: gst/rtpopus/gstrtpopuspay.c @@ +35,3 @@ + GST_PAD_SINK, + GST_PAD_ALWAYS, + GST_STATIC_CAPS ("audio/x-opus, " "rate = (int) [ 8000, 48000 ]") Same comment about not needing rate here. @@ +57,3 @@ + GST_TYPE_BASE_RTP_PAYLOAD) + + static void gst_rtp_opus_pay_base_init (gpointer klass) Newline after static void. gst-indent might cause trouble here... @@ +68,3 @@ + gst_element_class_set_details_simple (element_class, + "RTP Opus payloader", "Codec/Payloader/Network/RTP", + "Payloadv Opus GS buffers as RTP packets", "Payloadv". Also, what is GS ? @@ +133,3 @@ + payload = gst_rtp_buffer_get_payload (outbuf); + + memcpy (payload, data, payload_len); Should this not be size instead of payload_len ? If payload_len is larger than size, crash. If it's less, some of the Opus data will be discarded. @@ +149,3 @@ +{ + return gst_element_register (plugin, "rtpopuspay", + GST_RANK_NONE, GST_TYPE_RTP_OPUS_PAY); Init already done as well (with different rank though).
Created attachment 202333 [details] [review] New patch
(In reply to comment #1) > Review of attachment 202137 [details] [review]: > > I don't know much about RTP, so I may not have caught up any RTP semantics > issue. > You're using a gmail.com email for author info - maybe this needs to be the > Collabora one, > as well as a copyright + year line. > Comments follow: > > ::: gst/rtpopus/gstrtpopus.c > @@ +38,3 @@ > + return FALSE; > + > + gst_tag_register_musicbrainz_tags (); > > I don't think you need that. There will be no Vorbis comment metadata through > RTP, will there ? REMOVED > > @@ +46,3 @@ > + GST_VERSION_MINOR, > + "rtpopus", > + "RTP payloader/depayloader for OPUS encoder", > > "for Opus codec" ? Fixed > > ::: gst/rtpopus/gstrtpopusdepay.c > @@ +45,3 @@ > + GST_PAD_SRC, > + GST_PAD_ALWAYS, > + GST_STATIC_CAPS ("audio/x-opus, " "rate = (int) [ 8000, 48000 ]") > > Rates are constrained to 8000, 12000, 16000, 24000, 48000 only, but this is not > inherent to the Opus stream. So caps should be just audio/x-opus here. > Removed > @@ +98,3 @@ > + ret = gst_pad_set_caps (GST_BASE_RTP_DEPAYLOAD_SRCPAD (depayload), srccaps); > + > + GST_DEBUG ("set caps on source: %" GST_PTR_FORMAT " (ret=%d)", srccaps, > ret); > > Usually better to use the _OBJECT variant, it helps to dintinguish which object > the log comes from in debug logs. I agree, Fixed > > @@ +119,3 @@ > +{ > + return gst_element_register (plugin, "rtpopusdepay", > + GST_RANK_SECONDARY, GST_TYPE_RTP_OPUS_DEPAY); > > That registration already done in gstrtpopus.c Fixed > > ::: gst/rtpopus/gstrtpopuspay.c > @@ +35,3 @@ > + GST_PAD_SINK, > + GST_PAD_ALWAYS, > + GST_STATIC_CAPS ("audio/x-opus, " "rate = (int) [ 8000, 48000 ]") > > Same comment about not needing rate here. Fixed > > @@ +57,3 @@ > + GST_TYPE_BASE_RTP_PAYLOAD) > + > + static void gst_rtp_opus_pay_base_init (gpointer klass) > > Newline after static void. gst-indent might cause trouble here... Fixed. Missing a ; on the line before that, so gst-indent doesn't complain about it. > > @@ +68,3 @@ > + gst_element_class_set_details_simple (element_class, > + "RTP Opus payloader", "Codec/Payloader/Network/RTP", > + "Payloadv Opus GS buffers as RTP packets", > > "Payloadv". Also, what is GS ? Fixed > > @@ +133,3 @@ > + payload = gst_rtp_buffer_get_payload (outbuf); > + > + memcpy (payload, data, payload_len); > > Should this not be size instead of payload_len ? > If payload_len is larger than size, crash. If it's less, some of the Opus data > will be discarded. We can discuss that on IRC later. > > @@ +149,3 @@ > +{ > + return gst_element_register (plugin, "rtpopuspay", > + GST_RANK_NONE, GST_TYPE_RTP_OPUS_PAY); > > Init already done as well (with different rank though). Fixed.
Review of attachment 202333 [details] [review]: ::: gst/rtpopus/gstrtpopus.c @@ +25,3 @@ +#include "gstrtpopusdepay.h" + +#include <gst/tag/tag.h> Not needed. @@ +31,3 @@ +{ + gst_rtp_opus_pay_plugin_init (plugin); + gst_rtp_opus_depay_plugin_init (plugin); Should return FALSE if any of these returns FALSE. @@ +39,3 @@ + GST_VERSION_MINOR, + "rtpopus", + "RTP payloader/depayloader for OPUS encoder", Nitpick (sorry) for s/encoder/codec/. ::: gst/rtpopus/gstrtpopusdepay.c @@ +68,3 @@ + gst_static_pad_template_get (&gst_rtp_opus_depay_src_template)); + gst_element_class_add_pad_template (element_class, + gst_static_pad_template_get (&gst_rtp_opus_depay_sink_template)); These leak (though almost every element was till recently). Use gst_element_class_add_static_pad_template (new in core). ::: gst/rtpopus/gstrtpopusdepay.h @@ +2,3 @@ + * Opus Depayloader Gst Element + * + * @author: Danilo Cesar Lemes de Paula <danilo.eu@gmail.com> Still the old one :) ::: gst/rtpopus/gstrtpopuspay.c @@ +68,3 @@ + gst_static_pad_template_get (&gst_rtp_opus_pay_src_template)); + gst_element_class_add_pad_template (element_class, + gst_static_pad_template_get (&gst_rtp_opus_pay_sink_template)); Leaks too, same fix as the other element. @@ +150,3 @@ + gst_buffer_unref (buffer); + + g_warning ("SIZE: %d, PACKET: %d PAYLOAD: %d", size, packet_len, payload_len); GST_DEBUG_OBJECT @@ +159,3 @@ +{ + return gst_element_register (plugin, "rtpopuspay", + GST_RANK_NONE, GST_TYPE_RTP_OPUS_PAY); This may be intended, but this has rank NONE while the depayloader has rank PRIMARY, so I'll mention that in case there's anything going on here. ::: gst/rtpopus/gstrtpopuspay.h @@ +2,3 @@ + * Opus Payloader Gst Element + * + * @author: Danilo Cesar Lemes de Paula <danilo.eu@gmail.com> Old one.
Created attachment 202960 [details] [review] rtpopus: Improve payloader Here are some patches against your patch: - Just don't put the rate in the caps, as we don't know what they are from the RTP with Opus - gst_rtp_buffer_new_allocate() does the buffer size calculations for you - gst_pad_push eats the ref, so don't unref the buffer afterwards. That said, it still faisl with Empathy, and I'm really starting to believe that the problem lies somewhere in the overly complicated Encoder or decoder elements.
With the patch I just put on bug #665078, the RTP elements work with Empathy! The patches here should be squashed and committed I think.
Perhaps we could put the rtp payloader/depayloader into ext/opus as well until the spec is stable and we can move it to good/gst/rtp?
Ah, I disabled coupling in 631f42eee82c17c352d7fcab96eb387650018b61 since I could not understand how to get it working generically for multistream, but that also disabled it for stereo, sorry.
(In reply to comment #7) > Perhaps we could put the rtp payloader/depayloader into ext/opus as well until > the spec is stable and we can move it to good/gst/rtp? Yes, I can do that. Also I will merge Ocrete's commit and send it the patch again.
Created attachment 203014 [details] [review] latest patch version I'm submitting the patch again including: - merged Ocrete's patch - Tim's suggestion Vicent: is it ok from your POV to put the RTP thing with the enc/dec library?
Sure, the elements are separate anyway, so the RTP ones can easily be moved later to -good since there's no git history (yet).
Review of attachment 203014 [details] [review]: Looks good, lets go!
commit d5bf38d8bfec4ec068425ee19ea27d9b1043e358 Author: Danilo Cesar Lemes de Paula <danilo.cesar@collabora.co.uk> Date: Wed Dec 7 15:13:11 2011 -0200 Adding opus RTP payloader/depayloader element Adding OPUS RTP module based on the current draft: http://tools.ietf.org/id/draft-spittka-payload-rtp-opus-00.txt https://bugzilla.gnome.org/show_bug.cgi?id=664817