GNOME Bugzilla – Bug 733819
Port teletextdec to 1.0
Last modified: 2016-11-27 12:29:24 UTC
Created attachment 281814 [details] [review] Patch porting teletextdec to 1.0 The attachment contains a patch with a 1.0 port of the teletextdec plugin. I've been able to successfully build and run this plugin with the latest GStreamer from master. Other changes include : * The element no longer sends a "dummy buffer for preroll", as it relied on the src_setcaps function being called before pushing the first buffer. This is now fixed. * teletext.c has been deleted, as it did not serve any particular purpose. * GQueue and GMutex has been replaced with GstAtomicQueue. * The output function is now selected via a pointer-to-function instead of an enum. * Fix some warnings related to unused arguments and/or signed-unsigned comparison. * VBI structures (PES demuxer and HTML export module) are allocated only when needed, instead of always (in case of demuxer) or on every push (in case of exporter). This element can also accept VBI teletext data packed inside MPEG PES packets. This capability is exposed as "video/mpeg,mpegversion=2,systemstream=TRUE", which is obviously wrong, but I was not sure how to say "teletext packed inside a PES" in caps-speak. Suggestions welcome.
Created attachment 281822 [details] [review] Change caps to application/x-teletext, don't include gstatomicqueue.h The caps of teletextdec's sinkpad should be application/x-teletext, which allows to link it with tsdemux. Also, including gstatomicqueue.h is not necessary anymore.
Review of attachment 281814 [details] [review]: ::: ext/teletextdec/gstteletextdec.c @@ +126,3 @@ GST_PAD_ALWAYS, + GST_STATIC_CAPS ("video/mpeg,mpegversion=2,systemstream=TRUE;" + "private/teletext;") fwiw, supporting mpeg systemstream isn't that useful and not in sync with how gstreamer elements work (do one thing, do it well). That would remove quite a bit of code also. @@ +134,3 @@ GST_STATIC_CAPS + (GST_VIDEO_CAPS_MAKE ("RGBA") ";" + "text/x-raw, format={utf-8,pango-markup} ; text/html ;") While RGBA and text/pango output make sense ... text/html makes no sense whatsoever. How would one use it ? That would remove yet some more code. @@ +311,3 @@ gst_teletextdec_event_handler, teletext); + teletext->queue = gst_atomic_queue_new (16); Is there any real functional reason to switch from cond/gqueue to atomic queue ? Would be nice to keep the port patch as concise as possible. @@ +418,3 @@ + if (NULL == teletext->export_func) { + /* save the segment event and send it after sending caps. */ + teletext->segment = event; You should check if a segment event is already present (and not pushed), and if so remove the previous one. @@ +487,3 @@ + ret = + GST_ELEMENT_CLASS (gst_teletextdec_parent_class)->change_state (element, fwiw, you can avoid doing those changes by just using a #define gst_teletext_parent_class parent_class at the top of the file @@ +635,2 @@ GstTeletextDec *teletext = GST_TELETEXTDEC (user_data); + (void) dx; ?? @@ +685,3 @@ +static void +gst_teletextdec_negotiate_caps (GstTeletextDec * teletext, There is something missing in this, which is to do an ALLOCATION query to figure out if downstream can provide a GstBufferPool, and if so, use buffers from that pool instead of creating our own (assuming the underlying library can be provided buffers in which to write) @@ +757,2 @@ GstFlowReturn ret = GST_FLOW_OK; + (void) pad; ?? @@ +966,3 @@ + GstBuffer *lbuf; + GstMapInfo buf_map; + (void) teletext; ??
*** Bug 734002 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > @@ +966,3 @@ > + GstBuffer *lbuf; > + GstMapInfo buf_map; > + (void) teletext; > > ?? These are to silence compiler warnings if the function parameters are unused. Alternatively one can put G_GNUC_UNUSED to the parameters. But we generally don't care about this kind of compiler warning, so these things should just be removed :) Also our default compiler warning parameters don't report this.
Created attachment 282482 [details] [review] Patch porting teletextdec to 1.0, after review The patch porting teletextdec to 1.0, with the review comments addressed.
Some extra comments about the new patch : * Asking the peer pad for a buffer pool is done only if RGBA output was selected. This is mainly due to the fact that the text modes send out variable-sized buffers, making it impossible to use a pool. Also, text buffers are much smaller than RGBA buffers. * Some changes were needed regarding the usage of GMutex due to g_mutex_new() being deprecated in Glib 2.32 and replaced with g_mutex_init(). As a result of that, the GstTeletextDec structure contains an embedded instance of GMutex instead of a pointer. * Fake usages of unused parameters have been removed. I must've gotten overly zealous with the -Wextra. * All the code related to reading PES streams and HTML output has been removed.
What's the final decision about this patch? Should I change or add anything else?
Is there any movement on this port?
Tried it on Debian and works fine, but the clock should refreshing permanently and not only when a page update arrives. Maybe in the eventhandler for VBI_EVENT_TTX_PAGE the ev->ev.ttx_page.clock_update could be used to get the current time from the last received page and "mixed" this time with the currently displayed page?
I've tried to find a solution to this before. However, there is one very significant problem : there is no way of knowing where the clock appears. The teletext spec (ETS 300 706) does not place any restrictions on the placement of the clock, and different broadcasters put it in varying regions in the first line of the page. All the transmissions in Poland have the clock on the right side, but I've seen streams where the clock was placed on the left, or in the centre of the header. The most reasonable compromise, in my opinion, is to simply have the header refreshed with every page that the decoder receives, while maintaining the body of the page unmodified. However, this makes it look like the decoder is still "searching" for the page, since the page number in the header changes as well. Same as with the clock, the position of the page number is also up for the broadcaster to choose. However, analogue TVs were somehow capable of keeping the page number still after decoding the requested page, while still updating the clock. I would really like to know how this was done, as there is no data in the teletext packets that would enable the decoder to update only the requested region of the page header. This video shows this behaviour quite well : https://www.youtube.com/watch?v=Xbyxi5j_22s Anyhow, the current patch is based on the current git master, and updates the page header with every page update. Let me know what you think.
Created attachment 312092 [details] [review] Patch porting the teletext decoder to 1.0, with header updates
Thanks for your response Daniel. I wasn't aware that the clock can be at any position. The current approach is very irritating because of the refreshing page numbers. Another problem is that the colors differ on some pages (mixed teletext levels?). Maybe it's better to make this in a seperate patch to keep the port clean, because I have 2 patches (https://bugzilla.gnome.org/show_bug.cgi?id=666247 and https://bugzilla.gnome.org/show_bug.cgi?id=755523) submitted which are based on this one and I fear the changes will break it. Maybe you are interested for looking at these too and improve it? For example I didn't know how to force an instant page update and I'm not sure if the page_update() function is the correct way. It works only when setting the sink to false. Looks like nobody else is interested in teletext for gstreamer... :(
Created attachment 312183 [details] [review] Patch porting the teletext decoder to 1.0
The patch I just added contains the (hopefully) final version of the actual port. I'll create another bug report for the header updates once I finish it.
Dear gstreamer maintainers, could somebody please explain why there is absolutely no progress since over a year!?
Hi, Sorry for the delay, this was on my backlist of things to review/check. It looked clean enough and worked good enough (with hardcoded pipeline). So let's make it happen :) commit 717f9a287d2bac1f343b072616ca3e4ae9850426 Author: Daniel Kamil Kozar <dkk089@gmail.com> Date: Thu Sep 24 17:40:02 2015 +0200 port teletextdec to 1.0 https://bugzilla.gnome.org/show_bug.cgi?id=733819
Hi Edward, thank you very much! Could you please take a look at https://bugzilla.gnome.org/show_bug.cgi?id=666247 https://bugzilla.gnome.org/show_bug.cgi?id=755523 too?
(Just a note, it's important for submitters to ping to get attention. Submitters can also be to blame for long delays.)
> "it's important for submitters to ping to get attention" Well, how often submitters should ping? Look at the 2 patches above: no reaction (one of them was submitted in 2011). That's really frustrating, because these functions are required to make a teletext client user friendly. Sorry for off topic...