<html>
<head>
<base href="https://bugzilla.gnome.org/" />
</head>
<body><span class="vcard"><a href="page.cgi?id=describeuser.html&login=thiagossantos%40gmail.com" title="Thiago Sousa Santos <thiagossantos@gmail.com>"> <span class="fn">Thiago Sousa Santos</span></a>
</span> changed
<a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - Integrate curlhttpsrc into gst-plugins-bad"
href="https://bugzilla.gnome.org/show_bug.cgi?id=744261">bug 744261</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #296494 status</td>
<td>none
</td>
<td>reviewed
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - Integrate curlhttpsrc into gst-plugins-bad"
href="https://bugzilla.gnome.org/show_bug.cgi?id=744261#c1">Comment # 1</a>
on <a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - Integrate curlhttpsrc into gst-plugins-bad"
href="https://bugzilla.gnome.org/show_bug.cgi?id=744261">bug 744261</a>
from <span class="vcard"><a href="page.cgi?id=describeuser.html&login=thiagossantos%40gmail.com" title="Thiago Sousa Santos <thiagossantos@gmail.com>"> <span class="fn">Thiago Sousa Santos</span></a>
</span></b>
<pre>Review of <span class=""><a href="attachment.cgi?id=296494&action=diff" name="attach_296494" title="curlhttpsrc patch">attachment 296494</a> <a href="attachment.cgi?id=296494&action=edit" title="curlhttpsrc patch">[details]</a></span> <a href='/review?bug=744261&attachment=296494'>[review]</a>:
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?
<a href="https://developer.gnome.org/glib/stable/glib-Double-ended-Queues.html">https://developer.gnome.org/glib/stable/glib-Double-ended-Queues.html</a></pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the QA Contact for the bug.</li>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>