GNOME Bugzilla – Bug 701287
gnonlin: API should be revisited before1.X first versions
Last modified: 2014-06-22 13:52:03 UTC
While porting PiTiVi and GES to GStreamer 1.0 we have identified few API changes that we would like to be done in GNonLin before the next major release. I'm using this bug to open the discusion and API changes proposal. If anyone would like other changes that we have not thought about, it would be the time to do so. Rate adjustment: Current API suggest that rate can be ajusted by setting a media-duration different from the duration of the source. The idea was that rate adjustment would be done through the rate parameter of the segment. The problem with that approach is that you need an element at rendering to change that rate back to normal and add/remove frame and samples, a job normally done by the sink element. Also, we think it's not possible with that to implement smooth rate ramp up as available on many video editors. Proposed solution is to drop media-duration and rate propertie, and favour element that do the rate ajustment by removing or adding data as needed. On the audio side, such element already exist and is called pitch (scaletempo could also support this mode of operation). On video side, we found that videorate can be modified easily to also do that. In the long term, an OpenCV elements could be added to use the optical flow algorithm to reconstruct the missing images during slow motion. We would most likely need something to make rampup implementation a bit easier, as changing the rate usually change the output duration and position does not change in constant anymore. This could be an element in GNL, with few preset rampup model (and eventually ability to set a rampup path), that would return proper position and duration, or a new base class (though it would probably clash with other baseclass like videofilter and such). Commit base changes: Currently, the only safe way to modify a composition, or an object, is to pause the pipeline first. We think that this limitation eliminates many use cases for GNonLin. Few video editors let you create a draft of your video by applying multi-cam technic. Also people could want to eventually consider doing live video production, which obviously require being able to change the composition in playing state (and increase compositon duration as we go). The proposes solution is to make GNL fully commit base. Objects, Operations and Compositon would keep a copy of modified values, and only apply those changes when a commit is executed. This will allow making the changes (or group of changes) fully atomic without having to pause the pipeline. Another benifit is that thread safety will be greatly enhanced in general, resulting in less risk of randomness. Obviously, we would commit the state when moving from READY to PAUSED state in order to keep the simple cases simple. API Whise, this would introduce a commit action into GnlObject. Doing commit on a GnlComposition, will automatically commit sources. Sources with let say composition inside of it will not be commited, to provide greater control over sub-pipelines. The update property in GnlComposition would be dropped. Rename media-start as inpoint : This is probably the simplest change, after discussion and comparision between GES and GNL, we concluded that inpoint is a more correct term and will be better understood. It was also confusing to have twice the word start in our discussions. Support for stream-id: One thing that we can't do easily at the moment it to select 1 of many audio streams. GStreamer 1.0 has introduces stream-id into stream-start event to enable that. We would add a stream-id property to GnlSource to support that. We would keep the caps property, but move it to GnlSource, as it make no sense in GnlComposition and GnlOperation. MAXUINT32 expandables: This was probably how expandables worked prior to the expandable property. Using MAXUINT32 has some limitation and which are fully covered by the expandable property. We would drop this support, which will enabel some code cleanup. Better sinks property in GnlOperation: We want to find a better name for the sinks property of GnlOperation. We would also like to enable ranges to fully support elements that handle from n to m elements (like the frie0r elements, that can have 2 or 3 inputs, videomixer and adder, that supports from 1 to infinit input, etc). These detailed cannot be guest by the operation itself. The naming is to be defined. Automatic GAP filling: Currently doing automatic gap filling using expandable is limiting and complex. This will eventually be solved by adding a callback that would let the application know that a GAP has been found, and what type of gap, when is this gap and what's the duration. This way the application can do fancier stuff depending if this is about filling a fixed input video mixer or if it's a regular cap in the timeline. This is no blocker for this bug, as adding API can be done later, though, not filling a GAP might become an error. For this reason, we may introduce errors when gaps are detected at runtime, we should at least document it in this API change. I think this is it for now, please feel free to add more, our main focus is what would change the API and ABI.
Merge GES and GNonlin (not sure which on in which) so that you can do direct method calls (instead of solely relying on GStreamer API). This should allow you for example to do in one "atomic" call the change of several properties.
I started to implement that at: http://cgit.collabora.com/git/user/tsaunier/gnonlin/log/?h=spring_cleanup
I would move the gnonlin plugins into ges if one were to merge both. Or maybe it should go into -base? (after all -base is supposed to contain an example for each type of different element ;))
(In reply to comment #3) > I would move the gnonlin plugins into ges if one were to merge both. Or maybe > it should go into -base? (after all -base is supposed to contain an example for > each type of different element ;)) Even if I understand that merging GNL into GES is a good idea and would help us making them more integrated, I think that GNL is quite usable from the standard Gst/GLib APIs. I consider that GNL being part of -base could be a good opportunity for it to take a bigger place in the Gst ecosystem, gaining visibility... and hopefully getting more love. Also I hope that to plan to get it merged into -base would mean that one of you would take the time to actually review it deeply :)
Ok, I pushed my latest changes at: https://github.com/pitivi/gnonlin/commits/api_rework ready for a first review I think.
> gnl: Remove trailling whitespaces and tabs in .c files You have change tabs into space in few macros, but maybe it would be nice to add the missing spaces now to get all those trailing backslash to be aligned, while doing style patches. E.g.: -#define COMP_FLUSHING_UNLOCK(comp) G_STMT_START { \ - GST_LOG_OBJECT (comp, "unlocking flushing_lock from thread %p", \ - g_thread_self()); \ - g_mutex_unlock (&comp->priv->flushing_lock); \ +#define COMP_FLUSHING_UNLOCK(comp) G_STMT_START { \ + GST_LOG_OBJECT (comp, "unlocking flushing_lock from thread %p", \ + g_thread_self()); \ + g_mutex_unlock (&comp->priv->flushing_lock); \ > gnl: First implementation of the commit based API You move the implement outside of GnlObject later, would be nice to squash those commits. You could probably move "gnl: Move the commit signal from gnlobject to gnlcomposition" near that commit, and then squash, that should reduce conflict I think. CHECK_AND_EMIT and SET_PENDING_VALUE, you could indent the \ properly. Also, I don't get why CHECK_AND_EMIT need to emit, why not just marking as "need_commit" and signal later ... We don't really care if it's updated twice before commit, or does GES care ? Otherwise, maybe you'd prefer if the properties always report the latest (the future value), making the commit the other way around. object->start = object->pending_start = 0; + object->duration = object->pending_duration = 0; No need to set members to 0, all GObject a memset with 0. + /* We need to inhibit notify emition when the user changes properties, it + * will be emitted only when comiting */ + for (i = 0; i < G_N_ELEMENTS (notifies_to_inhibit); i++) + g_signal_connect (object, notifies_to_inhibit[i], + G_CALLBACK (_discard_notify_cb), NULL); see comment about CHECK_AND_EMIT, could make this not needed at all. > gnlcomposition: Remove uneeded indents You seem to be doing way more then that, I have no idea, can't see if anything changed here, all lines are affected, and + lines does not look like - lines. > gnlcomposition: Remove support for gaps Adds two white spaces, I don' get why. > composition: More reliably compute outgoing segment base time This is unrelated change (not fully trivial) > gnlcomposition: Remove the FLUSH_LOCK, us atomic operations instead This is also unrelated. Also I'm not 100% convince that this is correct (clearly not trivial). > gnlcomposition: suffix callbacks with _cb That one is unrelated but fall into the trivial case. I think more people should have a look. Cleaning up would definatly help reviewers. There seems to be some left over, maybe you could document what you haven't address (some of it seems mentionned in the commit logs).
(In reply to comment #6) > > gnl: Remove trailling whitespaces and tabs in .c files > > You have change tabs into space in few macros, but maybe it would be nice to > add the missing spaces now to get all those trailing backslash to be aligned, > while doing style patches. DONE > You move the implement outside of GnlObject later, would be nice to squash > those commits. You could probably move "gnl: Move the commit signal from > gnlobject to gnlcomposition" near that commit, and then squash, that should > reduce conflict I think. DONE > CHECK_AND_EMIT and SET_PENDING_VALUE, you could indent the \ properly. Also, I > don't get why CHECK_AND_EMIT need to emit, why not just marking as > "need_commit" and signal later ... We don't really care if it's updated twice > before commit, or does GES care ? Otherwise, maybe you'd prefer if the > properties always report the latest (the future value), making the commit the > other way around. This has been discussed on IRC, the idea is that a property should emit a notify signal only when the value is changed, which will happen only when commiting. So we inhibit the emission of the signal until the value is actually changed. I am concidering adding signals for pending value changes, emitting the pending value in the signal itself, like: g_signal_emit ("pending-start", object->pending_start); > > gnlcomposition: Remove uneeded indents > > You seem to be doing way more then that, I have no idea, can't see if anything > changed here, all lines are affected, and + lines does not look like - lines. I reworded the commit message expliciting more what is actually done (it is really only about review the indentation startegy) > > gnlcomposition: Remove support for gaps > > Adds two white spaces, I don' get why. Not sure what you mean here? > > > composition: More reliably compute outgoing segment base time > > This is unrelated change (not fully trivial) > > > gnlcomposition: Remove the FLUSH_LOCK, us atomic operations instead > > This is also unrelated. Also I'm not 100% convince that this is correct > (clearly not trivial). > > > gnlcomposition: suffix callbacks with _cb > > That one is unrelated but fall into the trivial case. I removed those commit from the beanch, it is now at: https://github.com/pitivi/gnonlin/commits/misc_improvements > I think more people should have a look. Cleaning up would definatly help > reviewers. There seems to be some left over, maybe you could document what you > haven't address (some of it seems mentionned in the commit logs). That would be great, but I think it is important to try to get that stuff in asap as we will need quite some debugging, and testing as it is pretty invasive changes.
(In reply to comment #0) > Rate adjustment: Implemented in "gnl: Remove the notion of media-duration" > Commit base changes: Implemented in: "gnl: First implementation of the commit based API" > Rename media-start as inpoint : Implemented in: "gnl: Rename media-start as inpoint" > Support for stream-id: > > One thing that we can't do easily at the moment it to select 1 of many audio > streams. GStreamer 1.0 has introduces stream-id into stream-start event to > enable that. We would add a stream-id property to GnlSource to support that. We > would keep the caps property, but move it to GnlSource, as it make no sense in > GnlComposition and GnlOperation. I am not sure we want to move the caps property of GnlSource, we could imagine operations with several srcpads, and in this case the caps property could be used there. Also we do use the caps property of the GnlComposition. The stream-id propertyparts still needs to be implemented, it is more a new feature than an API change fmpov. > MAXUINT32 expandables: Implemented in: "gnl: Setting priority == MAXUINT32 does not mean "default object"" > Better sinks property in GnlOperation: > > We want to find a better name for the sinks property of GnlOperation. We would > also like to enable ranges to fully support elements that handle from n to m > elements (like the frie0r elements, that can have 2 or 3 inputs, videomixer and > adder, that supports from 1 to infinit input, etc). These detailed cannot be > guest by the operation itself. The naming is to be defined. I was thinking about calling it num-sinks and make it a GST_TYPE_INT_RANGE, what do you think? > Automatic GAP filling: I implemented the first part in "gnlcomposition: Remove support for gaps", currently we post a STREAM_ERROR on the bus if there is a gap in the composition.
Subtile left over: White line, see gnlcomposition line 1855 and 2939 check/gnl/common.c line 209 Bad indent, see gnlobject.h lines 133,134,135 Other then that, I still think it would be better to have that scenario: start = 0; g_object_set (obj, "start", 1, NULL); g_object_get (obj, "start", &start, NULL); g_assert (start == 1); Which imply that notify:: signal are emited right away. Anyway, we sort right after having commited the objects. Then you can remove the code to hide the signals (which seems to fight against the way propreties work). It is cleaner, and emiting just in time does not provide much that can be used by the application. I have not found anything else in this pass.
(In reply to comment #9) > > Support for stream-id: > > > > One thing that we can't do easily at the moment it to select 1 of many audio > > streams. GStreamer 1.0 has introduces stream-id into stream-start event to > > enable that. We would add a stream-id property to GnlSource to support that. We > > would keep the caps property, but move it to GnlSource, as it make no sense in > > GnlComposition and GnlOperation. > > I am not sure we want to move the caps property of GnlSource, we could > imagine operations with several srcpads, and in this case the caps property > could be used there. Also we do use the caps property of the GnlComposition. I had in mind we could have both. I agree the caps, actually the media type should match for all object of a GnlComposition. > > The stream-id propertyparts still needs to be implemented, it is more a new > feature than an API change fmpov. agreed. > > Better sinks property in GnlOperation: > > > > We want to find a better name for the sinks property of GnlOperation. We would > > also like to enable ranges to fully support elements that handle from n to m > > elements (like the frie0r elements, that can have 2 or 3 inputs, videomixer and > > adder, that supports from 1 to infinit input, etc). These detailed > cannot be > > guest by the operation itself. The naming is to be defined. > > I was thinking about calling it num-sinks and make it a GST_TYPE_INT_RANGE, > what > do you think? Perfect, that was my first choice. > > > Automatic GAP filling: > > I implemented the first part in "gnlcomposition: Remove support for gaps", > currently we post a STREAM_ERROR on the bus if there is a gap in the > composition. That's fine, we won't be break API later this way.
(In reply to comment #10) > Subtile left over: > > White line, see gnlcomposition line 1855 and 2939 > check/gnl/common.c line 209 > Bad indent, see gnlobject.h lines 133,134,135 > > Other then that, I still think it would be better to have that scenario: > > start = 0; > g_object_set (obj, "start", 1, NULL); > g_object_get (obj, "start", &start, NULL); > g_assert (start == 1); > > Which imply that notify:: signal are emited right away. Anyway, we sort right > after having commited the objects. Then you can remove the code to hide the > signals (which seems to fight against the way propreties work). It is cleaner, > and emiting just in time does not provide much that can be used by the > application. > > I have not found anything else in this pass. I updated my branch to do it the way described here. It indeed makes the code simpler.
Bug 701287 - gnonlin: API should be revisited before1.X first versions - UNCONFIRMED
commit 5c026dfe236c6d6493e77c3eaafa643bb0dfd7ff Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Jun 13 20:33:00 2013 -0400 gnlcomposition: Remove support for gaps The way we were handling gap was weird and not natural at all for users, remove it for now. In the long term, we should have a proper way of filling gaps in the API and for now only emit an STREAM_ERROR message on the bus when a gap is detected. The user is now responsible for filling gaps in the composition https://bugzilla.gnome.org/show_bug.cgi?id=701287 commit 9545f1b58e42cf8a80744e7e269fbb650208f729 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Jun 13 19:39:55 2013 -0400 gnlcomposition: Remove uneeded indents Going from if (something) { do_something(); } else { } to if (!something) return; do_something (); commit bb8749f0c710a71f0afbd2499b0842d61cf0cc4d Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Jun 13 19:33:46 2013 -0400 gnlcomposition: Make the pipeline update protected from any child changes When calling the gnl_object_commit method, we need to make sure that no object can be changed and, it should be true until the pipeline has fully been udpated. We now need the update_pipeline function to be called with the COMP_OBJECTS_LOCK taken, and we do not unlock that lock during the whole pipeline updating process. https://bugzilla.gnome.org/show_bug.cgi?id=701287 commit 43fe6ac3c0f4e9ccd6c0ff8ffc5c7cffdcf3076e Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Jun 13 18:51:15 2013 -0400 gnl: Setting priority == MAXUINT32 does not mean "default object" This was how expandables worked prior to the expandable property. Using MAXUINT32 has some limitation and which are fully covered by the expandable property. Drop this support, as we will plan on implementing an automatic way of filling gaps. https://bugzilla.gnome.org/show_bug.cgi?id=701287 commit c99cc0ef17d5040105958bfc6085fe298cba2096 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Fri Jun 7 18:58:24 2013 -0400 gnl: First implementation of the commit based API Currently, the only safe way to modify a composition, or an object, is to pause the pipeline first. We think that this limitation eliminates many use cases for GNonLin. Few video editors let you create a draft of your video by applying multi-cam technic. Also people could want to eventually consider doing live video production, which obviously require being able to change the composition in playing state (and increase compositon duration as we go). This commit is a first step making GNL fully commit base. Objects, Operations and Compositon keep a copy of modified values, and only apply those changes when a commit is executed. This potentially allows making the changes (or group of changes) fully 'atomic' without having to pause the pipeline. Added API: ---------- GnlObject::commit action signal Removed API: ------------ GnlComposition:update property + Fix the tests https://bugzilla.gnome.org/show_bug.cgi?id=701287 commit d66b29d55d7506e0c0fed84439876f7e56c60127 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Fri Jun 7 18:17:25 2013 -0400 gnl: Remove trailling whitespaces and tabs in .c files commit f08303aa50283d1da784daf27521e640898217e0 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Fri Jun 7 11:39:17 2013 -0400 gnl: Remove the notion of media-duration Rational described at https://bugzilla.gnome.org/show_bug.cgi?id=701287 commit b57873c96e23d7b33750088ad990f80a98f5658e Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu May 30 17:48:47 2013 -0400 gnl: Rename media-start as inpoint Rational described at https://bugzilla.gnome.org/show_bug.cgi?id=701287
I think this is complete, anything else ?
Is it, or not?
It is, closing.