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 664817 - Adding opus RTP payloader/depayloader module
Adding opus RTP payloader/depayloader module
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 665078
Blocks:
 
 
Reported: 2011-11-25 15:44 UTC by Danilo Cesar
Modified: 2011-12-09 15:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
opus_pay_depay_module.patch (16.84 KB, patch)
2011-11-25 15:44 UTC, Danilo Cesar
reviewed Details | Review
New patch (17.06 KB, patch)
2011-11-28 21:57 UTC, Danilo Cesar
needs-work Details | Review
rtpopus: Improve payloader (2.38 KB, patch)
2011-12-07 05:16 UTC, Olivier Crête
none Details | Review
latest patch version (15.05 KB, patch)
2011-12-07 17:22 UTC, Danilo Cesar
committed Details | Review

Description Danilo Cesar 2011-11-25 15:44:58 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
Comment 1 Vincent Penquerc'h 2011-11-25 16:58:50 UTC
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).
Comment 2 Danilo Cesar 2011-11-28 21:57:30 UTC
Created attachment 202333 [details] [review]
New patch
Comment 3 Danilo Cesar 2011-11-28 21:57:57 UTC
(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.
Comment 4 Vincent Penquerc'h 2011-11-29 11:30:23 UTC
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.
Comment 5 Olivier Crête 2011-12-07 05:16:26 UTC
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.
Comment 6 Olivier Crête 2011-12-07 05:52:06 UTC
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.
Comment 7 Tim-Philipp Müller 2011-12-07 10:28:02 UTC
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?
Comment 8 Vincent Penquerc'h 2011-12-07 11:57:51 UTC
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.
Comment 9 Danilo Cesar 2011-12-07 13:00:00 UTC
(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.
Comment 10 Danilo Cesar 2011-12-07 17:22:41 UTC
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?
Comment 11 Vincent Penquerc'h 2011-12-07 17:55:52 UTC
Sure, the elements are separate anyway, so the RTP ones can easily be moved later to -good since there's no git history (yet).
Comment 12 Olivier Crête 2011-12-08 15:38:04 UTC
Review of attachment 203014 [details] [review]:

Looks good, lets go!
Comment 13 Vincent Penquerc'h 2011-12-09 15:09:22 UTC
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