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

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

::: configure.ac
@@ +3351,3 @@
+translit(dnm, m, l) AM_CONDITIONAL(USE_LIBLTC, true)
+AG_GST_CHECK_FEATURE(LIBLTC, [LTC library], libltc, [
+  AG_GST_PKG_CHECK_MODULES(libltc, libltc >= 1.1.4)

Is 1.1.4 what you tested with, or do you use API that only exists since 1.1.4?

As timecodestamper is in the gst directory currently, you should make this
optional

::: gst/timecode/gsttimecodestamper.c
@@ +88,3 @@

+static GstStaticPadTemplate gst_timecodestamper_ltc_template =
+GST_STATIC_PAD_TEMPLATE ("ltc",

"ltc_sink" ?

@@ +193,3 @@
           DEFAULT_FIRST_NOW, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+  g_object_class_install_property (gobject_class, PROP_LTC_OFFSET,
+      g_param_spec_uint64 ("ltc-offset",

This property is basically a timeout? You hold back video buffers for up to
that time for taking timecodes from LTC, and otherwise assume LTC is broken?

@@ +196,3 @@
+          "Maximum offset of LTC to video, in nanoseconds",
+          "Maximum number of nanoseconds the LTC audio may be ahead "
+          "or behind the video. Buffers not in this range are ingnored.",

Typo

@@ +539,3 @@
+  g_mutex_lock (&timecodestamper->ltc_mutex);
+
+  timecodestamper->video_recent_pts = GST_BUFFER_PTS (buffer);

You have to use the running time to compare times between pads, not the buffer
PTS

@@ +549,3 @@
+    if (ltc_decoder_read (timecodestamper->ltc_dec,
+            &timecodestamper->ltc_frame) != 1) {
+      break;

What does this case mean? Error? No LTC available yet?

@@ +555,3 @@
+
+    pts = timecodestamper->ltc_first_pts + GST_SECOND *
+        timecodestamper->ltc_frame.off_start /
timecodestamper->ltc_sample_rate;

gst_util_uint64_scale (GST_SECOND, off_start, sample_rate)

@@ +558,3 @@
+
+    if (pts > GST_BUFFER_PTS (buffer))
+      break;

If LTC is ahead of this video buffer, we break and don't wait for LTC for this
buffer?

I think it would be good to hold back video frames for a while longer
(configurable) to see if we can get LTC for a future video frame can be
retrieved, and then put timecodes on all of them by counting backwards.


Also for that condition: if the PTS of the LTC is > than the PTS of the video,
that condition is not enough. You probably want to test if [PTS;PTS+duration]
of both buffers are overlapping instead.

@@ +561,3 @@
+
+    if (GST_BUFFER_PTS (buffer) - pts < (GST_SECOND *
+            timecodestamper->vinfo.fps_d) / timecodestamper->vinfo.fps_n) {

You might want to round up here

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

Stack allocate this one to prevent useless short-lived allocations

@@ +599,3 @@
+
+    if (timecodestamper->ltc_intern_tc) {
+      gst_video_time_code_increment_frame (timecodestamper->ltc_intern_tc);

This always counts when we don't get LTC for this very frame, otherwise we take
the new timecode from LTC? What is the exact way of operation here?

@@ +692,3 @@
+
+  if (timecodestamper->video_recent_pts &&
+      ABS ((gint64) timecodestamper->video_recent_pts -

ABSDIFF maybe?

@@ +725,3 @@
+      timecodestamper->ltc_total);
+  timecodestamper->ltc_total += map.size;
+  gst_buffer_unmap (buffer, &map);

You're leaking all audio buffers here

@@ +760,3 @@
+
+      timecodestamper->ltc_dec =
+          ltc_decoder_create (samples_per_frame, DEFAULT_LTC_QUEUE);

You can get multiple caps events

@@ +765,3 @@
+      break;
+    default:
+      ret = gst_pad_event_default (pad, parent, event);

You probably don't want to forward FLUSH_*, SEGMENT, STREAM_START, TAG, EOS,
GAP downstream. Maybe just whitelist events that should be forwarded

::: gst/timecode/gsttimecodestamper.h
@@ +45,3 @@
+  TIME_CODE_STAMPER_INTERN,
+  TIME_CODE_STAMPER_EXISTING,
+  TIME_CODE_STAMPER_LTC

GST_TIME_CODE_STAMPER_LTC, etc

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