GNOME Bugzilla – Bug 370836
Looping feature for gnlobject
Last modified: 2018-11-03 12:50:53 UTC
Currently, making loops using gnonlin requires making many gnlsources and placing them one after another in the timeline. This is not optimal because the creation of many gnlsources will use more resources than necessary, and to move or change the source would require updating all the gnlsources. Gnlobject should handle looping by itself like so: 1. Create a gnlfilesource and set media-start to 0, media-stop to 5. 2. Set start to 0 and duration to 15. 3. Now when playing, the section of the file frrm 0 to 5 seconds should be played 3 times. In other words, when the gnlfilesource reaches media-stop (at 5 seconds), it should go back to media-start (0 seconds) and play the file again. This is repeated until duration is exceeded, at which point the gnlfilesource will be silent.
also should have the option to support different looping styles such as pingpong where when it hits the end, it starts playing backwards to the beggining. also, should be able to set loop points that are not nessesarily the start/end of the raw sample, then definite an envlope to overlap the beggining and the end of sample as it loops.
We could do this with an extra property. By default it would stretch time, but we could have other properties for 'repeat', 'ping-pong', etc...
Here's how it should be done (IMHO). First of all : only do it on gnlsource (my brain lost some cells while trying to figure out how to do it for any kind of gnlobject). You want to translate the seek event coming from outside the gnlsource. For that, modify the incoming seek handler in gnlghostpad. That part should be *roughly* straightforward, you basically want to send a seek for the first 'chunk' you want to loop on. Don't forget the seek event might not start from the beginning. It gets slightly trickier for the following chunks. Put an event handler on the controlled object source pad (that is : the pad that you're setting as the target of the gnlghostpad). When you get an event on that pad: * If it's not EOS, just return TRUE (to let the event go through). * If it's EOS: * Figure out if you're done looping, if so return TRUE to let EOS go through * Compute the new seek you need to send upstream * Call g_idle_add with a method that will send that new seek upstream * Return FALSE (you don't want THAT EOS to go through)
(In reply to comment #3) > [...] > * Call g_idle_add with a method that will send that new seek upstream > [...] Does gnonlin require a running GLib mainloop? If not you can't use g_idle_add(), the callback will never be called without a mainloop running for the default main context.
I have a patch for this that is almost done. I just saw this bug and noticed an interesting bit from Ed: "Call g_idle_add with a method that will send that new seek upstream." I'm currently using g_thread and condition variable/mutex. Is that going to make a significant difference? My patch works fine for audio, but does not work for video. I'll spend a bit more time on it. If I can't figure it out, I'll post what I have so far. I added to gnlobject a new property: loop-duration. If this is non-zero, a single loop duration will take loop-duration nanoseconds. If media-duration is less than loop-duration, there will be silence before the loop starts up again. The gnlobject will loop for duration nanoseconds.
(In reply to comment #5) > I have a patch for this that is almost done. I just saw this bug and noticed > an interesting bit from Ed: "Call g_idle_add with a method that will send that > new seek upstream." I'm currently using g_thread and condition variable/mutex. > Is that going to make a significant difference? Well, your approach is better, see bug #595187. The best would probably be, if gnonlin had some kind of global thread pool that always has a thread or two around for dispatching things like this to other threads. The problem with using the default main context is (other than that you require a mainloop running there), that the seeks and everything else could block the application. Why do you need a condition variable btw? To wait in the streaming thread for the seek to be handled? That's likely causing deadlocks
I use a condition variable to notify the seeking thread that a new seek event is ready. The mutex that is paired with the condition variable is only locked to store the event in a local variable (and again, just before g_cond_wait is called). It is not held during the send event. Even if I don't lock the mutex in the seeking thread I still see the same behavior. gst_data_queue_pop is waiting on a condition variable for a buffer to show up and gst_data_queue_push is waiting on a condition variable for a flush to occur or for the queue to not be full. I'll likely post the patch later today instead of just talking about the problems with it :)
Created attachment 168764 [details] [review] A patch adding --loop-duration to gnlobject This is a first crack at adding --loop-duration to gnlobject. This works only for audio. Video will not work. If an adder is in the pipeline, loop-duration does not work. Perhaps related to bug #574408?
Review of attachment 168764 [details] [review]: A couple of minor issues, it would make a lot more sense (and cleaner code) if the handling was done in gnlghostpad and just having to add a boolean regarding how to behave if duration != media_duration. ::: gnl/gnlobject.c @@ +492,3 @@ + || (object->loop_duration > 0 + && ((object->media_start + object->media_duration) != + object->media_start))) { a bit confusing, didn't you mean media_stop at the end ? ::: gnl/gnlobject.h @@ +101,3 @@ gboolean rate_1; + GstClockTime loop_duration; It would be better to re-use media_duration for the loop duration and then just add a gboolean looping variable. ::: gnl/gnlsource.c @@ +26,2 @@ #include "gnl.h" +#include "glib.h" This include is useless imho (should already have been imported by gst) @@ +256,3 @@ + source->priv->loop_probe = + gst_pad_add_event_probe (pad, G_CALLBACK (looping_eos_handler), + source); There already is an event handler in gnlghostpad.c (internalpad_event_function). It would be cleaner to re-use that @@ +636,3 @@ + GnlObject *object = GNL_OBJECT (source); + GST_LOG_OBJECT (source, "Received EOS"); + source->loop_progress += GNL_OBJECT_LOOP_DURATION (object); It would be better to just re-use GnlObject::media_duration ::: gnl/gnlsource.h @@ +50,3 @@ /* controlled source element, acces with gst_bin_[add|remove]_element */ GstElement *element; + GstClockTime loop_progress; It would be nicer if all this code was moved to gnlobject, so one could use it not only in sources, but also in compositions.
Thanks for the detailed review. I've updated the patch with most of your comments. >A couple of minor issues, it would make a lot more sense (and cleaner code) if >the handling was done in gnlghostpad and just having to add a boolean regarding >how to behave if duration != media_duration. I should have been clearer about my goal. I'll try to clarify here. loop-duration is how long one loop will take. If this is longer than media-duration, there will be silence after the clip plays for (loop-duration - media-duration) on every single pass through the loop. The total amount of time the gnlobject will loop is specified by duration. If we have a boolean regarding how to behave if duration != media_duration, how can we specify how much silence to have between loop iterations? My use case for this is looping for a metronome sound. loop-duration is the length of the beat. I'll submit another patch once we come to agreement on this.
(In reply to comment #10) > Thanks for the detailed review. I've updated the patch with most of your > comments. > > >A couple of minor issues, it would make a lot more sense (and cleaner code) if > >the handling was done in gnlghostpad and just having to add a boolean regarding > >how to behave if duration != media_duration. > > I should have been clearer about my goal. I'll try to clarify here. > > loop-duration is how long one loop will take. If this is longer than > media-duration, there will be silence after the clip plays for (loop-duration - > media-duration) on every single pass through the loop. > > The total amount of time the gnlobject will loop is specified by duration. > > If we have a boolean regarding how to behave if duration != media_duration, how > can we specify how much silence to have between loop iterations? My use case > for this is looping for a metronome sound. loop-duration is the length of the > beat. It'll still create a valid stream. If the file your source controls has a 40ms clip (the metronome tick), you can still set the media-duration to whatever you want, either less than that duration, the same... or more (like 1s). Therefore, if you have: * media-duration : 1s * duration : 60s * loop : TRUE it will loop 60 times, and your file will playback every second For the technical details, you will end up with the outgoing stream: * NEWSEGMENT (start:0, stop:40ms, position:0) <= the stop value doesn't matter that much, depending on the decoders/demuxers it will be 40ms (the *real* end of the clip) or 1s (the stop value from the seek)) * DATA (0-40ms) * NEWSEGMENT (0, 40ms, 1s) <== the segment will playback the second loop *at* 1s * DATA (0-40ms) * NEWSEGMENT (0, 40ms, 2s) * .... Or is there something else I'm missing ? > > I'll submit another patch once we come to agreement on this.
What Ed was missing was the ability to take an arbitrary file (say, WholeLottaLove.mp3), take just a snippet of that file and use that as a click for a metronome. This is too specialized of a use case compared to the complexity it adds to the code.
Created attachment 169295 [details] [review] Take 2 for adding -loop to gnlobject
Created attachment 169317 [details] [review] Take 3 for adding -loop to gnlobject initializes loop_thread to NULL
(In reply to comment #12) > What Ed was missing was the ability to take an arbitrary file (say, > WholeLottaLove.mp3), take just a snippet of that file and use that as a click > for a metronome. This is too specialized of a use case compared to the > complexity it adds to the code. you could still do that by: * creating a gnlsource with the segment you need without the looping mode * putting that source in a composition at the position you need * using that composition in the top-level composition with the looping mode on and the required media_duration Ex : you want to use .5s of a clip 2min in it and repeat it for an hour every second: * gnlurisource1 ** media-start : 120s # 2mins it ** media-duration : 0.5s # take 0.5s ** start : 0 # which will go at the beginning of gnlcomposition1 ** duration : 0.5s # for 0.5s ** looping : FALSE # without looping put gnlurisource1 in a composition (gnlcomposition1) * gnlcomposition1 ** media-start : 0 # standard ** media-duration : 1s # Take 1s of the composition ** start : 0 # which will go from the beginning of the parent # composition ** duration : 7200s # for 2hours ** looping : TRUE # and loop every second (media_duration) And then put gnlcomposition1 in your top-level composition.
Review of attachment 169317 [details] [review]: In addition to the comments below, could you create some unit tests to validate proper behaviour ? ::: gnl/gnlghostpad.c @@ +257,3 @@ + if (GNL_OBJECT_LOOP (object)) { + stop = start + GNL_OBJECT_MEDIA_DURATION (object); + } You are not allowed to modify the start/stop values. This must be removed, the time-shifting handling is done solely by modifying the stream position of the newsegment event.... ... which you forgot to do. You need to modify the stream position to reflect the looping: i.e. the first time the stream position will be object.start, the second time it will be object.start + media_duration, the third time object.start + 2xmedia_duration, etc.... @@ +263,3 @@ GST_TIME_FORMAT, GST_TIME_ARGS (start), GST_TIME_ARGS (stop), GST_TIME_ARGS (nstream)); + Please remove the unneeded whitespaces @@ +308,3 @@ + stop = start + GNL_OBJECT_MEDIA_DURATION (object); + } + Same comment as above. @@ +351,3 @@ + keepit = GNL_OBJECT_DURATION (object) > 0 + && GNL_OBJECT_LOOP_PROGRESS (object) >= + GNL_OBJECT_DURATION (object); GNL_OBJECT_DURATION (object) will always be > 0 at this point (else the seek would have failed). @@ +357,3 @@ + GST_SEEK_TYPE_SET, GNL_OBJECT_MEDIA_START (object), + GST_SEEK_TYPE_SET, + GNL_OBJECT_MEDIA_STOP (object)); You need to make sure you don't seek beyond the last loop. ex: with a media_duration of 1s and a duration of 4.5s you need to make sure the last seek has a stop value of MEDIA_START + 0.5s @@ +364,3 @@ + GST_LOG_OBJECT (object, "broadcasting loop seek event"); + g_cond_broadcast (GNL_OBJECT_LOOP_CONDITION (object)); + return FALSE; You need to return TRUE since you did process the event. ::: gnl/gnlobject.c @@ +288,3 @@ + object->loop_progress = 0; + object->loop_condition = g_cond_new (); + object->loop_mutex = g_mutex_new (); Those 4 variables could be moved to gnlghostpad since they're not being used at all by gnlobject.[ch] @@ +495,3 @@ + || (object->loop + && ((object->media_start + object->media_duration) != + object->media_stop))) { if ((A && B) || (C && B)) => if ((A || C) && B) @@ +518,3 @@ + } else { + object->rate = 1; + } It would be cleaner (and faster) to check whether looping is activated before this big if()
Regarding ::: gnl/gnlghostpad.c @@ +257,3 @@ + if (GNL_OBJECT_LOOP (object)) { + stop = start + GNL_OBJECT_MEDIA_DURATION (object); + } If the element is looping, should stop be set to media_start + media_duration in gnlobject.c update_values? The start above is wrong, I think. Other than this, I think I understand everything else in your comments. Thanks again.
Please ignore "the start above is wrong" comment in the previous comment. I was starting to write something, stopped and didn't proofread well before posting.
Created attachment 171231 [details] [review] Take 4 for adding -loop to gnlobject I think this addresses all of your previous comments. Any feedback on adding more rigorous tests are certainly welcome. Before this is really done, seeking needs to be addressed. It will be easier to test this portion once the adder element is sorted out. The adder currently discards the new segments.
Created attachment 171232 [details] [review] Take 5 for adding -loop to gnlobject Forgot to run gst-indent on loop.c I think this addresses all of your previous comments. Any feedback on adding more rigorous tests are certainly welcome. Before this is really done, seeking needs to be addressed. It will be easier to test this portion once the adder element is sorted out. The adder currently discards the new segments.
Edward?
Hello? :) Is anybody planning to review this and integrate it, or is this feature not wanted? The patch probably doesn't even apply anymore against latest git... Some decision is necessary here
The patch is obsolete, but looping is still something that could enhance GNL. At the moment you create multiple sources to do that, which is most likely a waste of resource, unless someone thinks this is enough ?
Comment on attachment 171232 [details] [review] Take 5 for adding -loop to gnlobject This is no longer compatible with head master, not marking obsolete as it useful to read first before reimplementing.
I tend to think that copying objects is enough in that case, moreover, it is now very simple to do that with GES. If someone rely pushes for that feature to be included, I am not against it either.
Keeping that bug open and moving to GES now that NLE replaced GNL and is currently internal to GES,
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-editing-services/issues/1.