GNOME Bugzilla – Bug 774209
splitmuxsink: Add option for timecode-based split
Last modified: 2017-01-12 23:44:19 UTC
If this option is given, it will also attach the next target timecode to the force key event.
Created attachment 339489 [details] [review] 0001-splitmuxsink-Add-option-for-timecode-based-split.patch
Created attachment 339614 [details] [review] 0001-splitmuxsink-Add-option-for-timecode-based-split.patch
Created attachment 340050 [details] [review] 0001-splitmuxsink-Add-option-for-timecode-based-split.patch Turns out that the timecode track isn't to be trusted, it may wrap around. For timecode-based splitting, we just calculate the timecode-based duration of the chunk (which is complicated for drop-frame cases) and split based on that duration in nsec. Timecode-based splitting will override time-based splitting for requesting a keyframe (should it? should we just request both?)
Review of attachment 340050 [details] [review]: I think this is not quite ready. Also, I'm working on a general rework of the way splitmuxsink handles buffering to make everything more deterministic. It might be worth holding off on reworking this patch until that's done. ::: gst/multifile/gstsplitmuxsink.c @@ +668,3 @@ + + parts = g_strsplit (splitmux->threshold_timecode_str, ":", 4); + if (!parts || parts[3] == NULL) { You shouldn't assume the array is large enough for this to be safe. Use g_strv_length() to check the number of entries. @@ +728,3 @@ + GST_DEBUG_OBJECT (splitmux, "Next max TC time: %" GST_TIME_FORMAT, + GST_TIME_ARGS (next_max_tc_time)); + gst_video_time_code_free (target_tc); This code seems like functionality that should be in the timecode manipulation library API? @@ +1099,3 @@ + queued_time > splitmux->threshold_time) || + (splitmux->next_max_tc_time != GST_CLOCK_TIME_NONE && + tc_time > splitmux->next_max_tc_time + 5 * GST_USECOND)))) { What's the 5 microsecond magic number about? @@ +1679,3 @@ if (GST_PAD_PAD_TEMPLATE (pad) && + g_str_equal (GST_PAD_TEMPLATE_NAME_TEMPLATE (GST_PAD_PAD_TEMPLATE + (pad)), "video")) Why are there whitespace changes?
Created attachment 343230 [details] [review] 0001-splitmuxsink-Add-option-for-timecode-based-split.patch Thanks, here's a new patch.
Created attachment 343239 [details] [review] 0001-splitmuxsink-Add-option-for-timecode-based-split.patch Oops, wrong file. This is the correct one :)
Thanks, pushed: commit 05db87de21159a124f744ca314520f74c3517e01 Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Thu Dec 22 17:40:40 2016 +0200 splitmuxsink: Add option for timecode-based split If this option is given, it will calculate the next split point based on timecode difference. https://bugzilla.gnome.org/show_bug.cgi?id=774209