Created attachment 373886 [details] [review]
splitmuxsink: Subtract daily jam offset when day wraps around

For drop-frame framerates, when the expected next max timecode wraps
around at the end of the day, we have to subtract the offset of the
daily jam, otherwise we end up with a duration that's a few frames too
long.

Review of attachment 373886 [details] [review]:

::: gst/multifile/gstsplitmuxsink.c
@@ +1184,3 @@
         splitmux->fragment_start_time;
+
+    if (cur_tc->config.flags | GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME &&

This needs parenthesis

@@ +1208,3 @@
+      next_max_tc_time -=
+          gst_util_uint64_scale (frames_of_daily_jam * GST_SECOND,
+          cur_tc->config.fps_d, cur_tc->config.fps_n);

Shouldn't this be part of some helper function that increments a timecode by a specific amount?

Also wouldn't it be better to do all calculations without the daily jam only based on the frame counter? And here you seem to be subtracting the 2 or 5 frames for every next timecode, not only when it wraps around a day?

Thanks, will update the patch later. Just a few comments for now:

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 373886 [details] [review] [review]:
> 
> ::: gst/multifile/gstsplitmuxsink.c
>
> Also wouldn't it be better to do all calculations without the daily jam only
> based on the frame counter?

It's not always possible. If you're not adding integer multiples of 10 minutes, the amount of frames can depend on the start timecode.

> And here you seem to be subtracting the 2 or 5
> frames for every next timecode, not only when it wraps around a day?

It's under this:

  /* Add fragment_start_time, accounting for wraparound */
  if (target_tc_time >= cur_tc_time) {
[...]
  } else {

That's not visible in the diff though.

And you're totally right about the helper function, gst_video_time_code_add_frames exists and I should have used it. :)

Created attachment 373896 [details] [review]
splitmuxsink: Subtract daily jam offset when day wraps around

For drop-frame framerates, when the expected next max timecode wraps
around at the end of the day, we have to subtract the offset of the
daily jam, otherwise we end up with a duration that's a few frames too
long.

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 373886 [details] [review] [review]:
> 
> ::: gst/multifile/gstsplitmuxsink.c
> @@ +1184,3 @@
>          splitmux->fragment_start_time;
> +
> +    if (cur_tc->config.flags | GST_VIDEO_TIME_CODE_FLAGS_DROP_FRAME &&
> 
> This needs parenthesis

Done.

> @@ +1208,3 @@
> +      next_max_tc_time -=
> +          gst_util_uint64_scale (frames_of_daily_jam * GST_SECOND,
> +          cur_tc->config.fps_d, cur_tc->config.fps_n);
> 
> Shouldn't this be part of some helper function that increments a timecode by
> a specific amount?

Actually not. I looked at the code again and next_max_tc_time is already in ns, so rather than subtracting frames from the timecode, converting again, and doing all the additions and subtractions needed to get to next_max_tc_time, it's easier to just convert the number of frames to ns and subtract it directly.

It won't be an issue though. The timecode has already wrapped around the 24h boundary, so we're not accumulating the error here.

Review of attachment 373896 [details] [review]:

commit 1219712da04ba75cc1e1c080ffbe9a4a2465697f (HEAD -> master, origin/master, origin/HEAD)
Author: Vivia Nikolaidou <vivia@ahiru.eu>
Date:   Tue Oct 9 16:39:11 2018 +0300

    splitmuxsink: Subtract daily jam offset when day wraps around

    For drop-frame framerates, when the expected next max timecode wraps
    around at the end of the day, we have to subtract the offset of the
    daily jam, otherwise we end up with a duration that's a few frames too
    long.

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