GNOME Bugzilla – Bug 342268
[playbin] add 'subtitle-encoding' property
Last modified: 2006-05-29 13:54:20 UTC
Hi. Yesterday, subparse plugin had 'encoding' property.(bug 339520) and I think It can resolve bug 342235. But I found totem use playbin plugin, not subparse. I want to add 'subtitle-encoding' property in playbin plugin. but I don't know how I can handle subparse property in playbin. Could someone can help me?
Created attachment 65848 [details] [review] add subtitle-encoding property in playbin plugin.
Created attachment 65889 [details] [review] add subtitle-encoding property in playbin plugin. use gst_bin_get_by_name() instead of use gst_bin_iterate_elements() for find subparse element. adding play_base_bin->subencoding init and dispose. copy subtitle-encoding property description from subparse plugin. :)
I think the way the subparse elements to operate on are found needs some more work. You should probably listen to decodebin's "element-added" and "element-removed" signals (and to the same signals on playbin itself), and then * in the element-added callback check whether the element added has a subtitle-encoding property and if yes, add it to a GList of such elements (and set the current encoding if any is set). * in the element-removed callback, check if the element is in that list and remove it from the list if so. * when the property on playbin is changed, just walk the list and g_object_set_value() on all elements in there.
Created attachment 66122 [details] [review] add subtitle-encoding property in playbin plugin Tim. your comments are very helpful. I updated patch as your suggestion. and renamed subparse plugins 'encoding' property to 'subtitle-encoding'.
Thanks for the updated patch, some more comments: (1) > play_base_bin->subtitle_elements = g_slist_alloc (); An empty GSList is simply NULL. What you are doing is allocate a single list node with an unset/garbage data pointer, that's probably not what you want here. Without that, you should also be able to remove the if (!element) return; check in set_encoding_element(). (2) There should be some locking around play_base_bin->subencoding in the element-added callback and the properties setter/getter. (3) In case ARG_SUBTITLE_ENCODING: it should be possible to set the subtitle encoding back to NULL for 'back to default'. Then in set_encoding_element(), you should use GST_STR_NULL(encoding) rather than just 'encoding', since on some platforms/with some libc (notably win32) printf might crash when it is asked to print a NULL string.
Created attachment 66291 [details] [review] add subtitle-encoding property in playbin plugin Thank you for comments. Updated patch as your advices. and now it is working properly with patch in bug 342464
Created attachment 66295 [details] [review] add subtitle-encoding property in playbin plugin remove locks in property getter and elements-removed callback. It seems these lead into dead lock when files open
Created attachment 66375 [details] [review] patch with locking done slightly differently Same patch, but with the locking done a bit differently. Could you try it and confirm it doesn't cause problems/deadlock for you either?
Committed with a few more small changes: 2006-05-29 Tim-Philipp Müller <tim at centricular dot net> Patch by: Young-Ho Cha <ganadist at chollian net> * gst/playback/gstplaybasebin.c: (gst_play_base_bin_class_init), (gst_play_base_bin_init), (gst_play_base_bin_dispose), (set_encoding_element), (decodebin_element_added_cb), (decodebin_element_removed_cb), (setup_subtitle), (setup_source), (gst_play_base_bin_set_property), (gst_play_base_bin_get_property): * gst/playback/gstplaybasebin.h: Add 'subtitle-encoding' property to playbin, so applications can force a subtitle encoding for non-UTF8 subtitles (#342268). * gst/subparse/gstsubparse.c: (gst_sub_parse_class_init), (gst_sub_parse_set_property): Rename recently-added 'encoding' property to 'subtitle-encoding' (so it can be proxied by playbin/decodebin in a generic way with less danger of false positives).
It works well. thank you very much.