[Bug 711084] rtpmanager: add new rtprtxsend and rtprtxreceive elements for retransmission

[GStreamer (bugzilla.gnome.org)]

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

Olivier Crete (Tester) <olivier.crete> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #264077|none                        |needs-work
             status|                            |

--- Comment #66 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-12-13 04:59:37 UTC ---
Review of attachment 264077:
 --> (https://bugzilla.gnome.org/review?bug=711084&attachment=264077)

Some comments, nothing major

::: gst/rtpmanager/gstrtprtxreceive.c
@@ +231,3 @@
+  GstRtpRtxReceive *rtx = GST_RTP_RTX_RECEIVE (object);
+
+  gst_rtp_rtx_receive_reset (rtx);

This just empties tables that you'll destroy just below, not necessary.

@@ +233,3 @@
+  gst_rtp_rtx_receive_reset (rtx);
+
+  if (rtx->ssrc2_ssrc1_map) {

No need for these checks, finalize() is never called once and only once.

@@ +506,3 @@
+      GUINT_TO_POINTER (payload_type), NULL, NULL);
+
+  g_mutex_unlock (&rtx->lock);

Why do you unlock, then access the rtx_pt_map hash table, then relock ?

@@ +527,3 @@
+    if (g_hash_table_lookup_extended (rtx->ssrc2_ssrc1_map,
+            GUINT_TO_POINTER (ssrc), NULL, &ssrc1)) {
+      GST_DEBUG

use GST_DEBUG_OBJECT (not the only place)

@@ +657,3 @@
+        gst_structure_free (rtx->pending_rtx_pt_map);
+      rtx->pending_rtx_pt_map = g_value_dup_boxed (value);
+      rtx->rtx_pt_map_changed = TRUE;

Like in rtxrecv, why the whole rtx_pt_map_changed dance? You hold the same
clock, you can change it here directly.

::: gst/rtpmanager/gstrtprtxreceive.h
@@ +46,3 @@
+  GstPad *srcpad;
+
+  GMutex lock;

Why the new lock? why not the object lock ?

::: gst/rtpmanager/gstrtprtxsend.c
@@ +199,3 @@
+
+static void
+gst_rtp_rtx_send_reset (GstRtpRtxSend * rtx, gboolean full)

Unused _full() parameter.

That said, it should probably do the reset on FLUSH also.

@@ +455,3 @@
+      s = gst_caps_get_structure (caps, 0);
+      gst_structure_get_uint (s, "ssrc", &ssrc);
+      data = gst_rtp_rtx_send_get_ssrc_data (rtx, ssrc);

Here you access ssrc_data without first taking the lock

@@ +456,3 @@
+      gst_structure_get_uint (s, "ssrc", &ssrc);
+      data = gst_rtp_rtx_send_get_ssrc_data (rtx, ssrc);
+      gst_structure_get_int (s, "clock-rate", &data->clock_rate);

Shouldn't the queue be flushed if the caps change ?

@@ +536,3 @@
+  /* get needed data from GstRtpRtxSend */
+  ssrc = gst_rtp_buffer_get_ssrc (&rtp);
+  data = gst_rtp_rtx_send_get_ssrc_data (rtx, ssrc);

Accessing the ssrc_data without the lock held. You probably need to create the
new packets in _chain() before releasing the lock there.

@@ +551,3 @@
+  /* If payload type is not set through SDP/property then
+   * just bump the value */
+  if (fmtp < 96)

I know <96 isn't value, but we should still allow any value between 0 and 127.

@@ +636,3 @@
+
+    /* add current rtp buffer to queue history */
+    item = g_new0 (BufferQueueItem, 1);

g_slice_new0(), helps reduce fragmentation

@@ +738,3 @@
+        gst_structure_free (rtx->pending_rtx_pt_map);
+      rtx->pending_rtx_pt_map = g_value_dup_boxed (value);
+      rtx->rtx_pt_map_changed = TRUE;

Why do the rtx_pt_map_changed dance if you hold the same lock here?

::: gst/rtpmanager/gstrtprtxsend.h
@@ +46,3 @@
+  GstPad *srcpad;
+
+  GMutex lock;

Why the new lock? Why not just use the GST_OBJECT_LOCK() ?

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