GNOME Bugzilla – Bug 695884
Frei0r plugins expose incorrectly split properties for RGB stuff
Last modified: 2018-11-03 13:14:56 UTC
Created attachment 238937 [details] screenshot gst-inspect-1.0 frei0r-filter-white-balance Yields the following element properties besides name, parent, qos: neutral-color-r : Choose a color from the source image that should be white. flags: readable, writable, controllable Float. Range: 0 - 1 Default: 1 neutral-color-g : Choose a color from the source image that should be white. flags: readable, writable, controllable Float. Range: 0 - 1 Default: 1 neutral-color-b : Choose a color from the source image that should be white. flags: readable, writable, controllable Float. Range: 0 - 1 Default: 1 green-tint : Adjust the level of green. flags: readable, writable, controllable Double. Range: 0 - 1 Default: 1.2 Looking at frei0r's balanc0r.c, I see only one single "neutral-color" property. I can guess that GStreamer somehow split that property into three. The problem is that the description incorrectly stays the same for all three virtual properties, so when you display them in a GUI like pitivi, the descriptions are all identical. What's worse however is that in pitivi (and this is both in 0.10 and 1.x git), the property "names" that are exposed to us by gstreamer are somehow incorrect, as the attached screenshot demonstrates: instead of "Neutral R color value", "Neutral G color value", "Neutral B color value", you get only "Neutral Color:" shown twice and one "Neutral Color-R:" Not only that, the actual value prop boundaries don't seem to make sense. If my understanding of the gst inspect output above is correct, it goes from 0.0 to 1.0... but RGB would be 0 to 255 each, no? This is not necessarily specific to frei0r's white balance filter, various other frei0r filters seem to be affected in similar ways, especially when it comes to the naming of human-readable properties names, or descriptions being missing. Is there a central place where you are overriding frei0r's strings somewhere in gstreamer?
You might find this bit of code interesting: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/frei0r/gstfrei0r.c#n59 It's how we seem to map 'color' properties. The range is generic really, the values don't have to be 0-255 (that way you can accommodate different color depths generically). > This is not necessarily specific to frei0r's white balance filter, various > other frei0r filters seem to be affected in similar ways, especially when it > comes to the naming of human-readable properties names, or descriptions being > missing. Is there a central place where you are overriding frei0r's strings > somewhere in gstreamer? Why yes, of course we have a big central table somewhere that garbles nice human readable property names and drops descriptions. That *would* be the most logical explanation, wouldn't it? Do you have any indication that there's an issue with the GStreamer wrappers not picking up some things that are present in the original plugin (rather than the frei0r plugin just not providing those)?
So, for color properties would you prefer an unsigned int where you set the color as (A)RGB value?
Created attachment 239028 [details] [review] Fix the incorrect nicknames This solves part of the problem when it comes to the nicknames showing up wrong in pitivi. You trickster, you.
Created attachment 239029 [details] [review] 2nd patch improving (and correcting) the readability of nicknames This makes it more human-readable.
Sorry about the 0.0 to 1.0 RGB values thing. After doing some research on the net, I realized that it's not just 0-255 like I'm used to seeing in GUIs/color pickers :) I'm still wondering what to do with this though. Perhaps your idea of a single integer to control the whole thing would be better... I don't know. For what it's worth, I looked at how Kdenlive presents the UI for those frei0r filters, and it certainly doesn't split this into three values, it simply provides a color chooser button and a color picker tool that governs a single property. Two other remaining issues are: - Although you have these dynamically split properties (R, G, B; or X, Y), you don't have dynamically generated property descriptions... so you have different prop names and nicknames, but still the same description that doesn't match anything. What would you suggest? - The properties seem to end up being added in disorder. For instance, one would expect that the R, G and B properties would be grouped together in order, but instead you end up with "G, B, Green Tint, R". Or at least, that's how it shows up in pitivi, and yet we simply loop through the available properties, so could it be that gstreamer is presenting them in disorder somehow?
Created attachment 239030 [details] screenshot of RGB properties exposed in disorder in GUIs
Created attachment 239032 [details] screenshot of various properties exposed in disorder This screenshot is the hardcore version. Not sure why, but the properties get shown in a very different order than what "gst-inspect-1.0 frei0r-filter-c0rners" hints at.
Ah, nice catch! No idea about the order. We seem to install them in the 'right' order, but I don't know if the GObject functions that you use to query properties guarantee any particular order. I wouldn't think so. As for the color property - I think there are use cases both for a split view, and a single property. The main issue is that if it's an integer then how do you know generically that this is a color property and not just a number from 0-0xffffff ?
We could think of introducting new types (and their gst-launch syntax). This would make life easyer (if it can ever be easy) for generic UI. Just an idea that would look like: typedef struct { gdouble red; gdouble green; gdouble blue; gdouble alpha } GstRGBA; struct { gdouble x, gdouble y } GstPoint; GstVideo has a color structure (which is fixed point iirc), but it's not a proper GType. That would be another option.
The problem with that would be that it wouldn't be accessible from gst-launch... unless we put the types into core. Putting GstRGBA into core seems ugly because core is supposed to be media agnostic.
Right, maybe a GstStructure could do ? (not to sure how that interact with gst-launch syntax).
How does that help? This is about semantics, otherwise we could just use plain strings.
(In reply to comment #12) > How does that help? This is about semantics, otherwise we could just use plain > strings. True it does not solve semantic, but I think we would also be able to change the color at once, not in 3 steps.
-- 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-plugins-bad/issues/89.