[Bug 784295] timecodestamper: LTC from audio
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Tue Jul 18 20:15:24 UTC 2017
https://bugzilla.gnome.org/show_bug.cgi?id=784295
Georg Lippitsch <glippitsch at toolsonair.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #354630|needs-work |none
status| |
Attachment #354630|0 |1
is obsolete| |
--- Comment #2 from Georg Lippitsch <glippitsch at toolsonair.com> ---
Created attachment 355884
--> https://bugzilla.gnome.org/attachment.cgi?id=355884&action=edit
timecodestamper: LTC from audio
(In reply to Sebastian Dröge (slomo) from comment #1)
> 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?
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
More information about the gstreamer-bugs
mailing list