After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 787922 - splitmuxsink: add a "split-now" and "split-at-running-time" action signal
splitmuxsink: add a "split-now" and "split-at-running-time" action signal
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 797218
Blocks:
 
 
Reported: 2017-09-19 21:52 UTC by jnikolaides
Modified: 2018-09-28 14:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
An implementation of the action signal described. (4.65 KB, patch)
2017-09-21 15:27 UTC, jnikolaides
none Details | Review
An implementation of the action signal described. (e-mail corrected) (4.54 KB, patch)
2017-09-21 18:20 UTC, jnikolaides
none Details | Review
An implementation of the action signal described, plus a correction of the e-mail and an implementation of slomo's suggestions. (3.62 KB, patch)
2017-10-06 12:40 UTC, jnikolaides
none Details | Review
Removed "split at running time", implemented "split now" instead. (4.13 KB, patch)
2017-10-27 19:11 UTC, jnikolaides
none Details | Review
An implementation of the action signal described. (4.89 KB, patch)
2017-11-10 23:39 UTC, jnikolaides
none Details | Review
An implementation of the action signal described. (4.09 KB, patch)
2017-11-23 12:02 UTC, jnikolaides
needs-work Details | Review
splitmuxsink: added a "split now" action signal (3.93 KB, patch)
2017-12-08 19:17 UTC, Vivia Nikolaidou
committed Details | Review
splitmuxsink: Added a split-at-running-time action signal (5.35 KB, patch)
2018-09-26 14:44 UTC, Vivia Nikolaidou
none Details | Review
splitmuxsink: Added a split-at-running-time action signal (7.16 KB, patch)
2018-09-27 14:53 UTC, Vivia Nikolaidou
none Details | Review
splitmuxsink: Added a split-at-running-time action signal (7.15 KB, patch)
2018-09-27 16:27 UTC, Vivia Nikolaidou
none Details | Review
splitmuxsink: Added a split-at-running-time action signal (7.28 KB, patch)
2018-09-28 11:39 UTC, Vivia Nikolaidou
none Details | Review
splitmuxsink: Added a split-at-running-time action signal (7.34 KB, patch)
2018-09-28 12:26 UTC, Vivia Nikolaidou
none Details | Review
splitmuxsink: Added a split-at-running-time action signal (7.39 KB, patch)
2018-09-28 13:04 UTC, Vivia Nikolaidou
none Details | Review
splitmuxsink: Added a split-at-running-time action signal (7.35 KB, patch)
2018-09-28 13:53 UTC, Vivia Nikolaidou
committed Details | Review

Description jnikolaides 2017-09-19 21:52:37 UTC
Exactly what it says on the tin.
Comment 1 jnikolaides 2017-09-21 15:27:34 UTC
Created attachment 360202 [details] [review]
An implementation of the action signal described.
Comment 2 jnikolaides 2017-09-21 18:20:43 UTC
Created attachment 360210 [details] [review]
An implementation of the action signal described. (e-mail corrected)
Comment 3 Sebastian Dröge (slomo) 2017-09-22 08:52:17 UTC
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.
Comment 4 Vivia Nikolaidou 2017-09-22 11:26:04 UTC
(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 ? :)
Comment 5 Tim-Philipp Müller 2017-09-22 15:56:49 UTC
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 :)
Comment 6 jnikolaides 2017-10-06 12:40:44 UTC
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.
Comment 7 Tim-Philipp Müller 2017-10-06 12:46:33 UTC
Any comments on my comment #5 ?
Comment 8 Vivia Nikolaidou 2017-10-06 12:51:15 UTC
(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? :)
Comment 9 Tim-Philipp Müller 2017-10-06 12:56:59 UTC
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.
Comment 10 Vivia Nikolaidou 2017-10-06 12:58:59 UTC
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.
Comment 11 Vivia Nikolaidou 2017-10-26 12:25:44 UTC
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.
Comment 12 Vivia Nikolaidou 2017-10-26 12:25:45 UTC
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.
Comment 13 Vivia Nikolaidou 2017-10-26 12:25:46 UTC
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.
Comment 14 Sebastian Dröge (slomo) 2017-10-26 12:28:25 UTC
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.
Comment 15 Vivia Nikolaidou 2017-10-26 12:32:04 UTC
(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.
Comment 16 Tim-Philipp Müller 2017-10-26 12:32:57 UTC
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.
Comment 17 Vivia Nikolaidou 2017-10-27 12:58:45 UTC
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.
Comment 18 jnikolaides 2017-10-27 19:11:10 UTC
Created attachment 362432 [details] [review]
Removed "split at running time", implemented "split now" instead.
Comment 19 Tim-Philipp Müller 2017-10-27 19:37:56 UTC
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}
Comment 20 jnikolaides 2017-11-10 23:39:13 UTC
Created attachment 363378 [details] [review]
An implementation of the action signal described.

Added locking.
Comment 21 Tim-Philipp Müller 2017-11-11 10:30:19 UTC
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 22 Tim-Philipp Müller 2017-11-11 10:54:22 UTC
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 :)
Comment 23 Tim-Philipp Müller 2017-11-11 10:55:32 UTC
Not the object lock, but still the splitmux lock.
Comment 24 jnikolaides 2017-11-16 09:31:34 UTC
Suggestions implemented and nits picked. Patch coming later. Anything else I should check?
Comment 25 Tim-Philipp Müller 2017-11-18 10:40:22 UTC
It's an iterative process - please just attach the patch and then we'll have another look :)
Comment 26 jnikolaides 2017-11-23 12:02:10 UTC
Created attachment 364265 [details] [review]
An implementation of the action signal described.

Nits picked.
Comment 27 Tim-Philipp Müller 2017-12-05 14:38:23 UTC
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?
Comment 28 Vivia Nikolaidou 2017-12-08 19:17:06 UTC
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.
Comment 29 Vivia Nikolaidou 2017-12-08 19:21:00 UTC
Rebased. Was a nice idea to factor out that huge unreadable if into a new function :)
Comment 30 Vivia Nikolaidou 2018-09-26 14:44:17 UTC
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.
Comment 31 Vivia Nikolaidou 2018-09-26 14:47:33 UTC
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.
Comment 32 Sebastian Dröge (slomo) 2018-09-26 15:01:29 UTC
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)
Comment 33 Vivia Nikolaidou 2018-09-26 15:04:10 UTC
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!
Comment 34 Vivia Nikolaidou 2018-09-26 15:38:51 UTC
Also use GQueue (or GstQueueArray or something) so that we can queue up splitting points.
Comment 35 Vivia Nikolaidou 2018-09-27 14:53:04 UTC
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.
Comment 36 Vivia Nikolaidou 2018-09-27 14:54:02 UTC
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.
Comment 37 Vivia Nikolaidou 2018-09-27 14:54:29 UTC
Note that the new version of the patch is using https://bugzilla.gnome.org/show_bug.cgi?id=797218 .
Comment 38 Vivia Nikolaidou 2018-09-27 16:27:03 UTC
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.
Comment 39 Vivia Nikolaidou 2018-09-27 16:28:19 UTC
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 :)
Comment 40 Sebastian Dröge (slomo) 2018-09-28 10:40:24 UTC
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
Comment 41 Vivia Nikolaidou 2018-09-28 11:39:06 UTC
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.
Comment 42 Sebastian Dröge (slomo) 2018-09-28 11:59:41 UTC
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
Comment 43 Vivia Nikolaidou 2018-09-28 12:26:43 UTC
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.
Comment 44 Sebastian Dröge (slomo) 2018-09-28 12:31:00 UTC
Review of attachment 373804 [details] [review]:

Looks good
Comment 45 Vivia Nikolaidou 2018-09-28 13:04:20 UTC
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.
Comment 46 Vivia Nikolaidou 2018-09-28 13:04:56 UTC
Rebased.
Comment 47 Vivia Nikolaidou 2018-09-28 13:53:39 UTC
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.
Comment 48 Vivia Nikolaidou 2018-09-28 13:54:36 UTC
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