After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695884 - Frei0r plugins expose incorrectly split properties for RGB stuff
Frei0r plugins expose incorrectly split properties for RGB stuff
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-03-14 22:11 UTC by Jean-François Fortin Tam
Modified: 2018-11-03 13:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (251.36 KB, image/png)
2013-03-14 22:11 UTC, Jean-François Fortin Tam
  Details
Fix the incorrect nicknames (2.75 KB, patch)
2013-03-16 17:36 UTC, Jean-François Fortin Tam
committed Details | Review
2nd patch improving (and correcting) the readability of nicknames (3.23 KB, patch)
2013-03-16 17:37 UTC, Jean-François Fortin Tam
committed Details | Review
screenshot of RGB properties exposed in disorder in GUIs (26.93 KB, image/png)
2013-03-16 17:47 UTC, Jean-François Fortin Tam
  Details
screenshot of various properties exposed in disorder (44.10 KB, image/png)
2013-03-16 17:51 UTC, Jean-François Fortin Tam
  Details

Description Jean-François Fortin Tam 2013-03-14 22:11:16 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?
Comment 1 Tim-Philipp Müller 2013-03-14 23:41:40 UTC
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)?
Comment 2 Tim-Philipp Müller 2013-03-16 12:52:25 UTC
So, for color properties would you prefer an unsigned int where you set the color as (A)RGB value?
Comment 3 Jean-François Fortin Tam 2013-03-16 17:36:34 UTC
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.
Comment 4 Jean-François Fortin Tam 2013-03-16 17:37:33 UTC
Created attachment 239029 [details] [review]
2nd patch improving (and correcting) the readability of nicknames

This makes it more human-readable.
Comment 5 Jean-François Fortin Tam 2013-03-16 17:46:07 UTC
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?
Comment 6 Jean-François Fortin Tam 2013-03-16 17:47:47 UTC
Created attachment 239030 [details]
screenshot of RGB properties exposed in disorder in GUIs
Comment 7 Jean-François Fortin Tam 2013-03-16 17:51:03 UTC
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.
Comment 8 Tim-Philipp Müller 2013-03-16 18:42:22 UTC
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 ?
Comment 9 Nicolas Dufresne (ndufresne) 2013-03-17 15:23:12 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2014-05-02 12:26:23 UTC
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.
Comment 11 Nicolas Dufresne (ndufresne) 2014-05-02 16:51:47 UTC
Right, maybe a GstStructure could do ? (not to sure how that interact with gst-launch syntax).
Comment 12 Tim-Philipp Müller 2014-05-02 17:01:33 UTC
How does that help? This is about semantics, otherwise we could just use plain strings.
Comment 13 Nicolas Dufresne (ndufresne) 2014-05-02 19:34:13 UTC
(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.
Comment 14 GStreamer system administrator 2018-11-03 13:14:56 UTC
-- 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.