[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 #263861|none                        |needs-work
             status|                            |

--- Comment #11 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-12-09 23:01:20 UTC ---
Review of attachment 263861:
 --> (https://bugzilla.gnome.org/review?bug=704881&attachment=263861)

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

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