Created attachment 360202 [details] [review]
An implementation of the action signal described.

Created attachment 360210 [details] [review]
An implementation of the action signal described. (e-mail corrected)

Review of attachment 360210 [details] [review]:

Thanks for the patch, this looks useful :)

::: gst/multifile/gstsplitmuxsink.c
@@ +299,3 @@
+  /**
+   * GstSplitMuxSink::split-at-running-time:
+   * @splitmux: An action signal that takes a GstClockTime as an argument. When called by the user, the video file is split (and a new one begins) as soon as the pipeline reaches or exceeds the running time specified. An argument of 0 splits the file immediately.

Here describe the arguments to the signal, shortly (and this is also why names are needed)

> @splitmux: the splitmuxsink
> @running_time: running time to split at

And then write some actual documentation in multiple lines saying what this does. It tries to split at that running time next. How does it interact with automatically selected split points (by key-frame, duration, etc)? Maybe there should also be a property to only ever split based on the signal and never automatically

@@ +302,3 @@
+   * 
+   *
+   * Returns: naught.

If it returns nothing, just don't put a "Returns: " line here

Also add a "Since: 1.14" line

@@ +333,2 @@
   GST_OBJECT_FLAG_SET (splitmux, GST_ELEMENT_FLAG_SINK);
+  splitmux->time_to_split = GST_CLOCK_TIME_NONE;

This should also be reset in the state change from PAUSED to READY

@@ +2326,3 @@
+
+static void
+set_time_to_split (GstSplitMuxSink * splitmux, GstClockTime _time_to_split)

No underscore in front of variables

::: gst/multifile/gstsplitmuxsink.h
@@ +174,3 @@
+  
+  /* actions */
+  void     (*split_at_running_time)   (GstSplitMuxSink *, GstClockTime);

We try to give names to the arguments here. While not required it helps understanding what the function pointers actually do.

(In reply to Sebastian Dröge (slomo) from comment #3)
> Maybe
> there should also be a property to only ever split based on the signal and
> never automatically

You mean the default properties of max-size-time=0 and max-size-bytes=0 ? :)

And how would the user/application know when to emit this action signal? pad probes etc? "not our problem"?

I'm thinking of the case where the input is not live, so time might advance fairly quickly, in which case there's a race between the streaming threads and the app thread.

Wonder if we can use/abuse GstController here in some way :)

Created attachment 361042 [details] [review]
An implementation of the action signal described, plus a correction of the e-mail and an implementation of slomo's suggestions.

Only unimplemented suggestion was the "ignore max-size-time, max-size-bytes and max-size-timecode" one, as it would essentially bring those properties back to their default "disabled" state. An option to "back up" those properties to re-enable them at a later time will be implemented upon request.

Any comments on my comment #5 ?

(In reply to Tim-Philipp Müller from comment #5)
> And how would the user/application know when to emit this action signal? pad
> probes etc? "not our problem"?

I'd go with "not our problem". :) The user might be monitoring the progress somehow, e.g. progressreport or some other element or ... etc.

> I'm thinking of the case where the input is not live, so time might advance
> fairly quickly, in which case there's a race between the streaming threads
> and the app thread.

I'd assume that the user is at least vaguely aware of how fast things are progressing.

> Wonder if we can use/abuse GstController here in some way :)

I can't see what you're thinking about, please elaborate? :)

To me this all seems more like a quick hack to make something work rather than doing it properly in a way that will work for all use cases.

Use case: The user is monitoring the progress of the resulting file and arbitrarily decides "I want to split now". I can't think of anything other than an action signal.

Review of attachment 361042 [details] [review]:

We would need a single patch here, that can be committed independently, instead of a history of patches that have to go on top of each other. Please merge the two commits into one and give us the unified patch.

Review of attachment 361042 [details] [review]:

We would need a single patch here, that can be committed independently, instead of a history of patches that have to go on top of each other. Please merge the two commits into one and give us the unified patch.

Review of attachment 361042 [details] [review]:

We would need a single patch here, that can be committed independently, instead of a history of patches that have to go on top of each other. Please merge the two commits into one and give us the unified patch.

We could use a bool "trigger" property instead of a signal for this. Setting the property to TRUE will do the thing *now* and immediately reset it to FALSE again.
Then GstController can be used for setting the times when it should trigger.

Main problem with this is that
a) GstController is stream-time based but who needs stream-time here really, you need running-time
b) Triggers need to be installed on the very exact timestamp where the frame starts


One could also add a property with a time value, for the next cut position. But that's not controller friendly and basically has the same problems as the signal now, just being more ugly.


Or we could just not add any API at all, and use a input buffer flag for marking "cut on this input buffer", or a custom serialized event. But is marking it on the input side enough or does it have to be marked on the output side?
One could allow the event to be send on the sink pads and srcpads (upstream+downstream event).


I think whatever solution we come up here, it will require applications to know what they're doing for everything but the "cut now" case.

(In reply to Sebastian Dröge (slomo) from comment #14)
> I think whatever solution we come up here, it will require applications to
> know what they're doing for everything but the "cut now" case.

Note that currently we're mostly interested in the "cut now" case. The "cut not-now" case is just something that we saw as an added bonus. For me, keeping only the split-now action signal and ditching the other one would also be a viable option. The advantage would be that it would require fewer man-hours to get it accepted.

I believe on IRC vivia said that the "cut now" case is all they needed. If that's the case, then I'm fine with a simple action signal.

It was the time parameter bit which I don't think is not very nice. As for GstController limitations, we could make it trigger things in a different time domain surely if we wanted to? (Not that I'm insisting on GstController, just saying.) It seems more akin to how we send force keyframe requests to video encoders, one should be able to send/queue multiple pre-arranged cut points to splitmuxsink imho, if this functionality is needed.

As discussed on IRC: The split-at-running-time signal will be replaced with a split-now signal, which will internally work with a boolean.

Created attachment 362432 [details] [review]
Removed "split at running time", implemented "split now" instead.

Comment on attachment 362432 [details] [review]
Removed "split at running time", implemented "split now" instead.

Thanks.

Probably needs either some locking (since it will be set from the app thread and read/cleared from a streaming thread)? Or g_atomic_int_{set|get}

Created attachment 363378 [details] [review]
An implementation of the action signal described.

Added locking.

Comment on attachment 363378 [details] [review]
An implementation of the action signal described.

>+  /**
>+   * GstSplitMuxSink::split-now:
>+   * @splitmux: the #GstSplitMuxSink
>+   * @split-now: A flag about whether to split the file or not.
>+   * 
>+   * When called by the user, this action signal splits the video file (and begins a new one) immediately. 
>+   * 
>+   * 
>+   * Since: 1.14
>+   */

What is the purpose of the split-now boolean?

Why would anyone call the "split-now" action signal if they didn't want to split the file? Isn't TRUE implied by the fact that the user emitted the signal in the first place?

Comment on attachment 363378 [details] [review]
An implementation of the action signal described.

Some more nitpicking :)


>@@ -367,6 +389,7 @@ gst_splitmux_sink_finalize (GObject * object)
>   g_list_foreach (splitmux->contexts, (GFunc) mq_stream_ctx_unref, NULL);
>   g_list_free (splitmux->contexts);
> 
>+
>   G_OBJECT_CLASS (parent_class)->finalize (object);
> }

Please remove the extra newline added here.


>@@ -1227,7 +1250,11 @@ handle_gathered_gop (GstSplitMuxSink * splitmux)
>   /* Timecode-based threshold accounts for possible rounding errors:
>    * 5us should be bigger than all possible rounding errors but nowhere near
>    * big enough to skip to another frame */
>-  if ((splitmux->fragment_total_bytes > 0 &&
>+
>+  GST_SPLITMUX_LOCK (splitmux);
>+
>+  if (splitmux->split_now ||
>+      (splitmux->fragment_total_bytes > 0 &&
>           ((splitmux->threshold_bytes > 0 &&
>                   queued_bytes > splitmux->threshold_bytes) ||
>               (splitmux->threshold_time > 0 &&
>@@ -1236,6 +1263,7 @@ handle_gathered_gop (GstSplitMuxSink * splitmux)
>                   splitmux->reference_ctx->in_running_time >
>                   splitmux->next_max_tc_time + 5 * GST_USECOND)))) {
> 
>+    splitmux->split_now = FALSE;
>     /* Tell the output side to start a new fragment */
>     GST_INFO_OBJECT (splitmux,
>         "This GOP (dur %" GST_STIME_FORMAT
>@@ -1259,6 +1287,8 @@ handle_gathered_gop (GstSplitMuxSink * splitmux)
>     gst_buffer_replace (&splitmux->reference_ctx->prev_in_keyframe, NULL);
>   }
> 
>+  GST_SPLITMUX_UNLOCK (splitmux);

We're taking the object lock for a very long time here, and also while we might call request_next_keyframe() which sends an event upstream. It's usually not a good idea to hold locks while calling to outside code, although I didn't check whether it is actually a problem here.

I wonder if using g_atomic_int_{set|get} might be easier here, instead of the lock (gboolean is typedefed to int).

>@@ -2298,3 +2331,13 @@ gst_splitmux_sink_ensure_max_files (GstSplitMuxSink * splitmux)
>     splitmux->fragment_id = 0;
>   }
> }
>+
>+
>+
>+static void
>+split_now (GstSplitMuxSink * splitmux, gboolean split_now)
>+{

One empty line between the functions is enough :)

Not the object lock, but still the splitmux lock.

Suggestions implemented and nits picked. Patch coming later. Anything else I should check?

It's an iterative process - please just attach the patch and then we'll have another look :)

Created attachment 364265 [details] [review]
An implementation of the action signal described.

Nits picked.

Comment on attachment 364265 [details] [review]
An implementation of the action signal described.

Patch does not apply to git master, could you rebase it please?

Created attachment 365269 [details] [review]
splitmuxsink: added a "split now" action signal

Now, the video file can be split at an arbitrary time chosen by the user.

Rebased. Was a nice idea to factor out that huge unreadable if into a new function :)

Created attachment 373772 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

A requirement turned up where we have to actually use the split-at-running-time action signal, like in the original patch. Rebased, fixed some minor things, and added here. I kept the original author's name and email address as the commit author.

Note that there is still the possibility of a race condition, where the target running time is reached before the action signal is completed. As discussed on IRC, I added a comment that suggests to call it from a place that will prevent further buffers from flowing into splitmuxsink, such as a pad probe.

Review of attachment 373772 [details] [review]:

::: gst/multifile/gstsplitmuxsink.c
@@ +429,3 @@
+   * Note that this is prone to race conditions, where said running time is
+   * reached and surpassed before we had a chance to split. The file will
+   * still split, but in order to make sure that the split doesn't happen too

Maybe mention *when* it will be split then (on the next opportunity, ASAP). Otherwise one might think that it will split on the next fragment only maybe :)

@@ +1869,3 @@
+  if (splitmux->reference_ctx->in_running_time >= time_to_split) {
+    GST_OBJECT_LOCK (splitmux);
+    splitmux->time_to_split = GST_CLOCK_TIME_NONE;

This is racy: Between taking the local copy of this value above, and resetting it here the signal could've been called again. You would then overwrite a *new* value here where splitting would happen.

Maybe just compare against the local version and only reset if it a) changed and b) the changed value is after the current split position (i.e. in the future, we still have the opportunity to split there)

Review of attachment 373772 [details] [review]:

::: gst/multifile/gstsplitmuxsink.c
@@ +3089,3 @@
+{
+  GST_SPLITMUX_LOCK (splitmux);
+  splitmux->time_to_split = split_time;

Request a keyframe here!

Also use GQueue (or GstQueueArray or something) so that we can queue up splitting points.

Created attachment 373792 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

Thanks, please review again.

(In reply to Sebastian Dröge (slomo) from comment #32)
> Review of attachment 373772 [details] [review] [review]:
> 
> ::: gst/multifile/gstsplitmuxsink.c
> @@ +1869,3 @@
> +  if (splitmux->reference_ctx->in_running_time >= time_to_split) {
> +    GST_OBJECT_LOCK (splitmux);
> +    splitmux->time_to_split = GST_CLOCK_TIME_NONE;
> 
> This is racy: Between taking the local copy of this value above, and
> resetting it here the signal could've been called again. You would then
> overwrite a *new* value here where splitting would happen.
> 
> Maybe just compare against the local version and only reset if it a) changed
> and b) the changed value is after the current split position (i.e. in the
> future, we still have the opportunity to split there)

Using a GstQueueArray now to queue up possible split times. This shouldn't be an issue.

Note that the new version of the patch is using https://bugzilla.gnome.org/show_bug.cgi?id=797218 .

Created attachment 373794 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

18:19 < thaytan> vivia, then I think your problem might be that the 'need_new_fragment' assessment is being applied to the most recently collected GOP
18:20 < thaytan> which we know because we just received a new keyframe on the input - so the input_running_time at that point is the start of the next GOP, but you're making a decision about whether the previous GOP should be passed or a new fragment started
18:23 < vivia> thaytan: oh, so I was comparing against the keyframe that's after the gop we just collected?
18:23 < thaytan> yes
18:24 < vivia> ok, in that case in_running_time > time_to_split is correct :)

Review of attachment 373794 [details] [review]:

::: gst/multifile/gstsplitmuxsink.c
@@ +1885,3 @@
+    while (ptr_to_time) {
+      time_to_split = *ptr_to_time;
+      if (splitmux->reference_ctx->in_running_time >= time_to_split) {

Above is >, here is >=. Is that correct?

@@ +1886,3 @@
+      time_to_split = *ptr_to_time;
+      if (splitmux->reference_ctx->in_running_time >= time_to_split) {
+        gst_queue_array_pop_head_struct (splitmux->times_to_split);

You need to do

> ptr_to_time = gst_queue_array_peek_head_struct (splitmux->times_to_split);

here again, to peek at the next one and then check if that is also in the past

@@ +3117,3 @@
+    GST_INFO_OBJECT (splitmux, "Requesting next keyframe at %" GST_TIME_FORMAT,
+        GST_TIME_ARGS (split_time));
+    if (!gst_pad_push_event (splitmux->reference_ctx->sinkpad, ev)) {

You want to release the lock before sending the event

Created attachment 373803 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

Review of attachment 373803 [details] [review]:

::: gst/multifile/gstsplitmuxsink.c
@@ +1890,3 @@
+        break;
+      }
+      ptr_to_time = gst_queue_array_peek_head_struct (splitmux->times_to_split);

Simpler:

while {
  if (y < x)
    break;

  pop();
  ptr_to_time = peek();
}

@@ +3116,3 @@
+    GstEvent *ev =
+        gst_video_event_new_upstream_force_key_unit (split_time, TRUE, 0);
+    GST_SPLITMUX_UNLOCK (splitmux);

You can simply unlock before the if

Created attachment 373804 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

Review of attachment 373804 [details] [review]:

Looks good

Created attachment 373805 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

Rebased.

Created attachment 373806 [details] [review]
splitmuxsink: Added a split-at-running-time action signal

The video file can now be split at an arbitrary time, given by the user
as an argument to the action signal.

Review of attachment 373806 [details] [review]:

commit 6fe214e7a9f40212d0eb10a2d5d579414b7b8e31 (HEAD -> master, origin/master, origin/HEAD)
Author: John Nikolaides <jnikolaides@toolsonair.com>
Date:   Wed Sep 26 17:43:05 2018 +0300

    splitmuxsink: Added a split-at-running-time action signal

    The video file can now be split at an arbitrary time, given by the user
    as an argument to the action signal.

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