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

[bugzilla at gnome.org]

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

--- Comment #2 from Sam Hurst <samuelh at rd.bbc.co.uk> ---
Hi Thiago,

Thanks very much for the comments, I've added some replies below:

> Why do you need this [GST_CURL_HTTP_VER] env var?

We wanted an easy way to force certain HTTP versions during testing, and I
couldn't see a nice easy way of doing this via playbin. Also, I don't think any
other HTTP uri handler actually allows you to set the HTTP Version number from
another element.

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

I'll be sure to move it.

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

Got it.

> +    *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

As far as I can tell, this should be done after the buffer has been pushed, so
I'm going to take that to mean I should be putting it at the beginning of the
_create() function as the GstPushSrc takes care of pushing the buffer upstream
for me?

> +      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?

A colleague of mine wanted me to set the caps to tell a downstream element what
content it's getting for ease of use in his own module.

> +  GMutex running;
> +  GstCurlHttpSrcQueueElement *next;
> +};
>
> Did you consider using the Glib helpers like GQueue to avoid implementing a queue yourself?

I did, and an initial attempt at a port to GSList ended in things breaking.
Maybe in the future I can port it over but as I can't devote an awful lot of
time to this at present I've elected to leave it as-is for now. 

As for your hang, that's not something I've ever seen before, and I have tested
requesting HTTP/2 upgrades to HTTP/1.1 servers. The transfer should time out
(assuming the timeout property isn't set as 0 which means it'll never time out)
and if it doesn't that might be a libcurl bug. Were you attempting to connect
via SSL or cleartext? Also, what versions of curl/nghttp are you running? I've
been pretty much tracking git latest during development and it may be something
that's been fixed.

Thanks again for the feedback, I'll take all your suggestions on board and I'll
put an amended patch up soon.

Cheers,
Sam

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