After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 342268 - [playbin] add 'subtitle-encoding' property
[playbin] add 'subtitle-encoding' property
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 0.10.8
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 342235
 
 
Reported: 2006-05-18 15:13 UTC by Young-Ho Cha
Modified: 2006-05-29 13:54 UTC
See Also:
GNOME target: ---
GNOME version: 2.13/2.14


Attachments
add subtitle-encoding property in playbin plugin. (5.81 KB, patch)
2006-05-19 16:45 UTC, Young-Ho Cha
none Details | Review
add subtitle-encoding property in playbin plugin. (5.44 KB, patch)
2006-05-20 10:56 UTC, Young-Ho Cha
needs-work Details | Review
add subtitle-encoding property in playbin plugin (6.41 KB, patch)
2006-05-24 12:49 UTC, Young-Ho Cha
reviewed Details | Review
add subtitle-encoding property in playbin plugin (6.72 KB, patch)
2006-05-26 15:16 UTC, Young-Ho Cha
none Details | Review
add subtitle-encoding property in playbin plugin (6.59 KB, patch)
2006-05-26 15:45 UTC, Young-Ho Cha
none Details | Review
patch with locking done slightly differently (8.69 KB, patch)
2006-05-28 15:00 UTC, Tim-Philipp Müller
none Details | Review

Description Young-Ho Cha 2006-05-18 15:13:05 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?
Comment 1 Young-Ho Cha 2006-05-19 16:45:40 UTC
Created attachment 65848 [details] [review]
add subtitle-encoding property in playbin plugin.
Comment 2 Young-Ho Cha 2006-05-20 10:56:03 UTC
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. :)
Comment 3 Tim-Philipp Müller 2006-05-24 09:34:05 UTC
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.





Comment 4 Young-Ho Cha 2006-05-24 12:49:01 UTC
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'.
Comment 5 Tim-Philipp Müller 2006-05-26 14:28:16 UTC
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.


Comment 6 Young-Ho Cha 2006-05-26 15:16:04 UTC
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
Comment 7 Young-Ho Cha 2006-05-26 15:45:21 UTC
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
Comment 8 Tim-Philipp Müller 2006-05-28 15:00:28 UTC
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?
Comment 9 Tim-Philipp Müller 2006-05-29 13:21:47 UTC
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).

Comment 10 Young-Ho Cha 2006-05-29 13:54:20 UTC
It works well. thank you very much.