GNOME Bugzilla – Bug 792798
video-overlay: Add helper to make render rectangle controllable with a property
Last modified: 2018-01-30 10:05:11 UTC
This is an attempt to make it easy to turn the GstVideoOverlay into properties. This was implemented in order to ease manual testing of the overlay rectangle feature. These helper are demonstrated through an xvimagesink implementation. As shown, changing the rectangle is made atomic by using dispatch_properties_changed virtual method of GObject class.
Created attachment 367248 [details] [review] video-overlay: Add helpers for render rect properties This is a set of helper that makes it easy to enable the render rectangle to be controllable through properties.
Created attachment 367249 [details] [review] xvimagesink: Make render rect controllable with properties
Created attachment 367250 [details] [review] kmssink: Make render rectangle property controllable
Not sure if that's a problem, but this means that the rectangle can't be set atomically. Also why not make them interface properties? Isn't it possible to do generic setters/getters in the interface, and overriding them in implementors is required?
I think if you make them interface properties then implementors are required to override the properties which breaks backwards compat? It's all .. a bit .. :)
As Tim said. I also tried to find some implicit or semi implicit tricks around GstVideoSink base class, but it ended up the same, so it was pointless to force the helpers in there. That was the least effort helpers I found yesterday. The only other way I can think of is to make this a single property. That would solve the very first concern. For this I'd: - box the rectangle (and make a type for it) - implement gvalue string converting + syntax - add gst_param for that It's more work behind the scene, but would probably fit better. What do you think ?
Ok, second thought on my proposal, this would first not be usable from GstStucture (but does it matter ?) but is also a lot more complex, I need to find a syntax that does not conflict and all. So, a middle ground would be to reuse GstValueArray, represent a rectangle as an array of 4 gint. With a single writable property, I no longer need to save the rectangle and a lot of the complexity goes away.
Could probably also do a new interface (GstVideoOverlayPropertySetterCommandLinethingy) that provides these properties and depends on the GstVideoOverlay interface. That way we can add properties without breaking backwards compat. But it's also not very nice :)
Regarding the atomicity, I've always wanted something like a gst_controller_set_properties_at_the_next_opportunity() that's basically like g_object_set() only that it schedules the setting atomically for the next time gst_object_sync() is called from the streaming thread, to ensure atomicity.
Oh, I didn't correct Sebastian on the atomicity claim, the previous implementation was atomic if use the right way, just read a little more the code, it will call set_render_rectangle() once when the property groups have been set (using dispatch_properties_changed virtual). This virtual is called once per group of property being set, that is when multiple property are set within the same call to g_object_set() or if g_object_set() is recursively calling itself. I'll go a new version now with single property, since this removes all this complexity for atomicity and having to store the rectangle meanwhile. (In reply to Tim-Philipp Müller from comment #8) > Could probably also do a new interface > (GstVideoOverlayPropertySetterCommandLinethingy) that provides these > properties and depends on the GstVideoOverlay interface. That way we can add > properties without breaking backwards compat. But it's also not very nice :) Sure, but then if you want to add another property later you have to create yet another interface, it's a bit crazy.
Created attachment 367333 [details] [review] video-overlay: Add helpers for render-rect property This is a set of helper that makes it easy to enable the render rectangle to be controllable through a property.
Created attachment 367334 [details] [review] xvimagesink: Allow changing render-rect through property This also enables setting the render rectangle before the window is provided or created.
fyi, for this new version, using the property will look like: gst-launch-1.0 videotestsrc ! xvimagesink render-rectangle="<80,80,100,100>"
Created attachment 367337 [details] [review] video-overlay: Add helpers for render-rect property This is a set of helper that makes it easy to enable the render rectangle to be controllable through a property.
Created attachment 367338 [details] [review] kmssink: Make render rectangle property controllable
So, I think this is less hacky then before, let me know if there is anything that would prevent this from going upstream. What's left is to re-read the doc, I can see couple of typos and an missing something in the main GstVideoOverlay description.
Created attachment 367621 [details] [review] glimagesink: Add render-rectangle property This allow controlling the render rectangle from gst-launch-1.0.
Created attachment 367622 [details] [review] glimagesink: Allow resetting render rectangle As documented, passing -1 to x and/or y should reset the render rectangle to the window/display size.
Attachment 367621 [details] pushed as 3b317ea - glimagesink: Add render-rectangle property Attachment 367622 [details] pushed as 17b118c - glimagesink: Allow resetting render rectangle Attachment 367337 [details] pushed as c70dd75 - video-overlay: Add helpers for render-rectangle property Attachment 367334 [details] pushed as e368b31 - xvimagesink: Allow changing render-rectangle through property
Attachment 367338 [details] pushed as a7c2076 - kmssink: Make render rectangle property controllable