[Bug 784295] timecodestamper: LTC from audio

[GStreamer (GNOME Bugzilla)]

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

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

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

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

::: gst/timecode/gsttimecodestamper.c
@@ +257,3 @@
+  timecodestamper->ltc_recent_runtime = 0;
+
+  g_mutex_init (&timecodestamper->mutex);

This mutex is used without LTC too now

@@ +274,3 @@
+#if HAVE_LTC
+  g_cond_clear (&timecodestamper->ltc_cond);
+  g_mutex_clear (&timecodestamper->mutex);

And here

@@ +295,3 @@
+    gst_video_time_code_free (timecodestamper->ltc_intern_tc);
+    timecodestamper->ltc_intern_tc = NULL;
+  }

All these timecodes that come from the stream (and not a property) should
probably be cleared already in PAUSED->READY

@@ +321,3 @@
           g_value_dup_boxed (value);
+      timecodestamper->ltc_current_tc->config.latest_daily_jam =
+          timecodestamper->current_tc->config.latest_daily_jam;

latest_daily_jam != NULL ? g_date_time_ref (latest_daily_jam) ? NULL

@@ +458,3 @@
+#if HAVE_LTC
+  g_mutex_lock (&timecodestamper->mutex);
+  timecodestamper->is_eos = TRUE;

This is more a case of "is_flushing", not EOS. Basically you probably want to
distinguish here between flushing/shutdown (same thing really, you immediately
wake up and return GST_FLOW_FLUSHING) and EOS of the LTC pad (you want to stop
waiting for LTC and continue as is)

@@ +600,3 @@
+  timecodestamper->ltc_recent_runtime =
+      gst_segment_to_running_time (&vfilter->segment, GST_FORMAT_TIME,
+      GST_BUFFER_PTS (buffer));

This is not really the LTC running time, but the running time of the video?

@@ +612,3 @@
+      if (!timecodestamper->is_eos) {
+        g_cond_signal (&timecodestamper->ltc_cond);
+        g_cond_wait (&timecodestamper->ltc_cond, &timecodestamper->mutex);

This is unsafe, use two condition variables. And waiting should always be used
in a loop while checking another condition

@@ +620,3 @@
+    frame_runtime = timecodestamper->ltc_recent_runtime +
+        gst_util_uint64_scale_int_ceil (GST_SECOND,
+        timecodestamper->vinfo.fps_d, timecodestamper->vinfo.fps_n * 2);

Comment why you go to the middle of the frame here

@@ +628,3 @@
+    if (ltc_runtime > frame_runtime) {
+      break;
+    }

If flushing after the waiting, you want to return GST_FLOW_FLUSHING immediately

@@ +641,3 @@
+      fps_n_div = timecodestamper->vinfo.fps_n /
+          timecodestamper->vinfo.fps_d > 30 ? 2 : 1;
+      gst_video_time_code_init (&ltc_intern_tc,

Need to clear this one again later, otherwise you might leak at least the daily
jam

@@ +657,3 @@
+            gst_video_time_code_copy (&ltc_intern_tc);
+
+        gst_video_time_code_init (timecodestamper->ltc_current_tc,

And this

@@ +736,1 @@
   }

You probably also want to free tc if !post_messages && tc

@@ +746,3 @@
+
+  timecodestamper->ltcpad = gst_pad_new_from_static_template
+      (&gst_timecodestamper_ltc_template, "ltc");

If without LTC support, this should probably just return NULL (and the pad
template not be installed)

@@ +772,3 @@
+    timecodestamper->ltcpad = NULL;
+
+#if HAVE_LTC

You also want to wake up both chain functions here

@@ +813,3 @@
+
+  if (timecodestamper->ltc_total == 0) {
+    timecodestamper->ltc_first_runtime = brt;

You need to do some detection for discontinuities/drifts here. Just taking the
first timestamp ever and then counting samples is not reliable. Check e.g. the
code in audiobuffersplit.

@@ +819,3 @@
+      !timecodestamper->is_eos) {
+    g_cond_signal (&timecodestamper->ltc_cond);
+    g_cond_wait (&timecodestamper->ltc_cond, &timecodestamper->mutex);

This is not safe. You need to use two different condition variables

Also, as you're also waiting in the ltc pad chain function, you need to unlock
it too when shutting down (when deactivating the ltc pad for example,
activatemode function)

@@ +822,3 @@
+  }
+
+  if (GST_BUFFER_DURATION (buffer) > GST_SECOND *

Buffers might not have durations (you can calculate the duration), and
timestamps (you can error out then)

@@ +877,3 @@
+        timecodestamper->ltc_dec =
+            ltc_decoder_create (samples_per_frame, DEFAULT_LTC_QUEUE);
+        timecodestamper->ltc_total = 0;

You might get first caps on the ltcpad and then on the videopad

@@ +887,3 @@
+    case GST_EVENT_FLUSH_START:
+    case GST_EVENT_EOS:
+      pad_finished (timecodestamper);

After FLUSH_STOP, you want to accept data again

@@ +930,3 @@
+
+  if (!active) {
+    pad_finished (timecodestamper);

The one on the srcpad too probably

::: gst/timecode/gsttimecodestamper.h
@@ +76,3 @@
   gboolean first_tc_now;
+  gboolean is_eos;
+  GMutex mutex;

Maybe document what this mutex protects, and put it above all the things it
protects

@@ +97,3 @@
 GType gst_timecodestamper_get_type (void);

+GType gst_timecodestamper_source_get_type(void);

Missing space in front of the ( :)

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