[Bug 704881] Partial ATSC_user_data and CEA-708 Closed Captions Support

Thu Jan 29 07:27:35 PST 2015

https://bugzilla.gnome.org/show_bug.cgi?id=704881
  GStreamer | gst-plugins-bad | git master

Thiago Sousa Santos <thiagossantos> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #295147|none                        |needs-work
             status|                            |

--- Comment #66 from Thiago Sousa Santos <thiagossantos at gmail.com> 2015-01-29 15:27:27 UTC ---
Review of attachment 295147:
 --> (https://bugzilla.gnome.org/review?bug=704881&attachment=295147)

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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.