GNOME Bugzilla – Bug 784295
timecodestamper: LTC from audio
Last modified: 2018-11-03 14:10:26 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
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
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.
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.
Created attachment 355949 [details] [review] timecodestamper: LTC from audio Fix compilation without libltc
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,
Created attachment 356014 [details] [review] timecodestamper: LTC from audio Fix other timecode sources than LTC
*** Bug 783038 has been marked as a duplicate of this bug. ***
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?
Created attachment 356787 [details] [review] timecodestamper: LTC from audio New patch adds another option to "timecode-source" to bring back that functionality.
Created attachment 356936 [details] [review] timecodestamper: LTC from audio Better sync between LTC and video
Created attachment 357112 [details] [review] timecodestamper: Add property for adding frames to LTC Useful if there is an offset between LTC source and video.
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.
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?
Created attachment 357401 [details] [review] timecodestamper: LTC from audio Don't acquire object lock anymore
Created attachment 357415 [details] [review] timecodestamper: LTC from audio Less incorrect syncing between ltc and video pad
Created attachment 357416 [details] [review] timecodestamper: LTC from audio Less incorrect syncing between ltc and video pad
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 (<c_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 (<c_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 ( :)
Created attachment 362573 [details] [review] timecodestamper: LTC from audio Use gst_audio_stream_align_*
Created attachment 372032 [details] [review] timecodestamper: LTC from audio Don't forget to reset internal timecode counter
-- 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.