GNOME Bugzilla – Bug 751371
souphttpsrc: add cookie jar
Last modified: 2018-11-03 15:01:11 UTC
In case of adaptive streaming, there is a additional http src element in the adaptive demux to download content fragments. In some cases, user-agent and cookie is needed to set to connect the http server but there is no way to get the current user-agent and cookie data from souphttpsrc. I think souphttpsrc should handle the custom query of user-agent and cookies.
Created attachment 305909 [details] [review] souphttpsrc: add handling query of user-agent and cookie I added custom query handling of user-agent and cookie and I added cookie jar to keep the cookie for each url. - related bug item of adaptive streaming https://bugzilla.gnome.org/show_bug.cgi?id=751372
In a quick read it looks OK. Will test in a little while.
Commit edc7d9027ec83bb0a028d807644ddd3e896086a9 in master pushes the http-headers as an event downstream. I guess this is enough for having adaptive demuxer getting that information. What still seems to be missing for upstream is the cookiejar support to have cookies persisted when souphttpsrc is reused.
Also see bug #640159 for cookiejar support.
Review of attachment 305909 [details] [review]: Following what Thiago said.
Created attachment 307389 [details] [review] souphttpsrc: add cookie jar I modified patch to remove custom query things and I added cookie jar only. When I checked bug #640159, it is for getting cookies from cookie jar file via property setting of the file path. so I think I can add the code to use session cookie jar in souphttpsrc internally. Thank you.
Created attachment 307390 [details] [review] souphttpsrc: add cookie jar I modified one bug and attached it again.
Review of attachment 307389 [details] [review]: ::: ext/soup/gstsouphttpsrc.c @@ +127,3 @@ }; +#define DEFAULT_USER_AGENT "Mozilla/5.0 (Linux; Tizen 2.3; SAMSUNG SM-Z130H) AppleWebKit/537.3 (KHTML, like Gecko) Version/2.3 Mobile Safari/537.3" You can of course override this in your app, but we are not going to pretend that GStreamer is Mozilla/Tizen. This also surpass the 80 character coding style limit. @@ +479,3 @@ } + src->cookie_jar = NULL; Everything is already set to 0 in a GObject structure. @@ +487,2 @@ static void +gst_soup_http_src_update_cookie (GstSoupHTTPSrc * src) In my opinion, you should improve readability of this function, try to avoid checking for too many condition (which do affectation, array =) if that same if. Here's an example: if (!src->sesion) return; if (!src->cookie_jar) { ... } if (!src->cookie_jar) return; @@ +525,3 @@ + + cookies = NULL; + if ((src->cookie_jar) && Just deal with no cookie_jar first, this will make things more readable. @@ +530,3 @@ + array = cookies; + for (c = cookie_list; c; c = c->next) { + *array++ = soup_cookie_to_set_cookie_header ((SoupCookie *) (c->data)); This is pretty ugly, I'd prefer if we avoid this kind of constructions. @@ +740,3 @@ break; case PROP_COOKIES: + g_value_set_boxed (value, gst_soup_http_src_get_cookie (src)); This leaks the cookies array. Use g_value_take_boxed(). It looks like this is ancient bug though. @@ +1706,3 @@ + + SoupURI *uri = NULL; + SoupCookie *cookie; You could declare this inside the scope of the for loop using it.
Review of attachment 307390 [details] [review]: I reviewed the wrong one, sorry, but most applies, ignore the comment about header, it seems you removed it.
Created attachment 307526 [details] [review] souphttpsrc: add cookie jar (3) I uploaded new patch according to the review comment. Thank you.
Created attachment 307873 [details] test application to show cookies being preserved with cookiejar support This test application receives a list of urls and downloads them using souphttpsrc, then prints the cookies property content. Build with: libtool --mode=link gcc souphttpsrc-cookies.c -o souphttpsrc-cookies `pkg-config --cflags --libs gstreamer` Without the cookiejar support the cookies list will always be empty as the user hasn't set any, when the cookiejar support is enabled it will increase over time as it gathers more cookies. The proposed patch atm will always add the new cookies set to the cookiejar and apparently provides no way of removing cookies. So, before reviewing this further let's decide on what to support regarding cookies. I can think of no drawbacks of haiving cookiejar enabled but what to do with the current cookies property? For example, suppose the cookie jar contains cookies A, B and the user sets a list with a single entry C. Should A and B be removed or just add C?
Thank you for your test application and comments. I have not seen any application which set the cookie property explicitly. If user set the cookie property, it means the cookie from server should be ignored? I think client can send all the cookies to server and server should recognize it. Do you think we need to handle them(from property / from response header) separately or with priority? If you know any special cases, please let me know. Thank you.
*** Bug 640159 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/194.