[Bug 758232] Add support for EBU-TT-D / TTML subtitles

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=758232

Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> changed:

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

--- Comment #29 from Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> ---
Review of attachment 329278:
 --> (https://bugzilla.gnome.org/review?bug=758232&attachment=329278)

Mostly trivial suggestions you might as well ignore

::: ext/ttml/gstttmlparse.c
@@ +102,3 @@
+    g_free (ttmlparse->detected_encoding);
+    ttmlparse->detected_encoding = NULL;
+  }

You don't need to check for NULL before g_free()

::: ext/ttml/gstttmlrender.c
@@ +1197,3 @@
+    GST_CAT_ERROR (ttmlrender_debug, "Failed to access memory at index %u.",
+        index);
+    return NULL;

I'm not particularly sure about this one but aren't you leaking a ref if mem is
valid but the map fails?

@@ +1261,3 @@
+
+  for (i = 2; cur; ++i) {
+    if (cur->element->suppress_whitespace) {

At a first glance it seems like the whole block that follows can be simplified
around g_strstrip()

@@ +1554,3 @@
+}
+
+static GstTtmlRenderRenderedImage *

Guess it wouldn't hurt to make this one explicitly "inline"

@@ +2234,3 @@
+    GST_LOG_OBJECT (render, "Text pad not linked");
+    GST_TTML_RENDER_UNLOCK (render);
+    ret = gst_pad_push (render->srcpad, buffer);

Maybe add a goto to #2381 and avoid needlessly nesting the long block that
follows

::: ext/ttml/ttmlparse.c
@@ +112,3 @@
+
+static void
+ttml_style_set_delete (TtmlStyleSet * style_set)

Every call to this helper is preceded by an if(style_set) check. Maybe just do
nothing on !style_set and be done with it?

@@ +722,3 @@
+  if (parent) {
+    GHashTableIter iter;
+    gpointer attr_name, attr_value;

Just return ret on !parent and save the needless nesting of the long block that
follows

@@ +758,3 @@
+       *   - tts:showBackground
+       *   - tts:unicodeBidi
+       */

If you are going to list every special case bellow you might as well say none
is inherited and make the commentary briefer / less redundant

@@ +853,3 @@
+  ttml_style_set_print (element->style_set);
+
+  return FALSE;

You don't need the boolean return if you always return FALSE

@@ +904,3 @@
+  ttml_style_set_print (element->style_set);
+
+  return FALSE;

Ditto

@@ +941,3 @@
+  parent = node->parent->data;
+  element->whitespace_mode = parent->whitespace_mode;
+  return FALSE;

Ditto

@@ +958,3 @@
+  TtmlElement *element = node->data;
+  ttml_delete_element (element);
+  return FALSE;

Yet another time...

@@ +995,3 @@
+  element->begin = MAX (element->begin, window->begin);
+  element->end = MIN (element->end, window->end);
+  return FALSE;

Ditto

@@ +1044,3 @@
+  }
+
+  return FALSE;

Ditto

@@ +1074,3 @@
+  }
+
+  return FALSE;

Ditto

@@ +1117,3 @@
+  }
+
+  return FALSE;

Ditto

@@ +1247,3 @@
+
+  if (element->text
+      && (element->whitespace_mode != TTML_WHITESPACE_MODE_PRESERVE)) {

Maybe just return on ~() ?

@@ +1277,3 @@
+  }
+
+  return FALSE;

Ditto (Always returning FALSE)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.