GNOME Bugzilla – Bug 779010
videotimecode: Validate for drop-frame correctness
Last modified: 2017-02-23 17:57:10 UTC
Created attachment 346312 [details] [review] videotimecode: Validate for drop-frame correctness In gst_video_time_code_is_valid, also check for invalid ranges when using drop-frame TC. Refactor some code which broke after the check was added.
Review of attachment 346312 [details] [review]: 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
Created attachment 346498 [details] [review] videotimecode: Validate for drop-frame correctness > Why _new() and _free()? Could just stack allocate here for simplicity Creating invalid timecode with new is what I actually want to test, and it's also consistent with the other tests. Other comments are corrected in new version of the patch.
Attachment 346498 [details] pushed as d15ad75 - videotimecode: Validate for drop-frame correctness