[Bug 766419] videotimecode: Added support for SMPTE time code metadata

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=766419

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #331364|none                        |needs-work
             status|                            |

--- Comment #22 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 331364:
 --> (https://bugzilla.gnome.org/review?bug=766419&attachment=331364)

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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.