GNOME Bugzilla – Bug 435120
cairosvgoverlay
Last modified: 2010-11-22 15:15:45 UTC
Scalable and relative svg overlay with cairo
Is this something you intend to work on?
no
It will be nice for something like small company logo overlay in corner e.t.c.
Created attachment 166262 [details] [review] Implement rsvgoverlay element This patch against current gst-plugins-bad adds the rsvgoverlay element to the rsvg pugin. It offers a simple way to overlay a SVG file (or directly feed it SVG data) to a video.
Created attachment 166265 [details] [review] Implement rsvgoverlay element This patch against current gst-plugins-bad adds the rsvgoverlay element to the rsvg plugin. It offers a simple way to overlay a SVG file (or directly feed it SVG data) to a video. A NULL test on property data was forgotten in the previous version.
Thank you, thank you, thank you! I wasn't present on bugzilla for quite a while. People here are really responsive. Now I will use your code instead mine ugly attempt to implement it. I would really like it going into the tree. It's small, but it will be useful for a lot of people.
Ping. What about this patch? Could it be considered for inclusion in the next -bad release? If not, what is missing?
Review of attachment 166265 [details] [review]: Looks good in general but some comments below and in general I think it might be better to not have properties for setting the filename or setting the SVG data but instead to have another sinkpad, where you push in the SVG data. That's more generic and allows the element to be used in more scenarios. See gst-plugins-good/gst/shapewipe for an example of an element, that takes some kind of "picture" on one sinkpad and uses this to transform the video stream from the other sinkpad. ::: ext/rsvg/gstrsvg.c @@ +34,3 @@ + return (gst_element_register (plugin, "rsvgoverlay", + GST_RANK_NONE, GST_TYPE_RSVG_OVERLAY) + & You want an && here... not that it matters much in this case ::: ext/rsvg/gstrsvgoverlay.c @@ +46,3 @@ + GST_PAD_SRC, + GST_PAD_ALWAYS, + GST_STATIC_CAPS (GST_VIDEO_CAPS_RGBA) This is wrong. cairo doesn't produce RGBA. It produces ARGB or BGRA depending on the hosts endianness. Use something like #if G_BYTE_ORDER == G_LITTLE_ENDIAN #define GST_RSVG_VIDEO_CAPS GST_VIDEO_CAPS_BGRA #else #define GST_RSVG_VIDEO_CAPS GST_VIDEO_CAPS_ARGB #endif and use that instead then. See gstrsvgdec.c @@ +56,3 @@ + +GST_BOILERPLATE (GstRsvgOverlay, gst_rsvg_overlay, GstBaseTransform, + GST_TYPE_BASE_TRANSFORM); It might be better to inherit from GstVideoFilter instead. It's like GstBaseTransform but additionally implements sane defaults for QoS. You only need to change this here and in the header @@ +68,3 @@ + switch (prop_id) { + case ARG_DATA: + case ARG_FILENAME: This is not threadsafe. You need to use some lock here and in the transform function @@ +135,3 @@ + + if (G_UNLIKELY (!gst_video_format_parse_caps (GST_BUFFER_CAPS (buf), + &format, &width, &height))) Do the caps parsing only once in the GstBaseTransform::setcaps vfunc @@ +200,3 @@ + + g_object_class_install_property (G_OBJECT_CLASS (klass), ARG_DATA, + g_param_spec_string ("data", "data", "RSVG data.", "", G_PARAM_WRITABLE)); Use G_PARAM_WRITABLE | G_PARAM_STATIC_STRINGS here and for the other properties ::: ext/rsvg/gstrsvgoverlay.h @@ +31,3 @@ + ARG_FILENAME, + ARG_FIT_TO_FRAME +}; Put this enum into the source file (and rename the elements to PROP_0, PROP_DATA, etc) Also, maybe properties for the x/y position, the x/y scale factor and a property to force keeping of the aspect ratio might be a good thing to have @@ +50,3 @@ +{ + GstBaseTransform element; + Add a comment like /* < private > */ here to let gtk-doc ignore the remaining fields of the struct.
Created attachment 169196 [details] [review] Implement rsvgoverlay element This patch against current gst-plugins-bad adds the rsvgoverlay element to the rsvg plugin. It offers a simple way to overlay a SVG file (or directly feed it SVG data) to a video. This new version addresses most comments from Sebastian review, except for the most important one (I assume): the non-gstreamic way of feeding SVG data. I gave it a try, but it is not as simple as writing a video filter: data is sent as multiple chunks, that have to be collected before being displayed. As there is no really similar examples, it takes a lot of guessing/rummaging through various plugins sources to achieve this. Worse, as the API evolved, there are (I guess) multiple different ways to achieve almost the same thing... I do not throw the towel, but I think the other changes are relevant enough to justify to post this new version (which adds x/y properties). That said, I will implement the data_sink approach, but would like to keep data and filename parameters, for the sake of simplicity (from the Zen of Python: "practicality beats purity", or in more common words, "Make simple things simple, complex things possible"). I really think that the following pipeline: gst-launch videotestsrc ! rsvgoverlay filename=/tmp/a.svg ! ffmpegcolorspace ! autovideosink is more simple/readable than the more gstreamish: gst-launch videotestsrc ! rsvgoverlay name=svg ! ffmpegcolorspace ! xvimagesink filesrc location=/tmp/a.svg ! image/svg ! svg.data_sink so both ways are desirable. The same is done in textoverlay/cairotextoverlay for instance.
Created attachment 169859 [details] [review] Implement rsvgoverlay element This patch against current gst-plugins-bad adds the rsvgoverlay element to the rsvg plugin. It offers a simple way to overlay a SVG file (or directly feed it SVG data) to a video. SVG data can be specified either though properties (filename/data) or fed through the data-sink pad.
I'd really prefer to not have the filename/data properties. There's absolutely no advantage from them and they only complicate the API of the element :) I'll review the code later
Please allow me, as both a developer and a user of this element, to disagree here. I need to dynamically overlay SVG data over a video, and setting the property is way simpler than pushing it through the data-sink pad. With this reasoning, why is there a text property in the textoverlay element?
Ok, after a long discussion with others lets go with the following solution: Remove the data property, make the pad a request pad and keep the filename property. The pad has preference over the property if it was requested. The data property can really go away because if you are really creating SVG data dynamically you should be able to handle request pads too... and if you only read it from somewhere there's always the pad (where you could link a uridecodebin or something) or filename property.
Please forgive my stubbornness, but I still fail to see why the proposed solution (filename property + request pad) is so much cleaner than the current solution. It implies adding a number of LOC to the filter (setting up the request pad), plus dozens of LOC to the calling code, to get less flexibility (which is a quality I appreciate in gstreamer) in the end. The current API provides 3 ways to input SVG data: 1/ through the data-sink pad 2/ through the location property as a filename 3/ through the data property as actual SVG data. It allows the user to pick the most appropriate way, according to his needs. Does that really make the API more complicated? I think it all boils down to the philosophical question of "how does the outside world interact with gstreamer". From what I understand, there are 3 ways of providing external data to gstreamer: 1. use an existing input element such as fdsrc, filesrc... 2. write your own element with appsrc (which replaces the fakesrc hack needed before) 3. set properties Is there any design policy that clearly prevents data to be fed through properties? In this case, bugs should be filed against numerous other elements (textoverlay, cairotextoverlay, clockoverlay...). I am conscious that I do not have the same overall design awareness as you, and that the level of quality code found in gstreamer can only be achieved by being cautious about new code. But as far as design is concerned, I really like to rely on the Alan Kay mantra ("make simple things simple and complex things possible") as well as the python motto ("practictality beats purity"), which I think are good design guidelines. And in this case, forcing users to write an appsrc to feed data into a request pad, instead of using a single line to set a property, looks like overkill to me.
as gstreamer user I agree with Olivier, regards Nicola
I went through all the same thoughts when working on the API for coglogoinsert. I finally decided on filename only. There were reasons for this: - You could easily create an equivalent pipeline using filesrc and videomixer, thus creating a complex API wasn't helpful. - I was too lazy to implement anything else. So, it got a location= property that takes a filename. I was too lazy to do the right thing in this case, which is to make the property take a GstBuffer and then add a helper function in the core to set a property directly from a file, and finally have gst_parse understand that setting a GstBuffer property with a string containing a URI means that it should load the file into a buffer. There are effectively two ways to look at this: Either it's a filter element, or a mixer element that is very restricted on the formats it accepts on one pad. The latter element is nearly useless, because it's neither simple to use nor general enough to be generally useful (like videomixer). On the other hand, I am against adding this element because we should really only have one logo insertion element that handles a limited number of useful formats, like SVG and PNG.
Regarding David's arguments, I have two remarks: - the "one logo insertion element" looks a bit narrow to me: SVG overlaying is not just for logo insertion (even though it is a very common use). I use it to display annotations over the video (see the screencast http://liris.cnrs.fr/advene/screencasts/advene-svg-overlay.ogv for instance). Some time ago, someone wanted to overlay a grid over the video. Presto: gst-launch videotestsrc ! rsvgoverlay data='<svg viewBox="0 0 800 600"><rect fill="none" stroke="green" x="0" y="0" width="100%" height="100%"/><rect fill="none" stroke="green" x="266" y="0" width="266" height="100%"/><rect fill="none" stroke="green" x="0" y="200" width="100%" height="200"/></svg>' ! ffmpegcolorspace ! xvimagesink It can be used to overlay bitmap images too: gst-launch videotestsrc ! rsvgoverlay data='<svg viewBox="0 0 800 600"><image x="0" y="0" width="100%" height="100%" xlink:href="foo.jpg" /></svg>' ! ffmpegcolorspace ! xvimagesink We could even imagine a "bitmap" property which would generate the appropriate SVG code as above to overlay a bitmap image, such as you requested. But this would qualify as "complicated API", wouldn't it? - the API with both a filename property and a data-sink is directly inspired by the textoverlay API. And as a matter of fact, I think it is perfectly appropriate. It can be used to overlay a static text, or it can be fed subtitles from the subparse element. Put your SVG content into a subtitle file, and you get graphical subtitles.
I have been using a self-made element cairoimageoverlay (http://git.holoscopio.com/git/hplugins.git/). One of the requirements that made me develop this was that I needed to change the image I was using to overlay while the pipeline was playing, and I needed to change the size and position of the image being displayed also with the pipeline playing, and that could change multiple times per second. Both of these requirements seemed hard to do using filesrc + imagefreeze + videomixer and much simpler to do if the element handled the image itself. I am planing to switch to cairosvgoverlay when it is available, but I would probably need to adapt my aplication and maybe the element. So, would it be desirable to have resizing in cairosvgoverlay? Maybe a resizing to a size relative to the frame size instead of the fit_to_frame property. Than, fit_to_frame would be achieved with the resize to 1.0? Or would this be achievable using the data property? What I really wouldn't want is to have to feed data through the pad and transform it using gstreamer, which would gave me the same complexity filesrc + imagefreeze + videomixer I am trying to avoid.
Is resizing to a size relative to the frame size really meaningful? Given that once you have resized your overlayed image, you will have to position it, there should be some relative positioning involved too, which complexifies the API further. It is trivial to add xscale/yscale properties (1.0/1.0 would emulate the previous fit_to_frame behaviour), but I do not know if it is really useful.
Hello With no answer to previous comments, I would like to know the status of this proposed element... To sum up the arguments, here is the current situation: the rsvgoverlay element overlays SVG graphics over a video stream. The problem lies in the specification of the SVG data. There are 3 ways to do this: 1. the gstreamer-way, feeding the SVG data through a data-sink pad. 2. the less gstreamer-way but more convenient/concise in common cases: specifying the location of the SVG file through a property. 3. the other less gstreamer-way, but more convenient also: specifying the actual SVG data through a property. Now, the first 2 methods cause no apparent problem, and the issue lies at point 3. If we compare the API to the textoverlay element one, there is similarity: we can feed it data either through the text_sink pad or through a "text" property. There is only 1 non-gstreamer way, since the common case it addresses is the specification of some static text. Now, back to the SVG overlay element, the nature of the SVG data is hybrid: on the one hand, it is a graphical format, generally available in the form of files (hence the filename property) ; on the other hand, it is text-based, and can be specified on the command-line too, which can help rapid prototyping. The dual nature of SVG data is IMO the justification of the presence of these 3 ways to specify data. Until now, the only argument against this proposal has been "The API is too complicated", which, without further explanations, does not really help me to figure out the real issue. I would really appreciate to have this solved before the next -bad release. What would be the best way to find a solution?
I'm personally not against any of above solutions. For me, the sole fact that I'm able to feed video with overlay through gst-launch is more than just good in comparison to what I had before.
ping ?
The data property looks a bit hackish, but makes it indeed a powerful tool for gst-launch usage. On the other hand one could probably do something like: echo "<svg viewBox="0 0 800 600"><image x="0" y="0" width="100%" height="100%" xlink:href="foo.jpg" /></svg>" | gst-launch videotestsrc ! rsvgoverlay name=o ! autovideosink fdsrc ! image/svg ! o. If we go for that, it should probably be added to the elements doc blob, as it is not very discoverable.
Review of attachment 169859 [details] [review]: Looks good to me. I'd rather get this in, than having it bitrotting here. ::: ext/rsvg/gstrsvgoverlay.c @@ +19,3 @@ + +/** + * SECTION:element-rsvg_overlay this should be "SECTION:element-rsvgoverlay" @@ +212,3 @@ + + gst_adapter_push (overlay->adapter, buffer); + /* FIXME: some code samples do GST_PAD_PARENT does not ref, gst_pad_parent() refs. In the chain function, you can avoid the ref. So just remove the FIXME: comment. And let us know of the broken "code samples" if you remember. @@ +356,3 @@ + g_param_spec_string ("data", "data", "RSVG data.", "", + G_PARAM_WRITABLE | G_PARAM_STATIC_STRINGS)); + g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_FILENAME, I think we usually call this "location". @@ +368,3 @@ + g_param_spec_int ("y", "y offset", + "Specify a y offset", 0, G_MAXINT, 0, G_PARAM_READWRITE)); + please add "| G_PARAM_STATIC_STRINGS" to the last three properties.
Created attachment 174106 [details] [review] Implement rsvgoverlay element This version addresses the remarks done in https://bugzilla.gnome.org/show_bug.cgi?id=435120#c24
Stefan's comments have been addressed in the latest attachment. Is there anything else that should be done?
Weekly ping about the status of this bug. Is there anything more that should be done?
commit c86b12ac95b3cf7d0ab7a57f469f9449f157f2ac Author: Olivier Aubert <olivier.aubert%40liris.cnrs.fr> Date: Mon Nov 22 15:10:26 2010 +0200 rsvgoverlay: scalable and relative svg overlay with cairo Add a cairo+librsvg based overlay element to the rsvg plugin.