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

[GStreamer (bugzilla.gnome.org)]

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

Olivier Crete (Tester) <olivier.crete> changed:

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

--- Comment #9 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-12-09 22:46:29 UTC ---
Review of attachment 263860:
 --> (https://bugzilla.gnome.org/review?bug=704881&attachment=263860)

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

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