[Bug 779010] videotimecode: Validate for drop-frame correctness

[GStreamer (GNOME Bugzilla)]

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

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

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

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

Generally looks good to me

::: gst-libs/gst/video/gstvideotimecode.c
@@ +67,3 @@
   g_return_val_if_fail (tc != NULL, FALSE);

+  fr = gst_util_uint64_scale_round (1, tc->config.fps_n, tc->config.fps_d);

Why gst_util_uint64_scale*() if all arguments and the result fits into a normal
int?

@@ +86,3 @@
   }
+  if ((tc->config.flags & GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME) &&
+      tc->minutes % 10 && tc->seconds == 0 && tc->frames < fr / 15) {

This needs a comment I guess :)

@@ +336,3 @@
  * @frames: How many frames to add or subtract
  *
+ * Adds or subtracts @frames amount of frames to @tc. tc needs to

@tc instead of just tc

@@ +544,1 @@
  * @latest_daiy_jam reference is stolen from caller.

typo

@@ +699,1 @@
  * @latest_daiy_jam reference is stolen from caller.

typo

@@ +701,3 @@
  * Initializes @tc with the given values.
+ * The values are not checked for being in a valid range. To see if your
+ * timecode actually has valid content, use #gst_video_time_code_is_valid.

gst_video_time_code_is_valid() instead of #gst_video_time_code_is_valid
(everywhere)

@@ +817,3 @@
       tc_inter->minutes, tc_inter->seconds, tc_inter->frames, 0);
+
+  df = gst_util_uint64_scale_round (1, tc->config.fps_n, tc->config.fps_d) /
15;

Same question here :)

::: tests/check/libs/videotimecode.c
@@ +592,3 @@
+      GST_VIDEO_TIME_CODE_FLAGS_NONE, 1, 67, 4, 5, 0);
+  fail_unless (gst_video_time_code_is_valid (tc) == FALSE);
+  gst_video_time_code_free (tc);

Why _new() and _free()? Could just stack allocate here for simplicity

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