GNOME Bugzilla – Bug 766419
videotimecode: Added support for SMPTE time code metadata
Last modified: 2016-08-04 16:09:32 UTC
Can be attached as GstMeta into a video frame.
Created attachment 327861 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch
Created attachment 327889 [details] [review] 0002-videometa-Added-video-time-code-meta.patch
Created attachment 328270 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch Small bugs found and fixed.
Created attachment 328271 [details] [review] 0002-videometa-Added-video-time-code-meta.patch
Created attachment 328272 [details] [review] 0003-timeoverlay-Add-support-to-display-timecode.patch timeoverlay: Add support to display timecode Choosing time-mode=time-code will display the time code attached to the buffer, or 00:00:00:00 if no time code is found.
Created attachment 328273 [details] [review] 0001-decklinkvideosrc-Add-support-for-GstVideoTimeCode.patch decklinkvideosrc: Add support for GstVideoTimeCode The timecode will be fetched from the decklink source and attached to the video buffer.
Created attachment 328274 [details] [review] 0002-decklinkvideosink-Add-support-for-GstVideoTimeCode.patch decklinkvideosink: Add support for GstVideoTimeCode The timecode will be fetched from the video buffer and outputted on the decklink video sink.
Created attachment 328275 [details] [review] 0003-timecodestamper-New-element-to-attach-SMPTE-timecode.patch timecodestamper: New element to attach SMPTE timecode to buffers The timecodestamper element attaches a SMPTE timecode to each video buffer. This timecode corresponds to the current stream time. It is probably wrong on the timecode it attaches to interlaced (mixed) buffers.
Attached more commits (described above, the latest three are for gst-plugins-bad). The timecodestamper element has the "field-number" for interlaced content always set to 0. If someone knows exactly which buffer flags I should check (RFF and ONEFIELD?) and how I can know whether the buffer contains only field 1 or field 2, please let me know. For now I added a FIXME.
Created attachment 328394 [details] [review] 0002-decklinkvideosink-Add-support-for-GstVideoTimeCode.patch Minor fixes for decklinkvideosink. timecodestamper has been tested with the new timeoverlay. Furthermore, the combination of decklinkvideosrc, decklinkvideosink and timeoverlay has been tested with the following pipeline: gst-launch-1.0 decklinkvideosrc mode=11 device-number=1 ! queue ! timeoverlay time-mode=3 halignment=left ! decklinkvideosink mode=11 device-number=2 decklinkvideosrc mode=11 device-number=3 ! queue ! timeoverlay time-mode=3 halignment=right ! xvimagesink where devices 2 and 3 are hardware-looped. The resulting pipeline keeps the timecode correctly all the way from input of device 1 to xvimagesink. We tried with different modes.
Review of attachment 328270 [details] [review]: Just some short review, generally looks good but I'd like also some other opinions about the API and if maybe something is missing here :) ::: gst-libs/gst/video/gstvideotimecode.c @@ +21,3 @@ + +gchar * +gst_video_time_code_to_string (const GstVideoTimeCode * tc) All the functions need some documentation, and the struct too :) @@ +65,3 @@ + if ((tc->config.flags & GST_VIDEO_TIME_CODE_FLAGS_INTERLACED) + && tc->field_count == 1) + add_us = 1000000 * (tc->frames - 0.5) * tc->config.fps_d / tc->config.fps_n; These calculations should ideally be without floating point arithmetic (1000000*tc->frames - 500000) @@ +97,3 @@ + * minutes < 60 + * seconds < 60 + * this can't overflow */ Should probably get some g_return_val_if_fail() in the constructor, and also some checks for those in the various functions. To ensure that these invariants are indeed true @@ +110,3 @@ + +void +gst_video_time_code_add_frames (GstVideoTimeCode * tc, gint64 frames) Can frames be negative? Or will the calculations fall apart then? @@ +121,3 @@ + gst_util_fraction_to_double (tc->config.fps_n, tc->config.fps_d, &ff); + if (tc->config.fps_d == 1001) { + ff_nom = tc->config.fps_n / 1000; This would do an integer division but you probably want float division? @@ +123,3 @@ + ff_nom = tc->config.fps_n / 1000; + } else { + ff_nom = ff; All the things with ff here should ideally be done with integer arithmetic. Just keep the numerator and denominator and calculate with those :) There's also gst_utils API for that @@ +215,3 @@ + str1, str2); + g_free (str1); + g_free (str2); The string stuff here should be in a #ifndef GST_DISABLE_GST_DEBUG block @@ +217,3 @@ + g_free (str2); + if (tc1->hours > tc2->hours) { + ret = 1; You can keep the indentation level much lower by just doing return 1; and return -1; instead of handling the == case in the else part @@ +272,3 @@ +gst_video_time_code_new (guint hours, guint minutes, guint seconds, + guint frames, guint field_count, guint fps_n, guint fps_d, + GDateTime * latest_daily_jam, GstVideoTimeCodeFlags flags) Should document that latest_daily_jam is stolen from the caller @@ +286,3 @@ +gst_video_time_code_init (GstVideoTimeCode * tc, guint hours, guint minutes, + guint seconds, guint frames, guint field_count, guint fps_n, guint fps_d, + GDateTime * latest_daily_jam, GstVideoTimeCodeFlags flags) Also here ::: gst-libs/gst/video/gstvideotimecode.h @@ +26,3 @@ + +typedef struct _GstVideoTimeCodeConfig GstVideoTimeCodeConfig; +typedef struct _GstVideoTimeCode GstVideoTimeCode; As mentioned earlier, these need boxed types to be registered for them, which then needs copy/free functions @@ +30,3 @@ +typedef enum +{ + GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME = (1<<0), Drop frame can be detected based on the framerate, right? @@ +36,3 @@ +struct _GstVideoTimeCodeConfig { + guint fps_n; + guint fps_d; Do timecodes make sense with arbitrary framerates? Not in your code I think, so maybe this should be an enum instead with conversion functions from/to fractions? @@ +48,3 @@ + guint seconds; + guint frames; + guint field_count; It should be documented that for progressive this is always 0, for interlaced it will be 1 or 2
Created attachment 328786 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch Thanks for your comments. Please review the updated version of the patch. I also added a function to return the number of frames since daily jam, according to internal feedback that I received. Otherwise the header file looks OK to them too. Just a few comments: 1) Drop frame cannot be detected based on the framerate. The same 30000/1001 and 60000/1001 framerates can exist in both drop-frame and non-drop-frame mode, hence the flag. 2) I think that not having an enum simplifies the code. Right now, the only place where arbitrary framerates are not supported are when calculating frames (frames_since_daily_jam and add_frames) and the rest works. Integer framerates are supported everywhere, as well as 30000/1001 and 60000/1001 in both drop-frame and non-drop-frame modes. I decided to print a warning when detecting an unsupported framerate, as well as document the supported ones, instead of adding the complexity of an enum.
Created attachment 328828 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch Small fixes
Created attachment 328829 [details] [review] 0002-videometa-Added-video-time-code-meta.patch Small fix
Created attachment 328830 [details] [review] 0003-timecodestamper-New-element-to-attach-SMPTE-timecode.patch Small fix
Created attachment 328831 [details] [review] 0004-timecodewait-New-element-to-wait-for-a-specific-time.patch timecodewait: New element to wait for a specific timecode timecodewait receives a timecode as an argument (either as string or as GstVideoTimeCode - one is gst-launch-friendly and the other is code-friendly), and it will drop all audio and video buffers until that timecode has been reached.
Created attachment 330660 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch Typo found and fixed.
Created attachment 330663 [details] [review] 0005-timecodestamper-Add-option-to-get-initial-timecode-f.patch timecodestamper: Add option to get initial timecode from existing clock The user can just give a GstClock instance as an argument and timecodestamper will query that for the initial timecode. An additional option for drop frame is added, which will only take effect in 29.97 and 59.94 FPS.
Created attachment 330712 [details] [review] 0004-timecodewait-New-element-to-wait-for-a-specific-time.patch Small issues found and fixed.
Created attachment 331363 [details] [review] 0004-timecodewait-New-element-to-wait-for-a-specific-time.patch Issues fixed where they belong
Created attachment 331364 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch Do not fail assertions when initializing with an unknown framerate (0/1) Last small issue found and fixed. Please review and feedback.
Review of attachment 331364 [details] [review]: All docs need Since: 1.10 markers ::: gst-libs/gst/video/gstvideotimecode.c @@ +27,3 @@ + * gst_video_time_code_to_string: + * @tc: #GstVideoTimeCode to convert + * Should document what it looks like, and if any, according to which standard (SMPTE something?) @@ +36,3 @@ + gboolean top_dot_present; + gchar sep; + g_return_val_if_fail() if timecode is not valid, e.g. seconds overflow. It does weird things otherwise @@ +69,3 @@ + GDateTime *ret2; + gdouble add_us; + g_return_val_if_fail() @@ +93,3 @@ + && tc->field_count == 1) + add_us = + (1000000 * tc->frames - 500000) * tc->config.fps_d / tc->config.fps_n; G_USEC_PER_SEC and G_USEC_PER_SEC/2 @@ +98,3 @@ + + ret2 = g_date_time_add_seconds (ret, add_us + tc->seconds); + g_free (ret); g_date_time_unref() @@ +100,3 @@ + g_free (ret); + ret = g_date_time_add_minutes (ret2, tc->minutes); + g_free (ret2); g_date_time_unref() @@ +102,3 @@ + g_free (ret2); + ret2 = g_date_time_add_hours (ret, tc->hours); + g_free (ret); g_date_time_unref() @@ +116,3 @@ +gst_video_time_code_nsec_since_daily_jam (const GstVideoTimeCode * tc) +{ + gdouble nsec; g_return_val_if_fail() @@ +131,3 @@ + nsec = + gst_util_uint64_scale (GST_SECOND, + (tc->frames - 0.5) * tc->config.fps_d, tc->config.fps_n); scale(GST_SECOND * frames - 500 * GST_MSECOND, fps_d, fps_n) @@ +134,3 @@ + else + nsec = + gst_util_uint64_scale (GST_SECOND, tc->frames * tc->config.fps_d, scale(GST_SECOND * frames, fps_d, fps_n) for consistency maybe @@ +156,3 @@ +{ + guint ff_nom; + gdouble ff; g_return_val_if_fail() @@ +180,3 @@ + else if (tc->config.fps_n == 60000) + dropframe_multiplier = 4; + else { More {} @@ +223,3 @@ + guint64 h_new, min_new, sec_new, frames_new; + gdouble ff; + guint ff_nom; g_return_val_if_fail() @@ +325,3 @@ +gst_video_time_code_compare (const GstVideoTimeCode * tc1, + const GstVideoTimeCode * tc2) +{ g_return_val_if_fail() @@ +379,3 @@ + + dt1 = gst_video_time_code_to_date_time (tc1); + dt2 = gst_video_time_code_to_date_time (tc2); dt1/dt2 need to be freed @@ +394,3 @@ + * @fps_n: Numerator of the frame rate + * @fps_d: Denominator of the frame rate + * @latest_daily_jam: The latest daily jam of the #GstVideoTimeCode The code has this as (transfer full) but it's not marked as such. It probably makes more sense to just g_date_time_ref() though and have it (transfer none) @@ +425,3 @@ + * @fps_n: Numerator of the frame rate + * @fps_d: Denominator of the frame rate + * @latest_daily_jam: The latest daily jam of the #GstVideoTimeCode Same as above @@ +441,3 @@ + g_return_if_fail (minutes < 60); + g_return_if_fail (seconds < 60); + g_return_if_fail ((frames <= fps_n / fps_d) || (fps_n == 0 && fps_d == 1)); g_return_val_if_fail() with the new validate function :) ::: gst-libs/gst/video/gstvideotimecode.h @@ +37,3 @@ + */ +typedef enum +{ GST_VIDEO_TIME_CODE_FLAGS_NONE = 0 @@ +39,3 @@ +{ + GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME = (1<<0), + GST_VIDEO_TIME_CODE_FLAGS_INTERLACED = (1<<1) ffmpeg has AV_TIMECODE_FLAG_24HOURSMAX and AV_TIMECODE_FLAG_ALLOWNEGATIVE, which both can be added later (TODO comment inside the code). Current behaviour is 24HOURMAX (i.e. we wrap around) and no negatives allowed (should be documented) @@ +47,3 @@ + * @fps_d: Denominator of the frame rate + * @flags: the corresponding #GstVideoTimeCodeFlags + * @latest_daily_jam: The latest daily jam information ... if present or %NULL @@ +50,3 @@ + * + * Supported frame rates: 30000/1001, 60000/1001 (both with and without drop + * frame), and integer frame rates e.g. 25/1, 30/1, 50/1, 60/1. Could be useful to add a gst_video_time_code_config_is_valid() if not all possibilities are valid. Could be used everywhere in g_return_val_if_fail() and also by user code Maybe also a gst_video_time_code_is_valid() that checks for e.g. second overflows, etc. @@ +72,3 @@ + * @field_count must be 0 for progressive video and 1 or 2 for interlaced. + * + * A representation of a SMPTE time code. Should be documented that minutes, seconds, frames, field_count can't have every possible value and are *not* automatically normalized @@ +101,3 @@ +void gst_video_time_code_add_frames (GstVideoTimeCode *tc, gint64 frames); + +GstVideoTimeCode * gst_video_time_code_new (guint hours, guint minutes, guint seconds, guint frames, guint field_count, guint fps_n, guint fps_d, GDateTime * latest_daily_jam, GstVideoTimeCodeFlags flags); Parameter order is inconsistent with the struct definition (which has first the config, then the timecode). The struct order makes more sense (also latest_daily_jam and flags are swapped around). @@ +103,3 @@ +GstVideoTimeCode * gst_video_time_code_new (guint hours, guint minutes, guint seconds, guint frames, guint field_count, guint fps_n, guint fps_d, GDateTime * latest_daily_jam, GstVideoTimeCodeFlags flags); + +GstVideoTimeCode * gst_video_time_code_copy (GstVideoTimeCode * tc); the argument can be const ::: tests/check/libs/videotimecode.c @@ +336,3 @@ + tcase_add_test (tc, videotimecode_addframe_60drop_framedropped); + tcase_add_test (tc, videotimecode_addframe_60drop_wrapover); + tcase_add_test (tc, videotimecode_addframe_loop); Should get a test or two for timecodes with the daily jam and conversion to nsecs, datetime, etc
Review of attachment 328829 [details] [review]: ::: gst-libs/gst/video/gstvideometa.c @@ +653,2 @@ return meta->upload (meta, texture_id); + Useless newline @@ +928,3 @@ +GstVideoTimeCodeMeta * +gst_buffer_add_video_time_code_meta (GstBuffer * buffer, GstVideoTimeCode * tc) +{ g_return_val_if_fail() @@ +941,3 @@ + GstVideoTimeCodeMeta *meta; + + g_return_val_if_fail (GST_IS_BUFFER (buffer), NULL); Check for valid timecode ::: gst-libs/gst/video/gstvideometa.h @@ +300,3 @@ + * GstVideoTimeCodeMeta: + * @meta: parent #GstMeta + * @roi_type: GQuark describing the semantic of the Roi (f.i. a face, a pedestrian) Copy&paste mistake (also "f.i."?!) @@ +304,3 @@ + * @tc: the GstVideoTimeCode to attach + * + * Extra buffer metadata describing the GstVideoTimeCode of the frame Since: 1.10 in the docs everywhere @@ +320,3 @@ + ((GstVideoTimeCodeMeta*)gst_buffer_get_meta((b),GST_VIDEO_TIME_CODE_META_API_TYPE)) +GstVideoTimeCodeMeta *gst_buffer_add_video_time_code_meta (GstBuffer * buffer, + GstVideoTimeCode * tc); While we're at it, indentation is weird. @@ +323,3 @@ + +GstVideoTimeCodeMeta * +gst_buffer_add_video_time_code_meta_full (GstBuffer * buffer, guint hours, guint minutes, guint seconds, guint frames, guint field_count, guint fps_n, guint fps_d, GDateTime * latest_daily_jam, GstVideoTimeCodeFlags flags); Same thing with the order as with gst_video_time_code_new()
Review of attachment 328829 [details] [review]: ::: gst-libs/gst/video/gstvideometa.h @@ +298,3 @@ +/** + * GstVideoTimeCodeMeta: Should document that we assume that elements that add a timecode, add it on every single frame instead of letting downstream worry about interpolating. It's easy enough for producers.
Review of attachment 328273 [details] [review]: Should there also be something for the video sink to send timecodes to the hardware? ::: sys/decklink/gstdecklink.cpp @@ +754,3 @@ + res = + video_frame-> + GetTimecode (GST_DECKLINK_VIDEO_SRC (videosrc)->timecode_format, Put a FIXME comment here, it's not nice that there's this circularity between gstdecklink.cpp and gstdecklinkvideosrc.cpp but also not that much of a problem ::: sys/decklink/gstdecklinkvideosrc.cpp @@ +532,3 @@ CaptureFrame *f; + const GstDecklinkMode *bmode; + GstVideoTimeCodeFlags flags = (GstVideoTimeCodeFlags) 0; GST_VIDEO_TIME_CODE_FLAGS_NONE @@ +533,3 @@ + const GstDecklinkMode *bmode; + GstVideoTimeCodeFlags flags = (GstVideoTimeCodeFlags) 0; + guint field_count = 1; Should be 0 for progressive
Review of attachment 328394 [details] [review]: ... and here the patch for the video sink is :) ::: sys/decklink/gstdecklinkvideosink.cpp @@ +366,3 @@ + flags = bmdVideoOutputVITC; + else + flags = bmdVideoOutputRP188; This needs some documentation: why are all of them either mapping to one or another? Also what happens now if you enable timecode output (which you always do) but never provide timecodes? Does it have different behaviour on the output side, e.g. outputting a zero-timecode now while outputting nothing before?
Review of attachment 328830 [details] [review]: ::: configure.ac @@ +519,3 @@ AG_GST_CHECK_PLUGIN(subenc) AG_GST_CHECK_PLUGIN(stereo) +AG_GST_CHECK_PLUGIN(timecodestamper) Maybe name the plugin "timecode" and combine both new elements in there ::: gst/timecodestamper/gsttimecodestamper.c @@ +105,3 @@ + g_param_spec_boolean ("override-existing", "Override existing timecode", + "If set to true, any existing timecode will be overridden", + FALSE, Make a constant for the default value above @@ +107,3 @@ + FALSE, + (GParamFlags) (G_PARAM_CONSTRUCT | G_PARAM_READWRITE | + G_PARAM_STATIC_STRINGS))); Cast can go away and G_PARAM_CONSTRUCT as well @@ +123,3 @@ +gst_timecodestamper_init (GstTimeCodeStamper * timecodestamper) +{ + timecodestamper->override_existing = FALSE; And use the constant here too @@ +133,3 @@ + GstTimeCodeStamper *timecodestamper = GST_TIME_CODE_STAMPER (object); + + gst_video_time_code_free (timecodestamper->current_tc); You can store the timecode in the instance, i.e. use init() and reset(). Or you have to check for NULL here and set to NULL (dispose can be called multiple times) @@ +145,3 @@ + switch (prop_id) { + case PROP_OVERRIDE_EXISTING: + timecodestamper->override_existing = g_value_get_boolean (value); Maybe a property for the daily jam? @@ +186,3 @@ + if (segment.format != GST_FORMAT_TIME) + return FALSE; + if (timecodestamper->vinfo.fps_n == 0) This should probably be already be rejected when the CAPS event is received? Or is this a "do we have caps" check? If so you want to reset that in GstBaseTransform::stop(). @@ +187,3 @@ + return FALSE; + if (timecodestamper->vinfo.fps_n == 0) + return FALSE; GST_ERROR_OBJECT() here, otherwise this is annoying to debug @@ +192,3 @@ + timecodestamper->vinfo.fps_d * GST_SECOND); + GST_DEBUG_OBJECT (timecodestamper, + "Got %lu frames when segment time is %" GST_TIME_FORMAT, frames, %lu is not portable, use G_GUINT64_FORMAT @@ +224,3 @@ + return FALSE; + timecodestamper->current_tc->config.fps_n = timecodestamper->vinfo.fps_n; + timecodestamper->current_tc->config.fps_d = timecodestamper->vinfo.fps_d; You might want to invalidate the timecode, the frames value at least is weird now. Maybe update the daily jam with the current value or something? @@ +253,3 @@ + stream_time = + gst_segment_to_stream_time (&vfilter->segment, GST_FORMAT_TIME, + buffer->pts); Need to check if timecode_time and stream_time are valid (i.e. not NONE). @@ +257,3 @@ + || (stream_time > timecode_time + && stream_time - timecode_time > GST_SECOND)) { + gchar *tc_str = gst_video_time_code_to_string (timecodestamper->current_tc); We really need a GST_TIME_CODE_FORMAT ;)
Review of attachment 331363 [details] [review]: ::: gst/timecodewait/gsttimecodewait.c @@ +126,3 @@ + g_param_spec_string ("target-timecode-str", "Target timecode (string)", + "Timecode to wait for (string). Must take the form 00:00:00:00", + "00:00:00:00", G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); #define for the default @@ +129,3 @@ + + g_object_class_install_property (gobject_class, PROP_TARGET_TIME_CODE_OBJ, + g_param_spec_boxed ("target-timecode-obj", "Target timecode (object)", I would call them target-timecode and target-timecode-string @@ +254,3 @@ + GstTimeCodeWait *self = GST_TIMECODEWAIT (object); + + self->running_time_of_timecode = GST_CLOCK_TIME_NONE; No need to set this to NONE @@ +255,3 @@ + + self->running_time_of_timecode = GST_CLOCK_TIME_NONE; + if (self->tc) You can also store here the timecode directly and use init/reset @@ +275,3 @@ + g_value_take_string (value, gst_video_time_code_to_string (self->tc)); + else + g_value_set_string (value, "00:00:00:00"); Why not NULL as default? @@ +282,3 @@ + g_value_set_boxed (value, self->tc); + else + g_value_set_boxed (value, NULL); The if is redundant @@ +304,3 @@ + + tc_str = g_value_get_string (value); + parts = g_strsplit (tc_str, ":", 4); Maybe all this should become a gst_video_time_code_from_string()? @@ +328,3 @@ + } + case PROP_TARGET_TIME_CODE_OBJ:{ + self->tc = g_value_get_boxed (value); dup_boxed() and check if we already have one (and if so, unref) @@ +348,3 @@ + case GST_EVENT_SEGMENT: + g_mutex_lock (&self->mutex); + g_mutex_unlock (&self->mutex); Why? The unlock probably should be moved a bit lower? @@ +351,3 @@ + gst_event_copy_segment (event, &self->vsegment); + if (self->vsegment.format != GST_FORMAT_TIME) + return FALSE; GST_ERROR_OBJECT() @@ +365,3 @@ + g_mutex_lock (&self->mutex); + gst_segment_init (&self->vsegment, GST_FORMAT_UNDEFINED); + g_cond_signal (&self->cond); Signalling and unlocking should probably be in FLUSH_START? @@ +373,3 @@ + gst_event_parse_caps (event, &caps); + GST_DEBUG_OBJECT (self, "Got caps %" GST_PTR_FORMAT, caps); + if (!gst_video_info_from_caps (&self->vinfo, caps)) Maybe needs mutex? Or can it only ever be used and be changed in the streaming thread of this very pad? @@ +396,3 @@ + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_SEGMENT: + self->running_time_of_timecode = GST_CLOCK_TIME_NONE; Maybe needs mutex? Or can it only ever be used and be changed in the streaming thread of this very pad? @@ +399,3 @@ + gst_event_copy_segment (event, &self->asegment); + if (self->asegment.format != GST_FORMAT_TIME) + return FALSE; GST_ERROR_OBJECT() @@ +442,3 @@ + timestamp = GST_BUFFER_TIMESTAMP (inbuf); + g_mutex_lock (&self->mutex); + self->vsegment.position = timestamp; timestamp might be NONE (and you want to return GST_FLOW_ERROR) @@ +482,3 @@ + GstClockTime running_time_at_end = GST_CLOCK_TIME_NONE; + + timestamp = GST_BUFFER_TIMESTAMP (inbuf); timestamp might be NONE (and you want to return GST_FLOW_ERROR) @@ +487,3 @@ + current_running_time = + gst_segment_to_running_time (&self->asegment, GST_FORMAT_TIME, + self->asegment.position); Need to check if current_running_time is valid @@ +491,3 @@ + video_running_time = + gst_segment_to_running_time (&self->vsegment, GST_FORMAT_TIME, + self->vsegment.position); And the same for video_running_time (it might be before segment!) @@ +510,3 @@ + running_time_at_end = + gst_segment_to_running_time (&self->asegment, GST_FORMAT_TIME, + self->asegment.position + duration); And this might become NONE because it goes after segment.stop @@ +544,3 @@ + GstIterator *it = NULL; + GstPad *opad; + GValue val = { 0, }; G_VALUE_INIT
Review of attachment 330663 [details] [review]: Maybe squash together with the other patch ::: gst/timecodestamper/gsttimecodestamper.c @@ +115,3 @@ + FALSE, + (GParamFlags) (G_PARAM_CONSTRUCT | G_PARAM_READWRITE | + G_PARAM_STATIC_STRINGS))); Same as with the other patch @@ +168,3 @@ + break; + case PROP_SOURCE_CLOCK: + timecodestamper->source_clock = g_value_get_object (value); dup_object() and unref any already existing one. And unref in finalize @@ +203,3 @@ + if (timecodestamper->drop_frame && timecodestamper->vinfo.fps_d == 1001 && + (timecodestamper->vinfo.fps_n == 2997 || + timecodestamper->vinfo.fps_d == 5994)) 30000 and 60000 @@ +208,3 @@ + else + timecodestamper->current_tc->config.flags &= + !GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME; ~ instead of ! @@ +336,3 @@ + GstClockTime timecode_time; + + g_assert_nonnull (timecodestamper->current_tc); It is by construction @@ +341,3 @@ + ref_time = + gst_segment_to_stream_time (&vfilter->segment, GST_FORMAT_TIME, + buffer->pts); Times could all be invalid @@ +350,3 @@ + ") has drifted more than one second from stream time %" + GST_TIME_FORMAT, tc_str, GST_TIME_ARGS (timecode_time), + GST_TIME_ARGS (ref_time)); And then? Shouldn't we resync something? Otherwise this will complain for every following frame :)
Created attachment 332567 [details] [review] 0001-videotimecode-Added-support-for-SMPTE-time-code-meta.patch Thanks for your review comments, they are addressed in the new patches. Please feedback.
Created attachment 332568 [details] [review] 0002-videometa-Added-video-time-code-meta.patch
Created attachment 332569 [details] [review] 0003-timeoverlay-Add-support-to-display-timecode.patch
Created attachment 332570 [details] [review] 0001-decklinkvideosrc-Add-support-for-GstVideoTimeCode.patch
Created attachment 332571 [details] [review] 0002-decklinkvideosink-Add-support-for-GstVideoTimeCode.patch
Created attachment 332572 [details] [review] 0003-timecodestamper-New-element-to-attach-SMPTE-timecode.patch
Created attachment 332573 [details] [review] 0004-timecodewait-New-element-to-wait-for-a-specific-time.patch
Pushed with a few small fixups commit 7f7d667e0ff56585b38e5b39876196d690385013 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Aug 4 19:06:45 2016 +0300 videotimecode: Add to docs and exports list commit a993dd40b64c8729f298dd1af055eab2d590a58b Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Wed May 18 19:30:52 2016 +0300 timeoverlay: Add support to display timecode Choosing time-mode=time-code will display the time code attached to the buffer, or 00:00:00:00 if no time code is found. https://bugzilla.gnome.org/show_bug.cgi?id=766419 commit 8d8384f20ad81a58101e6e165bc06082037341f5 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Sat May 14 17:59:20 2016 +0300 videometa: Added video time code meta It attaches a GstVideoTimeCodeMeta (SMPTE timecode) as metadata to a buffer. https://bugzilla.gnome.org/show_bug.cgi?id=766419 commit ab35d7394efb47162195311881ccf84f9a2cc8f9 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Sat May 14 12:20:38 2016 +0300 videotimecode: Added support for SMPTE time code metadata Can be attached as GstMeta into a video frame. https://bugzilla.gnome.org/show_bug.cgi?id=766419
commit c3d8890c2d4935e3748e5b781744f7dd542c3181 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Tue May 31 19:21:17 2016 +0300 timecodewait: New element to wait for a specific timecode timecodewait receives a timecode as an argument (either as string or as GstVideoTimeCode - one is gst-launch-friendly and the other is code-friendly), and it will drop all audio and video buffers until that timecode has been reached. https://bugzilla.gnome.org/show_bug.cgi?id=766419 commit 21f0cd640f233105c90b58928caefd84b7bd1489 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Fri May 20 18:17:52 2016 +0300 timecodestamper: New element to attach SMPTE timecode to buffers The timecodestamper element attaches a SMPTE timecode to each video buffer. This timecode corresponds to the current stream time. https://bugzilla.gnome.org/show_bug.cgi?id=766419 commit a61f608ff9ae8c35f458836d6377b3dac2d053d3 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Sun May 15 17:25:44 2016 +0300 decklinkvideosink: Add support for GstVideoTimeCode The timecode will be fetched from the video buffer and outputted on the decklink video sink. https://bugzilla.gnome.org/show_bug.cgi?id=766419 commit a9cc72a94a90c31e81a5d35ccd729953d5b05ea8 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Sun May 15 16:04:14 2016 +0300 decklinkvideosrc: Add support for GstVideoTimeCode The timecode will be fetched from the decklink source and attached to the video buffer. https://bugzilla.gnome.org/show_bug.cgi?id=766419