[Bug 792131] rtspsrc: add an action signal to send SET_PARAMETER

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=792131

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

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

--- Comment #8 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 372950:
 --> (https://bugzilla.gnome.org/review?bug=792131&attachment=372950)

Looks generally good, thanks!

::: gst/rtsp/gstrtspsrc.c
@@ +1143,3 @@
+  }
+
+  parameters = g_malloc0 (2 * sizeof (gchar *));

This can be stack allocated :)

@@ +1175,3 @@
+  req->body = g_string_new (NULL);
+  while (*parameters) {
+    g_string_append_printf (req->body, "%s:\r\n", *parameters);

Should we do some sanity-checks for parameters here? They should probably not
contain \n or \r at least, or stuff will fail in interesting ways

@@ +1227,3 @@
+  if (gst_rtspsrc_loop_send_cmd (src, CMD_WAIT, CMD_LOOP)) {
+    GST_RTSP_STREAM_LOCK (src);
+    GST_RTSP_STREAM_UNLOCK (src);

Why do we block here (and above)? The response comes async anyway

@@ +1294,3 @@

+  src->set_param_q = g_queue_new ();
+  src->get_param_q = g_queue_new ();

If you use a GQueue, just store it directly inside the instance struct and use
g_queue_init() here

@@ +1311,3 @@
+  ParameterRequest *req = data;
+
+  gst_promise_unref (req->promise);

Should probably get gst_promise_expire() here if we're freeing and will never
ever reply to it again

@@ +5796,3 @@
+      break;
+    case CMD_SET_PARAMETER:
+      GST_ELEMENT_PROGRESS (src, ERROR, "request", ("SET_PARAMETER failed"));

And these should probably get some kind of error response on the promise?

@@ +8997,3 @@
+
+  res = gst_rtsp_message_add_header (&request, GST_RTSP_HDR_CONTENT_TYPE,
+      req->content_type == NULL ? "text/parameters" : req->content_type->str);

Why use a GString for the content_type btw? Could be a plain gchar* or not?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.