[Bug 784295] timecodestamper: LTC from audio

[GStreamer (GNOME Bugzilla)]

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.