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 704881 - Partial ATSC_user_data and CEA-708 Closed Captions Support
Partial ATSC_user_data and CEA-708 Closed Captions Support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.15.1
Assigned To: Edward Hervey
GStreamer Maintainers
Depends on: 794901
Blocks: 678146
 
 
Reported: 2013-07-25 17:14 UTC by Steve Maynard
Modified: 2018-05-28 13:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
CEA-708 support attempt #2 (99.09 KB, patch)
2013-07-25 17:14 UTC, Steve Maynard
none Details | Review
png of current pipe (525.52 KB, image/png)
2013-07-25 17:16 UTC, Steve Maynard
  Details
png of previous test impl. (1.05 MB, image/png)
2013-07-25 17:17 UTC, Steve Maynard
  Details
unapproved patch (39.60 KB, patch)
2013-12-09 22:16 UTC, Steve Maynard
none Details | Review
ccdec patch (30.81 KB, patch)
2013-12-09 22:18 UTC, Steve Maynard
needs-work Details | Review
cea708dec patch (66.42 KB, patch)
2013-12-09 22:19 UTC, Steve Maynard
needs-work Details | Review
non-working mpegvideoparse patch (14.61 KB, patch)
2013-12-09 22:32 UTC, Steve Maynard
none Details | Review
updated ccdec patch (29.93 KB, patch)
2013-12-10 20:04 UTC, Steve Maynard
none Details | Review
updated cea708dec patch (64.68 KB, patch)
2013-12-10 20:05 UTC, Steve Maynard
none Details | Review
maintainers suggestions implemented (65.81 KB, patch)
2013-12-10 20:07 UTC, Steve Maynard
none Details | Review
combination of previous patches (112.63 KB, patch)
2014-06-26 05:34 UTC, cjunwang
none Details | Review
different codeset support (10.30 KB, patch)
2014-06-26 05:44 UTC, cjunwang
none Details | Review
add pango capability (8.62 KB, patch)
2014-06-26 05:53 UTC, cjunwang
none Details | Review
utf-8 pango-markup/webvtt output (11.97 KB, patch)
2014-06-26 06:02 UTC, cjunwang
none Details | Review
correct PTS and segment format. (3.20 KB, patch)
2014-06-26 06:06 UTC, cjunwang
none Details | Review
seperate pango and webvtt display (21.17 KB, patch)
2014-08-05 09:02 UTC, cjunwang
none Details | Review
enhance CEA708 spec support (7.42 KB, patch)
2014-08-05 09:10 UTC, cjunwang
none Details | Review
add capability service-number (5.88 KB, patch)
2014-08-05 09:14 UTC, cjunwang
none Details | Review
add pango span attributes (24.13 KB, patch)
2014-08-05 09:17 UTC, cjunwang
none Details | Review
remove webvtt output (35.57 KB, patch)
2014-08-05 09:23 UTC, cjunwang
none Details | Review
fix coredump when additioanl data exist (3.98 KB, patch)
2014-08-05 09:26 UTC, cjunwang
none Details | Review
updated patch "remove webvtt output" (35.56 KB, patch)
2014-08-06 03:39 UTC, cjunwang
none Details | Review
change ccdec two src pad to one (12.42 KB, patch)
2014-08-06 07:01 UTC, cjunwang
none Details | Review
cea708 closed caption support (125.09 KB, patch)
2014-08-12 10:04 UTC, cjunwang
none Details | Review
subtract closed caption packet from mpeg video user data field (8.03 KB, patch)
2015-01-22 07:42 UTC, cjunwang
none Details | Review
gstmpegvideoparse expose closed caption capability (3.93 KB, patch)
2015-01-22 07:52 UTC, cjunwang
none Details | Review
new mpegvideodemux plugin (25.98 KB, patch)
2015-01-22 07:55 UTC, cjunwang
none Details | Review
decodebin skip same type parse element (2.06 KB, patch)
2015-01-22 07:58 UTC, cjunwang
none Details | Review
new closedcaptionoverlay plugin (142.85 KB, patch)
2015-01-22 08:00 UTC, cjunwang
none Details | Review
updated patch "subtract closed caption packet from mpeg video user data field" (7.51 KB, patch)
2015-03-13 02:02 UTC, cjunwang
none Details | Review
update patch "gstmpegvideoparse expose closed caption capability" (3.94 KB, patch)
2015-03-13 02:05 UTC, cjunwang
none Details | Review
updated patch "new mpegvideodemux plugin" (25.95 KB, patch)
2015-03-13 05:30 UTC, cjunwang
none Details | Review
updated patch "new closedcaptionoverlay plugin" (141.38 KB, patch)
2015-03-13 05:32 UTC, cjunwang
none Details | Review
updated patch "decodebin skip same type parse element" (2.06 KB, patch)
2015-03-13 05:34 UTC, cjunwang
none Details | Review
updated patch "decodebin skip same type parse element" (2.05 KB, patch)
2015-03-13 05:53 UTC, cjunwang
none Details | Review
ceaccoverlay: New CEA-708 Closed Caption decoder and overlayer (144.80 KB, patch)
2018-03-12 06:54 UTC, Edward Hervey
committed Details | Review

Description Steve Maynard 2013-07-25 17:14:57 UTC
Created attachment 250128 [details] [review]
CEA-708 support attempt #2

In an effort to begin support for Closed Captions (see bug #678146), a proposal was made to create an element to auto-plug after mpegvideoparse that would pass thru video and split out user_data packets (see ATSC A/53 Part 4) and send them to yet another new element that would decode the user_data into MPEG_cc_data.

After discussions with the maintainers it was decided this was not the correct path and mpegvideoparse itself should be modified to add new "sometimes" source pad(s) that would have subtitle/x-cea-708  (or 608) capabilities.  Then new elements could be added to decode the respective input caps and generate text/x-raw output (which in-turn could be handled with elements to convert to text/vtt if so desired)...

An initial pass at this is enclosed in a patch against master as of the creation of this issue.  It seems that running this code with a stream that has user_data actually prevents the video portion of the pipe from running, while audio and text portions do run to completion.

While comparing the PNGs of the original test and the current implementation it appears that a multiqueue was added after the original element but not after mpegvparse now (could this be a part of the problem?)

I'm testing with the following command line (you'll have to insert your favorite MPEG2-TS in place of mine):

rm *.dot; GST_DEBUG_DUMP_DOT_DIR=./  gst-launch-1.0  --gst-debug-no-color --gst-debug-level=3 --gst-debug="playbin:4,ccfilter:4,mpegvideoparse:4" playbin uri=file:///home/smaynard/Videos/mpeg2ts_cea708.mpg text-sink="capsfilter caps=text/x-raw ! filesink location=out.txt" 2>&1 | tee test.log

NOTE: defining DROP_USER_DATA in mpegvideoparse.c will allow normal operation...

Any assistance or comments are welcomed...
Comment 1 Steve Maynard 2013-07-25 17:16:26 UTC
Created attachment 250129 [details]
png of current pipe
Comment 2 Steve Maynard 2013-07-25 17:17:17 UTC
Created attachment 250130 [details]
png of previous test impl.
Comment 3 Sebastian Dröge (slomo) 2013-07-26 07:25:41 UTC
Looks indeed like a good start, but as you said this would seem better to be integrated into mpegvideoparse, h264parse, etc. Maybe with a helper library to put common code in a single place.

However for adding multiple srcpads to a parser there are changes necessary in decodebin:
1) decodebin needs to detect these multi-srcpad parsers still as parsers (thus use the capsfilter hack for its main srcpad, but only for that) and additionally detect them as demuxers (thus use the multiqueue after all srcpads).
2) decodebin/uridecodebin/playbin need support for late pads. That is pads that are added after no-more-pads (or after multiqueue overrun, which is handled like no-more-pads). These should be included in the current stream group instead of being ignored. This shouldn't be too difficult but might break some assumptions in a few places.


Also would be good to split the patch into smaller pieces to make it easier to review :)
Comment 4 Steve Maynard 2013-12-09 22:15:22 UTC
Having just returned to this effort, I have not been able to perform the required modifications to decodebin/uridecodebin/playbin to accept pads added late or mpegvideoparse to deal with an additional sometimes pad.  I am adding the respective patches to add CC decoding from user_data, cea708 decoding from raw CC data into VTT, and the unapproved changes required to test these patches.

The unapproved patch need not be reviewed as the maintainers already instructed that it was not the way to go forward.

The ccdec patch includes the element required to convert ATSC video user_data into CC packets for either cea608 or cea708 decoder consumption.  There is currently no decoder for cea608.

The cea708 patch includes the element required to convert CC data packets into WebVTT cue text and is required for WebKit to display CC text streams.  This patch relies on the ccdec patch for the plugin and makefile.
Comment 5 Steve Maynard 2013-12-09 22:16:57 UTC
Created attachment 263859 [details] [review]
unapproved patch

required only to build/test the ccdec and cea708dec patches
Comment 6 Steve Maynard 2013-12-09 22:18:16 UTC
Created attachment 263860 [details] [review]
ccdec patch

ccdec element for review
Comment 7 Steve Maynard 2013-12-09 22:19:08 UTC
Created attachment 263861 [details] [review]
cea708dec patch

cea708dec element for review
Comment 8 Steve Maynard 2013-12-09 22:32:53 UTC
Created attachment 263864 [details] [review]
non-working mpegvideoparse patch

This is an incomplete attempt at moving the required code from the unapproved patch into mpegvparse as the maintainers suggest.  It lacks the code necessary to support an additional sometimes pad from a parser.  It is here for reference and if anyone picks up this effort or assists in its completion.
Comment 9 Olivier Crête 2013-12-09 22:46:29 UTC
Review of attachment 263860 [details] [review]:

Thanks, this is something we were really missing! Your element generally looks good, just some small stylistic details.

Also, you don't happen to have unit tests lying around, that's not a blocker for -bad, but they'll be needed before it can get to -good.

::: gst/ccdec/gstccdec.c
@@ +60,3 @@
+#include "gstcea708dec.h"
+
+#define GST_LICENSE "LGPL"

No need to put this here

@@ +63,3 @@
+
+#undef SUPPORT_CEA608
+#undef STANDALONE_ELEMENT

Remove the STANDALONE_ELEMENT macro

@@ +105,3 @@
+/* This element's private data */
+struct _GstCcDecoder
+{

Put the struct in the .h, it's not installed anyway

@@ +130,3 @@
+
+struct _GstCcDecoderClass
+{

Same for this struct

@@ +155,3 @@
+  gstelement_klass = GST_ELEMENT_CLASS (klass);
+
+  gobject_klass->dispose = GST_DEBUG_FUNCPTR (gst_cc_decoder_dispose);

No use to FUNCPTR here

@@ +170,3 @@
+      "CableLabs RUIH-RI Team <ruihri@cablelabs.com>");
+
+#ifndef STANDALONE_ELEMENT

Remove macro

@@ +175,3 @@
+#ifdef DEBUG_ELEMENT_SIZE
+  g_message ("%s %d = sizeof(_GstCcDecoder)\n", __func__,
+      sizeof (struct _GstCcDecoder));

Remove this too

@@ +187,3 @@
+    gst_buffer_unref (decoder->data_list->data);
+    decoder->data_list = g_slist_remove (decoder->data_list,
+        decoder->data_list->data);

g_slist_free_full() is what you want

@@ +201,3 @@
+  gst_segment_init (&decoder->segment, GST_FORMAT_UNDEFINED);
+
+  // Add sink pad

Please only use /* */ style comments

@@ +208,3 @@
+  GST_INFO_OBJECT (decoder, "sink pad added to decoder");
+
+  decoder->cea608_srcpad = NULL;

No need to initialize membenrs to 0/NULL, GObject memsets all objects to 0.

@@ +236,3 @@
+gst_cc_decoder_send_new_segment (GstCcDecoder * decoder, GstPad * srcpad)
+{
+  if (NULL != decoder && NULL != srcpad) {

How can decoder or srcpad be NULL here ?

@@ +240,3 @@
+
+    if (decoder->segment.format == GST_FORMAT_UNDEFINED) {
+      GST_WARNING_OBJECT (decoder, "No segment received before first buffer");

Sending a segment event with format == UNDEFINED is bad, in this case, just fail. In Gst 1.x, you should always have a segment event before the first buffer.

@@ +293,3 @@
+    g_free (start_id);
+    gst_element_add_pad (GST_ELEMENT (decoder), *pad);
+    gst_cc_decoder_send_new_segment (decoder, *pad);

Also push the caps event before the segment event. Ideally push both of those before adding the pad.

@@ +369,3 @@
+  // we need at least 6 bytes of data to process 1 ATSC packet
+  if (user_data->length < 6) {
+    GST_ERROR ("ATSC user data packet underflow!");

GST_ERROR_OBJECT()

@@ +515,3 @@
+
+  gst_buffer_map (GST_BUFFER (element_A), &elt_map_A, GST_MAP_READ);
+  user_data_A = (UserData *) elt_map_A.data;

This seems to assume alignment of the struct. You better read the header manually to fill the struct.

@@ +524,3 @@
+    ret_val = 1;
+  } else {
+    GST_ERROR ("duplicated TR(%d) w/o GOP processing!", user_data_B->tr);

GST_ERROR_OBJeCT()

@@ +629,3 @@
+
+  if (g_str_has_prefix ((const gchar *) (user_data->data), "GA94") ||
+      g_str_has_prefix ((const gchar *) (user_data->data), "DTG1")) {

Check if the data is long enough before doing this, it's not a NULL terminated string.

@@ +637,3 @@
+        user_data->data[1], user_data->data[2], user_data->data[3]);
+    gst_buffer_unmap (buf, &buf_map);
+  } else if (g_str_has_prefix ((const gchar *) (user_data->data), "GOP\0")) {

Same here

@@ +688,3 @@
+    GST_ERROR_OBJECT (decoder, "unknown data, slist:%p", decoder->data_list);
+    gst_buffer_unmap (buf, &buf_map);
+    gst_buffer_unref (buf);

Might make sense to put the unmap/unref outside the if() (and just add a ref in the first case)

@@ +694,3 @@
+}
+
+#ifdef STANDALONE_ELEMENT

Remove this section

::: gst/ccdec/plugin.c
@@ +44,3 @@
+GST_PLUGIN_DEFINE (GST_VERSION_MAJOR,
+    GST_VERSION_MINOR,
+    ccdecbad,

No need to put "bad" in the plugin name, there is no cc plugin anywhere else
Comment 10 Olivier Crête 2013-12-09 22:48:04 UTC
Review of attachment 263864 [details] [review]:

All of the actual parsing of the data shouls probably go into the codecparsers library.
Comment 11 Olivier Crête 2013-12-09 23:01:20 UTC
Review of attachment 263861 [details] [review]:

Generally looks good, stylistic details from the other patch also apply here. Added a couple more.

::: gst/ccdec/gstcea708dec.c
@@ +187,3 @@
+static GstStaticPadTemplate src_template = GST_STATIC_PAD_TEMPLATE ("src",
+    GST_PAD_SRC,
+    GST_PAD_SOMETIMES,

Why is this a sometimes pad? Is there any case where CEA-708 data would not produce anything ?

@@ +289,3 @@
+  // Free memory allocated in gst_cea708dec_init()
+  for (i = 0; i < MAX_708_WINDOWS; i++) {
+    if (decoder->cc_windows[i]) {

No need for the if(), g_free() checks for NULL

@@ +295,3 @@
+  }
+
+  if (decoder->text_list) {

No need for the if()

@@ +697,3 @@
+
+  if (NULL != command) {
+    GST_DEBUG_OBJECT (decoder, "Process 708 command (%02X): %s", c, command);

Stuff that happens during normal streaming should be reported at the LOG level.

@@ +723,3 @@
+      map.data[0] = 0;
+      g_slist_foreach (*text_list, get_cea708dec_bufcat, &map);
+      GST_INFO_OBJECT (decoder, "sent \n%s\n", map.data);

This should probablu be LOG level too?

@@ +725,3 @@
+      GST_INFO_OBJECT (decoder, "sent \n%s\n", map.data);
+      gst_buffer_unmap (outbuf, &map);
+      gst_pad_push (pad, outbuf);

Please return the return value upstream.

@@ +731,3 @@
+      *text_list = NULL;
+    } else {
+      GST_ERROR_OBJECT (decoder, "ERROR allocating %d", length);

This will never happen, the default allocators relies on GSlice/g_malloc() which aborts() if allocation fails.

@@ +745,3 @@
+  va_start (args, format);
+
+  if (NULL != (str = g_malloc0 (len))) {

g_malloc() never returns NULL (it will call abort() if malloc fails).

@@ +1666,3 @@
+}
+
+#ifdef STANDALONE_ELEMENT

Remove this section
Comment 12 Olivier Crête 2013-12-09 23:02:05 UTC
I wonder if the new elements should use one of the base classes (potentially BaseParse?)
Comment 13 Steve Maynard 2013-12-10 20:04:08 UTC
Created attachment 263941 [details] [review]
updated ccdec patch

updated per suggestions
Comment 14 Steve Maynard 2013-12-10 20:05:09 UTC
Created attachment 263942 [details] [review]
updated cea708dec patch

updated per suggestions
Comment 15 Steve Maynard 2013-12-10 20:07:04 UTC
Created attachment 263943 [details] [review]
maintainers suggestions implemented

diff based on prev ccdec and cea708dec patches with suggested changes (might be easier to see what was changed)
Comment 16 Steve Maynard 2013-12-10 20:12:43 UTC
(In reply to comment #10)
> Review of attachment 263864 [details] [review]:
> 
> All of the actual parsing of the data shouls probably go into the codecparsers
> library.

This change-set is incomplete.  I have a week or so left on this project and if you can make specific requests I can try to implement and test.  This was an attempt to implement what slomo suggested, but I didn't know how to complete it:
i.e. he specifically asked me to:
1) decodebin needs to detect these multi-srcpad parsers still as parsers (thus
use the capsfilter hack for its main srcpad, but only for that) and
additionally detect them as demuxers (thus use the multiqueue after all
srcpads).
2) decodebin/uridecodebin/playbin need support for late pads. That is pads that
are added after no-more-pads (or after multiqueue overrun, which is handled
like no-more-pads). These should be included in the current stream group
instead of being ignored. This shouldn't be too difficult but might break some
assumptions in a few places.

But I'm unsure of how to do either of these, more information would be much appreciated.
Comment 17 Edward Hervey 2014-02-05 15:31:13 UTC
Is it possible to get unique patches for this work ? Instead of patches-over-previous-patches.

Regarding handling decodebin, that can be done in a second step (and maybe separate blocking bug ?).
Comment 18 Steve Maynard 2014-02-12 14:44:49 UTC
I'm no longer at CableLabs - I believe they will reassign this to another engineer...
Comment 19 Thiago Sousa Santos 2014-04-10 10:26:47 UTC
Hello, any updates here?
Comment 20 cjunwang 2014-06-06 02:37:59 UTC
It's a useful feature. I'd like to continue work on this bug.
Comment 21 cjunwang 2014-06-26 05:34:59 UTC
Created attachment 279291 [details] [review]
combination of previous patches

Thank you, Steve. I summarized all of patches from you.
meanwhile remove other bugs' related unsubmitted codes, which prevent compilation success. Reviewer can directly get this patch to look at previous work.
Comment 22 cjunwang 2014-06-26 05:44:57 UTC
Created attachment 279292 [details] [review]
different codeset support

In CEA-708, there are different codeset, from C0-C3, G0-G3.
All of these should be dealed in different way.
for example, The G1 Table is basically the ISO 8859-1 Latin-1 character set.
should be converted to utf-8.
Comment 23 cjunwang 2014-06-26 05:53:38 UTC
Created attachment 279293 [details] [review]
add pango capability

change cea708dec src pad sometimes to always, add pango-markup capbility to output pango text.
Comment 24 cjunwang 2014-06-26 06:02:14 UTC
Created attachment 279294 [details] [review]
utf-8 pango-markup/webvtt output

convert char ucs4 to utf-8, and output decoded cc data as pango-markup/webvtt format text.
Comment 25 cjunwang 2014-06-26 06:06:35 UTC
Created attachment 279295 [details] [review]
correct PTS and segment format.

change segment to GST_FORMAT_TIME, and the unit of PTS is nanosecond.
Comment 26 cjunwang 2014-06-26 06:09:42 UTC
Comment on attachment 279295 [details] [review]
correct PTS and segment format.

you can test like this:
gst-launch-1.0 filesrc location=708_digital_caption_BRO_CEAv1.2zero.ts ! tsdemux name=d d.video_0021 ! mpegvideoparse name=m m.src ! queue max-size-buffers=0 max-size-time=0 ! avdec_mpeg2video skip-frame=5 ! textoverlay name=txt wait-text=false ! xvimagesink sync=false m.user_data_src ! queue max-size-buffers=0 max-size-time=0 ! ccdec !  cea708dec ! "text/x-raw" ! txt.
Comment 27 cjunwang 2014-08-05 09:02:38 UTC
Created attachment 282524 [details] [review]
seperate pango and webvtt display
Comment 28 cjunwang 2014-08-05 09:10:49 UTC
Created attachment 282525 [details] [review]
enhance CEA708 spec support

support bottom-to-up scroll, and fix 0x0D(\r) command dealing.
Comment 29 cjunwang 2014-08-05 09:14:13 UTC
Created attachment 282526 [details] [review]
add capability service-number

user can select the service that will be decoded. default is 1 (primary caption service)
Comment 30 cjunwang 2014-08-05 09:17:16 UTC
Created attachment 282527 [details] [review]
add pango span attributes

support text part's font size,fg/bg color, underline, italics
Comment 31 cjunwang 2014-08-05 09:23:03 UTC
Created attachment 282529 [details] [review]
remove webvtt output

remove src pad capability "webvtt". because webvtt format output will not directly render to screen, and pango to webvtt convertion can be done by other plugin such as webvttenc.
Comment 32 cjunwang 2014-08-05 09:26:25 UTC
Created attachment 282530 [details] [review]
fix coredump when additioanl data exist

when video userdata's ATSC_reserved_user_data exist, buffer excceed maximum size
Comment 33 cjunwang 2014-08-05 09:35:36 UTC
I created a seperate bug https://bugzilla.gnome.org/show_bug.cgi?id=734220.
decodebin/uridecodebin/playbin need support multiple srcpads in a parser.


and to make textoverlay better to support closed caption, created another bug
https://bugzilla.gnome.org/show_bug.cgi?id=734219
when text buffer display, it's not determined yet when text will be destroyed.
Comment 34 Faham Negini 2014-08-05 21:45:12 UTC
Hi cjun wang, I'm trying to follow up your work on closed captioning, I'm also adding a cea608dec element providing support for the cea608 closed captioning. Today looking at your new patches, I cannot apply the patch number 282529, after applying 282527. it looks there is something missing between these patches. Do you know how can I access the branch you are working on?

Thanks,
Faham
Comment 35 cjunwang 2014-08-06 03:39:08 UTC
Created attachment 282622 [details] [review]
updated patch "remove webvtt output"

there is a minor error in previous patch number 282529, replace with it.
@Faham Negini
glad to know you working on 608dec. 
Did you already upload your patches, and could you share the link with me?
I am working on company's git, which can't be accessed by the public.
Anyway I will try to apply to create a public repository.
Comment 36 cjunwang 2014-08-06 07:01:15 UTC
Created attachment 282635 [details] [review]
change ccdec two src pad to one

Sorry for omitting this patch, should be applied before patch number 282524.

change ccdec two sometimes pad(cea608/708) to one always pad, with different capability x-cea608-raw/x-cea708-raw.
Comment 37 cjunwang 2014-08-06 07:47:22 UTC
There are still 2 issues.

1. whether combine ccdec and cea708dec plugin together. 
If combined, new plugin need implement both 708 and 608 decoding. It seems more complicated. but one of its merit is, it's easier to use. In most of circumstances user just want watch only one of 608/708 services: NTSC_CC_FIELD_1, NTSC_CC_FIELD_2, or one of 708 service. http://en.wikipedia.org/wiki/CEA-708

If seperated, every 608/708 decoder can focus on itself. 



2. whether cea708dec or cea608dec need output image directly, or there should be a new plugin for closed caption overlay?
Now cea708dec output is pango-markup text, which can be rendered with textoverlay, or textrender. 

If do not write new plugin, closed caption has some features, which need modify textoverlay/textrender. 
1)decoded cc buffer's duration is not fixed. That means when text buffer display, it's not determined yet when text will be destroyed. 
Does GstBuffer need add support?
https://bugzilla.gnome.org/show_bug.cgi?id=734219
2)cc window's position is not fixed
3)it's possible to have multiple closed caption windows displayed at same time, although it's seldom used. 
4)...
If write new plugin, new plugin will deal with these things.

Any suggestion or comments are welcomed...
Comment 38 Edward Hervey 2014-08-11 14:24:28 UTC
cjunwang, can you just give one patch instead for review ? And mark all the others as deprecated.
Comment 39 cjunwang 2014-08-12 01:19:55 UTC
Thank you, Edward. I will do that.
Comment 40 cjunwang 2014-08-12 10:04:19 UTC
Created attachment 283171 [details] [review]
cea708 closed caption support

merge all of patches to one
Comment 41 Faham Negini 2014-08-21 00:59:15 UTC
Why the ccdec has changed its strategy providing different user_data packet types on a same always source pad? I'm currently working on support for a number of other user_data packet types, like the afd_data, luma_PAM and bar_data, along with adding support for cea608dec. But with current design, it is not possible to expand the ccdec to do that.

I'm thinking to expand ccdec to a more generic userdatadec element that detect and provides various user_data packet types carried in the video stream. I though the best way to do that is to add multiple always source pads to the userdatadec (previously known as ccdec) element for each user_data packet type. Otherwise, carrying all these different user_data packet types on different caps of a same src_pad makes only one packet type effectively accessible.

Please let me know if I'm taking a wrong path.

Faham
Comment 42 Olivier Crête 2014-08-21 17:39:45 UTC
Can all of these user_data types be present on the same stream? IF yes, they should indeed be separate pads, but please make them sometimes pad, so playbin can know which ones exist in the stream and which one to connect.
Comment 43 cjunwang 2014-08-22 02:13:22 UTC
from scratch ccdec is designed to subtract cea608/708 data. but we can change ccdec to userdatademux, to include subtracting other user_data packet types(afd,etc). userdatademux should have seperated sometimes pads for cea608,708, afd, ...
This need current mpegvideoparse userdata parsing modification moved to userdatademux. and mpegvideoparse is only responsible to subtract userdata and pass to userdatademux.
and it's possible to make userdatademux cover other video user data, such as H.264/H.265.
Comment 44 Edward Hervey 2014-08-22 07:26:25 UTC
yes, if you can "identify" the different types of user-data, they should be indeed exposed as different types.

That being said ... the types you mention (Active Format Description, Bar data and luma modifiers) are needed for proper video display. They are going to be needed for proper display, i.e. by the sink (and potentially other video conversion elements). Furthermore, they seem pretty minimalistic to parse and are really metadata (as opposed 

I'm wondering if those shouldn't just be parsed and set as GstMeta on the video stream instead. Or maybe as a custom sticky event if they don't change ?
I expect that in the old-school way of doing things (decoder is tightly connected to the display system) the decoder/display bundle handled that in one go. I'm not sure the current decoders we have can handle that.

BTW, I'm not at all a fan of the userdatademux (in case that wasn't clear).
1) If it's trivial to parse, represents metadata and doesn't change often, it should be handled in the parser (or demuxer) and pushed out as GstMeta or sticky events on the relevant pads.
2) If it represents a changing data stream (such as closed caption). It should be properly identified and pushed as a new clearly identified stream.
Comment 45 Faham Negini 2014-08-22 18:31:55 UTC
Thanks, then I'm going to remove the AFD part from ccdec, outputting that as GstMeta to be used by the decoder. For that I'm going to patch mpegvideoparse,h264parse and vaapidecode elements.

I'm also adding support for user_data to h264parse element. but the confusing part of that for me is how to decide the value of temporal_ref in the UserData struct. h264 has no such data in its NAL packets, but it has slice.pic_order_cnt_lsb. Does anyone know what is the best way to emulate temporal_ref there.
Comment 46 cjunwang 2014-08-28 09:13:45 UTC
since ccdec only accept closed caption, I will refine mpegvideoparse to output only closed caption data to ccdec.

one more thing, cea708 closed caption have some features such as multiple varying windows. after consideration, to fully support cea708 spec, the better way is to make cea708dec directly decode out images insteadof pango-markup text.

Any suggestion are welcomed.
Comment 47 Edward Hervey 2014-08-28 12:33:02 UTC
The best would be to make it an "overlay" element (takes both video and cc, and outputs overlayed video).
Comment 48 cjunwang 2014-08-29 02:19:03 UTC
Considering sometimes video hardware decoder & zero-copy between decoder&sink,
video & closed caption are displayed seperately in different layer, at this time no software overlay is needed.

Simliar with textrender & textoverlay in gst-plugins-base, maybe both cea708overlay and cea708render plugin is needed. cea708overlay is used in general senario(software overlay); cea708render, output a transparent big image including several closed caption windows, which can be showed on the upside of video layer. 

cea708 decoding codes could be put in a common place to be used by both cea708overlay and cea708render.
Comment 49 Edward Hervey 2014-08-29 05:22:17 UTC
(In reply to comment #48)
> Considering sometimes video hardware decoder & zero-copy between decoder&sink,
> video & closed caption are displayed seperately in different layer, at this
> time no software overlay is needed.

  A "overlay" element doesn't have to "burn-in" the caption/subtitles. There is a GstMeta (GstVideoOverlayCompositionMeta) that one can use (if downstream supports it).
  This enables you to still do the decoding of CC, know the video properties (and therefore render the CC with the right size/properties), and specify where exactly (in x/y location) it should be located. But instead of burning it in, you just attach it on to the buffer so the downstream element can then send it to a different layer.

  Of course if downstream doesn't support that GstMeta, then you actually burn in the video.
Comment 50 cjunwang 2014-08-30 06:53:09 UTC
Yes. it works.
in product there could be this kind of cea708overlay with GstVideoOverlayCompositionMeta. but in zero-copy solution decoded video buffer is dmabuf, used to directly render, seldom used for overlay. this works but seems a bit unsuitable to connect to overlay plugin. if the purpose is only to know the video properties, mpegvideoparse already know them by parsing sequence header. we can send them downstream, or set as part of capbility, to let downstream cc plugin know.

for open source, maybe in first step we just need implement cea708overlay element without GstVideoOverlayCompositionMeta. in future consider more complicated senario.
Comment 51 Thiago Sousa Santos 2014-09-09 22:26:29 UTC
Do we have a final decision on how the data is going to be extracted from the mpeg4video stream? Allowing parsers to have dynamic pads? Using GstMeta for the encoded CC? A small demuxer before the parser?
Comment 52 Tim-Philipp Müller 2014-09-09 22:31:26 UTC
Not really, as far as I can tell..
Comment 53 Sebastian Dröge (slomo) 2014-09-12 12:34:10 UTC
The simplest might be to have GstMeta for this, and then inside playbin before the selectors we would have a closed-caption-demux element that will extract the metas and puts them into a separate stream which will then go to the subtitle input-selector.
Comment 54 cjunwang 2014-09-13 02:14:58 UTC
list possible solution pipeline(SW codec).

1. meta data in parser
mpegvideoparse->vdec->ccoverlay->videosink

2. new cc pad in parser
mpegvideoparse->vdec->ccoverlay->videosink
 |                      |
 |                      |
 ------------>-----------

3. small demuxer before the parser
ccdemuxer->mpegvideoparse->ccoverlay->videosink
      |                         |
      |                         |                      
      -------------->------------

4. more?

method 1 seems simplest to implement, the shortingcoming I can know is, video and cc decoding is serialized, costing more time. 
method 2 need GStreamer support parser having more than one pad, but have better performance.
method 3 need create a new plugin ccdemuxer and move some logics here, from mpegvideoparse, h264parse, etc... because multiple video format could have closed caption data.
Comment 55 Sebastian Dröge (slomo) 2014-09-13 09:28:25 UTC
The cleanest way after some more discussion on IRC seems to be:

- Add a h264demux that splits off the CC stream(s) and forwards the normal h264 bitstream, additional to a h264parse which would come after it (h264demux has a higher rank). h264demux would also use the codecparsers lib, so will be relatively small. Reason for a separate element: our parser handling is difficult enough already and the concept was always that a parser has one always sinkpad and one always srcpad. This thing here is conceptionally exactly what we call demuxer elsewhere.
- Add support for dynamic pads that are added later (after no-more-pads) to decodebin/uridecodebin/playbin. Without having them handled as a new group, and without breaking group handling.

The second part is not really trivial :)
Comment 56 Olivier Crête 2014-10-29 18:40:55 UTC
@slomo: Would the h264demux do with AFD, etc? Add a GstMeta?
Comment 57 cjunwang 2015-01-22 07:42:30 UTC
Created attachment 295145 [details] [review]
subtract closed caption packet from mpeg video user data field
Comment 58 cjunwang 2015-01-22 07:52:10 UTC
Created attachment 295146 [details] [review]
gstmpegvideoparse expose closed caption capability

when parse mpeg video, check whether carry closed caption, if carry, expose as part of capability
Comment 59 cjunwang 2015-01-22 07:55:57 UTC
Created attachment 295147 [details] [review]
new mpegvideodemux plugin

split off the CC stream and forward the normal mpeg video stream
Comment 60 cjunwang 2015-01-22 07:58:45 UTC
Created attachment 295148 [details] [review]
decodebin skip same type parse element

parser apparently accepts its own output as input. it's possible to have a demuxer behind a parser. add judgement to avoid infinite loop.
Comment 61 cjunwang 2015-01-22 08:00:40 UTC
Created attachment 295149 [details] [review]
new closedcaptionoverlay plugin

accept x-closedcaption data, decode and overlay on top of video
Comment 62 cjunwang 2015-01-22 08:21:06 UTC
updated as maintainer's suggestions, using small mpeg video demuxer and closedcaption overlay element. Most of implementation is same. There is a minor changement. Currently use mpegvideodemux behind mpegvideoparse. mpegvideoparse parses and frames mpeg video data, and do not really change input bitstream.  and in decodebin, there is already reusable logics to skip same type parse element in pipeline.


to test it, use 
gst-launch-1.0 filesrc location=cc.ts ! tsdemux name=d d.video_0021 ! decodebin name=b b. ! ccoverlay name=over ! videoconvert ! ximagesink sync=false b. ! over.

or

gst-launch-1.0 filesrc location=cc.ts ! tsdemux name=d d.video_0021 ! mpegvideoparse ! closedcaptiondemux name=m m.mpegv_src ! queue max-size-buffers=0 max-size-time=0 ! avdec_mpeg2video skip-frame=5 ! ccoverlay name=lay1 ! videoconvert ! ximagesink sync=false m.cc_data_src ! queue max-size-buffers=0 max-size-time=0 ! lay1.
Comment 63 Thiago Sousa Santos 2015-01-29 13:47:58 UTC
Review of attachment 295145 [details] [review]:

::: gst-libs/gst/codecparsers/gstmpegvideoparser.c
@@ +1113,3 @@
+ * Returns: %TRUE if the cc could be parsed correctly, %FALSE otherwize.
+ *
+ * Since: 1.5

Please use 1.6 here as it is the next stable release. 1.5 is going to be the unstable release that will lead to 1.6

@@ +1119,3 @@
+    GstMpegVideoClosedCaption * cc)
+{
+  GstBitReader br;

It seems that you are reading data always aligned to byte boundary so I'd recommend using GstByteReader that is more suitable for the task. Check the API here: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer-libs/html/gstreamer-libs-GstByteReader.html

@@ +1133,3 @@
+  memset (cc, 0, sizeof (GstMpegVideoClosedCaption));
+  gst_bit_reader_peek_bits_uint64 (&br, &temp64, 64);
+  ud_ID = (guint32) ((temp64 >> 32) & 0xFFFFFFFF);

Instead of reading 64 bits at once, it would make more sense to use:
ud_ID = gst_byte_reader_get_uint32_be_unchecked();

Reasons:
1) You only need 32 bits here
2) it will already skip the 32 bits and be ready to read from the next position
3) you already checked that the size is >= 8 above, so you can read the 32 bits with the _unchecked variant to skip the size check.

@@ +1137,3 @@
+  // "GA94" or "DTG1" are the allowed UD identifiers for ATSC A/53 part 4
+  if (0x47413934 == ud_ID) {
+    user_data_type_code = (gint8) ((temp64 & 0xFF000000) >> 24);

here you can read another byte from the stream instead of doing these bitwise operations. It makes the code more readable.

::: gst-libs/gst/codecparsers/gstmpegvideoparser.h
@@ +481,3 @@
+ * TODO:  atsc_reserved_user_data bit string following em_data
+ *
+ * Since: 1.5

Since: 1.6

@@ +502,3 @@
+ * The x-closedcaption type buffer carried data
+ *
+ * Since: 1.5

Since: 1.6
Comment 64 Thiago Sousa Santos 2015-01-29 14:02:41 UTC
Review of attachment 295145 [details] [review]:

::: gst-libs/gst/codecparsers/gstmpegvideoparser.h
@@ +510,3 @@
+  gboolean is_gop;
+  GstMpegVideoClosedCaption cc;
+} CCData;

Is this used? It should have the GstMpegVideo prefix
Comment 65 Thiago Sousa Santos 2015-01-29 14:06:24 UTC
Review of attachment 295146 [details] [review]:

::: gst/videoparsers/gstmpegvideoparse.c
@@ +762,3 @@
   gst_caps_set_simple (caps, "systemstream", G_TYPE_BOOLEAN, FALSE,
+      "parsed", G_TYPE_BOOLEAN, TRUE,
+      "userdata", G_TYPE_BOOLEAN, mpvparse->userdata ? TRUE : FALSE, NULL);

Where is this used? Is this needed? What's the use case?
Comment 66 Thiago Sousa Santos 2015-01-29 15:27:27 UTC
Review of attachment 295147 [details] [review]:

Should it be part of the gst/videoparsers/ anyway? I think it makes sense but I'd like a second opinion here.

Also, you seem to use gst_cc_ as the prefix for the functions. It should be gst_mpeg_video_demux as it is the class name.

::: gst/videoparsers/gstmpegvideodemux.c
@@ +45,3 @@
+ * SECTION:element-mpegvideodemux
+ *
+ * mpegvideodemux subtract closed caption stream from mpeg2 video stream.

It exposes it. So I think the term 'subtract' here is wrong.

@@ +51,3 @@
+ * |[
+ * gst-launch-1.0 filesrc location=clip3.ts ! tsdemux name=d d.video_0021 ! mpegvideoparse name=p ! mpegvideodemux name=m m.mpegv_src ! queue max-size-buffers=0 max-size-time=0 ! avdec_mpeg2video skip-frame=5 ! ccoverlay name=lay1 ! videoconvert ! ximagesink sync=false m.cc_data_src ! queue max-size-buffers=0 max-size-time=0 ! lay1. 
+ * ]|

break this into multiple lines for readability

@@ +70,3 @@
+#include "gstmpegvideodemux.h"
+
+#define GST_LICENSE "LGPL"

This is not used, please remove.

@@ +93,3 @@
+
+static GstStaticPadTemplate mpegv_src_template =
+GST_STATIC_PAD_TEMPLATE ("mpegv_src",

I think just using 'video' here for the name makes more sense.

@@ +103,3 @@
+
+static GstStaticPadTemplate cc_data_src_template =
+GST_STATIC_PAD_TEMPLATE ("cc_data_src",

And use 'closedcaptions' here

@@ +160,3 @@
+      "Demux", "Demux closed caption data from mpeg2 video frame",
+      "Chengjun Wang <cjun.wang@samsung.com>");
+#ifndef STANDALONE_ELEMENT

What is this define?

@@ +199,3 @@
+      gst_event_parse_caps (event, &dmux->caps);
+      structure = gst_caps_get_structure (dmux->caps, 0);
+      if (NULL != (caps_str = gst_structure_to_string (structure))) {

AFAIK caps can't have structures with NULL names, so you don't need this check.

@@ +200,3 @@
+      structure = gst_caps_get_structure (dmux->caps, 0);
+      if (NULL != (caps_str = gst_structure_to_string (structure))) {
+        GST_INFO_OBJECT (dmux, "Received CAPS (%s)", caps_str);

You can simply use GST_INFO_OBJECT (dmux, "Received caps %" GST_PTR_FORMAT, dmux->caps);

@@ +205,3 @@
+        // Preserve the new caps and remove the userdata field...
+        dmux->caps = gst_caps_copy (dmux->caps);
+        dmux->caps = gst_caps_make_writable (dmux->caps);

no need to make writable as you just copied it. It will be the only ref and, hence, writable.

@@ +208,3 @@
+        structure = gst_caps_get_structure (dmux->caps, 0);
+        gst_structure_remove_field (structure, "userdata");
+        gst_pad_set_caps (dmux->mpegv_srcpad, dmux->caps);

Is the removal of the userdata field for decodebin to avoid plugging multiple times the demuxer? Shouldn't it set the field to FALSE instead of removing it? What is the purpose of this field?

@@ +233,3 @@
+  }
+
+  ret_val = gst_pad_event_default (pad, parent, event);

I think you shouldn't call the default handler for every event. For example, for the caps one you are handling it all in this element, so you shouldn't need to escalate to the default handler.

@@ +246,3 @@
+  gst_segment_init (&segment, GST_FORMAT_TIME);
+  GST_INFO_OBJECT (dmux, "Sending segment %" GST_SEGMENT_FORMAT, &segment);
+  pEvent = gst_event_new_segment (&segment);

The element should respect upstream's new segments. Any segment pushed by upstream is being ignored here. This can break seeking, for example.

@@ +380,3 @@
+        gst_buffer_new_wrapped_full (GST_MINI_OBJECT_FLAG_LOCK_READONLY,
+        cc_data, sizeof (CCData), 0, sizeof (CCData), cc_data,
+        cc_data_destroy_notify);

Why do you need CCData to have those extra fields? Can't those be represented with the gstreamer timing and flags information?

So the buffer would only have the real stream data.

@@ +546,3 @@
+  guint32 remain_size = 0;
+
+  if (!dmux->flushing) {

When the pad is flushing gstreamer won't call your chain function, buffers will be discarded automatically for you. So you don't need to have this flushing flag.

@@ +553,3 @@
+     * If need more data to process frame, would save remained data as dmux->residual to parse next time */
+    if (dmux->residual) {
+      GST_ERROR_OBJECT (dmux, "dmux->residual exists");

This is not an ERROR, please use LOG or DEBUG here.

@@ +561,3 @@
+      gst_buffer_copy_into (current_buf, buf, GST_BUFFER_COPY_MEMORY, 0, -1);
+      gst_buffer_unref (dmux->residual);
+      dmux->residual = NULL;

It is simpler to use gst_buffer_append to concatenate the buffers. Or maybe take a look at the GstAdapter API to see if it would help you here.

@@ +577,3 @@
+      GST_BUFFER_PTS (dmux->residual) = GST_BUFFER_PTS (current_buf);
+      GST_BUFFER_DTS (dmux->residual) = GST_BUFFER_DTS (current_buf);
+      gst_buffer_unmap (dmux->residual, &residual_map);

Using GstAdapter can make it easier to manage the input and discard the amount of bytes you want.

@@ +584,3 @@
+
+    if (ret != GST_FLOW_OK) {
+      GST_ERROR_OBJECT (dmux, "Failed to push buffer. GstFlowReturn is %d",

Some flow returns might not be fatal, so avoid using GST_ERROR here. For example, NOT_LINKED returns.

::: gst/videoparsers/gstmpegvideodemux.h
@@ +1,2 @@
+/* GStreamer mpeg2 video closed caption data demux
+  * Copyright (C) 2013 CableLabs, Louisville, CO 80027

I guess the header here needs to be updated.
Comment 67 Thiago Sousa Santos 2015-01-29 15:31:48 UTC
Review of attachment 295149 [details] [review]:

Shouldn't the CEA608 and CEA708 elements be separate? I also believe the x-closecaptions name for the caps isn't a good idea. There might be many formats of closedcaptions. Better to use closedcaptions/cea-708 or subpicture/cea-708 or similar
Comment 68 cjunwang 2015-03-13 02:02:46 UTC
Created attachment 299258 [details] [review]
updated patch "subtract closed caption packet from mpeg video user data field"

refine as maintainer suggestion
Comment 69 cjunwang 2015-03-13 02:05:19 UTC
Created attachment 299259 [details] [review]
update patch "gstmpegvideoparse expose closed caption capability"
Comment 70 cjunwang 2015-03-13 05:30:24 UTC
Created attachment 299261 [details] [review]
updated patch "new mpegvideodemux plugin"
Comment 71 cjunwang 2015-03-13 05:32:39 UTC
Created attachment 299262 [details] [review]
updated patch "new closedcaptionoverlay plugin"
Comment 72 cjunwang 2015-03-13 05:34:03 UTC
Created attachment 299263 [details] [review]
updated patch "decodebin skip same type parse element"
Comment 73 cjunwang 2015-03-13 05:53:27 UTC
Created attachment 299264 [details] [review]
updated patch "decodebin skip same type parse element"
Comment 74 cjunwang 2015-03-13 06:00:37 UTC
refined all patches as suggestions. 
test command like this:
gst-launch-1.0 filesrc location=clip3.ts ! tsdemux name=d d.video_0021 ! mpegvideoparse ! mpegvideodemux name=m m.video ! queue max-size-buffers=0 max-size-time=0 ! avdec_mpeg2video skip-frame=5 ! ceaccoverlay name=lay1 ! videoconvert ! videoscale ! video/x-raw, width=720 ! ximagesink sync=false m.closedcaption ! queue max-size-buffers=0 max-size-time=0 ! lay1.
Comment 75 ozgunb 2017-03-09 07:31:08 UTC
What is the situation of this bug? Is there any improvement til 2015?
Comment 76 Edward Hervey 2018-02-23 10:56:42 UTC
Something went wrong at some point. The cea708decoder handles the CC ... but nothing ever gets displayed.
Comment 77 Edward Hervey 2018-02-23 11:07:43 UTC
The reason seems to be that there are various places where the captions should be outputted/rendered *during* the parsing of the DTVCC packet.

But the way it was refactored it ends up with everything cleared when the DTVCC packet parsing returns ... so there's nothing to display. Ideally there should be a callback that a user of the cea708 decoder could provide where it can then grab the current contents.

Also the actual rendering (or conversion to pango) should be put outside of the actual cea708 decoder. That would be more flexible (instead of being tied to pango-only rendering).
Comment 78 Edward Hervey 2018-03-12 06:54:27 UTC
Created attachment 369561 [details] [review]
ceaccoverlay: New CEA-708 Closed Caption decoder and overlayer

This new plugin allows decoding and overlaying CEA-708 Closed Caption
streams over video.

* Supports CDP and cc_data closedcaption/x-cea-708 streams
* Uses pango to render CC stream
* Support GstVideoOverlayComposition meta if downstream supports is

Tested on various test files.

Remains to be fixed/improved:
* Switch to GstByteReader (for code safety)
* Switch to GString (instead of manual pango string construction)
* Move pango/rendering code outside of main 708 decoder file (so
  that actual CC parser/decoder can be (re)used in other scenarios).

Initial patches and improvements by:
* CableLabs RUIH-RI Team <ruihri@cablelabs.com>
* Steve Maynard <steve@secondstryke.com>
* cjun.wang" <cjun.wang@samsung.com>
Comment 79 Edward Hervey 2018-05-05 09:26:27 UTC
Assigning to myself, have revised patches
Comment 80 Edward Hervey 2018-05-28 13:30:24 UTC
I have pushed the main decoder/overlayer to bad.

I am closing this bug now, any other fixes/improvements should be
opened as other bug reports


Attachment 369561 [details] pushed as a41fb3c - ceaccoverlay: New CEA-708 Closed Caption decoder and overlayer