[Bug 711271] soup: Add a SoupServer sink

[GStreamer (bugzilla.gnome.org)]

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

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

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

--- Comment #4 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-12-06 02:15:38 UTC ---
Review of attachment 258790:
 --> (https://bugzilla.gnome.org/review?bug=711271&attachment=258790)

Thanks for you contribution, there are a couple things that need improving
before we can merge it. The biggest is the use of the default GMainContext.

::: ext/soup/gstsouphttpserversink.c
@@ +66,3 @@
+{
+  PROP_0,
+  PROP_PATH,

The matching g_object_class_install_property() is missing

@@ +122,3 @@
+gst_soup_http_server_sink_init (GstSoupHttpServerSink * souphttpsink)
+{
+  g_mutex_init (&souphttpsink->mutex);

Why not use the GST_OBJECT_LOCK() ?

@@ +199,3 @@
+  soup_server_add_handler (souphttpsink->server, souphttpsink->path,
+      gst_soup_http_server_new_message, souphttpsink, NULL);
+  soup_server_run_async (souphttpsink->server);

This will make libsoup use the default GMainContext/GMainLoop, sadly GStreamer
must work without one running. So you need to create a GMainContext and then in
a thread run your own GMainLoop there. Look at how souphttpclientsink does it.

@@ +246,3 @@
+  gpointer key, value;
+
+  if (!gst_buffer_map (buffer, &info, GST_MAP_READ)) {

You may want to iterate over each GstMemory inside the buffer to avoid doing
copies if there are multiple non-contiguous memories.

@@ +261,3 @@
+    SoupMessage *msg = SOUP_MESSAGE (key);
+    soup_message_body_append (msg->response_body, SOUP_MEMORY_COPY, info.data,
+        info.size);

It would be nice to use soup_buffer_new_with_owner() to avoid making a copy.

@@ +280,3 @@
+
+  soup_message_headers_set_content_type (msg->response_headers, "text/plain",
+      NULL);

Why text/plain? Is it possible to just not put the header? Worst cast,
application/octet-stream unless the caps are really plaintext.

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