GNOME Bugzilla – Bug 639055
discoverer: add support for subtitle streams
Last modified: 2011-08-26 08:08:58 UTC
Would be nice if the discoverer API also handled the subtitle streams like it does Audio and Video
bug #632291 should go along with this (but is not a clone).
Created attachment 194493 [details] [review] discoverer: add subtitles API
There's one niggle left, in that I can't seem to be able to find the language-code to fill out the language property, though it's there in the tags. Will look at it later, maybe.
Review of attachment 194493 [details] [review]: Looks good but with the language code problem you mean that you don't get any language at all for the subtitle streams, i.e. info->language is always NULL? ::: gst-libs/gst/pbutils/gstdiscoverer-types.c @@ +256,3 @@ +{ + if (info->language) + g_free (info->language); g_free() is NULL-safe ::: gst-libs/gst/pbutils/gstdiscoverer.c @@ +447,3 @@ "subpicture/x-pgs; subpicture/x-dvb; application/x-subtitle-unknown; " "application/x-ssa; application/x-ass; subtitle/x-kate; " + "application/x-kate; video/x-dvd-subpicture; "); Ideally you would make this a separate commit @@ +831,3 @@ g_str_has_prefix (name1, "image/")) && + g_str_has_prefix (name2, "video/x-raw")) || + (g_str_has_suffix (name1, "/x-kate"))) { Why?
> Looks good but with the language code problem you mean that you don't get any > language at all for the subtitle streams, i.e. info->language is always NULL? Correct. The tags appear to be on another related pad, as there is a chain created by decodebin, and the way it's all setup still eludes me fully. The tags are there *somewhere* though, including the language-code tag with the correct language, it's just an issue of working out just how to get to it. > g_free() is NULL-safe Yes, all the other cleanup code was testing for NULLness, including g_free calls. I went along with that too, but I don't mind either way. > ::: gst-libs/gst/pbutils/gstdiscoverer.c > @@ +447,3 @@ > "subpicture/x-pgs; subpicture/x-dvb; application/x-subtitle-unknown; " > "application/x-ssa; application/x-ass; subtitle/x-kate; " > + "application/x-kate; video/x-dvd-subpicture; "); > > Ideally you would make this a separate commit OK. > @@ +831,3 @@ > g_str_has_prefix (name1, "image/")) && > + g_str_has_prefix (name2, "video/x-raw")) || > + (g_str_has_suffix (name1, "/x-kate"))) { > > Why? Ah, that does warrant an explanation indeed. Originally, the Kate plugin output text via katedec, and overlaid the stream on video via tiger. It was requested that katedec, when getting images, be able to output DVD subpctures (so it'd work with things like dvdspu). So without considering Kate streams as final, decodebin will plug in a DVD SPU decoder, and consider that the stream is actually video (video/x-dvd-subpicture), which is true in a sense, but not what we want here. I can make that one a separate commit with that explanation as well, or include the explanation in the code and leave in the same commit, whichever you prefer.
Created attachment 194599 [details] [review] discoverer: add subtitles API
Created attachment 194600 [details] [review] discoverer: get language from other tags if we did not get it already
Created attachment 194601 [details] [review] discoverer: add application/x-kate to subtitles caps
Created attachment 194602 [details] [review] discoverer: consider subtitles as raw Otherwise, discoverer will generated an "inner" codec where there can be a tranformation (eg, kate -> DVD SPU, and various ->text/x-pango-markup).
Actually, it seems best to consider all subtitles as raw, I saw some ASS -> text inner "codec" being detected too. Patch added to do that. Language is now detected from tags. I went on a wild goose chase for the better part of a day when the information was just next to me. Gah. Should all work fine now :)
Review of attachment 194600 [details] [review]: ::: gst-libs/gst/pbutils/gstdiscoverer.c @@ +754,3 @@ + if (gst_tag_list_get_string (((GstDiscovererStreamInfo *) info)->tags, + GST_TAG_LANGUAGE_CODE, &language)) { + info->language = g_strdup (language); Return value of gst_tag_list_get_string() is already owned by the caller, no need to copy
Review of attachment 194599 [details] [review]: ::: gst-libs/gst/pbutils/gstdiscoverer.c @@ +739,3 @@ + GST_TYPE_STRUCTURE, &tags_st, NULL); + + language = gst_structure_get_string (caps_st, GST_TAG_LANGUAGE_CODE); Why don't you use the GstTagList API here? If there are multiple language tags in that tag list it will be stored as an array, not a string, and you might want to get the first one only then
> Why don't you use the GstTagList API here? If there are multiple language tags > in that tag list it will be stored as an array, not a string, and you might > want to get the first one only then I don't know :) I just copied the block from the existing audio and video cases, I don't know the rationale for using that API. Shall I change it anyway ?
Created attachment 194679 [details] [review] discoverer: get language from other tags if we did not get it already Fix leak found in review
commit e51cbc136b2c6d8e9818c20442cf39476ab80e26 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Aug 24 15:09:47 2011 +0100 discoverer: consider subtitles as raw Otherwise, discoverer will generated an "inner" codec where there can be a tranformation (eg, kate -> DVD SPU, and various ->text/x-pango-markup). https://bugzilla.gnome.org/show_bug.cgi?id=639055 commit 143a6207011996ec37332d9cbaf04c1bd6513c7f Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Aug 24 15:05:38 2011 +0100 discoverer: add application/x-kate to subtitles caps https://bugzilla.gnome.org/show_bug.cgi?id=639055 commit 8438bc038a2a4cf920f017ec290f4335761eb4b1 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Aug 24 14:59:38 2011 +0100 discoverer: get language from other tags if we did not get it already https://bugzilla.gnome.org/show_bug.cgi?id=639055 commit 4b5bfb1fd4955b33675fcdbaa0182922c9bc3b89 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Aug 24 15:04:50 2011 +0100 discoverer: add subtitles API https://bugzilla.gnome.org/show_bug.cgi?id=639055