I'm not sure this is the right design, or at least it does not seem coherent with GstColorBalance interface. ColorBalance, suggest that an element can have an arbitrary amount of Channels that can be controlled. Base on that, if an element wants to offload it's work to downstream it should negotiate the required channels. This negotiation could be made dynamic, in a way that if downstream support only hue, and only hue is set to non default, then we would still do passthrough. But at soon as "unsupported" brightness gets set, then it will use software path (if allocation permits it, e.g. system memory and known color format).

Created attachment 268814 [details] [review]
colorbalance: add video colorbalance meta API

Created attachment 268815 [details] [review]
tests: add colorbalance meta API unit test

(In reply to comment #1)
> I'm not sure this is the right design, or at least it does not seem coherent
> with GstColorBalance interface. ColorBalance, suggest that an element can have
> an arbitrary amount of Channels that can be controlled. Base on that, if an
> element wants to offload it's work to downstream it should negotiate the
> required channels. This negotiation could be made dynamic, in a way that if
> downstream support only hue, and only hue is set to non default, then we would
> still do passthrough. But at soon as "unsupported" brightness gets set, then it
> will use software path (if allocation permits it, e.g. system memory and known
> color format).

From what I understand, currently, playsink only uses one videobalance element
(the more upstream one).

So I agree with you that upstream videobalance elements should look if
downstream supports the colorbalance API (per channel) and enter passthrough
mode depending on what downstream supports.

IMHO, a kind of API like the one proposed here is still needed because only one
element is controlled by playsink and this element will need to tell the
downstream videobalance element to modify the videobalance properties.

The allocation query might be used here to see if downstream supports the colorbalance API and which channels are supported.

You could do the negotiation about which channels are supported with the allocation query with the second parameter of gst_query_add_allocation_meta().

I think in general this approach is good, but you shouldn't limit the powers of the colorbalance interface :) Even if nobody uses anything else than the 4 channels.

(In reply to comment #5)
> You could do the negotiation about which channels are supported with the
> allocation query with the second parameter of gst_query_add_allocation_meta().
> 
> I think in general this approach is good, but you shouldn't limit the powers of
> the colorbalance interface :) Even if nobody uses anything else than the 4
> channels.

So do you think that having contrast, brightness, hue and saturation in the meta is enough or not ?

Yes, not enough.


Well, I think it is enough and also the colorbalance interface should only use these 4... but that's not how the API is currently. Something to fix in 2.0.

So the meta API should also use channels like in the interface, right ?

I think so

I'm considering doing the following stuff:

GstVideoColorBalanceMeta owns a GstVideoColorBalance reference.

typedef GstVideoColorBalance {
  GArray *channels; // Array of GstColorBalanceChannel
  GArray *values; // Array of gint
};

Adding the two following functions:

gboolean gst_video_color_balance_set_value (GstVideoColorBalance * colorbalance, GstColorBalanceChannel *, int);
int gst_video_color_balance_get_value (GstVideoColorBalance * colorbalance, GstColorBalanceChannel *);

What do you think ?

(In reply to comment #10) 
> typedef GstVideoColorBalance {

Isn't it a bit confusing, gst_color_balance vs gst_video_color_balance ?

(In reply to comment #11)
> (In reply to comment #10) 
> > typedef GstVideoColorBalance {
> 
> Isn't it a bit confusing, gst_color_balance vs gst_video_color_balance ?

gst_color_balance_ and GstColorBalance is already taken by the interface.

By the way, the GstVideoColorBalance object should only contains a GHashTable to make the association between the channel label (gchar*) and the value (int).

Created attachment 268954 [details] [review]
colorbalance: add video colorbalance meta API

Here is a WIP:
  * GstVideoColorBalanceMeta now owns a GstVideoColorBalance object.
  * The GstVideoColorBalance constains an hashtable to associate a channel label to a value. Maybe a GstVideoColorBalanceChannel is needed ?
  * It introduces: gst_video_color_balance_(set|get)_value + gst_video_color_balance_get_channels which returns the list of channel labels.
  * Documentation not done yet.

What do you think ?

Created attachment 268955 [details] [review]
tests: add colorbalance meta API unit test

Unit tests updated according to the last changes in the API.

You could let the meta implement GstColorBalance to make the confusion complete ;)

Don't use a GHashTable for this btw, the overhead will be immense for the usually 4 channels that are usually supported.

Instead of a hash table it could just be a list of struct { name, value }

As a follow up, I think this would be against what is currently implemented in playbin for this. Currently playbin will introspect the sink to check if it supports ColorBalance interface and at least the four mandatory channels, BRIGHTNESS, CONTRAST, HUE and SATURATION. If this is found, it will not plug a color balance element, which is another way to solve the passthrough issue.

It requires the application or a smart element like playbin to make decision, but it solves the problem at smaller complexity cost. Leaving as needinfo for now, but I cannot come with argument that would let me think we should go for a more complex mechanism.

So what should we do about this?

The fact is that playbin already do (at least tries to do) the right thing for this use case. This bug wasn't filed with an actual issue, just a let's do this. I would close/invalid this one.

Transformation metas would be nice at some point, but they will need to be a single meta that stack the transformation to maintain order. The problem that would remains is that if you have:

trans1 ! trans2 ! trans3 ! sink

And sink does not support trans3, then trans1 and trans2 need to do the job in SW and not passthrough.

Now, it's a nice feature to be able to stack transformations, but at the same time, we have the opengl elements that can do that already, so I don't feel it's essential. For cogl/clutter, I think the way forward is to keep pushing for interoperability, and then we can focus on enhancing playsink toward fully GL if possible, otherwise standard SW elements we've been using so far. Remains to find how, but to be honest I'd keep it simple.

Setting back to NEW because it's not NEEDINFO anymore.

-- 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-base/issues/111.