GNOME Bugzilla – Bug 785878
curlhttpsrc: critical warning in class_init
Last modified: 2017-08-08 08:08:33 UTC
(gst-plugin-scanner:4776): GLib-GObject-CRITICAL **: g_param_spec_float: assertion 'default_value >= minimum && default_value <= maximum' failed (gst-plugin-scanner:4776): GLib-GObject-CRITICAL **: g_object_class_install_property: assertion 'G_IS_PARAM_SPEC (pspec)' failed The http-version property here is initialized with default_value=2.0, min=1.0, max=1.1. Attaching a trivial patch fixing this :)
Created attachment 357051 [details] [review] proposed patch
Comment on attachment 357051 [details] [review] proposed patch Ideally this should just be an enum, not a float. A float makes no sense for this :) Feel free to push your fix, or to fix it properly instead
Hum doesn't seem so easy to have an enum there because there's both a build-time check and a runtime test. In the mean time I'll push this patch if that's OK :)
Why difficult? Just make the HTTP 2 part compile-time in the enum with an #ifdef, and for runtime fall-back to HTTP 1.1 if 2 is selected but not supported :) But sure, push as-is to make things work better at least. But let's keep this bug open until it's solved properly.
Created attachment 357092 [details] [review] updated patch
Adding Sam. Can you test this on your side please?
Review of attachment 357092 [details] [review]: Looks mostly good to me, thanks :) ::: ext/curl/gstcurlhttpsrc.c @@ +204,3 @@ http_env = g_getenv ("GST_CURL_HTTP_VER"); if (http_env != NULL) { + gfloat http_version = (gfloat) g_ascii_strtod (http_env, NULL); Please don't use floats here, just take the string and use strcmp() for the 3 possibilities :)
Created attachment 357108 [details] [review] updated patch
Just tested this on my side with different versions of libcurl as well as against http/1.1 and h2/h2c servers and Philippe's patch works in all cases. > Just make the HTTP 2 part compile-time in the enum with an #ifdef, and > for runtime fall-back to HTTP 1.1 if 2 is selected but not supported :) If libcurl doesn't support the HTTP version specified, it will fall back to HTTP/1.1 as a default. However, I note that because the libcurl features check has been removed, we get a slightly clunky WARN message in ::_create() when libcurl is >=7.33.0 but not built against nghttp2. Might be worth a check to see if setting CURL_HTTP_VERSION_2_0 failed due to lack of HTTP/2 support or some bigger underlying libcurl failure (it is possible to build curl without http support...), e.g. @@ -1008,8 +1003,15 @@ gst_curl_http_src_create_easy_handle (GstCurlHttpSrc * s) #ifdef CURL_VERSION_HTTP2 case GSTCURL_HTTP_VERSION_2_0: GST_DEBUG_OBJECT (s, "Setting version as HTTP/2.0"); - gst_curl_setopt_int (s, handle, CURLOPT_HTTP_VERSION, - CURL_HTTP_VERSION_2_0); + if (curl_easy_setopt (handle, CURLOPT_HTTP_VERSION, + CURL_HTTP_VERSION_2_0) != CURLE_OK) { + if (gst_curl_http_src_curl_capabilities->features & CURL_VERSION_HTTP2) { + GST_WARNING_OBJECT (s, + "Cannot set unsupported option CURLOPT_HTTP_VERSION"); + } else { + GST_INFO_OBJECT (s, "HTTP/2 unsupported by libcurl at this time"); + } + } break; #endif default:
Created attachment 357122 [details] [review] updated patch with extra http/2 error checking An updated patch incorporating the change I proposed above
Can this go in a patch on top of my original change maybe? Hijacking the authorship of the original patch isn't very nice, imho anyway :)
Oops, sorry! For some reason I thought that simply amending it wouldn't change the author. I'll split it out as a separate patch now.
Created attachment 357123 [details] [review] Add extra http/2 error checking
Thanks Sam!
Review of attachment 357108 [details] [review]: Looks good to me
Review of attachment 357123 [details] [review]: Also looks good
commit 6baa66a889624abc15813719d19ec39c68958df7 (HEAD -> master, origin/master, origin/HEAD) Author: Sam Hurst <Sam.Hurst@bbc.co.uk> Date: Mon Aug 7 16:41:27 2017 +0100 curlhttpsrc: Does version set fail because of HTTP2 Check to see if setting CURL_HTTP_VERSION_2_0 failed due to lack of HTTP/2 support or some bigger underlying libcurl failure https://bugzilla.gnome.org/show_bug.cgi?id=785878 commit 5bf092bd64684e4b1f1446c25b07b6771ce97ba6 Author: Philippe Normand <philn@igalia.com> Date: Mon Aug 7 10:25:17 2017 +0100 curlhttpsrc: set http-version class property as enum This matches better with the preferred_http_version which was already declared as enum. https://bugzilla.gnome.org/show_bug.cgi?id=785878