After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 784295 - timecodestamper: LTC from audio
timecodestamper: LTC from audio
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 783038 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-06-28 15:54 UTC by Georg Lippitsch
Modified: 2018-11-03 14:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
timecodestamper: LTC from audio (24.58 KB, patch)
2017-06-28 15:54 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (25.21 KB, patch)
2017-07-18 20:15 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (26.07 KB, patch)
2017-07-19 13:44 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (26.04 KB, patch)
2017-07-19 14:04 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (26.78 KB, patch)
2017-07-20 09:00 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (28.28 KB, patch)
2017-08-02 15:08 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (28.54 KB, patch)
2017-08-04 11:12 UTC, Georg Lippitsch
none Details | Review
timecodestamper: Add property for adding frames to LTC (5.25 KB, patch)
2017-08-07 12:56 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (29.94 KB, patch)
2017-08-11 09:23 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (31.06 KB, patch)
2017-08-11 13:53 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (31.06 KB, patch)
2017-08-11 13:54 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (35.68 KB, patch)
2017-10-30 18:54 UTC, Georg Lippitsch
none Details | Review
timecodestamper: LTC from audio (36.04 KB, patch)
2018-05-14 16:56 UTC, Georg Lippitsch
none Details | Review

Description Georg Lippitsch 2017-06-28 15:54:07 UTC
Created attachment 354630 [details] [review]
timecodestamper: LTC from audio

Add support for parsing linear time code from
an audio source using libltc

https://github.com/x42/libltc
Comment 1 Sebastian Dröge (slomo) 2017-07-04 07:15:17 UTC
Review of attachment 354630 [details] [review]:

::: 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
Comment 2 Georg Lippitsch 2017-07-18 20:15:24 UTC
Created attachment 355884 [details] [review]
timecodestamper: LTC from audio

(In reply to Sebastian Dröge (slomo) from comment #1)
> Review of attachment 354630 [details] [review] [review]:
> 
> ::: 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?

That's the version I used during development. According to release logs,
newer versions are API and ABI compatible.
See https://github.com/x42/libltc/releases

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

Done with a lot of ugly #if

> 
> ::: gst/timecode/gsttimecodestamper.c
> @@ +88,3 @@
>  
> +static GstStaticPadTemplate gst_timecodestamper_ltc_template =
> +GST_STATIC_PAD_TEMPLATE ("ltc",
> 
> "ltc_sink" ?

Done

> 
> @@ +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?

Yes, I compare audio and video timestamps. If they are within this timeout,
I hold back buffers until they differ less than one frame. Otherwise I
consider LTC as unmatchable to the video.

> 
> @@ +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

Done

> 
> @@ +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

Done

> 
> @@ +549,3 @@
> +    if (ltc_decoder_read (timecodestamper->ltc_dec,
> +            &timecodestamper->ltc_frame) != 1) {
> +      break;
> 
> What does this case mean? Error? No LTC available yet?

No LTC available yet, so try again next frame.

> 
> @@ +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)

done

> 
> @@ +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?

If LTC is already ahead, what do you want to wait for? Then it becomes
even more ahead.
I rather keep the LTC in the queue and use it when the frames catch up.

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

I do hold back frames when LTC is behind. But the other way round
doesn't look particularly useful to me. 

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

Same as above: When it is ahead, it can never match that video buffer.
I certainly need to queue the LTC until the video buffer catch up.
See the next condition: I use the LTC only if it is behind, one frame
at maximum (which is probably the the overlapping you meant).

> 
> @@ +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

Done

> 
> @@ +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

Done

> 
> @@ +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?

Once it has a LTC, it uses it's own counter and only resyncs to the LTC
if there are jumps in it. The reason is that the video and LTC could run
at slightly different speeds.

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

Done

> 
> @@ +725,3 @@
> +      timecodestamper->ltc_total);
> +  timecodestamper->ltc_total += map.size;
> +  gst_buffer_unmap (buffer, &map);
> 
> You're leaking all audio buffers here

Done

> 
> @@ +760,3 @@
> +
> +      timecodestamper->ltc_dec =
> +          ltc_decoder_create (samples_per_frame, DEFAULT_LTC_QUEUE);
> 
> You can get multiple caps events

Done

> 
> @@ +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

Not sure what to forward and what not, but when I don't call
gst_pad_event_default here, nothing seems to work at all anymore.

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

Done

I also fixed a deadlock if video EOS was received before
audio EOS.
Comment 3 Georg Lippitsch 2017-07-19 13:44:05 UTC
Created attachment 355948 [details] [review]
timecodestamper: LTC from audio

Another update of the patch, with audio events not forwarded downstream.
Also, the wait condition is signaled when a pad is deactivated, in addition to FLUSH_START and EOS events.
Comment 4 Georg Lippitsch 2017-07-19 14:04:22 UTC
Created attachment 355949 [details] [review]
timecodestamper: LTC from audio

Fix compilation without libltc
Comment 5 Vivia Nikolaidou 2017-07-19 17:10:14 UTC
Issue found:

Currently, in the LTC chain function, if the timecode source isn't LTC, it's waiting there forever in the g_cond. That fills up all queues upstream, blocking us from receiving any further events and forcing us to sit on the stream lock all the time. That can actually lead to deadlocks.

This fixes it for me:

diff --git a/gst/timecode/gsttimecodestamper.c b/gst/timecode/gsttimecodestamper.c
index 27ca99979..fe3493108 100644
--- a/gst/timecode/gsttimecodestamper.c
+++ b/gst/timecode/gsttimecodestamper.c
@@ -732,6 +732,9 @@ gst_timecodestamper_ltcpad_chain (GstPad * pad,
   if (!timecodestamper->vinfo.fps_n || !timecodestamper->vinfo.fps_d)
     goto beach;
 
+  if (timecodestamper->tc_source != GST_TIME_CODE_STAMPER_LTC)
+    goto beach;
+
   g_mutex_lock (&timecodestamper->ltc_mutex);
 
   brt = gst_segment_to_running_time (&timecodestamper->ltc_segment,
Comment 6 Georg Lippitsch 2017-07-20 09:00:10 UTC
Created attachment 356014 [details] [review]
timecodestamper: LTC from audio

Fix other timecode sources than LTC
Comment 7 Sebastian Dröge (slomo) 2017-07-20 11:33:36 UTC
*** Bug 783038 has been marked as a duplicate of this bug. ***
Comment 8 Vivia Nikolaidou 2017-07-28 10:24:45 UTC
The previously existing functionality "use this timecode source, but if the incoming video stream already has a timecode, don't replace it" seems to be gone with this new option - or is it just me?
Comment 9 Georg Lippitsch 2017-08-02 15:08:56 UTC
Created attachment 356787 [details] [review]
timecodestamper: LTC from audio

New patch adds another option to "timecode-source" to bring back that functionality.
Comment 10 Georg Lippitsch 2017-08-04 11:12:07 UTC
Created attachment 356936 [details] [review]
timecodestamper: LTC from audio

Better sync between LTC and video
Comment 11 Georg Lippitsch 2017-08-07 12:56:16 UTC
Created attachment 357112 [details] [review]
timecodestamper: Add property for adding frames to LTC

Useful if there is an offset between LTC source and video.
Comment 12 Sebastian Dröge (slomo) 2017-08-08 07:57:33 UTC
Review of attachment 357112 [details] [review]:

Generally this seems like a bug in one of the elements, or the code in timecodestamper.

Setting an offset like this should only be needed if there is an actual hardware capture bug (where things are delayed more than signalled, e.g. in your case in ALSA). It's also suspicious that it's *exactly* one frame (is it?). The alsasrc is not capturing buffers of the duration of one video frame.

::: gst/timecode/gsttimecodestamper.c
@@ +213,3 @@
+          "Add this number of frames to LTC timecode, "
+          "useful if there is an offset between your LTC source and video.",
+          G_MININT, G_MAXINT, 0, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Should it be in frames or nanoseconds? Usually we call this property "ts-offset" and it's in gint64/nanoseconds.
Comment 13 Georg Lippitsch 2017-08-09 07:07:24 UTC
LTC and video are perfectly in sync with latest patch and https://bugzilla.gnome.org/show_bug.cgi?id=785740

Anyways, setting a frame offset was a requested feature. Some people want to feed incorrect LTC into the pipeline, and correct it later in software.

Not sure about the nanoseconds though. LTC has a resolution of frames at it's best, so do you think it still makes sense to use nanoseconds here?
Comment 14 Georg Lippitsch 2017-08-11 09:23:57 UTC
Created attachment 357401 [details] [review]
timecodestamper: LTC from audio

Don't acquire object lock anymore
Comment 15 Georg Lippitsch 2017-08-11 13:53:28 UTC
Created attachment 357415 [details] [review]
timecodestamper: LTC from audio

Less incorrect syncing between ltc and video pad
Comment 16 Georg Lippitsch 2017-08-11 13:54:50 UTC
Created attachment 357416 [details] [review]
timecodestamper: LTC from audio

Less incorrect syncing between ltc and video pad
Comment 17 Sebastian Dröge (slomo) 2017-08-15 08:48:04 UTC
Review of attachment 357416 [details] [review]:

::: 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 ( :)
Comment 18 Georg Lippitsch 2017-10-30 18:54:35 UTC
Created attachment 362573 [details] [review]
timecodestamper: LTC from audio

Use gst_audio_stream_align_*
Comment 19 Georg Lippitsch 2018-05-14 16:56:25 UTC
Created attachment 372032 [details] [review]
timecodestamper: LTC from audio

Don't forget to reset internal timecode counter
Comment 20 GStreamer system administrator 2018-11-03 14:10:26 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/577.