[Bug 744261] Integrate curlhttpsrc into gst-plugins-bad

[bugzilla at gnome.org]

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

Thiago Sousa Santos <thiagossantos at gmail.com> changed:

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

--- Comment #1 from Thiago Sousa Santos <thiagossantos at gmail.com> ---
Review of attachment 296494:

Thanks for the patch!

The code looks good overall but there are some things to fix.

Also on my tests if I try to use http 2.0 in a server that doesn't support it
seems to hang rather than error out.
Some other comments and questions below.

::: ext/curl/Makefile.am
@@ +14,3 @@
             gstcurlftpsink.c \
             $(gstcurlsshsink_SOURCES) \
+            gstcurlsmtpsink.c \

missing the gstcurlqueue.c here

::: ext/curl/gstcurlhttpsrc.c
@@ +167,3 @@
+
+  gst_curl_http_src_curl_capabilities = curl_version_info (CURLVERSION_NOW);
+  http_env = g_getenv ("GST_CURL_HTTP_VER");

Why do you need this env var?

@@ +602,3 @@
+   * Check that the CURL worker thread is running. If it isn't, start it.
+   */
+  gst_curl_http_src_ref_multi (source);

It would make more sense to do this in the NULL -> READY transition. In NULL
state the element should have resources allocated.

@@ +678,3 @@
+
+  GST_INFO_OBJECT (src, "Closing instance, worker thread refcount is now %u",
+      --klass->multi_task_context.refcount);

Don't do operations inside of GST_DEBUG and friends macros. They can be
disabled at compile time and your decrement would never happen.

@@ +702,3 @@
+
+  /* Unref the curl_multi task. If nothing else holds a reference, stop it. */
+  gst_curl_http_src_unref_multi (src);

And do this in the READY -> NULL state change

@@ +1020,3 @@
+    *buf = gst_buffer_new_allocate (NULL, src->len, NULL);
+    gst_buffer_map (*buf, &info, GST_MAP_READWRITE);
+    memcpy (info.data, src->msg, (size_t) src->len);

Remember to unmap the buffer

@@ +1041,3 @@
+      src->caps = gst_caps_make_writable (src->caps);
+      gst_caps_set_simple (src->caps, "content-type", G_TYPE_STRING,
+          src->headers.content_type, NULL);

What is the use case for this?

::: ext/curl/gstcurlqueue.h
@@ +54,3 @@
+  GMutex running;
+  GstCurlHttpSrcQueueElement *next;
+};

Did you consider using the Glib helpers like GQueue to avoid implementing a
queue yourself?

https://developer.gnome.org/glib/stable/glib-Double-ended-Queues.html

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/gstreamer-bugs/attachments/20150211/a1d952be/attachment-0001.html>