Created attachment 250129 [details]
png of current pipe

Created attachment 250130 [details]
png of previous test impl.

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 :)

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.

Created attachment 263859 [details] [review]
unapproved patch

required only to build/test the ccdec and cea708dec patches

Created attachment 263860 [details] [review]
ccdec patch

ccdec element for review

Created attachment 263861 [details] [review]
cea708dec patch

cea708dec element for review

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.

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

Review of attachment 263864 [details] [review]:

All of the actual parsing of the data shouls probably go into the codecparsers library.

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

I wonder if the new elements should use one of the base classes (potentially BaseParse?)

Created attachment 263941 [details] [review]
updated ccdec patch

updated per suggestions

Created attachment 263942 [details] [review]
updated cea708dec patch

updated per suggestions

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)

(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.

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 ?).

I'm no longer at CableLabs - I believe they will reassign this to another engineer...

Hello, any updates here?

It's a useful feature. I'd like to continue work on this bug.

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.

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.

Created attachment 279293 [details] [review]
add pango capability

change cea708dec src pad sometimes to always, add pango-markup capbility to output pango text.

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.

Created attachment 279295 [details] [review]
correct PTS and segment format.

change segment to GST_FORMAT_TIME, and the unit of PTS is nanosecond.

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.

Created attachment 282524 [details] [review]
seperate pango and webvtt display

Created attachment 282525 [details] [review]
enhance CEA708 spec support

support bottom-to-up scroll, and fix 0x0D(\r) command dealing.

Created attachment 282526 [details] [review]
add capability service-number

user can select the service that will be decoded. default is 1 (primary caption service)

Created attachment 282527 [details] [review]
add pango span attributes

support text part's font size,fg/bg color, underline, italics

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.

Created attachment 282530 [details] [review]
fix coredump when additioanl data exist

when video userdata's ATSC_reserved_user_data exist, buffer excceed maximum size

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.

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

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.

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.

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...

cjunwang, can you just give one patch instead for review ? And mark all the others as deprecated.

Thank you, Edward. I will do that.

Created attachment 283171 [details] [review]
cea708 closed caption support

merge all of patches to one

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

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.

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.

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.

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.

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.

The best would be to make it an "overlay" element (takes both video and cc, and outputs overlayed video).

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.

(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.

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.

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?

Not really, as far as I can tell..

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.

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.

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 :)

@slomo: Would the h264demux do with AFD, etc? Add a GstMeta?

Created attachment 295145 [details] [review]
subtract closed caption packet from mpeg video user data field

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

Created attachment 295147 [details] [review]
new mpegvideodemux plugin

split off the CC stream and forward the normal mpeg video stream

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.

Created attachment 295149 [details] [review]
new closedcaptionoverlay plugin

accept x-closedcaption data, decode and overlay on top of video

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.

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

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

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?

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.

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

Created attachment 299258 [details] [review]
updated patch "subtract closed caption packet from mpeg video user data field"

refine as maintainer suggestion

Created attachment 299259 [details] [review]
update patch "gstmpegvideoparse expose closed caption capability"

Created attachment 299261 [details] [review]
updated patch "new mpegvideodemux plugin"

Created attachment 299262 [details] [review]
updated patch "new closedcaptionoverlay plugin"

Created attachment 299263 [details] [review]
updated patch "decodebin skip same type parse element"

Created attachment 299264 [details] [review]
updated patch "decodebin skip same type parse element"

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.

What is the situation of this bug? Is there any improvement til 2015?

Something went wrong at some point. The cea708decoder handles the CC ... but nothing ever gets displayed.

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).

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>

Assigning to myself, have revised patches

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