[Bug 664817] Adding opus RTP payloader/depayloader module

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=664817
  GStreamer | gst-plugins-bad | git

Vincent Penquerc'h <vincent.penquerch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #202137|none                        |reviewed
             status|                            |

--- Comment #1 from Vincent Penquerc'h <vincent.penquerch at collabora.co.uk> 2011-11-25 16:58:50 UTC ---
Review of attachment 202137:
 --> (https://bugzilla.gnome.org/review?bug=664817&attachment=202137)

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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.