[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

--- Comment #3 from Danilo Cesar <danilo.eu at gmail.com> 2011-11-28 21:57:57 UTC ---
(In reply to comment #1)
> Review of attachment 202137 [details]:
> 
> 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.

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