GNOME Bugzilla – Bug 534581
Subtitle support
Last modified: 2010-12-12 11:53:14 UTC
Since now Banshee 1.x supports video playback it needs to support subtitles too like totem and mplayer. This is a very wanted feature for me since English isn't my mother tongue. This is the only thing that makes me not using banshee for video atm. Thanks and keep up with good work!
> my mother tongue That would be hard to have mothers tongue in mouth :> . Tongue !== language. Language is what whe speak, while tongue is our "speaking device" :> . However, subtitles would be nice.
[0]gnome-subtitle use SubLib[1] [0] http://gnome-subtitles.sf.net/ [1] http://sublib.sourceforge.net/ Not sure if gstreamer task of work with subtitle. http://bugzilla.gnome.org/show_bug.cgi?id=481075
GStreamer already supports subtitles, maybe some formats not that good, maybe some formats not at all but in general it works good ;) So this is a pure banshee feature enhancement request IMHO.
"Mother Tongue" means the native language or the first language... http://en.wikipedia.org/wiki/First_language I don´t know if you have the expression in Inglish but in spanish we have a quote that is something like "Is better to remain in silent and be thought a fool than to speak out and remove all doubt. just kidding!!! every day we learn something new is better to be open. bye
This it seems is a priority for Intel; I attach the hornsey code that handles subtitles, that I guess we need to re-implement for banshee. On the face of it - looks like ~180 LOC - setting the "suburi" property on the playbin.
Created attachment 157466 [details] subtitles setting from hornsey
moovida code player (a fork of banshee) implements subtitles (but I have not tested it), you can try to see their code!
A lot of video formats are capable of containing Subtitles; sadly, without subtitle support Banshee is unable to activate/deactivate them. Please, for the sake of all involved, implement this! :)
Created attachment 174218 [details] [review] small patch for subtitle It just detect if .srt file exists in same folder in local path and load it. Still need an option in player to switch between embedded subtitle. I have try to test it but no bp_debug appear in my console. So guess patch is wrong or my install is wrong. but it is a beginning...
Review of attachment 174218 [details] [review]: You could add a debug printout of the playbin's "n-text" property to see how many subtitle streams are available, and get/set the "current-text" property to get/change which subtitle stream is used. ::: libbanshee/banshee-player-pipeline.c @@ +127,2 @@ g_object_set (G_OBJECT (player->playbin), "uri", uri, NULL); + bp_lookup_for_subtitle (player, uri); bp_next_track_starting is only used for a gapless transition. You should probably add this call to bp_open (and maybe bp_set_next_track?) in banshee-player.c
Created attachment 174814 [details] [review] A more complete patch It add support for embded subtitle too now ;) Next todo: test it and debug it but need to know if design is good or not...
Review of attachment 174814 [details] [review]: I think the approach is fine. The patch has indentation issues -- libbanshee uses hard tabs, IIRC. ::: libbanshee/banshee-player-video.c @@ +107,3 @@ + + int n_text; + g_object_get (G_OBJECT (player->playbin), "n-text", &n_text, NULL); should use bp_get_subtitle_count here, no? ::: libbanshee/banshee-player.c @@ +65,3 @@ + dot = strrchr (uri2, '.'); + if (dot == NULL) + return; uri2 get leaked here @@ +69,3 @@ + + suburi = g_strconcat (uri2, ".srt", NULL); + if (g_file_test(suburi, G_FILE_TEST_EXISTS)) should also test for | G_FILE_TEST_IS_REGULAR here @@ +155,3 @@ g_object_set (G_OBJECT (player->playbin), "uri", uri, NULL); + //lookup for subtitle file with same name/folder put a space between // and the comment
About the gui part to provide selection of the subtitle stream. What do you prefer: - code in nowplaying extention - code in playerEngine - other place? About GUI part, I can add a submenu in playback menu name. I will join a mockup.
Created attachment 174845 [details] mockup subtitle mockup
Just wanted to jump in to suggest olivier to extend his patch to include changes to the gstreamer-sharp version too ;)
I think the best approach is to have an option in reproduce to select a subtitle to attach and also when is movie is playing when right click. Thanks Oliver for your work it's the most important missing feature in banshee imo.
Olivier, I think the UI should go in Banshee.ThickClient/Banshee.Gui/PlaybackActions.cs. The Banshee.Services PlayerEngineService and PlayerEngine should be modified to add subtitle support, and then the GStreamer backend should implement it (w/ calls to libbanshee).
Created attachment 174905 [details] [review] patch 3 with backend and start on frontend for subtitle Here is a patch for subtitle. I have follow your comment and start to add frontend part. TODO: action to load subtitle file
Created attachment 174990 [details] [review] subtitle patch 4 OK this patch add menu item. It do not throw exception but seems to not work on a mkv file which is the only one I have with subtitle. If someone know a good video sample to test subtitle. The subtitle menu appear but not the submenu... if a gtk expert can take a look because I refresh menu correctly I guess....
Review of attachment 174990 [details] [review]: Getting close! Great work on this, Olivier. ::: libbanshee/banshee-player-pipeline.c @@ -49,3 @@ } - no need for this whitespace change, right? ::: libbanshee/banshee-player-video.c @@ +103,3 @@ +bp_set_subtitle (BansheePlayer *player, int index) +{ + g_return_if_fail (IS_BANSHEE_PLAYER (player)); I was wrong before; libbanshee uses 4 spaces for tabs like the rest of Banshee does; you have hard tabs in this method ::: libbanshee/banshee-player.c @@ +51,3 @@ +static void +bp_lookup_for_subtitle (BansheePlayer *player, const gchar *uri) lots of indentation issues in this method too @@ +74,3 @@ + if (g_file_test (suburi, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR)) { + bp_debug ("[subtitle]: Found srt file: %s", suburi); + g_object_set (G_OBJECT (player->playbin), "suburi", suburi, NULL); might as well use the bp_set_subtitle_uri method here, no? ::: src/Backends/Banshee.GStreamer/Banshee.GStreamer/PlayerEngine.cs @@ +701,3 @@ + + public override SafeUri SubtitleUri { + set { should add a getter too ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +239,3 @@ + public override int SubtitleIndex { + set { + if (value >= 0 && value < SubtitleCount) 1) indentation is messed up 2) use curly braces {} @@ +244,3 @@ + } + + public SafeUri SubtitleUri { doesn't this need to be 'override'? @@ +246,3 @@ + public SafeUri SubtitleUri { + set { + playbin.Suburi = value.AbsoluteUri; make this set {} all one line ::: src/Core/Banshee.Services/Banshee.MediaEngine/PlayerEngine.cs @@ +320,3 @@ } + + public abstract int SubtitleCount { make all 3 of these properties one line each ::: src/Core/Banshee.ThickClient/Banshee.Gui/PlaybackSubtitleActions.cs @@ +3,3 @@ +// +// Author: +// Olivier Dufour <olivier (dot) duff (at) gmail (dot) com> we generally don't obfuscate our e-mails like this...it isn't obfuscated in the git log...do you really care? @@ +5,3 @@ +// Olivier Dufour <olivier (dot) duff (at) gmail (dot) com> +// +// Copyright 2010 Copyright whom? put your name here @@ +24,3 @@ +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +using System; put blank line between header and first 'using' @@ +88,3 @@ + private void AddEmbeddedSubtitle (int i) + { + RadioActionEntry new_action = new RadioActionEntry (String.Format(Catalog.GetString ("Subtitle{0}"), i), null, HACKING; space between method name and arg list And not that I like "Subtitle1" (would prefer "English Subtitles" -- is there a way to get a name like that for them), but at least should be "Subtitle 1" right? And probably need to add 1 to i so we don't have "Subtitle 0"? @@ +103,3 @@ + return; + } + embedded_subtitles_actions.Add (new RadioActionEntry (Catalog.GetString ("WithoutSubtitle"), null, Change to "None", maybe? @@ +105,3 @@ + embedded_subtitles_actions.Add (new RadioActionEntry (Catalog.GetString ("WithoutSubtitle"), null, + Catalog.GetString ("Without subtitle"), null, + Catalog.GetString ("Unactivate subtitle on the current video"), -1)); Change to "Hide subtitles" @@ +106,3 @@ + Catalog.GetString ("Without subtitle"), null, + Catalog.GetString ("Unactivate subtitle on the current video"), -1)); + for (int i = 0; i < sub_count; i++) use {}'s ::: src/Core/Banshee.ThickClient/Resources/core-ui-actions-layout.xml @@ +109,3 @@ <menuitem name="RepeatMenu" action="RepeatMenuAction"/> <menuitem name="ShuffleMenu" action="ShuffleMenuAction"/> + <menuitem name="SubtitleMenu" action="SubtitleMenuAction"/> I wonder if this doesn't belong in the View menu instead. Thoughts?
Created attachment 175070 [details] [review] subtitle patch 5 follow comment and add subtitle description. TODO: load of subtitle file TODO2: find sample to test this feature!
TODO 3 : from 1 to i for sub name, menu in view part ?
Review of attachment 175070 [details] [review]: ::: src/Backends/Banshee.GStreamer/Banshee.GStreamer/PlayerEngine.cs @@ +729,3 @@ + desc_ptr = bp_get_subtitle_description (handle, index); + return GLib.Marshaller.Utf8PtrToString (desc_ptr); + } finally { indentation is messed up ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +178,3 @@ + public override string GetSubtitleDescription (int index) + { + TagList tags = playbin.GetTextTags (index); replace this whole method with: return playbin.GetTextTags (index) .GetTag (Gst.Tag.LanguageCode) .FirstOrDefault (t => t != null); @@ +244,3 @@ + + public override int SubtitleCount { + get { make this property one single line ::: src/Core/Banshee.Services/Banshee.MediaEngine/PlayerEngine.cs @@ +322,3 @@ } + + public abstract int SubtitleCount { make these properties one single line ::: src/Core/Banshee.ThickClient/Banshee.Gui/PlaybackSubtitleActions.cs @@ +83,3 @@ + private void ClearEmbeddedSubtitles () + { + foreach (RadioActionEntry action in embedded_subtitles_actions) {}
Created attachment 175135 [details] [review] embedded and external subtitle patch 6 I have good news and bad news. the good ones, 1) I have added the menu to load .srt file and it load subs. 2) I get embedded subtitle description the bad ones, 1) Setting suburi seems to do nothing ;( 2) I has never succeed to show menu items. So patch need a lot of debug ;(
Created attachment 175205 [details] [review] subtitle patch 7 Here is a new patch. Change: sub autoload: fix uri of subtitle and restrict to all gstreamer subtitle format supported open sub dialog : filter with mime type The list of embedded sub still not appear whereas it is added in submenu (as log show) so I must have missed something on how refresh submenu. help is welcome ;)
Created attachment 175676 [details] [review] subtitle patch 8 So here we have a new patch. Great news: the embedded subtitle work just fine! The load of external subtitle file is not fully supported in playbin2. In fact, it can only be set before video... So to get it working, we have to set pipeline to ready add sub and set pipeline to playing and seek to previous position. I have code that but seems buggy... bug known: I will have to set flags on open because if previous video have been set to no subs, the next one will be unactivate too.
Created attachment 175729 [details] [review] subtitle patch 9 subtitle patch is now ready for full test and feedback. I have test and all seems working even the load of subtitle during playback. Please review and commit ;))
Review of attachment 175729 [details] [review]: Looking great, Olivier! Just a few minor tweaks and I think it's ready. ::: libbanshee/banshee-player.c @@ +439,3 @@ + + g_object_set (G_OBJECT (player->playbin), "suburi", uri, NULL); + gst_element_set_state (player->playbin, GST_STATE_PLAYING); Looks like if the video was paused and you set the subtitle uri, then it will start playing, no? Can we keep it paused in that case? @@ +452,3 @@ +bp_get_subtitle_uri (BansheePlayer *player) +{ + gchar *uri = '\0'; why is this initialized to '\0'? ::: src/Core/Banshee.ThickClient/Banshee.Gui/PlaybackSubtitleActions.cs @@ +125,3 @@ + { + string desc = ServiceManager.PlayerEngine.GetSubtitleDescription (i); + if (string.IsNullOrEmpty (desc)) put {} around if, using 'String.IsNull.." not string.IsNull
Oh, I meant to ask: are there any other tags returned when you do the get-text-tags emit? An actual name/title like "English" would be better than the language code.
Created attachment 175741 [details] [review] subtitle patch 10 must be perfect now ;) just have follow feedback and even handle "und" standard code because I found that on a test video. maybe have to handle "mis" too... http://en.wikipedia.org/wiki/ISO_639-2#Special_situations
Review of attachment 175741 [details] [review]: Great work on this, Olivier. Committed: commit 9e86951d4234d3abdd25d301837f86475795687e Author: Olivier Dufour <olivier.duff@gmail.com> Date: Wed Nov 10 16:23:48 2010 Support embedded and external subtitles (bgo#534581) Signed-off-by: Gabriel Burt <gabriel.burt@gmail.com>
Is it possible to set encoding for subtitles, and adjust the font-size?
open a new bug for that.