[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 #328270|none                        |reviewed
             status|                            |

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

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

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