[Bug 766419] videotimecode: Added support for SMPTE time code metadata

[GStreamer (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

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

--- Comment #28 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 331363:
 --> (https://bugzilla.gnome.org/review?bug=766419&attachment=331363)

::: gst/timecodewait/gsttimecodewait.c
@@ +126,3 @@
+      g_param_spec_string ("target-timecode-str", "Target timecode (string)",
+          "Timecode to wait for (string). Must take the form 00:00:00:00",
+          "00:00:00:00", G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

#define for the default

@@ +129,3 @@
+
+  g_object_class_install_property (gobject_class, PROP_TARGET_TIME_CODE_OBJ,
+      g_param_spec_boxed ("target-timecode-obj", "Target timecode (object)",

I would call them target-timecode and target-timecode-string

@@ +254,3 @@
+  GstTimeCodeWait *self = GST_TIMECODEWAIT (object);
+
+  self->running_time_of_timecode = GST_CLOCK_TIME_NONE;

No need to set this to NONE

@@ +255,3 @@
+
+  self->running_time_of_timecode = GST_CLOCK_TIME_NONE;
+  if (self->tc)

You can also store here the timecode directly and use init/reset

@@ +275,3 @@
+        g_value_take_string (value, gst_video_time_code_to_string (self->tc));
+      else
+        g_value_set_string (value, "00:00:00:00");

Why not NULL as default?

@@ +282,3 @@
+        g_value_set_boxed (value, self->tc);
+      else
+        g_value_set_boxed (value, NULL);

The if is redundant

@@ +304,3 @@
+
+      tc_str = g_value_get_string (value);
+      parts = g_strsplit (tc_str, ":", 4);

Maybe all this should become a gst_video_time_code_from_string()?

@@ +328,3 @@
+    }
+    case PROP_TARGET_TIME_CODE_OBJ:{
+      self->tc = g_value_get_boxed (value);

dup_boxed() and check if we already have one (and if so, unref)

@@ +348,3 @@
+    case GST_EVENT_SEGMENT:
+      g_mutex_lock (&self->mutex);
+      g_mutex_unlock (&self->mutex);

Why? The unlock probably should be moved a bit lower?

@@ +351,3 @@
+      gst_event_copy_segment (event, &self->vsegment);
+      if (self->vsegment.format != GST_FORMAT_TIME)
+        return FALSE;

GST_ERROR_OBJECT()

@@ +365,3 @@
+      g_mutex_lock (&self->mutex);
+      gst_segment_init (&self->vsegment, GST_FORMAT_UNDEFINED);
+      g_cond_signal (&self->cond);

Signalling and unlocking should probably be in FLUSH_START?

@@ +373,3 @@
+      gst_event_parse_caps (event, &caps);
+      GST_DEBUG_OBJECT (self, "Got caps %" GST_PTR_FORMAT, caps);
+      if (!gst_video_info_from_caps (&self->vinfo, caps))

Maybe needs mutex? Or can it only ever be used and be changed in the streaming
thread of this very pad?

@@ +396,3 @@
+  switch (GST_EVENT_TYPE (event)) {
+    case GST_EVENT_SEGMENT:
+      self->running_time_of_timecode = GST_CLOCK_TIME_NONE;

Maybe needs mutex? Or can it only ever be used and be changed in the streaming
thread of this very pad?

@@ +399,3 @@
+      gst_event_copy_segment (event, &self->asegment);
+      if (self->asegment.format != GST_FORMAT_TIME)
+        return FALSE;

GST_ERROR_OBJECT()

@@ +442,3 @@
+  timestamp = GST_BUFFER_TIMESTAMP (inbuf);
+  g_mutex_lock (&self->mutex);
+  self->vsegment.position = timestamp;

timestamp might be NONE (and you want to return GST_FLOW_ERROR)

@@ +482,3 @@
+  GstClockTime running_time_at_end = GST_CLOCK_TIME_NONE;
+
+  timestamp = GST_BUFFER_TIMESTAMP (inbuf);

timestamp might be NONE (and you want to return GST_FLOW_ERROR)

@@ +487,3 @@
+  current_running_time =
+      gst_segment_to_running_time (&self->asegment, GST_FORMAT_TIME,
+      self->asegment.position);

Need to check if current_running_time is valid

@@ +491,3 @@
+    video_running_time =
+        gst_segment_to_running_time (&self->vsegment, GST_FORMAT_TIME,
+        self->vsegment.position);

And the same for video_running_time (it might be before segment!)

@@ +510,3 @@
+    running_time_at_end =
+        gst_segment_to_running_time (&self->asegment, GST_FORMAT_TIME,
+        self->asegment.position + duration);

And this might become NONE because it goes after segment.stop

@@ +544,3 @@
+  GstIterator *it = NULL;
+  GstPad *opad;
+  GValue val = { 0, };

G_VALUE_INIT

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