GNOME Bugzilla – Bug 631673
add "redirect" message to core
Last modified: 2016-07-25 10:00:21 UTC
We now have already several plugins defining the redirect message. $ find gst* -name "*.c" -exec grep -Hn "gst_structure_new (\"redirect\"" {} \; gst-plugins-bad/ext/libmms/gstmms.c:449: gst_structure_new ("redirect", "new-location", G_TYPE_STRING, url, gst-plugins-bad/gst/sdp/gstsdpdemux.c:1372: gst_structure_new ("redirect", gst-plugins-good/gst/qtdemux/qtdemux.c:2754: gst_structure_new ("redirect", gst-plugins-good/gst/qtdemux/qtdemux.c:7067: ref->structure = gst_structure_new ("redirect", gst-plugins-good/gst/qtdemux/qtdemux.c:7285: gst_structure_new ("redirect", gst-plugins-ugly/gst/realmedia/pnmsrc.c:201: gst_structure_new ("redirect", "new-location", G_TYPE_STRING, url, NULL)); The message looks like this: gst_message_new_element (element, gst_structure_new ("redirect", "new-location", G_TYPE_STRING, url, NULL))); Would be good to add this to core at some point. Then we can properly document it. Tools like gst-launch could at least catch it and print an error (or try to handle it by iterating uri-handlers and try to set it).
I agree that we should make this an official message. It's not always *just* a new-location though, there may also be a "locations" field with a list of structures containing new-location + maximum-bitrate fields (see qtdemux_process_redirects).
(In reply to comment #1) > I agree that we should make this an official message. > > It's not always *just* a new-location though, there may also be a "locations" > field with a list of structures containing new-location + maximum-bitrate > fields (see qtdemux_process_redirects). Imo we should at least try and handle the case where a single URI is proposed. It is clear that in that case we want a simple redirection.
Created attachment 327706 [details] [review] Patch to add new redirect message Here is a patch that adds a redirect message. The message can contain one or more entries. Each entry can have a location and a structure, which can be parsed independently. This way, we can add more complex metadata without making it cumbersome to pull just the first location string for example.
Review of attachment 327706 [details] [review]: Thanks for the patch, it looks good to me. I am a bit uncertain about the free-form structures. Can we use e.g. a GstTagList there, then e.g. we get defined semantics for BITRATE. Could you take a look at the existing uses and list what extra data they would need to put into the structure?
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #4) > Review of attachment 327706 [details] [review] [review]: > > Thanks for the patch, it looks good to me. I am a bit uncertain about the > free-form structures. Can we use e.g. a GstTagList there, then e.g. we get > defined semantics for BITRATE. Could you take a look at the existing uses > and list what extra data they would need to put into the structure? Having free form structures is useful if for example the sources are some custom music services that provide extra metadata that can be used by the application to decide which location to pick.
Review of attachment 327706 [details] [review]: free-form structures make sense imho but there should be some API for the common cases ::: gst/gstmessage.c @@ +2543,3 @@ + */ +GstMessage * +gst_message_new_redirect (GstObject * src) Maybe parameters for the normal case of just having a single redirect URL, and other new functions for other common cases? @@ +2572,3 @@ +void +gst_message_add_redirect_entry (GstMessage * message, const gchar * location, + const GstStructure * entry_struct) Common fields and their types should at least be documented, even better some API to set/get common fields (e.g. bitrate). ::: gst/gstquark.h @@ +208,3 @@ + GST_QUARK_MESSAGE_REDIRECT = 177, + GST_QUARK_ENTRY_LOCATIONS = 178, + GST_QUARK_ENTRY_STRUCTURES = 179, Maybe these should contain the word "redirect"
I was actually considering to make the location string a structure field. I didn't because the location string is fundamental for this message. But if we add more functions for common cases, we might as well do the struct idea and turn these common cases into convenience functions for directly setting/getting these common values. Or, these common values live in a separate, internal structure.
Come to think of it, why not just add a tag list field to the redirect message? This pretty much covers the need for additional-but-standardized information such as bitrates.
(In reply to Carlos Rafael Giani from comment #8) > Come to think of it, why not just add a tag list field to the redirect > message? This pretty much covers the need for additional-but-standardized > information such as bitrates. That is what I proposed in comment #4 :)
Oh, I worded it improperly :) I meant in addition to the location string and the extra structure. You proposed a tag list instead of the structure.
I think having both is a good idea. There might be additional custom information you want to provide that does not have (and does not need) a registered tag. Maybe something that even only makes sense for your specific application.
Created attachment 329048 [details] [review] Patch to add new redirect message, v2 Updated version with additional tag list support.
Review of attachment 329048 [details] [review]: Looks good to me. How about others? ::: tests/check/gst/gstmessage.c @@ +379,3 @@ } + /* GST_MESSAGE_REDIRECT */ + { We should really be splitting this into individual tests. But not in this commit.
Review of attachment 329048 [details] [review]: Generally looks good to me but ::: gst/gstmessage.c @@ +2529,3 @@ + * gst_message_new_redirect: + * @src: The #GstObject whose property changed (may or may not be a #GstElement) + * @location: (transfer none) (allow-none) location string for the new entry I don't think this should be allow-none really :) What's the point of a redirect without a location? @@ +2542,3 @@ + * when the redirect message is created, with the given location, tag_list, + * entry_struct arguments. Use gst_message_add_redirect_entry() to add more + * entries. Maybe should specify that, given no other information to decide, they should be preferred in the order they are added? Or the same preference? @@ +2598,3 @@ + * gst_message_add_redirect_entry: + * @message: a #GstMessage of type %GST_MESSAGE_REDIRECT + * @location: (transfer none) (allow-none) location string for the new entry And here. Should IMHO be a g_return_val_if_fail() in both cases too.
The original thought was that redirections might not necessarily happen based on URLs. But I guess this is not how redirections happen in 99,9% of the time. I can make them mandatory, yeah.
Also, > Maybe should specify that, given no other information to decide, they should be preferred in the order they are added? Or the same preference? This would mean that the element that posted the message added the entries in a meaningful order, and the receiver has to follow that order. I was seeing it from the opposite perspective - the receivers decide by themselves which entry to pick, without following any instructions. This could be combined: by default, receivers should prefer the entries in the order they are present (so, entry #0 has the highest preference, entry #1 the second highest etc.), but if they rather choose on their own, they are free to do so.
Then this should be mentioned in the docs probably, that the entries are in no particular order :)
One more thought - I am not 100% happy with both structure and taglist. A taglist is just a structure without nesting. So maybe we only use a GstStructure and drop the additional TagList. In the GstStructure we use e.g. GST_TAG_BITRATE for the key. The custom entries would be used as plain strings. Not sure if this really improves things though.
The fact that GstTagList is internally implemented using GstStructure is no longer 'public API' fwiw.
Created attachment 329288 [details] [review] Patch to add new redirect message, v3 Updated patch. I kept the tag list as it is, because, as Tim said, tag lists are not publicly guaranteed to be GstStructures. Furthermore, having them separate acts as a hard separation, similar to what can be achieved by namespacing. Mixing free-form and predefined fields OTOH could lead to confusing situations. I compare it to forms which have a bunch of predefined fields and an additional "further/miscellaneous comments" field.
I'd like to propose this patch for the 1.9.2 release.
Review of attachment 329288 [details] [review]: Should be added to the docs (docs/gst/*-sections.txt) and the win32 .def file (just run "make check-exports" and you see what I mean) ::: gst/gstmessage.c @@ +2565,3 @@ +GstMessage * +gst_message_new_redirect (GstObject * src, const gchar * location, + GstTagList * tag_list, const GstStructure * entry_struct) Why don't you create an empty list here and just call gst_message_add_redirect_entry()? Less code duplication :)
Created attachment 332092 [details] [review] 329288: Patch to add new redirect message, v4
The updated patch also solves conflicts that occurred because the stream messages got added to the repo in the meantime.
commit eead9cf827018288d9f883ac339331f4d30c2ce9 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Mon Jul 25 11:22:36 2016 +0200 message: Add redirect message Redirection messages are already used in fragmented sources and in uridecodebin, so it makes sense to introduce these as an official message type. https://bugzilla.gnome.org/show_bug.cgi?id=631673