Thanks for the patch, this looks useful indeed :)

I think having something less heavy than a GObject for this would be beneficial, also it should be renamed to the GStreamer namespace and follow the general coding style (e.g. isWritable -> is_writable)

Would probably also make sense to just put it into gst-plugins-bad/gst-libs/gst/uridownloader until all this (dash, hls and smoothstreaming) is merged into a single plugin.

Created attachment 270481 [details] [review]
Add IETF RFC 3986 url joiner to uridownloader and use in dashdemux

Sebastian,

Thanks for the comments.

I've moved the GUrl class to uridownloader and changed it into a GstMiniObject called GstMiniUrl. ...makeWritable, ...isWritable and ...asString renamed to lower case with underscores to match the coding standard. I think that addresses everything.

Here's the new patch.

Review of attachment 270481 [details] [review]:

Thanks for the changes, I was more thinking of a simple C struct without refcounting. All the GObject/GstMiniObject code around that seems more lines of code than the actual logic :)

I figured that it may be useful to hold URLs in other structures/elements such as the MPD structure. This would mean that the URL is only parsed/split once and then combined multiple times with other URLs at later times. If this is the case then having ref counts for URL objects that may be passed around seemed like a sensible idea.

Review of attachment 270481 [details] [review]:

It's really annoying to see this being implemented again, see SoupURI, which is just a little more complete. I also agree that having an object is a bit overkill for such a small structure. For reference, see: https://git.gnome.org/browse/libsoup/tree/libsoup/soup-uri.c

::: ext/dash/gstdashdemux.c
@@ +1673,3 @@
+  gst_mini_url_unref(hdr_url);
+  gst_mini_url_unref(path_url);
+  gst_mini_url_unref(base_url);

An API like soup_uri_new_with_base() would be more conveniant then having to create two URI, combine in a third one and then unref.

There was/is also some GURI or GURL branch for GLib, but don't think it ever landed unfortunately...

So yeah, either this should be simplified a lot or made into a GstUri object in a core or base library that implements generic URI handling like the GUri stuff or SoupUri. I think we need something like this in our stack.

I've been looking at putting the code into core, however I've noticed that there is already a gsturi.[ch] which holds the GstUriHandler class. Should I use gsturl.c and call my object GstUrl or should I rename gsturi.[ch] to gsturihandler.[ch] and put my new class in gsturi.[ch] as GstUri?

Just putting it into gsturi.[ch] should be fine.

Created attachment 274241 [details] [review]
Patch to add GstUri class to libgstreamer

This patch adds a GstUri mini object to libgstreamer and includes unit tests to check for correct working.

Created attachment 274242 [details] [review]
Use new GstUri objects in dashdemux to join URLs.

This patch uses the GstUri class, added to libgstreamer in gstreamer-add-gst-uri.patch, to join the URLs from the MPD in an IETF RFC 3986 complaint manner.

Requires the other patch to be present.

Sebastian,

Sorry for the delay in getting these new patches ready, I was given urgent work on another project for a couple of weeks. Hopefully these fulfil your requirements.

Review of attachment 274241 [details] [review]:

::: gst/gsturi.h
@@ +213,3 @@
+ */
+
+GstUri * gst_uri_new                       (const gchar * uri) G_GNUC_MALLOC;

I would prefer a component-based constructor here that automatically escapes the components... and then a gst_uri_from_string()

@@ +222,3 @@
+gboolean gst_uri_is_writable               (GstUri * uri);
+GstUri * gst_uri_make_writable             (GstUri * uri);
+gchar *  gst_uri_as_string                 (const GstUri * uri) G_GNUC_MALLOC;

gst_uri_to_string()

@@ +231,3 @@
+gchar *  gst_uri_escape_query              (const gchar * query);
+gchar *  gst_uri_escape_http_query_element (const gchar * element);
+gchar *  gst_uri_escape_fragment           (const gchar * fragment);

These escape functions should maybe be private

But additionally I think per-component setters would be useful (that automatically escape), and a gst_uri_get_parent() that walks the path components and a gst_uri_append_path() and things like that to make the path handling a bit simpler

https://developer.gnome.org/libsoup/2.44/SoupURI.html is an API from which we can take some inspiration :)

For more inspiration also see bug #489862

Thanks for the feedback, I'm working on it, just taking a little longer than I hoped due to other interruptions.

Created attachment 282251 [details] [review]
Patch to add GstUri class to libgstreamer (ver.2)

Revision of GstUri mini-object class to provides methods closer to Soup API. I think this addresses previous comments. (needs ver.2 of the gst-plugins-bad patch too)

Created attachment 282252 [details] [review]
Use new GstUri objects in dashdemux to join URLs (ver.2)

Review of attachment 282251 [details] [review]:

Looks very good in general, just some minor comments

::: gst/gsturi.c
@@ +909,3 @@
+ * using the algorithm described in RFC3986.
+ *
+ * Last reviewed on never ()

Remove that part :)

@@ +929,3 @@
+ * fragment NULL.
+ *
+ * Returns: A new GstUri object. [transfer full]

No need to add gtk-doc documentation for private functions

@@ +1458,3 @@
+ * escaped except where indicated.
+ *
+ * Returns: (transfer full): A new #GstUri object.

Add Since: 1.6 at the end of the gtk-doc comments of every public function

@@ +1778,3 @@
+{
+  const gchar *r_scheme;
+  GstUri *t;

Add guards to *all* public functions, e.g. here

g_return_val_if_fail (base_uri != NULL, NULL);
g_return_val_if_fail (ref_uri != NULL, NULL);

@@ +2433,3 @@
+ * gst_uri_set_query_table:
+ * @uri: (transfer none)(nullable): The #GstUri to modify.
+ * @query_table: (transfer none)(nullable): The new query table to use.

Not sure if a GHashTable is ideal for this. Are the GI annotations to tell about the key and value types?

Maybe we need a GstUriQueryElement struct, and then here use a GList of GPtrArray of them?

@@ +2560,3 @@
+ * Get a list of the query keys from the URI.
+ *
+ * Returns: (transfer full)(element-type gchar*): A list of keys from

This is "transfer container" as the gchar* don't have to be freed

::: gst/gsturi.h
@@ +173,3 @@
+ */
+typedef struct _GstUri GstUri;
+struct _GstUri

Make this struct private and put it into the .c file. If any direct accesses are needed, add getters (and setters?) for them

Comment on attachment 282252 [details] [review]
Use new GstUri objects in dashdemux to join URLs (ver.2)

Looks good to me :)

Created attachment 286820 [details] [review]
Patch to add GstUri class to libgstreamer (ver.3)

Required for patch 282252

Created attachment 286821 [details] [review]
Patch to add GstUri class to libgstreamer (ver.4)

Missed a few "Since: 1.6" when a few methods were moved from gsturi.h file.

Required for patch 282252.

(In reply to comment #19)
> Review of attachment 282251 [details] [review]:
> 
> Looks very good in general, just some minor comments
> 
> ::: gst/gsturi.c
> @@ +909,3 @@
> + * using the algorithm described in RFC3986.
> + *
> + * Last reviewed on never ()
> 
> Remove that part :)
> 
Done.

> @@ +929,3 @@
> + * fragment NULL.
> + *
> + * Returns: A new GstUri object. [transfer full]
> 
> No need to add gtk-doc documentation for private functions
> 
Removed those although did leave in an explanation of one function as a simple comment.

> @@ +1458,3 @@
> + * escaped except where indicated.
> + *
> + * Returns: (transfer full): A new #GstUri object.
> 
> Add Since: 1.6 at the end of the gtk-doc comments of every public function
> 
Done.

> @@ +1778,3 @@
> +{
> +  const gchar *r_scheme;
> +  GstUri *t;
> 
> Add guards to *all* public functions, e.g. here
> 
> g_return_val_if_fail (base_uri != NULL, NULL);
> g_return_val_if_fail (ref_uri != NULL, NULL);
> 
Done.

> @@ +2433,3 @@
> + * gst_uri_set_query_table:
> + * @uri: (transfer none)(nullable): The #GstUri to modify.
> + * @query_table: (transfer none)(nullable): The new query table to use.
> 
> Not sure if a GHashTable is ideal for this. Are the GI annotations to tell
> about the key and value types?
> 
Yes, those detail the key and value types.

> Maybe we need a GstUriQueryElement struct, and then here use a GList of
> GPtrArray of them?
> 
I've left this as a GHashTable for now as I didn't want to reinvent a key/value pair type. I thought the hash table would be the best fit here as most of the time you'd want to lookup a query value string by it's key name, or insert key/value pairs. I couldn't see a situation where this structure wouldn't fit, so it seemed best. It maybe worth revisiting later.

> @@ +2560,3 @@
> + * Get a list of the query keys from the URI.
> + *
> + * Returns: (transfer full)(element-type gchar*): A list of keys from
> 
> This is "transfer container" as the gchar* don't have to be freed
> 
Done.

> ::: gst/gsturi.h
> @@ +173,3 @@
> + */
> +typedef struct _GstUri GstUri;
> +struct _GstUri
> 
> Make this struct private and put it into the .c file. If any direct accesses
> are needed, add getters (and setters?) for them

Moved this to the .c file along with a few inline functions which no longer have access directly to the internal parts of the structure.

Thanks for the patch update :)

(In reply to comment #23)

> > @@ +929,3 @@
> > + * fragment NULL.
> > + *
> > + * Returns: A new GstUri object. [transfer full]
> > 
> > No need to add gtk-doc documentation for private functions
> > 
> Removed those although did leave in an explanation of one function as a simple
> comment.

That's ok (and good :) )

> > @@ +2433,3 @@
> > + * gst_uri_set_query_table:
> > + * @uri: (transfer none)(nullable): The #GstUri to modify.
> > + * @query_table: (transfer none)(nullable): The new query table to use.
> > 
> > Not sure if a GHashTable is ideal for this. Are the GI annotations to tell
> > about the key and value types?
> > 
> Yes, those detail the key and value types.

Ah yes, there's apparently "(element-type KTYPE VTYPE)". But you didn't add those. Can you add them? Otherwise I'll add that when merging :)

> > Maybe we need a GstUriQueryElement struct, and then here use a GList of
> > GPtrArray of them?
> > 
> I've left this as a GHashTable for now as I didn't want to reinvent a key/value
> pair type. I thought the hash table would be the best fit here as most of the
> time you'd want to lookup a query value string by it's key name, or insert
> key/value pairs. I couldn't see a situation where this structure wouldn't fit,
> so it seemed best. It maybe worth revisiting later.

Conceptionally it makes sense, yes. I was just worried about the bindings friendliness here :)

Created attachment 287328 [details] [review]
Patch to add GstUri class to libgstreamer (ver.5)

Fixed missing element-type annotation.

Thanks, merged :)

commit b9bf5d5ff22446cf00e0a9916772887095662543
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Sep 29 12:19:35 2014 +0300

    uri: Fix memory leak in gst_uri_join()
    
    The merged path segments are a deep-copied list and we need to free the
    contained strings too instead of just the list nodes themselves.

commit 54c5f1855cfe87189ef2c32d74ffa9b3c40ed66d
Author: David Waring <david.waring@rd.bbc.co.uk>
Date:   Thu Jul 31 22:18:53 2014 +0100

    GstUri: Add GstUri miniobject to handle URIs in an RFC 3986 compliant fashion
    
    https://bugzilla.gnome.org/show_bug.cgi?id=725221

I am looking at GUri branch proposed in bug 489862 compared to GstUri:

I wonder about usefulness of {get,set}_path_segments() and append_path*(), as well as all the query functions. Imho, g_uri_parse_params() was more flexible because it can be configured, and then you just manipulate the hashtable as usual.

(apparently, gst_uri_set_path() == gst_uri_set_path_string(), but I guess this is for consistency reasons

What's the timeline for GUri? Maybe it makes sense to make GstUri a renamed copy of GUri for consistency, and then at a later point once we depend on new enough glib just make it a wrapper around GUri.