[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 #328830|none                        |needs-work
             status|                            |

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

::: 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 ;)

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